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

Add exclude as field parameter #2231

Merged
merged 7 commits into from May 1, 2021

Conversation

daviskirk
Copy link
Contributor

@daviskirk daviskirk commented Jan 1, 2021

Change Summary

  • Add "exclude" as a field parameter so that it can be configured using model config instead of purely at .dict / .json export time.
  • Main reference used is this comment: add exclude to Config #660 (comment)

Related issue number

Checklist

  • Unit tests for the changes exist
  • Tests pass on CI and coverage remains at 100%
  • Documentation reflects the changes where applicable
  • changes/<pull request or issue id>-<github username>.md file added describing change
    (see changes/README.md for details)

@codecov
Copy link

codecov bot commented Jan 1, 2021

Codecov Report

Merging #2231 (508209d) into master (619ff26) will not change coverage.
The diff coverage is 100.00%.

❗ Current head 508209d differs from pull request most recent head be897bc. Consider uploading reports for the commit be897bc to get more accurate results

@@            Coverage Diff            @@
##            master     #2231   +/-   ##
=========================================
  Coverage   100.00%   100.00%           
=========================================
  Files           25        25           
  Lines         5109      5120   +11     
  Branches      1050      1045    -5     
=========================================
+ Hits          5109      5120   +11     
Impacted Files Coverage Δ
pydantic/fields.py 100.00% <100.00%> (ø)
pydantic/main.py 100.00% <100.00%> (ø)
pydantic/utils.py 100.00% <100.00%> (ø)

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 619ff26...be897bc. Read the comment docs.

@samuelcolvin
Copy link
Member

No changes here, just the change description ATM.

tests/test_main.py Outdated Show resolved Hide resolved
Copy link
Contributor

@Bobronium Bobronium left a comment

Choose a reason for hiding this comment

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

Happy to see it's finally getting implemented, thank you!

pydantic/main.py Outdated Show resolved Hide resolved
pydantic/utils.py Outdated Show resolved Hide resolved
pydantic/utils.py Outdated Show resolved Hide resolved
@daviskirk daviskirk force-pushed the 660-add-exclude-to-config branch 2 times, most recently from af2ea07 to 652bcaf Compare February 19, 2021 15:28
@daviskirk daviskirk marked this pull request as ready for review February 19, 2021 16:28
@daviskirk
Copy link
Contributor Author

daviskirk commented Feb 19, 2021

Implementation wise, this is just about the way I'd like it.
Since there are probably multiple opinions on how the logic and strategy of merging the various parts should work, it would be great to have some other eyes on this just to make sure I didn't make any stupid assumptions.

A few key points here:

  • I consolidated a lot of the "merging" logic from ValueItems. Since I was having to merge in many places the older logic (with 2 mutliple merge functions depending on the keys and types) were to much for my brain to process. If this was done for performance reasons and has to stay, I can try and reimplement it. The main changes here are:
    • Only one merging method (ValueItems.merge)
    • During processing only use dictionaries instead of checking everywhere if the values are dictionaries or sets (or combinations).
  • Merge config settings, field level settings and explicit settings. The order of preference here is:
    • Explicit exclude/include on dict/json.
    • Field level settings ala ``Field(..., exclude=..., include=...)```
    • Config level settings via "fields": fields = {"a": {"exclude": ..., "include": ...}}
  • Merging logic:
    • Merging of exclude settings happens as a "union" so overriding a field level exclude of {"a"} with dict level exclude of {"b"} will exclude: {"a", "b"}
    • Merging of include settings happens as an "intersection" so overriding a field level include of {"a", "b"} with a dict level include of {"b"} will include: {"b"}.
    • These rules are the most consistent with the way fields are handled on the objects (when I include, ONLY the fields I set in the "include" setting are included, when I exclude everything EXCEPT the fields I set are included). It also should help with not having users to "accidentally" expose a field that was supposed to be kept unexported (for example having set a "mysecretfield" field to "exclude").

In summary: reviews are very welcome

@Bobronium
Copy link
Contributor

  • Merging of include settings happens as an "intersection" so overriding a field level include of {"a", "b"} with a dict level include of {"b"} will include: {"a"}.

I am probably missing something, but why {"a", "b"} | {"b"} should result in {"a"} rather than {"b"}?

@daviskirk
Copy link
Contributor Author

I am probably missing something, but why {"a", "b"} | {"b"} should result in {"a"} rather than {"b"}?

nope, not missing anything. Typo on my part, sorry. I updated the comment.

docs/examples/exporting_models_exclude4.py Outdated Show resolved Hide resolved
pydantic/utils.py Show resolved Hide resolved
pydantic/utils.py Outdated Show resolved Hide resolved
pydantic/utils.py Outdated Show resolved Hide resolved
tests/conftest.py Outdated Show resolved Hide resolved
pydantic/utils.py Outdated Show resolved Hide resolved
- Add "exclude" / "include" as a field parameter so that it can be
  configured using model config (or fields) instead of purely at
  `.dict` / `.json` export time.
- Unify merging logic of advanced include/exclude fields
- Add tests for merging logic and field/config exclude/include params
- Closes pydantic#660
* Increase test coverage
* Remove (now) redundant type checks in Model._iter: New
  exclusion/inclusion algorithms guarantee that no sets are passed further down.
Running benchmarks this vs. master is at:

this: pydantic best=33.225μs/iter avg=33.940μs/iter stdev=1.120μs/iter version=1.7.3
master: pydantic best=32.901μs/iter avg=33.276μs/iter stdev=0.242μs/iter version=1.7.3
* Fix/simplify type annotations
* Allow both ``True`` and ``Ellipsis`` to be used to indicate full field
  exclusion
* Reenable hypothesis plugin (removed by mistake)
* Update advanced include/include docs to use ``True`` instead of ``...``
This way, the model field object does not need to concern itself with
dealing with field into specific fields.
(Same was done for alias in a previous commit).
@Nemesis7
Copy link

@samuelcolvin when can we expect this functionality to be released?

@PrettyWood
Copy link
Member

@daviskirk Do you think it could be possible to also set a callable as a value?
For example:

class User(BaseModel):
    name: str
    friends: list[str] | None = Field(None, exclude=lambda x: x is None)

assert User(name="Alone", friends=None).json() == '{"name": "Alone"}'
assert User(name="Popular", friends=["Bob", "Alice"]).json() == '{"name": "Popular", "friends": ["Bob", "Alice"]}'

WDYT?

@daviskirk
Copy link
Contributor Author

@daviskirk Do you think it could be possible to also set a callable as a value?

I think that would be possible... I do think that it would open up a pretty large field of possibilities for users to "use"/"misuse" that I'm not sure I can predict right now.

Given all the larger changes and v2 coming along I'm not sure if we want to commit to such a broad feature without lot's of users actually really wanting it.

For example, I can behavior the behavior of "exclude" (as implemented here already with being able to drill down into nested fields) being popular enough that we might want to expand it to other fields like
I can imagine that if we allow something like this there will quickly be people that would want the callable to take the entire object as an argument as well (similar to how validators work with the optional "values"), or using this in weird ways using "nested" values as flags and stuff like that.

Not that I'm against this... I think it would be a pretty fancy feature, just trying to look out for you and samuel who might be stuck supporting it ;)

@PrettyWood
Copy link
Member

You're absolutely right. I just shared a recent need and got away easily with custom dict. I didn't have the nesting in mind when I shared the idea but it will surely be expected if a callable support is added. Let's not do that now 😬
The feature is already super nice and complete 👍
Thanks for the detailed answer

Copy link
Member

@samuelcolvin samuelcolvin left a comment

Choose a reason for hiding this comment

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

otherwise this looks great.

docs/usage/exporting_models.md Outdated Show resolved Hide resolved
@samuelcolvin samuelcolvin merged commit db697cc into pydantic:master May 1, 2021
@samuelcolvin
Copy link
Member

This is awesome. Thank you so much @daviskirk 🎉

PrettyWood added a commit to PrettyWood/pydantic that referenced this pull request Sep 11, 2021
…all fields

Small regression in pydantic#2231.
The shallow copy done with `Config.copy_on_model_validation = True` (default behaviour)
was using excluded / included fields when it should just copy everything

closes pydantic#3195
PrettyWood added a commit to PrettyWood/pydantic that referenced this pull request Sep 11, 2021
…all fields

Small regression in pydantic#2231.
The shallow copy done with `Config.copy_on_model_validation = True` (default behaviour)
was using excluded / included fields when it should just copy everything

closes pydantic#3195
@rickerp
Copy link

rickerp commented Dec 14, 2021

Any release predictions on this?

samuelcolvin pushed a commit that referenced this pull request Dec 19, 2021
…all fields (#3201)

Small regression in #2231.
The shallow copy done with `Config.copy_on_model_validation = True` (default behaviour)
was using excluded / included fields when it should just copy everything

closes #3195
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

add exclude to Config
6 participants