[python] Fix Circular imports on inherited discriminators.

[tom300z]

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

• [x] Read the contribution guidelines.
• [x] 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.
• [x] 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
  

  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*. For Windows users, please run the script in Git BASH.
• [x] File the PR against the correct branch: master (upcoming 7.1.0 minor release - breaking changes with fallbacks), 8.0.x (breaking changes without fallbacks)
• [x] If your PR is targeting a particular programming language, @mention the technical committee members, so they are more likely to review the pull request. @cbornet @tomplus @krjakbrjak @fa0311 @multani

[fa0311]

Since Pydantic v2, the generator does not support Circular imports. Circular imports implementation needs more discussion.

[tom300z]

Since Pydantic v2, the generator does not support Circular imports. Circular imports implementation needs more discussion.

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)

[fa0311]

All circular imports are pending in the new generator. It appears that this PullRequest only supports circular imports that occur at inheritance time. So we may have to revert this change when we support all circular imports.

Not sure when we will support all circular imports...

[multani]

@tom300z I explained the current problem with circular dependencies inhttps://github.com/OpenAPITools/openapi-generator/pull/16624#issuecomment-1732648095 :

---

Circular dependencies

The "circular dependency" test (CircularReferenceModel imports FirstRef, which imports SecondRef, which imports CircularReferenceModel) in petstore_api/models/circular_reference_model.py totally doesn't work :facepalm: This seems to be more a Python/Pydantic issue though, see:

• https://github.com/pydantic/pydantic/issues/707
• https://github.com/pydantic/pydantic/issues/1873

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 model_rebuild function, see https://docs.pydantic.dev/2.3/usage/postponed_annotations/#cyclic-references

---

I'm not even really sure how it worked before :/

[markallanson]

To be clear - which current python client generator actually supports allOf correctly without causing a circular imports problem?

[fa0311]

To be clear - which current python client generator actually supports allOf correctly without causing a circular imports problem?

The circular referencing issue is a result of the migration to Pydantic v2. So if you use a generator that works with the earlier Pydantic v1, it will work fine. Use an older build (#16685 or earlier) or python-pydantic-v1.

[markallanson]

python-pydantic-v1 appears to be missing from 7.0.1 release so I guess it's reverting to an older version for now.

[markallanson]

I tried using python-pydantic-v1 directly from the source and it is broken anyway. It's still pointing to the python template directory instead of python-pydantic-v1.

When fixing that manually It also seems to generate code with circular import problems as well.

[wing328]

It's still pointing to the python template directory instead of python-pydantic-v1.

Filed https://github.com/OpenAPITools/openapi-generator/pull/16973 (merged) to fix it

[mtraynham]

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... https://github.com/OpenAPITools/openapi-generator/blob/v7.2.0/modules/openapi-generator/src/main/resources/python/model_generic.mustache

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}}

[fa0311]

@mtraynham

The Python generator is pretty broken for discriminators at the moment given this issue, right?

Yes, if circular references are present, an error is likely to occur.

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 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?

[rc65]

I have created a duplicate of this PR as the original seems to have been abandoned by @tom300z .

[wing328]

closed via https://github.com/OpenAPITools/openapi-generator/pull/17886