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

Allow case where __pydantic_extra__ is None, even if extra='allow' #1236

Merged
merged 4 commits into from
Mar 21, 2024

Conversation

sydney-runkle
Copy link
Member

@sydney-runkle sydney-runkle commented Mar 19, 2024

Fix pydantic/pydantic#9043. The issue reported has been simplified in the example below:

For the following setup:

from pydantic import BaseModel, ConfigDict, Field, TypeAdapter


class Base(BaseModel):
    a: int = Field(default=1)
    
    model_config = ConfigDict(extra='allow')


class Sub(Base):
    b: str = Field(default="2")

    model_config = ConfigDict(extra='forbid')


sub = Sub()
ta = TypeAdapter(Base)

Before this fix:

result = ta.dump_python(sub)
"""
/Users/programming/pydantic_work/pydantic/pydantic/type_adapter.py:356: UserWarning: Pydantic serializer warnings:
  Expected `general-fields` but got `tuple` - serialized value may not be as expected
  return self.serializer.to_python(
"""
print(result)
#> ({'a': 1, 'b': '2'}, None)

After this fix:

result = ta.dump_python(sub)
print(result)
#> {'a': 1}

Selected Reviewer: @adriangb

@sydney-runkle
Copy link
Member Author

I can either add a test here, or in the existing serialize as any PR (which currently has a failing test bc of this bug), or in pydantic itself.

@sydney-runkle
Copy link
Member Author

Please review

Copy link

codspeed-hq bot commented Mar 19, 2024

CodSpeed Performance Report

Merging #1236 will improve performances by 43.9%

Comparing fix_extra_ser (3fd8dae) with main (abff9ba)

Summary

⚡ 6 improvements
✅ 142 untouched benchmarks

🆕 4 new benchmarks

Benchmarks breakdown

Benchmark main fix_extra_ser Change
test_complete_core_json 2.6 ms 2.1 ms +22.02%
test_core_json_fs 736.6 µs 542.6 µs +35.76%
test_dict_of_ints_core_json 13.3 ms 10.1 ms +31.55%
🆕 test_enum_int_core N/A 20.5 µs N/A
🆕 test_enum_int_python N/A 39.1 µs N/A
🆕 test_enum_str_core N/A 20.9 µs N/A
🆕 test_enum_str_python N/A 38.5 µs N/A
test_list_of_ints_core_json 5.1 ms 4 ms +28.74%
test_set_of_ints_core_json 5.9 ms 4.8 ms +22.55%
test_set_of_ints_core_json_duplicates 3.6 ms 2.5 ms +43.9%

@adriangb
Copy link
Member

A test here would be nice

Copy link
Contributor

@davidhewitt davidhewitt left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm not entirely sure on the justification, maybe a test is helpful to explain why?

I couldn't resist some Rust golfing...

src/serializers/fields.rs Outdated Show resolved Hide resolved
sydney-runkle and others added 2 commits March 19, 2024 12:53
Co-authored-by: David Hewitt <david.hewitt@pydantic.dev>
@sydney-runkle
Copy link
Member Author

Alright, odd issue, the current test passes, but if you use extra_behavior='allow' instead of config=core_schema.CoreConfig(extra_fields_behavior='allow'), then things fail...

@sydney-runkle
Copy link
Member Author

Alright, odd issue, the current test passes, but if you use extra_behavior='allow' instead of config=core_schema.CoreConfig(extra_fields_behavior='allow'), then things fail...

I think worth addressing this in a different issue, I just discovered this while trying to write the test here

@sydney-runkle sydney-runkle merged commit 1287d22 into main Mar 21, 2024
27 of 28 checks passed
@sydney-runkle sydney-runkle deleted the fix_extra_ser branch March 21, 2024 14:20
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Unexpected model serialization based on the field annotation and the extra configuration
3 participants