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

improvements to 'from_orm' to work better with root validators #822

Merged
merged 4 commits into from Sep 30, 2019

Conversation

@samuelcolvin
Copy link
Owner

samuelcolvin commented Sep 20, 2019

Fix #821

Change Summary

  • add getter_dict to Config
  • modify GetterDict to be a proper Mapping object and thus easier to work with
  • add tests demonstrating usage of getter_dict and root_validator(pre=True) for ORM decomposition

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>.rst file added describing change
    (see changes/README.md for details)
@codecov

This comment has been minimized.

Copy link

codecov bot commented Sep 20, 2019

Codecov Report

Merging #822 into root-validators will decrease coverage by 0.06%.
The diff coverage is 94.44%.

@@                 Coverage Diff                 @@
##           root-validators     #822      +/-   ##
===================================================
- Coverage              100%   99.93%   -0.07%     
===================================================
  Files                   17       17              
  Lines                 3066     3095      +29     
  Branches               592      598       +6     
===================================================
+ Hits                  3066     3093      +27     
- Misses                   0        2       +2
Impacted Files Coverage Δ
pydantic/main.py 100% <100%> (ø) ⬆️
pydantic/utils.py 98.18% <93.54%> (-1.82%) ⬇️

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 9644ec8...ad147af. Read the comment docs.

@samuelcolvin samuelcolvin force-pushed the from-orm-improvements branch from 9d3ef5a to 7f1b4e5 Sep 20, 2019
@samuelcolvin samuelcolvin added this to the Version 1 milestone Sep 20, 2019
@samuelcolvin samuelcolvin referenced this pull request Sep 20, 2019
3 of 5 tasks complete
Copy link
Collaborator

tiangolo left a comment

LGTM 🎉

@dmontagu

This comment has been minimized.

Copy link
Collaborator

dmontagu commented Sep 20, 2019

This looks great! Thanks for implementing this!

@@ -93,22 +93,63 @@ def almost_equal_floats(value_1: float, value_2: float, *, delta: float = 1e-8)
class GetterDict:

This comment has been minimized.

Copy link
@dmontagu

dmontagu Sep 20, 2019

Collaborator

I don't see any way to set items (in case you wanted to replace the value of an existing key). But I'm assuming the point is that if you want that capability, you should subclass the GetterDictand put it in the model config. Works for me!

This comment has been minimized.

Copy link
@dmontagu

dmontagu Sep 20, 2019

Collaborator

Also, I think it's fine that it doesn't inherit from Mapping -- it may make it easier to test in the root_validator whether you got there by way of from_orm or __init__ kwargs.

This comment has been minimized.

Copy link
@samuelcolvin

samuelcolvin Sep 23, 2019

Author Owner

Agreed, but also cython failed if it inherited from Mapping so no option.

Wee could implement setting items but it would involve another hidden dict that took precedence over the object and checking that every time.

I don't mind which, would you prefer better performance or more flexibility?

This comment has been minimized.

Copy link
@dmontagu

dmontagu Sep 23, 2019

Collaborator

Let me do some perf tests and think about this (if anyone else has an opinion I’d be interested)

This comment has been minimized.

Copy link
@dmontagu

dmontagu Sep 25, 2019

Collaborator

I thought about it a little more (but haven't perf tested). I'm hesitant to make any sacrifices to the perf of from_orm since I think it is an extremely commonly used function in fastapi, and in the vast majority of cases supporting setting isn't necessary.

I could imagine shipping a GetterSetterDict that is like GetterDict but supports setting, but then requiring an override (similar to what I suggested in this comment) to actually use it with from_orm.

There might be an easier way to accomplish overriding (e.g. via config, but I don't want to pollute the config either..), but I would prefer an implementation that adds zero cost if not making use of the setting capability (like the override-based approach).

This comment has been minimized.

Copy link
@dmontagu

dmontagu Sep 26, 2019

Collaborator

Oh, I forgot you had already implemented the getter_dict via config. Yeah I think this is great, we should just also include a GetterSetterDict in the library for people to use if they need to override things. Would make it really easy to answer related issues.

@bharling

This comment has been minimized.

Copy link

bharling commented Sep 25, 2019

This looks great, I think it'd definitely solve my current issue related to #380 - would love to see it merged.

@bharling

This comment has been minimized.

Copy link

bharling commented Sep 25, 2019

I just wanted to verify this would fix the issue I'm encountering and it does seem to do so - I've added a test case to a fork to demonstrate, you're welcome to include it if you think it's any use:

bharling@09cd0fd

Was mainly just to check that the _decompose_class function is called properly in derived classes and the correct getter_dict is returned.

* added test for derived classes using custom getter_dict config

* fix linting

* fix formatting
@samuelcolvin samuelcolvin merged commit bd150f6 into root-validators Sep 30, 2019
11 checks passed
11 checks passed
Header rules No header rules processed
Details
Pages changed All files already uploaded
Details
Redirect rules No redirect rules processed
Details
Mixed content No mixed content detected
Details
codecov/project 99.93% (-0.07%) compared to 9644ec8
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
continuous-integration/travis-ci/push The Travis CI build passed
Details
deploy/netlify Deploy preview ready!
Details
samuelcolvin.pydantic Build #20190930.20 succeeded
Details
samuelcolvin.pydantic (Job Python36) Job Python36 succeeded
Details
samuelcolvin.pydantic (Job Python37) Job Python37 succeeded
Details
@samuelcolvin samuelcolvin deleted the from-orm-improvements branch Sep 30, 2019
samuelcolvin added a commit that referenced this pull request Sep 30, 2019
* root validators and rename __obj__ -> __root__

* implement root validation

* tweak Validator

* dataclass and generic tests, docs

* repeat and signature checks

* fix inheritance

* tweaks tests and var names

* improvements to 'from_orm' to work better with root validators (#822)

* improvements to 'from_orm' to work better with root validators

* cython compatibility and tweaks

* tweak config order

* added test for derived classes using custom getter_dict config (#833)

* added test for derived classes using custom getter_dict config

* fix linting

* fix formatting

* cleanup
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
4 participants
You can’t perform that action at this time.