marshmallow
marshmallow copied to clipboard
Field.error_messages should allow dict and list values
According to typing, passing error_messages
to Field
should only accept Dict[str, str]
:
https://github.com/marshmallow-code/marshmallow/blob/dev/src/marshmallow/fields.py#L156
def __init__(
self,
*,
default: typing.Any = missing_,
missing: typing.Any = missing_,
data_key: str = None,
attribute: str = None,
validate: typing.Union[
typing.Callable[[typing.Any], typing.Any],
typing.Iterable[typing.Callable[[typing.Any], typing.Any]],
] = None,
required: bool = False,
allow_none: bool = None,
load_only: bool = False,
dump_only: bool = False,
error_messages: typing.Dict[str, str] = None,
**metadata
) -> None:
However in test_required_message_can_be_changed
, a dict
and list
value is used along with str
.
https://github.com/marshmallow-code/marshmallow/blob/09c5b26bee77be5e23927ddeb472d5ea56733266/tests/test_deserialization.py#L1850
This affects a few things with any attempt at implementing #334
Ah, it does appear that the typing is incorrect. It should allow dicts and lists. PRs welcome to fix.
@sloria In the Url Field, there's a line that retrieves an error from the error_messages dict directly (error=self.error_messages["invalid"]
) that expects a string.
If self.error_messages["invalid"]
returns a dict or list, what should the expected behavior be? Blindly convert it to a string? Or is there a bigger modification needed here?
class Url(String):
"""A validated URL field. Validation occurs during both serialization and
deserialization.
:param default: Default value for the field if the attribute is not set.
:param relative: Whether to allow relative URLs.
:param require_tld: Whether to reject non-FQDN hostnames.
:param schemes: Valid schemes. By default, ``http``, ``https``,
``ftp``, and ``ftps`` are allowed.
:param kwargs: The same keyword arguments that :class:`String` receives.
"""
#: Default error messages.
default_error_messages = {"invalid": "Not a valid URL."}
def __init__(
self,
*,
relative: bool = False,
schemes: types.StrSequenceOrSet = None,
require_tld: bool = True,
**kwargs
):
super().__init__(**kwargs)
self.relative = relative
self.require_tld = require_tld
# Insert validation into self.validators so that multiple errors can be stored.
validator = validate.URL(
relative=self.relative,
schemes=schemes,
require_tld=self.require_tld,
error=self.error_messages["invalid"],
)
self.validators.insert(0, validator)
In Email field there's the same problem. I think converting list or dict to string in this cases is not a good idea.
If no one is working on this right now, can I take it? I'll work on field, and check up on email field as well as suggested by p4m.
FWIW, I just added
assert all((isinstance(v, str) for v in (error_messages or {}).values()))
at the beginning of Field.__init__
and only test_required_message_can_be_changed
breaks.
This test dates back to #78 and was intentional. Rationale in PR comments (https://github.com/marshmallow-code/marshmallow/pull/122#issuecomment-68978143).
However, this doesn't work with validators that call format on the (potentially custom) error message.
I never used the feature, but I like the idea of being able to pass codes as I find it awkward to parse error messages to get information (I'm referring to other contexts, like database ORM error messages, not only marshmallow).
OTOH, validators make heavy use of format
even if sometimes the default message had no placeholder for the value. From a quick look, default messages may include placeholders for information about the validator (range min/max) but no placeholder for the input value (Email and URL pass it but it is ignored by default message).
@repole wrote in https://github.com/marshmallow-code/marshmallow/issues/334#issuecomment-660621479, that only Email and URL fields expect strings (because they pass those messages to validators expecting strings). And I agree we could sacrifice that. But it would make sense to have the same policy for validators and then there are many more involved than only URL and Email.
I think the "philosophy" or "usage" in marshmallow is not to return values in errors. We could remove the feature allowing to pass value
in error messages, but we'd still have to deal with error messages containing validator parameters. So I'm in a bit of a dead end, here.
I'm wondering how usable in practice is the "dict/list" case.I'd be curious to see a code sample using it. When building an API, I may use codes on errors and pass those to error messages when returning an error to the client. But it doesn't seem feasible in practice to do it at field level. My point is perhaps (unless a sample code or an explanation proves me wrong) the dict/list error message feature could be removed, meaning fields should only allow string error messages.
In any case, we might have to live with the "contradiction" until marshmallow 4.