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 support for mapping types as custom root #958

Merged
merged 8 commits into from Nov 25, 2019

Conversation

@dmontagu
Copy link
Collaborator

dmontagu commented Nov 2, 2019

Change Summary

Modifies parse_obj and MetaModel to allow mapping types with a custom root.

I needed this to address some of the feedback on #934

Related issue number

Closes #908

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

This comment has been minimized.

Copy link

codecov bot commented Nov 2, 2019

Codecov Report

Merging #958 into master will not change coverage.
The diff coverage is n/a.

@@          Coverage Diff          @@
##           master   #958   +/-   ##
=====================================
  Coverage     100%   100%           
=====================================
  Files          19     19           
  Lines        3299   3293    -6     
  Branches      651    651           
=====================================
- Hits         3299   3293    -6
Impacted Files Coverage Δ
pydantic/fields.py 100% <0%> (ø) ⬆️
pydantic/typing.py 100% <0%> (ø) ⬆️
pydantic/utils.py 100% <0%> (ø) ⬆️

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 043186c...c6b4f34. Read the comment docs.

@dmontagu dmontagu force-pushed the dmontagu:custom-root-mapping branch from 704a83e to c5f52ad Nov 2, 2019
pydantic/main.py Outdated Show resolved Hide resolved
@dmontagu dmontagu force-pushed the dmontagu:custom-root-mapping branch from c5f52ad to 4e0daa4 Nov 11, 2019
docs/usage/models.md Outdated Show resolved Hide resolved
@dmontagu dmontagu mentioned this pull request Nov 11, 2019
4 of 4 tasks complete
@dmontagu dmontagu requested a review from samuelcolvin Nov 12, 2019
Copy link
Owner

samuelcolvin left a comment

otherwise LGTM.

docs/usage/models.md Outdated Show resolved Hide resolved
docs/usage/models.md Outdated Show resolved Hide resolved
@samuelcolvin

This comment has been minimized.

Copy link
Owner

samuelcolvin commented Nov 13, 2019

On second thoughts I think we should have both tests and documentation describing the peculiarities of root validation if you provide {'__root__': 'x'} vs {'__root__': 'x', '__boot__': 'y'} or {'__foot__': 'x'} etc.

@dmontagu

This comment has been minimized.

Copy link
Collaborator Author

dmontagu commented Nov 16, 2019

On second thoughts I think we should have both tests and documentation describing the peculiarities of root validation if you provide {'__root__': 'x'} vs {'__root__': 'x', '__boot__': 'y'} or {'__foot__': 'x'} etc.

I thought about this more while attempting to address the feedback, and I think there is a problem here that we need to address: if the __root__ type is Dict[str, str] (or has other value type), and an external input is provided with only a single key __root__, you may get unexpected behavior. In other words, with the current implementation, it's harder than it should be to prevent problematic external input.

Because of this, I think when the root type is a mapping, we should not have different behavior depending on the presence of the __root__ key: we should either always add it prior to parsing, or never add it prior to parsing.

Proposal:

  • For mapping-type custom root models, always place the data provided to parse_obj inside a new dict with the key __root__.
  • In v2, make this the behavior for all custom root models.
    • Possibly also add a .dump_obj() method that would extract the __root__ key's value from .dict() (following #1001), simplifying the implementation of dump-reparse cycles where necessary.

Discussion / Justification:

If we had no backwards compatibility requirements, I currently think it would be best to change the behavior so that we always assumed there was no __root__ key in the input to parse_obj, and always inject it if the model has a custom root. So you would always use __root__ in the __init__, and never use it in parse_obj. Let me know if I'm missing something, but I can't think of any functionality this would fundamentally limit, besides breaking a dump-reparse cycle (i.e., Model.parse_obj(model.dict())).

However, given the backwards compatibility requirement, we obviously need to continue supporting both approaches for non-mapping root models. But I think since mapping root-models aren't currently supported, we can be more opinionated here (and just document the choice).


  • Option 1: Never place the data provided to parse_obj inside a new dict with the key __root__ (for a mapping-type custom root model). (So you'd have to manually create the input with __root__ as a key.)

    • Pro: We'd have to do it this way if we wanted to be able to call parse_obj on the output of instance.dict()
    • Con: Seems less ergonomic since you have to special case calls to parse_obj when dealing with a __root__ type
  • Option 2: Always place the data provided to parse_obj inside a new dict with the key __root__ (for a mapping-type custom root model).

    • Pro: You can provide inputs directly to parse_obj whether there is a custom root or not.
    • Con: Can't call parse_obj on instance.dict()

I personally would prefer "Option 2" above, since the handling a dump-reparse cycle seems like a substantially rarer requirement than parsing in the first place, so I'd rather prioritize having a clean API for parse_obj.

The behavior would also be easy to change if desired by users by just overriding the relevant methods (i.e., parse_obj, dict, etc.).

We could also (eventually) address the lack of symmetry by introducing a dump_obj public method that uses the same logic as .json() for handling custom root models, which would produce output that could be passed back to parse_obj to produce the same object. (This would probably depend on / be related to #1001)

This could also be addressed by adding an unpack_custom_roots kwarg (or similar) to .dict().

@samuelcolvin

This comment has been minimized.

Copy link
Owner

samuelcolvin commented Nov 18, 2019

I think right now we should do the thing which maximises backwards comparability, that is:

if cls.__custom_root_type__:
    obj = {ROOT_KEY: obj}

This would mean Model.parse_obj(model.dict()) wouldn't work, but Model(model.dict()) would work. I think this is fine.

I'm happy to omit the "and not (isinstance(obj, dict) and obj.keys() == {'__root__'})" magic for clarity. We should just be clear about this in the documentation.

I think this is the same as your option 2 above, so we're agreeing.

@dmontagu

This comment has been minimized.

Copy link
Collaborator Author

dmontagu commented Nov 24, 2019

I'm happy to omit the "and not (isinstance(obj, dict) and obj.keys() == {'root'})" magic for clarity. We should just be clear about this in the documentation.

@samuelcolvin To be clear, is that a v2 change? Otherwise, it could break existing code that relies on this behavior, right?

I'm currently planning to use the following check:

        if cls.__custom_root_type__ and (
            not (isinstance(obj, dict) and obj.keys() == {'__root__'})
            or cls.__fields__["__root__"].shape == SHAPE_MAPPING
        ):
            obj = {ROOT_KEY: obj}

I could add a deprecation warning for the case where __root__ is provided as a key to parse_obj?

@dmontagu dmontagu force-pushed the dmontagu:custom-root-mapping branch from de2d243 to 781b4b4 Nov 24, 2019
@samuelcolvin

This comment has been minimized.

Copy link
Owner

samuelcolvin commented Nov 25, 2019

Sorry, I was being dumb. You check looks good.

We can change/reconsider before v2.

David Montague added 2 commits Nov 25, 2019
David Montague
David Montague
@dmontagu

This comment has been minimized.

Copy link
Collaborator Author

dmontagu commented Nov 25, 2019

On second thoughts I think we should have both tests and documentation describing the peculiarities of root validation if you provide {'__root__': 'x'} vs {'__root__': 'x', '__boot__': 'y'} or {'__foot__': 'x'} etc.

@samuelcolvin I added some tests and docs discussing this. Let me know if you think anything is wrong / missing.

Otherwise, I think this is ready to merge. (Assuming the cython builds go through...)

Copy link
Owner

samuelcolvin left a comment

otherwise lgtm.

pydantic/main.py Show resolved Hide resolved
tests/test_main.py Show resolved Hide resolved
David Montague
tests/test_main.py Outdated Show resolved Hide resolved
David Montague
@dmontagu

This comment has been minimized.

Copy link
Collaborator Author

dmontagu commented Nov 25, 2019

I need to go to bed now, feel free to finish this up (and #934 if desired) if you notice any remaining minor issues, otherwise I'll finish it some time tomorrow.

@samuelcolvin

This comment has been minimized.

Copy link
Owner

samuelcolvin commented Nov 25, 2019

great, thank you so much.

@samuelcolvin samuelcolvin merged commit 62bc930 into samuelcolvin:master Nov 25, 2019
11 checks passed
11 checks passed
Header rules No header rules processed
Details
Pages changed 6 new files uploaded
Details
Redirect rules No redirect rules processed
Details
Mixed content No mixed content detected
Details
codecov/project 100% remains the same compared to 043186c
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
deploy/netlify Deploy preview ready!
Details
samuelcolvin.pydantic #20191125.13 succeeded
Details
samuelcolvin.pydantic (Job Python36) Job Python36 succeeded
Details
samuelcolvin.pydantic (Job Python37) Job Python37 succeeded
Details
samuelcolvin.pydantic (Job Python38) Job Python38 succeeded
Details
MrMrRobat added a commit to MrMrRobat/pydantic that referenced this pull request Nov 28, 2019
* Add support for mapping types as custom root

* Incorporate feedback

* Add changes

* Incorporate feedback

* Add docs and tests

* Fix linting issue

* Incorporate more feedback

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