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

[7.x] Refactor support for 'unknown' with location map #544

Merged
merged 2 commits into from
Sep 11, 2020

Conversation

sirosen
Copy link
Collaborator

@sirosen sirosen commented Sep 9, 2020

Based on the suggestion in #514, this is my attempt at putting together what I think is ideal behavior for webargs: we have sensible defaults for unknown based on locations, but it's easy to opt-out by just setting unknown=None or overriding the defaults.
It should be possible to use the query, headers, and similar locations out of the box without issue. By passing unknown=EXCLUDE for these locations, that is achieved.

This introduces a new Parser-class variable, DEFAULT_UNKNOWN_BY_LOCATION, which maps location names to
unknown values. It populates that and DEFAULT_UNKNOWN to pass EXCLUDE in the general case and RAISE for request bodies.

The new behavior is layered and defined with lower precedence than Parser.unknown or the unknown parameter on parse calls. In order to implement this and allow users to opt out of the behavior (i.e. in order to use schema-defined values), the way these values are set is subtly changed. Instead of having a default value of None, parse calls and parser __init__ have a default of unknown="_default". When that value is detected, the various layers of defaults (DEFAULT_UNKNOWN and DEFAULT_UNKNOWN_BY_LOCATION) are applied. However, if a user passes None, that will take effect with the meaning "do not pass a value for unknown".

The changelog has been updated significantly in order to handle this and a new section of the advanced usage docs covers the behavior.

If this seems good, I'd like to switch the branches around so that we have a 6.x maintenance branch and 7.0 is in progress in dev, as @lafrech suggested. I think we could release this as 7.0.0b1, if that sounds acceptable. And then other work (like dropping 3.5) can happen during the 7.0 beta period.

This introduces a new Parser-class variable,
`DEFAULT_UNKNOWN_BY_LOCATION`, which maps location names to
`unknown` values. It populates that and `DEFAULT_UNKNOWN` to pass
`EXCLUDE` in the general case and `RAISE` for request bodies.

The new behavior is layered and defined with lower precedence than
`Parser.unknown` or the `unknown` parameter on parse calls. In
order to implement this and allow users to opt *out* of the behavior
(i.e. in order to use schema-defined values), the way these values are
set is subtly changed. Instead of having a default value of `None`,
parse calls and parser __init__ have a default of
`unknown="_default"`. When that value is detected, the various layers
of defaults (DEFAULT_UNKNOWN and DEFAULT_UNKNOWN_BY_LOCATION) are
applied. However, if a user passes `None`, that will take effect with
the meaning "do not pass a value for `unknown`".

For parsers which define additional locations, they extend the base
DEFAULT_UNKNOWN_BY_LOCATION as appropriate.

The changelog has been updated significantly in order to handle this
and a new section of the advanced usage docs covers the behavior.
@sirosen sirosen requested a review from lafrech September 9, 2020 19:52
Copy link
Member

@lafrech lafrech left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM.

Still sounds a bit complicated, but I don't see how we could make things simpler and at least the defaults are sensible.

@sirosen
Copy link
Collaborator Author

sirosen commented Sep 9, 2020

The only idea I've had for making this simpler which really appeals to me at all is that we could get rid of Parser.DEFAULT_UNKNOWN.
That would mean that DEFAULT_UNKNOWN_BY_LOCATION is the only way to specify class-level values. Maybe that gets a bit tedious, but it does make the interface smaller and easier to explain.

I also thought about getting rid of Parser.unknown, but I'm not sure how I feel about that idea.

I'll leave this open for today, since I don't have time left in the day to handle doing a 7.0.0b1 release and such, as I said I would. But I plan to merge it and do the branch management tomorrow. We can look at removing DEFAULT_UNKNOWN after that, if you're interested in that approach.

@lafrech
Copy link
Member

lafrech commented Sep 9, 2020

The only idea I've had for making this simpler which really appeals to me at all is that we could get rid of Parser.DEFAULT_UNKNOWN.
That would mean that DEFAULT_UNKNOWN_BY_LOCATION is the only way to specify class-level values. Maybe that gets a bit tedious, but it does make the interface smaller and easier to explain.

Good plan

Doing this before releasing a beta would avoid mentioning DEFAULT_UNKNOWN in the changelog, so the user would go straight to DEFAULT_UNKNOWN_BY_LOCATION in one step.

Removing this value removes a layer of defaults and potential
ambiguity from the interfaces. Docs and changelog update to remove all
references to DEFAULT_UNKNOWN.
@sirosen
Copy link
Collaborator Author

sirosen commented Sep 10, 2020

I removed DEFAULT_UNKNOWN from this branch last night, but I forgot to ask: are you comfortable with me releasing this after merging? I think it's pretty good in this state.

@lafrech
Copy link
Member

lafrech commented Sep 11, 2020

Great. Thanks for taking care of all this.

I suppose you mean release a beta. Sure, go ahead.

There are still a few things in the 7.0 milestone before we ship stable.

@sirosen
Copy link
Collaborator Author

sirosen commented Sep 11, 2020

Yeah, I meant a beta. Sorry if my phrasing was confusing. I just didn't want to put anything new out -- even a beta -- without giving you a chance to weigh in.
I'll be doing this now.

@sirosen sirosen merged commit 4785d3e into marshmallow-code:7.0-dev Sep 11, 2020
@sirosen sirosen deleted the location-default-unknown branch September 11, 2020 15:38
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants