Properly retain types of Mapping subclasses

[ofek]

Change Summary

This change prevents Mapping subclasses from always being coerced to dict

Related issue number

Resolves #2323
closes #1536

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 #2325 (651b160) into master (c8883e3) will decrease coverage by 0.13%.
The diff coverage is 100.00%.

@@            Coverage Diff             @@
##           master    #2325      +/-   ##
==========================================
- Coverage   99.90%   99.76%   -0.14%     
==========================================
  Files          25       25              
  Lines        5039     5062      +23     
  Branches     1031     1037       +6     
==========================================
+ Hits         5034     5050      +16     
- Misses          1        5       +4     
- Partials        4        7       +3     

Impacted Files	Coverage Δ
pydantic/fields.py	100.00% <100.00%> (ø)
pydantic/main.py	99.06% <100.00%> (-0.01%)	⬇️
pydantic/schema.py	100.00% <100.00%> (ø)
pydantic/typing.py	95.59% <0.00%> (-4.41%)	⬇️

---

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 c8883e3...8b431df. Read the comment docs.

[ofek]

@PrettyWood I'm not sure how to fix the type checking errors

[ofek]

@PrettyWood The current job failure is due to https://pypi.org/project/cryptography/3.4.1/ emitting a warning in the test suite of FastAPI

[PrettyWood]

Yep can't do anything about that now but it should be handled rapidly on FastAPI side.
In the meantime you can add some tests to keep coverage at 100% :)

[ofek]

Done!

[PrettyWood]

@ofek We should still check the set type and try to coerce when needed.
For example

class M(BaseModel):
    x: Dict[str, int]

should coerce to dict. Right now there is no difference between Dict and Mapping...
I updated https://github.com/PrettyWood/pydantic/pull/70/files if you want

---

Note that I think we should probably have a proper validator for Dict (at least) before Mapping to avoid any performance impact on normal dictionaries (like it's done for Tuple or List, which are treated before Sequence)

[ofek]

Updated based on your branch

[PrettyWood]

I still reckon we should at least have SHAPE_DICT (extra could be SHAPE_DEFAULTDICT, SHAPE_COUNTER...) before SHAPE_MAPPING to have no perf impact.
Or at least if we go with it we should have a benchmark to see the impact

[ofek]

@PrettyWood Addressed. I'm not familiar with the concept of shapes here. Can you explain?

[PrettyWood]

@ofek pydantic is fast because a lot is done in the metaclass. You can have a look at ModelField._type_analysis(), which is run in ModelMetaclass when ModelField.infer() is called.
In _type_analysis a lot is done to set the right attributes to each ModelField (type, shape, validators...)
And the shape is used to call the right default validators to speed up the parsing

[ofek]

I just tried that but many tests are failing now

[ofek]

should I revert the last commit for now?

[ofek]

I'm still trying but I don't understand what's wrong 🙁

[ofek]

@PrettyWood Could you please take a look when you get a chance?

[PrettyWood]

I'll have a look tonight or tomorrow 👍

[ofek]

Thank you, feel free to push directly to my branch!

[PrettyWood]

Hi @ofek
Had a quick look and got all tests working in local PrettyWood#70
I have some guests coming around soon but I think you should be able to work it out without a problem
I think you still need to add some tests to showcase a random mapping getting coerced into dict when Dict[...] is set.
Hope it helps

[ofek]

@PrettyWood Done! Added that test too

I let @samuelcolvin have a look. Thanks!

[samuelcolvin]

otherwise this is looking good.

[ofek]

@samuelcolvin Thanks, addressed!

[ofek]

Done this look alright now?

[ofek]

@samuelcolvin Just let me know if I have to do anything else! FYI, after this is merged and released, every Datadog Agent integration will exclusively use pydantic for user configuration 🙂

[samuelcolvin]

This is great. Thank you.

@samuelcolvin Just let me know if I have to do anything else! FYI, after this is merged and released, every Datadog Agent integration will exclusively use pydantic for user configuration 🙂

I don't see pydantic as a dependency, what am I missing?

[PrettyWood]

@samuelcolvin DataDog/integrations-core#8675

[ofek]

DataDog/integrations-core#8675 is merged and I've started the syncing of every integration: https://github.com/DataDog/integrations-core/pulls 😄

[ofek]

https://datadoghq.dev/integrations-core/meta/config-models/