eng: fix bug in type generation for nullable/union props BNCH-111776 #219
Conversation
| type: ["string", "null"] | ||
| enum: ["a", "b", null] | ||
| MyNullOnlyEnum: | ||
| EnumOfNullOnly: |
There was a problem hiding this comment.
The changes in this file are just to cut out some test cases that I moved into test_unions.py, because ultimately the issue isn't about the enum itself, it's about the union type (created either explicitly with oneOf, or implicitly by adding "null" as an allowable type).
eli-bl
force-pushed
the
eli.bishop/BNCH-111776-nullables
branch
from
40702a8 to
8059d24
Compare
| class TestNullableEnums: | ||
| def test_nullable_enum_prop(self, MyModel, MyEnum, MyEnumIncludingNullType1): | ||
| # Note, MyEnumIncludingNullType1 should be named just MyEnumIncludingNull - | ||
| # known bug: https://github.com/openapi-generators/openapi-python-client/issues/1120 |
There was a problem hiding this comment.
This was one of the cases affected by the bug. In the new version of this test that's in test_unions.py, the model class name is correctly rendered as MyEnumIncludingNull.
| assert False, ( | ||
| f"Couldn't find import \"{name}\" in \"{self.base_module}{module_path}\"." | ||
| f" Available imports in that module are: {existing}" | ||
| ) |
There was a problem hiding this comment.
Just some nicer failure output. In a case like the one I mentioned above, where the generator has created an incorrect type name, if you try to import NameOfThing you'll see something like:
AssertionError: Couldn't find import "NameOfThing" in "testapi_client.models". Available imports in that module are: NameOfThingType0, SomeOtherType, [etc.]
| @@ -52,7 +52,7 @@ | |||
| ) | |||
| from .model_with_additional_properties_refed import ModelWithAdditionalPropertiesRefed | |||
| from .model_with_any_json_properties import ModelWithAnyJsonProperties | |||
| from .model_with_any_json_properties_additional_property_type_0 import ModelWithAnyJsonPropertiesAdditionalPropertyType0 | |||
| from .model_with_any_json_properties_additional_property import ModelWithAnyJsonPropertiesAdditionalProperty | |||
There was a problem hiding this comment.
An example of the effect that the bug was having on the reference code in the "golden record"-based tests, causing "Type0" and "_type_0" suffixes to be added for no good reason.
| @@ -80,22 +80,44 @@ def build( | |||
| sub_properties: list[PropertyProtocol] = [] | |||
|
|
|||
| type_list_data = [] | |||
| if isinstance(data.type, list): | |||
| if isinstance(data.type, list) and not (data.anyOf or data.oneOf): | |||
There was a problem hiding this comment.
This is for a simple case where the schema is just like:
MyType:
type: ["string", "integer"]
That's equivalent in meaning to anyOf: [{type: string}, {type: int}], so on lines 84-85 we convert it into those explicit variant types. However, if there was redundantly an anyOf or oneOf and a multiple type:, then we do not want to do that, because the variants that are described in the anyOf/oneOf list already fully describe the possible types. For instance:
MyType:
type: ["string", "integer"]
oneOf:
- type: string
format: date-time
- type: integer
default: 3In that example, prior to my fix, the union would've ended up having four variants instead of two.
eli-bl
force-pushed
the
eli.bishop/BNCH-111776-nullables
branch
from
8059d24 to
b92fed6
Compare
eli-bl
force-pushed
the
eli.bishop/BNCH-111776-nullables
branch
from
c4dda2c to
f4d3735
Compare
| if (not use_original_name_for) and len(schemas_with_classes) == 1: | ||
| # An example of this scenario is a oneOf where one of the variants is an inline enum or | ||
| # model, and the other is a simple value like null. If the name of the union property is | ||
| # "foo" then it's desirable for the enum or model class to be named "Foo", not "FooType1". | ||
| # So, we'll do a second pass where we tell ourselves to use the original property name | ||
| # for that item instead of "{name}_type_{i}". | ||
| # This only makes a functional difference if the variant was an inline schema, because | ||
| # we wouldn't be generating a class otherwise, but even if it wasn't inline this will | ||
| # save on pointlessly long variable names inside from_dict/to_dict. | ||
| return process_items(use_original_name_for=schemas_with_classes[0]) |
There was a problem hiding this comment.
It took me a minute to really understand the logic of the two-pass loop in process_items
In addition to (or in place of) this comment block here, I think an example-based description of what this code is doing might be better.
Also, would it be possible to use some sort of lookup to track the schema, and then the single name schema gets updated at the end of the loop if only one named schema was encountered 🤔
I'm not sure if that will be necessarily easier/shorted to read than the current implementation but I think the two loops is more on the less-trivial scale.
There was a problem hiding this comment.
Also, would it be possible to use some sort of lookup to track the schema, and then the single name schema gets updated at the end of the loop if only one named schema was encountered
Unfortunately no, that's not possible. I mentioned the reason for this when we talked, but I should clarify it in the comment too: there are maps in schemas that already contain references to the processed classes by name, so we can't just modify one object in place.
There was a problem hiding this comment.
ahhh yes, I remember you mentioning it, but I think I hadn't grokked the code enough to fully understand.
This is equivalent to my upstream PR openapi-generators#1121. I described the symptoms in detail in openapi-generators#1120, but basically this was a problem that happened with certain patterns of using "oneOf" or "nullable" with an enum, or with an anonymous object schema that was declared inline. The symptom was that it would add spurious suffixes like "Type1" to the class name, and/or generate extra duplicates of the class.
Why we need this fix: we have patterns like this in our API. We fixed the bug in a different, no-longer-applicable way in our old fork.
The main difference between this and my upstream PR is that I was able to use the new functional test framework to verify that the right classes are generated in various slightly different permutations of the problem scenarios.