marshmallow icon indicating copy to clipboard operation
marshmallow copied to clipboard

Field.error_messages should allow dict and list values

Open repole opened this issue 3 years ago • 5 comments

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

repole avatar Jul 19 '20 23:07 repole

Ah, it does appear that the typing is incorrect. It should allow dicts and lists. PRs welcome to fix.

sloria avatar Jul 27 '20 14:07 sloria

@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)

repole avatar Aug 02 '20 02:08 repole

In Email field there's the same problem. I think converting list or dict to string in this cases is not a good idea.

platonoff-dev avatar Sep 04 '20 16:09 platonoff-dev

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.

cwkang1998 avatar Oct 03 '21 03:10 cwkang1998

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.

lafrech avatar Oct 04 '21 21:10 lafrech