Skip to content

fix: Allow None in enum properties #512

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

Closed
wants to merge 2 commits into from
Closed

fix: Allow None in enum properties #512

wants to merge 2 commits into from

Conversation

juspence
Copy link
Contributor

Needed for some OpenAPI schemas generated by django-rest-framework / drf-spectacular.

Needed for some OpenAPI schemas generated by django-rest-framework / drf-spectacular.
@dbanty
Copy link
Collaborator

dbanty commented Oct 12, 2021

Thanks for throwing this together. Could you create an e2e test so we can see how this looks with generated code? Instructions are in the CONTRIBUTING.md. I'm worried that the output will confuse type checkers if it's something like:

class SomeEnum(str, Enum):
  VALUE = "VALUE"
  NONE = None

In which case, a better approach might be to ignore the None as an Enum value and instead force the property to be Nullable. So you'd get something like this:

class ObjThatUsesEnum:
  enum_prop: Optional[SomeEnum]

And then users would input None instead of SomeEnum.NONE

@juspence
Copy link
Contributor Author

juspence commented Oct 12, 2021

Sure, I can throw a test together hopefully tomorrow. I did test the new code with our OpenAPI schema that was breaking before. Now it generates code like below:

from enum import Enum


class NullEnum(str, Enum):
    VALUE_0 = "None"

    def __str__(self) -> str:
        return str(self.value)

And then the fields that use this enum look like below:

    field_name: Union[BlankEnum, FieldNameEnum, None, NullEnum, Unset] = UNSET

In our case Django is auto-generating an enum with only one possible value (null / None). I think this is because we have null=True, blank=True on certain model fields.

So we end up with NullEnum (only supported value is null) to handle a model field that's null, and BlankEnum (only supported value is "") to handle a model field that's blank. The fact these appear at all is probably a bug in drf-spectacular that autogenerates the OpenAPI schema, but for now I think we're stuck with it.

Details from #504:

Client generation fails if an enum like below is present in the generated OpenAPI schema.

NullEnum:
  enum:
  - null

The context here is that we have a Django model with char / text fields that have null=True, blank=True set. We use Django Rest Framework / drf-spectacular to generate an OpenAPI schema from our models and REST API endpoints. This generated schema automatically includes logic like below:

ModelName:
  type: object
  description: ModelName serializer
  properties:
    field_name:
      nullable: true
      oneOf:
      - $ref: '#/components/schemas/FieldNameEnum'
      - $ref: '#/components/schemas/BlankEnum'
      - $ref: '#/components/schemas/NullEnum'

FieldNameEnum, BlankEnum, and NullEnum are like below:

FieldNameEnum:
  enum:
  - stringvalue1
  - stringvalue2
  type: string
BlankEnum:
  enum:
  - ''
NullEnum:
  enum:
  - null

NullEnum accepts a literal null / None, not a string "null" (it's not a string enum).

@dbanty
Copy link
Collaborator

dbanty commented Oct 12, 2021

Interesting, the generated code you put above has the string "None" which wouldn't be the valid value it's expecting I think.

For enums which only allow None, add new null_enum.py.jinja template
For enums with mixed values, add logic to remove None from list
Mark enum property as nullable if None isn't the only value
Add tests for enums with mixed values and only null values
TODO: Make the type checker stop failing, but PTO next week
@juspence
Copy link
Contributor Author

@dbanty
For the more common use case, if the value None is present in an enum with other values, it's removed and the property is marked as nullable.

For our use case, we have enums with only 1 value, a literal None. So we can't remove this value and make the enum optional.
I added a null_enum.py.jinja template to generate a new NullEnum class, which accepts only a literal None as a value (not a string "None").
The issue above, where "None" ended up in the template instead of None, was caused by how the str_enum.py.jinja template formatted values.

I also added tests, which generate the right code as far as I can tell, but the type checker is reporting an error somewhere. I will be on PTO next week so I ran out of time to get that fixed.

@dbanty
Copy link
Collaborator

dbanty commented Oct 17, 2021

Thanks for all the work on this @juspence! I'm finishing it off in #516 by fixing some tests and simplifying the implementation a bit (I think). Instead of generating an enum with only one None variant, I'm simply setting any properties that use that type to None. Hopefully that works for your use case. If not, let me know when you come back. Enjoy your PTO!

@dbanty dbanty closed this Oct 17, 2021
dbanty added a commit that referenced this pull request Oct 17, 2021
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.

2 participants