Skip to content

fix: include additionalProperties in schema traversal for model_rebuild#438

Merged
commonism merged 4 commits into
commonism:masterfrom
hsunner:feature/fix-schema-additionalProperties-nullable-allof
May 27, 2026
Merged

fix: include additionalProperties in schema traversal for model_rebuild#438
commonism merged 4 commits into
commonism:masterfrom
hsunner:feature/fix-schema-additionalProperties-nullable-allof

Conversation

@hsunner
Copy link
Copy Markdown
Contributor

@hsunner hsunner commented May 25, 2026

_get_combined_attributes omitted additionalProperties, so inline schemas used as map value types (e.g. nullable+allOf+$ref) were never registered in the types dict and never had model_rebuild called. This left generated Pydantic models with unresolved ForwardRefs (pydantic_complete=False).

Fixes the case:
additionalProperties:
nullable: true
allOf:
- $ref: '#/components/schemas/SomeModel'

_get_combined_attributes omitted additionalProperties, so inline schemas
used as map value types (e.g. nullable+allOf+$ref) were never registered
in the types dict and never had model_rebuild called. This left generated
Pydantic models with unresolved ForwardRefs (__pydantic_complete__=False).

Fixes the case:
  additionalProperties:
    nullable: true
    allOf:
      - $ref: '#/components/schemas/SomeModel'
@codecov
Copy link
Copy Markdown

codecov Bot commented May 25, 2026

Codecov Report

✅ All modified and coverable lines are covered by tests.
✅ Project coverage is 93.90%. Comparing base (068bd58) to head (ce6e008).

Additional details and impacted files
@@            Coverage Diff             @@
##           master     #438      +/-   ##
==========================================
+ Coverage   93.89%   93.90%   +0.01%     
==========================================
  Files         104      104              
  Lines        8397     8414      +17     
==========================================
+ Hits         7884     7901      +17     
  Misses        513      513              
Flag Coverage Δ
core 93.90% <100.00%> (+0.01%) ⬆️
extras 93.90% <100.00%> (+0.01%) ⬆️

Flags with carried forward coverage won't be shown. Click here to find out more.

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

@hsunner
Copy link
Copy Markdown
Contributor Author

hsunner commented May 25, 2026

I can produce a test for this too, but copilot placed the reference schema in tests/fixtures, and by the README.md I'm not sure this is a good idea. I will push it to the branch as a new commit

(Sorry for ghosting you when I last worked with this codebase; I got pulled to something else after that summer. Should have replied once I realised I had left you hanging)

hsunner and others added 3 commits May 25, 2026 12:10
@commonism
Copy link
Copy Markdown
Owner

Hi,

types with multiple identities (here: object|null) have to be RootModels, else there is inconsistent behavior.
Here - when specifying the type of the additionalPropert as "object" it will enforce a RootModel.

This changes the access to the model - using .root on the model.

I added the missing bits to make this happen.

Don't worry about the PR interaction in 2023, to me everything was fine.

@commonism commonism merged commit 41fb350 into commonism:master May 27, 2026
12 checks passed
@hsunner
Copy link
Copy Markdown
Contributor Author

hsunner commented May 27, 2026

Awesome, thank you!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants