Add support for mapping types as custom root

[dmontagu]

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]

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.

[samuelcolvin]

otherwise LGTM.

[samuelcolvin]

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]

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 Move all public methods on a model into a single namespace #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]

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]

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?

[samuelcolvin]

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

We can change/reconsider before v2.

[dmontagu]

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...)

[samuelcolvin]

otherwise lgtm.

[dmontagu]

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]

great, thank you so much.