Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Should parse allow arguments to be passed through to schema.load and/or dict2schema? #480

Closed
sirosen opened this issue Feb 27, 2020 · 7 comments

Comments

@sirosen
Copy link
Collaborator

sirosen commented Feb 27, 2020

I'm looking at the features of marshmallow and Parser.parse (and, by extension, use_args too) and thinking about whether or not this will expose more marshmallow features while not adding too much code (or even, my real goal, simplifying things?).

I have two basic ideas I'm toying with:

  1. the validate argument to parse is basically the same as a schema with validates_schema methods
    • but the validate errors have slightly different messages -- they don't show the schema name (is that good for generated schemas? can a validates_schema method do that?)
    • can these be unified, maybe deprecate validate=..., and remove in 7.0 or later?
  2. there's no way to tell parse to pass unknown to schema.load -- it would be nice to be able to say parse(..., unknown=ma.EXCLUDE)
    • should this just be a container for schema.load arguments, like parse(..., load_params=dict(unknown=ma.INCLUDE, partial=True))? It's a less nice interface to use but more flexible
    • you can handle this today by making different schema objects with your desired behaviors
    • maybe this is doable along with (1) for the dict2schema case by setting attributes of Meta?

This is really low priority. You can do everything today in 6 by using full schema definitions. Supporting unknown=... or load_params=... is really just a convenience feature.

@lafrech
Copy link
Member

lafrech commented Feb 27, 2020

1/ I agree validate is not that useful now that validates_schema are called from parse.

This code from the docs would have to be reworked by creating a Schema but would it be that bad?

from webargs import fields
from webargs.flaskparser import parser

argmap = {"age": fields.Int(), "years_employed": fields.Int()}

# ...
result = parser.parse(
    argmap, validate=lambda args: args["years_employed"] < args["age"]
)

It's only a pity for people using dicts rather than Schemas, with a validation involving several fields, and simple enough to be expressed in a lambda.

If the validation must be expressed in a function, the code is not that compact already, and creating a Schema doesn't make it much worse IMHO.

2/ Maybe. There's a trade-off between simplicity here in webargs code and API, and concision in client code. I'm defining many Schemas in my app code and it is not really an issue. It is very declarative and simple. In practice, I don't have much redundancy, thanks to inheritance. YMMV.

Wondering how this would fit with auto-documentation à la apispec.

@sirosen
Copy link
Collaborator Author

sirosen commented Feb 28, 2020

It's only a pity for people using dicts rather than Schemas, with a validation involving several fields, and simple enough to be expressed in a lambda.

I'm not sure, but it sounds like you might be suggesting simply dropping validate=... in a future release, since it's only one narrow case that's hurt by this? I'd be okay with that.

We could change the docs around to use more explicit schema classes in the examples, and shift to preferring schemas over dicts throughout the documentation.


Let's maybe just drop (2) from discussion -- it's only really relevant for the "dict schema" usage, and I think it's fine to just say that that usage is less flexible and "you should use real schemas if you want more features".

Wondering how this would fit with auto-documentation à la apispec.

I haven't used apispec, but I think this is another reason not to bend over backwards to handle "dict schemas" better. Users can always (on marshmallow3) just use myschema = ma.Schema.from_dict(...)() themselves, if they're determined not to use the class keyword! 😝

@reinhard-mueller
Copy link

Sorry to jump into this discussion and especially into the topic (2), but I think that the single additional parameter unknown added to parse, use_args, and use_kwargs would make it possible to continue using the "dict schema" at least in the cases when it was possible in 5.x.

On the other hand I see the argument "if you want to do something non-trivial, just use a full schema definition, it's not bad / not an issue..." again and again, and I see the point that

class SubscribeSchema(ma.Schema):
  name = fields.String(required=True)
  email = fields.Email(required=True)

is neither much more work to write nor less intuitive or understandable than

subscribe_params = {
  name = fields.String(required=True)
  email = fields.Email(required=True)
}

so if we more and more reduce the number of cases when the second variant works, why not depreciate it and move towards a consistent usage of the first variant?

@sirosen
Copy link
Collaborator Author

sirosen commented Apr 28, 2020

I ran into another case where passing unknown=... with dict2schema would be useful at work just now, and on my way back to note it, I saw #500, which seems to be another situation where it's necessary for the dict2schema version to be usable.

if we more and more reduce the number of cases when the second variant works, why not depreciate it and move towards a consistent usage of the first variant?

I think this is one of our options. It's either that, or enhance dict-schema support to bring it closer to parity with "real" schemas.

If we don't at least let users specify unknown=EXCLUDE with a dict specification of arguments, then a lot of the trivial cases -- where you might want to just use the dict -- aren't going to work.

For example,

@use_args({"myheader": fields.String(data_key="X-My-Header")}, location="header")

will fail, but I'd rather the rewrite be

@use_args({"myheader": fields.String(data_key="X-My-Header")}, location="header", unknown=EXCLUDE)

than something where the user has to stop using the dict -- why let them use the dict in the first place, if that's going to be the case?

I'm going to try to carve out some time for webargs work soon. I can at least look at how well it would go to add unknown=... as an argument (which gets passed to schema.load if set).

@zenglanmu
Copy link

zenglanmu commented Apr 29, 2020

Maybe it is better to set unknown=EXCLUDE as the default behaviour. As in some cases, args are not handled by web_args, rather in other libraries. Such as page query args including cur_page, per_page etc. Explicately set those parameters in every query schemas wouldn't be an elegant solution. As a results, current implication would cause many broken changes from version 5 to version 6.
I just made a pull request attempting to add the unknown parameter in 'use_args' function, check #506

@lafrech
Copy link
Member

lafrech commented Apr 29, 2020

I opened #507 to discuss unknown default value.

@sirosen
Copy link
Collaborator Author

sirosen commented Sep 11, 2020

We have #507 which tracks the unknown behavior -- and we're working on actually implementing that -- so I'm going to close this.
Especially given that webargs 7 is going to have per-location defaults for unknown, I don't think the idea of passing through arbitrary arguments is very compelling.

@sirosen sirosen closed this as completed Sep 11, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

4 participants