RFC: Change the way we store metadata?

[sloria]

Users are often bit by the fact that fields store arbitrary keyword arguments as metadata. See #683.

...The reasons we use **kwargs instead of e.g. metadata= are mostly historical. The original decision was that storing kwargs 1) was more concise and 2) saved us from having to come up with an appropriate name... "metadata" didn't seem right because there are use cases where the things your storing aren't really metadata. At this point, it's not worth breaking the API.

Not the best reasons, but I think it's not terrible. We've discussed adding a whitelist of metadata keys in the past, but we decided it wasn't worth the added API surface.

Originally posted by @sloria in #779 (comment)

Possible solutions:

1. Use metadata=.
2. Specify a whitelist of allowed metadata arguments.

Feedback welcome!

[sloria]

Solution 1. is preferable to 2., I think. That said, there are some use cases where it's awkward to call additional kwargs "metadata". location in webargs is one that comes to mind.

# current API
"some_query_param": fields.Bool(location="query")

though we could probably wrap all the fields in webargs to take the additional location argument. 🤔

[sirosen]

I wanted to note that even webargs' location doesn't necessarily make the case against metadata=.... I was surprised/confused at first when I went looking for location in marshmallow and found no mention of it. At the cost of a little bit of verbosity, it would make it easier to understand how marshmallow is functioning.

Relatedly, the plan in marshmallow-code/webargs#419 includes making location=... for webargs a thing of the past.

[sloria]

cc @jtrakk . This was your suggestion in #779 (comment) . I'm leaning towards this more and more.

[vgavro]

+1 on this, IMHO biggest problem of self.metadata=kwargs is not that it's unexpected, but that it's not generating errors on wrong keyword arguments, which is pretty annoying due frequent api changes :-) so - you can find mistakes only later, all your typos in metadata field...

[lafrech]

One one hand, I think it is better to specify metadata=. More explicit.

OTOH, this will make my models a bit more verbose:

class MyModel(ma.Schema:
    some_int = ma.fields.Int(
        required=True,
        validate=ma.validate.OneOf([1, 2, 3]),
        metadata={"description": "This string explains what this is all about"}
    )

For such a use case, the shortcut of using extra kwargs as metadata is nice.

If we went with solution 2, users would be able to extend the whitelist with their own stuff. Apispec could extend it with the keyword arguments it expects (valid OpenAPI attributes) and we could even catch typos inside metadata, while solution 1 blindly accepts anything in metadata.

However, this would prevent accepting arbitrary attributes in metadata, which sucks. E.g. in apispec, we also accept any "x-..." attribute. So we'd need to genericize the whitelist to a callable mechanism. And we end up with a gas factory feature while we wanted to make thing simple.

Overall, perhaps the downside of 1 (model verbosity) is not that bad.

[sloria]

Agreed. Consider this accepted.

My plan is to deprecate metadata=kwargs in a later 3.x release. Let's let the dust settle on v3 before bombarding users with DeprecationWarnings 😅

[sirosen]

As I was integrating apispec into one of our large codebases at work, I found I really wanted to be able to start using metadata={"type": "date-time"}. Especially with some of the openapi names which come up in apispec, it's hard to clarify that some of those keys (format, type, pattern) are not actually part of how a field evaluates.

I'd like to be able to start using an explicit metadata parameter today, under 3.x, if we think it's okay to make that change.
I've opened a draft PR with an initial cut at this change to help figure out if it's safe/okay to do.

[lovasoa]

This will make specifying metadata in marshmallow-dataclass very awkward. We will have to write things like

@dataclass
class C:
   my_attr: float = field(metadata={"metadata": {"description": "my description"}})

[sloria]

That issue is rather specific to the marshmallow-dataclass, and I think the benefits still outweigh the cost. It is much better to have a proper exception get raised when a user passes an invalid keyword argument rather than passing silently.

[Diveafall]

I'm now getting multiple warnings about "Passing field metadata as a keyword arg is deprecated." and I have no idea where that metadata is 😄

[lafrech]

You (or a third-party library) are instantiating fields with arbitrary keyword arguments marshmallow doesn't know about.

Turn warnings into errors to get a full traceback and find out where it happens.

[ethagnawl]

@lafrech How would one "turn warnings into errors?"

[lafrech]

How would one "turn warnings into errors?"

https://stackoverflow.com/questions/5644836/in-python-how-does-one-catch-warnings-as-if-they-were-exceptions

[ethagnawl]

Thanks, @lafrech, that's a great tip. Not only did it solve my problem with respect to this warning (I was using Required=False instead of required=False), but I'll definitely be using it again in the future.