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 explicitly 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.
| ) | ||
| if isinstance(sub_prop, PropertyError): | ||
| return PropertyError(detail=f"Invalid property in union {name}", data=sub_prop_data), schemas | ||
| sub_properties.append(sub_prop) |
There was a problem hiding this comment.
Here's the main fix. Basically, when we were iterating through the variants of a union, we were making up inline type names like "MyProperty_type_0", "MyProperty_type_1", etc... which would be used in case the variant was an inline schema for an object or an enum. However, in cases where only one of the variants is a schema like that— and the others are basic types like int or null, which don't require their own class name— this was resulting in us making up a new type when none was needed.
eli-bl
force-pushed
the
eli.bishop/BNCH-111776-nullables
branch
from
8059d24
to
b92fed6
Compare
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.
The bug does show up when running the generator on our v2 API, but we had fixed it in some other 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.