Pass all args to schemas, but default to unknown=EXCLUDE (work in progress) #410
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
If Parser.pass_all_args is set (it defaults to false for backwards compatibility), then argument parsing becomes a two step process. First, args are parsed based on the schema fields, as today. Then, second, they are all treated as marshmallow.fields.Raw , parsed by webargs , and then passed to the schema for validation. If Parser.pass_all_args is set on marshmallow versions < 3, it is a ValueError. In order to collect arguments to pass to the schema, adds a new function required on all parsers, get_args_by_location , which collects all argument names, typically dict keys, categorized by location name. This is only implemented here in the flask parser, as a proof of concept.
This commit is an intermediate step meant to show the distinction between a standalone pass_all_args flag and *always* passing all args but allowing the user to specify a value for `unknown` to pass to `Schema.load()`. In this version, the webargs.Parser object is able to use `Schema.load(unknown=EXCLUDE)` on marshmallow 3 as the default for backwards compatibility. Users can pass `unknown=None` to specify that the Parser should not pass a value and the schema default should be used. In this way, the backwards-incompatible change to be made in a future webargs release will be to change the default for Parser.unknown from EXCLUDE to None (or to eliminate it, and make the `None` behavior the only option).
sirosen
force-pushed
the
schema-sees-extra-args
branch
from
8a85d5e
to
52bab64
Compare
Add implementations of this method for all existing parsers. For the most part, the pattern is quite simple and nearly identical. Usually, JSON data-loading needs to be moved into a helper for use both in get_arbs_by_location and parse_json. The asyncparser is particularly significant, as it reimplements much of the core parser in order to make get_args_by_location an async (awaitable) method.
|
I got impatient and just added implementations of |
|
Thanks for the PR. Apologies for the delay.. I'll need to carve out time to give this a proper review. I've not forgotten about it! |
|
closing for now, since you're carrying on with #420 |
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
This is a work in progress / demonstration of a solution for #267 .
It's based on the suggestion in that thread (big thanks to @toonalbers for that!).
I did two versions of this and have kept it in separate commits so that both can be reviewed if desirable. I prefer the latter form and will squash the work if it's going to be accepted in a form even remotely like this.
In the first version, if Parser.pass_all_args is set (it defaults to false for backwards compatibility), then argument parsing becomes a two step process.
First, args are parsed based on the schema fields, as today. Then, second, they are all treated as
marshmallow.fields.Raw, parsed by webargs, and the result of that parsing is passed to the schema for validation.If Parser.pass_all_args is set on marshmallow versions < 3, it is a ValueError.
As a refinement on this, I successfully swapped out Parser.pass_all_args for Parser.unknown, which sets the value to pass for
Schema.load(unknown=...). On marshmallow v3, it defaults toEXCLUDE, which makes this backwards-compatible. Callers may passNoneto send no value and obey the schema default. All arguments are always passed to the schema, and we're just using the veryunknown=...behavior we're trying to expose to maintain backwards compatibility.(Note: this is arguable backwards incompatible because certain usages with strict schema validators were passing and will start to fail.)
In both versions, in order to collect arguments to pass to the schema, this adds a new function required on all parsers,
get_args_by_location, which collects all argument names, typically dict keys, categorized by location name.This is only implemented here in the flask parser, as a proof of concept.
There's a lot more work that I would need to do for this to be ready to merge. Namely:
get_args_by_locationHowever, I don't want to put in the labor to do the kind of testing I think this change needs without gettings eyes on this work from a webargs maintainer and confirmation that this whole approach is okay. If it is, I'll happily put together the necessary work to make all of the parsers handle this and do a lot more testing.