Skip to content

Conversation

@mgoltzsche
Copy link
Contributor

@mgoltzsche mgoltzsche commented Aug 6, 2024

  • Don't let generated model code extend the type specified within the additionalProperties schema (or the array item schema) but the pydantic BaseModel.
  • Support additionalProperties the pydantic v2 way within __dict__: Previously, the generated additional_properties field showed up within the response of the generated API as opposed marshaling the model so that its fields are added to/embedded into the root object. Apparently that is because pydantic v2 does not honour the generated to_dict methods (which would have mapped the object to the correct representation) but, instead, supports additional properties natively by specifying extra=allow within the model_config. Correspondingly, the following changes have been applied:
    • To allow additional fields, specify extra=allow within the model_config.
    • Don't generate the additional_properties field but use __dict__ - users can use pydantic's built-in model.model_extra instead.
    • Don't generate the {to|from}_{dict|json} methods since pydantic is taking care of the model mapping based on the declared fields and model_config - users can use pydantic's model.model_dump[_json] instead.
    • Let the generated {to|from}_{dict|json} methods delegate to Pydantic's model_dump[_json] methods.
    • Let the generated API endpoints enable the exclude_unset response marshalling option in order to omit fields from the response that weren't explicitly set by the code. This is so that non-required fields don't show up with null values within the response (which would be invalid according to the OpenAPI spec, unless those fields are explicitly marked as nullable within the OpenAPI schema).
  • Support oneOf and anyOf schemas the Pydantic v2 way:
    • Let the generated model extend the pydantic RootModel.
    • Generate (discriminated) Unions and leave it to Pydantic to validate and figure out the type.
    • Generate model constructors that forcefully set the discriminator field in order to ensure it is included in the marshalled representation (otherwise it would be ignored when not explicitly set due to the exclude_unset marshalling option being enabled).

Closes #19311
Relates to #17703 (might also close that one)
Relates to #19454

cc @cbornet @tomplus @krjakbrjak @fa0311 @multani

PR checklist

  • Read the contribution guidelines.
  • Pull Request title clearly describes the work in the pull request and Pull Request description provides details about how to validate the work. Missing information here may result in delayed response from the community.
  • Run the following to build the project and update samples:
    ./mvnw clean package 
    ./bin/generate-samples.sh ./bin/configs/*.yaml
    ./bin/utils/export_docs_generators.sh
    
    (For Windows users, please run the script in Git BASH)
    Commit all changed files.
    This is important, as CI jobs will verify all generator outputs of your HEAD commit as it would merge with master.
    These must match the expectations made by your contribution.
    You may regenerate an individual generator by passing the relevant config(s) as an argument to the script, for example ./bin/generate-samples.sh bin/configs/java*.
    IMPORTANT: Do NOT purge/delete any folders/files (e.g. tests) when regenerating the samples as manually written tests may be removed.
  • File the PR against the correct branch: master (upcoming 7.6.0 minor release - breaking changes with fallbacks), 8.0.x (breaking changes without fallbacks)
  • If your PR is targeting a particular programming language, @mention the technical committee members, so they are more likely to review the pull request.

mgoltzsche added a commit to mgoltzsche/beets-websearch that referenced this pull request Aug 10, 2024
Previously additionalProperties were indented wrongly so that they appeared as a regular property.

Also, adjust the controller code correspondingly.

Depends on the openapi-generator PR OpenAPITools/openapi-generator#19312 (otherwise additionalProperties support does not work with the python-fastapi generator)
@denosaurtrain
Copy link

denosaurtrain commented Aug 30, 2024

Thanks for the PR! I'll need something like this PR to move off python-pydantic-v1.

I have a question about this bit:

Don't generate the {to|from}_{dict|json} methods since pydantic is taking care of the model mapping based on the declared fields and model_config - users can use pydantic's model.model_dump[_json] instead.

Why remove {to|from}_{dict|json} (and to_str) rather than replace the body of those methods with the right pydantic calls? I imagine this could be a substantial breaking change for some folks.

@mgoltzsche
Copy link
Contributor Author

Why remove {to|from}_{dict|json} (and to_str) rather than replace the body of those methods with the right pydantic calls? I imagine this could be a substantial breaking change for some folks.

The idea is to let the generated code fully leverage the fastapi/pydantic platform idioms, which include letting the route methods return typed objects that pydantic then maps and validates based on the type annotation.
I didn't want to bloat the generated code unnecessarily and I was thinking the python-fastapi generator is young enough so that I could still make such a breaking change. However, to make it backward-compatible, I'll adjust the PR to let it generate those methods again but letting them delegate to pydantic's methods...

@mgoltzsche mgoltzsche marked this pull request as draft September 1, 2024 22:10
@mgoltzsche mgoltzsche force-pushed the fix-python-additionalproperties-import branch from 24b70a9 to 371a3e5 Compare September 1, 2024 22:52
@mgoltzsche mgoltzsche marked this pull request as ready for review September 1, 2024 23:13
@mgoltzsche
Copy link
Contributor Author

mgoltzsche commented Sep 1, 2024

I updated the PR correspondingly but unfortunately this is still a breaking change since

  • nullable field semantics are not honoured anymore - would need to be generated into the type annotation like this maybe or applied via the exclude_unset marshalling option.
  • Discriminator logic for oneOf schemas is not generated - would need to be generated into the type annotation like this.

I can look into making the adjustments some other evening.

@mgoltzsche mgoltzsche force-pushed the fix-python-additionalproperties-import branch 4 times, most recently from ab40757 to e99c74f Compare September 3, 2024 23:12
@mgoltzsche mgoltzsche marked this pull request as draft September 3, 2024 23:17
@mgoltzsche mgoltzsche force-pushed the fix-python-additionalproperties-import branch 5 times, most recently from f3161f8 to 66d1ff9 Compare September 4, 2024 23:53
@denosaurtrain
Copy link

I like it! I anticipate using this as a client to replace python-pydantic-v1. Hopefully a maintainer can chime in/approve when you are ready!

@mgoltzsche mgoltzsche force-pushed the fix-python-additionalproperties-import branch 2 times, most recently from d83b593 to 001894e Compare September 13, 2024 23:33
@mgoltzsche mgoltzsche force-pushed the fix-python-additionalproperties-import branch 8 times, most recently from 1605ab8 to 94330f0 Compare October 19, 2024 23:55
mgoltzsche added a commit to mgoltzsche/openapi-generator that referenced this pull request Mar 8, 2025
@mgoltzsche mgoltzsche force-pushed the fix-python-additionalproperties-import branch 3 times, most recently from b06b295 to ef34d9d Compare March 17, 2025 21:28
@mgoltzsche
Copy link
Contributor Author

Turns out the failed python client test was fixed here now. Correspondingly, I applied the fix also to the python-pydantic-v1 client test which failed for the same reason.
Also, I rebased the PR.

Now that the merge conflicts are resolved and the CI build succeeds again, this PR is ready for review again.

@ampedandwired
Copy link

Thanks for your efforts with this MR, it would be really great to see this get merged (along with a few of the other python-fastapi fixes).

@srnbckr
Copy link

srnbckr commented Jun 11, 2025

Thank you for your work - as many I probably found this PR because I ran into similar problems with the python-fastapi generator. It would be really nice if the python-fastapi fix PRs could be merged.

@mgoltzsche mgoltzsche force-pushed the fix-python-additionalproperties-import branch from ef34d9d to 22f78fa Compare June 14, 2025 19:04
@mgoltzsche mgoltzsche marked this pull request as draft June 14, 2025 19:09
@mgoltzsche
Copy link
Contributor Author

mgoltzsche commented Jun 14, 2025

I rebased the PR now again to resolve the merge conflicts (which dropped my last commit that fixed the python-pydantic-v1 test since the fix is on the master branch now).

However, now the Python client deserialization test fails with the following error:

tests/test_deserialization.py:301: error: Item "None" of "Union[Cat, Dog, None]" has no attribute "class_name"  [union-attr]
tests/test_deserialization.py:302: error: Item "Dog" of "Union[Cat, Dog, None]" has no attribute "declawed"  [union-attr]
tests/test_deserialization.py:302: error: Item "None" of "Union[Cat, Dog, None]" has no attribute "declawed"  [union-attr]
tests/test_deserialization.py:303: error: Item "None" of "Union[Cat, Dog, None]" has no attribute "to_json"  [union-attr]

Though, that problem is not related to this PR which the master branch build of #21415 shows which also fails with the same error! (The same problem also makes the build of PR #21402 fail.)

Thus, this PR is ready for review again!

@mgoltzsche mgoltzsche marked this pull request as ready for review June 14, 2025 19:23
@mgoltzsche mgoltzsche force-pushed the fix-python-additionalproperties-import branch from 22f78fa to 95942a5 Compare June 21, 2025 20:30
@mgoltzsche mgoltzsche marked this pull request as draft June 21, 2025 20:50
@mgoltzsche mgoltzsche force-pushed the fix-python-additionalproperties-import branch 2 times, most recently from ef34d9d to b69a8ee Compare June 21, 2025 20:59
@mgoltzsche mgoltzsche marked this pull request as ready for review June 21, 2025 21:12
@mgoltzsche
Copy link
Contributor Author

mgoltzsche commented Jun 21, 2025

Rebased the PR again to include the python unit test fix of #21423.
Now the docs and samples are reported to be outdated but that has nothing to do with this PR but is the case within the master branch.

This PR is ready for review again!

Don't let models inherit the value type of additionalProperties and arrays.

This is to fix a bug where the `python-fastapi` server generator generated invalid models that inherited the value type specified within additionalProperties.
Previously, the generated `additional_properties` field showed up within
the response of the generated API as opposed marshaling the model so that its fields are added to the root object.
Apparently that is because pydantic v2 does not honour the generated `to_dict` methods anymore (which would have mapped the object to the correct representation) but, instead, supports additional properties natively by specifying `extra=allow` within the `model_config`.

Correspondingly, the following changes have been applied:
* To allow additional fields, specify `extra=allow` within the `model_config`.
* Don't generate the `additional_properties` field - users can use pydantic's built-in `model.extra_fields` instead.
* Let the `{to|from}_{dict|json}` methods delegate to Pydantic's `model_dump[_json]` methods.
Exclude unset fields when marshalling api endpoint response bodies.
* Support oneOf and anyOf schemas the pydantic v2 way by generating them as Unions.
* Generate model constructor that forcefully sets the discriminator field to ensure it is included in the marshalled representation.
@mgoltzsche mgoltzsche force-pushed the fix-python-additionalproperties-import branch from b69a8ee to ef7c699 Compare June 25, 2025 19:57
@mgoltzsche
Copy link
Contributor Author

I rebased the PR again to include the fixes from the main branch.

Please review!

@mgoltzsche
Copy link
Contributor Author

@wing328 can you review the PR by any chance?

@wing328
Copy link
Member

wing328 commented Jul 2, 2025

@mgoltzsche thanks for the PR 🙏

it's a huge change. do you mind pinging me via Slack (https://app.slack.com/client/TLQFRCNJZ/dms) when you've time tomorrow or Friday for a few quick questions on this PR?

@unkindypie
Copy link

Hey guys bumping the pull request would be great to get it in the mainstream branch

@mgoltzsche
Copy link
Contributor Author

Unfortunately it doesn't look like this PR will be merged.
@wing328 told me via Slack that it is too big to be reviewed and that the actual Pydantic v2 support should be implemented in the client generator first in form of multiple small incremental PRs since there are more tests for that. Afterwards the changes could be applied to the FastAPI server generator similarly. However, I don't have time for that any time soon.

@MoritzProg
Copy link

MoritzProg commented Nov 6, 2025

thanks for your effort so far!

I can confirm that anyOf and oneOf schemas are also not supported for the client generator, which generates erroneous code for those.

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.

[BUG] [python-fastapi] doesn't support additionalProperties properly

9 participants