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

Fix config behavior with plain dataclass #9150

Open
wants to merge 1 commit into
base: main
Choose a base branch
from

Conversation

sydney-runkle
Copy link
Member

@sydney-runkle sydney-runkle commented Apr 2, 2024

Fix #8693

Selected Reviewer: @hramezani

@sydney-runkle sydney-runkle added the relnotes-fix Used for bugfixes. label Apr 2, 2024
Copy link

Deploying pydantic-docs with  Cloudflare Pages  Cloudflare Pages

Latest commit: 90d6324
Status: ✅  Deploy successful!
Preview URL: https://a23b477c.pydantic-docs2.pages.dev
Branch Preview URL: https://fix-ta-with-dataclass-config.pydantic-docs2.pages.dev

View logs

Copy link

codspeed-hq bot commented Apr 2, 2024

CodSpeed Performance Report

Merging #9150 will not alter performance

Comparing fix_ta_with_dataclass_config (90d6324) with main (02ee0f9)

Summary

✅ 13 untouched benchmarks

@sydney-runkle
Copy link
Member Author

Please review

@davidhewitt
Copy link
Contributor

Something seems strange here. We seem to have lots of tests in test_dataclasses.py which use config. Why are those passing?

@sydney-runkle
Copy link
Member Author

sydney-runkle commented Apr 2, 2024

@davidhewitt,

I believe it's because we're only testing validation focused settings, and the __pydantic_config__ is pulled off of a class when schema is generated, see

config: ConfigDict | None = get_attribute_from_bases(typed_dict_cls, '__pydantic_config__')
for example.

But in this case, we also make sure to pass said config to the constructors for SchemaValidator and SchemaSerializer

@sydney-runkle
Copy link
Member Author

That being said, I think we could clean this up a bit, I can create an issue for that.

@davidhewitt
Copy link
Contributor

Right I see. So I don't think this is the right solution, because as you say the config is already pulled from the type into the schema.

I think actually if there's a bug, it's probably in core. See https://github.com/pydantic/pydantic-core/blob/e73b2d1edc5c3c062f1e0b381cda29d6ed6e5901/src/serializers/type_serializers/dataclass.rs#L83, where we do pull the config out of the schema to build a dataclass serializer. But why is that config not applying?

@sydney-runkle
Copy link
Member Author

Good question. 30 mins of digging didn't help me much, but I'd be happy to look into this further in core. I still think that we might want this change though, as the schema serializer is what's used in accordance with the relevant settings here?

@davidhewitt
Copy link
Contributor

I think that in all cases the schema should already contain the config fetched here; in effect doing this seems to me like it plasters over a core bug by sending the same config in twice. So I'd rather we wait to understand the interaction in core before considering this further.

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

Successfully merging this pull request may close these issues.

ser_json_inf_nan not respected when used with an Any list
3 participants