-
-
Notifications
You must be signed in to change notification settings - Fork 6.4k
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
[python] Fix Circular imports on inherited discriminators. #16809
[python] Fix Circular imports on inherited discriminators. #16809
Conversation
Signed-off-by: Tom Hörberg <[email protected]>
Signed-off-by: Tom Hörberg <[email protected]>
Signed-off-by: Tom Hörberg <[email protected]>
Since Pydantic v2, the generator does not support Circular imports. |
…_map. Signed-off-by: Tom Hörberg <[email protected]>
Signed-off-by: Tom Hörberg <[email protected]>
Signed-off-by: Tom Hörberg <[email protected]>
…_map Signed-off-by: Tom Hörberg <[email protected]>
Could you elaborate what you mean by that? Does the new generator not support inherited discriminators (which causes the circular imports) or do you not support Errors caused by circular Imports in general? I've also fixed the tests in my PR. Although there might be a cleaner way, the tests pass and the fix works well for me on a very large API (151 Endpoints) |
All circular imports are pending in the new generator. Not sure when we will support all circular imports... |
@tom300z I explained the current problem with circular dependencies inhttps://github.com//pull/16624#issuecomment-1732648095 : Circular dependenciesThe "circular dependency" test (
To be honest, I don't know how it even worked before :) This is solvable, if we bundle all the circular dependencies in the same module, and then use Pydantic I'm not even really sure how it worked before :/ |
To be clear - which current python client generator actually supports |
The circular referencing issue is a result of the migration to Pydantic v2. |
|
I tried using When fixing that manually It also seems to generate code with circular import problems as well. |
Filed #16973 (merged) to fix it |
The Python generator is pretty broken for discriminators at the moment given this issue, right? Since I'd preferred not to wait on a code change in the generator, I came up with a template only change that can be used for overriding the template with the released CLI. This override is for 7.2.0... diff --git a/modules/openapi-generator/src/main/resources/python/model_generic.mustache b/modules/openapi-generator/src/main/resources/python/model_generic.mustache
index 1b676e46955..6ebe78f50c4 100644
--- a/modules/openapi-generator/src/main/resources/python/model_generic.mustache
+++ b/modules/openapi-generator/src/main/resources/python/model_generic.mustache
@@ -1,4 +1,5 @@
from __future__ import annotations
+import importlib
import pprint
import re # noqa: F401
import json
@@ -93,16 +94,30 @@ class {{classname}}({{#parent}}{{{.}}}{{/parent}}{{^parent}}BaseModel{{/parent}}
__discriminator_property_name: ClassVar[List[str]] = '{{discriminator.propertyBaseName}}'
# discriminator mappings
- __discriminator_value_class_map: ClassVar[Dict[str, str]] = {
- {{#mappedModels}}'{{{mappingName}}}': '{{{modelName}}}'{{^-last}},{{/-last}}{{/mappedModels}}
- }
+ __discriminator_value_class_map: ClassVar[Union[Dict[str, str], None]] = None
+
+ @classmethod
+ def _get_discriminator_value_class_map(cls) -> ClassVar[Dict[str, str]]:
+ if cls.__discriminator_value_class_map == None:
+ # Prevent circular imports caused by mutually referencing classes
+ {{#mappedModels}}
+ globals()['{{modelName}}'] = importlib.import_module(
+ "{{packageName}}.models.{{model.classVarName}}"
+ ).{{modelName}}
+ {{/mappedModels}}
+ cls.__discriminator_value_class_map = {
+ {{#mappedModels}}
+ '{{{mappingName}}}': '{{{modelName}}}'{{^-last}},{{/-last}}
+ {{/mappedModels}}
+ }
+ return cls.__discriminator_value_class_map
@classmethod
def get_discriminator_value(cls, obj: Dict) -> str:
"""Returns the discriminator value (object type) of the data"""
discriminator_value = obj[cls.__discriminator_property_name]
if discriminator_value:
- return cls.__discriminator_value_class_map.get(discriminator_value)
+ return cls._get_discriminator_value_class_map().get(discriminator_value)
else:
return None
@@ -250,7 +265,7 @@ class {{classname}}({{#parent}}{{{.}}}{{/parent}}{{^parent}}BaseModel{{/parent}}
else:
raise ValueError("{{{classname}}} failed to lookup discriminator value from " +
json.dumps(obj) + ". Discriminator property name: " + cls.__discriminator_property_name +
- ", mapping: " + json.dumps(cls.__discriminator_value_class_map))
+ ", mapping: " + json.dumps(cls._get_discriminator_value_class_map()))
{{/discriminator}}
{{/hasChildren}}
{{^hasChildren}}
@@ -375,10 +390,3 @@ class {{classname}}({{#parent}}{{{.}}}{{/parent}}{{^parent}}BaseModel{{/parent}}
return _obj
{{/hasChildren}}
-{{#vendorExtensions.x-py-postponed-model-imports.size}}
-{{#vendorExtensions.x-py-postponed-model-imports}}
-{{{.}}}
-{{/vendorExtensions.x-py-postponed-model-imports}}
-# TODO: Rewrite to not use raise_errors
-{{classname}}.model_rebuild(raise_errors=False)
-{{/vendorExtensions.x-py-postponed-model-imports.size}}
|
Yes, if circular references are present, an error is likely to occur.
This approach using importlib looks good. Can you send a pull request so we can run detailed tests? Or, @tom300z , can you resolve this PullRequest conflict? |
I have created a duplicate of this PR as the original seems to have been abandoned by @tom300z . |
closed via #17886 |
When generating a python client using the current snapshot of the openapi generator, a circular import occurs when a discriminator is inherited via AllOf.
See Issue #16808 for example schema.
I fixed this by moving the initialization of the "__discriminator_value_class_map" attribute and all required imports to the "get_discriminator_value" method.
I know that imports outside of the top level aren't the most beautiful thing in the world but i can't think of any other way to break the import loop. This also should not affect performance too much as the attribute only gets initialized once.
PR checklist
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*
.For Windows users, please run the script in Git BASH.
master
(upcoming 7.1.0 minor release - breaking changes with fallbacks),8.0.x
(breaking changes without fallbacks)@cbornet @tomplus @krjakbrjak @fa0311 @multani