Skip to content
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

Support x-enum-varnames openapi generator vendor extension for better generated enum names #1056

Closed
kavdev opened this issue Aug 13, 2023 · 7 comments

Comments

@kavdev
Copy link

kavdev commented Aug 13, 2023

OpenAPI Generator has a vendor extension that makes integer enums a bit more clear to work with.

I'm assuming adding support for this extension could turn something like:

/**
 * * `1` - Monday
 * @export
 */
export const WeekdayEnum = {
    NUMBER_1: 1
} as const;

into

/**
 * * `1` - Monday
 * @export
 */
export const WeekdayEnum = {
    MONDAY: 1
} as const;

In an ideal world, there would be a way to get the enum names from the models.TextChoices/models.Integerchoices class used (maybe through a setting or model property?).

As an mvp, simply capitalizing and replacing non-alphanumeric characters with underscores in the label (if available) and using that as the name would be incredibly useful.

This would have to be behind a setting, since it would be a breaking change and the vendor prefix is tied to openapi.

See also OpenAPI Generator PR for more context: OpenAPITools/openapi-generator#917

@kavdev kavdev changed the title Support x-enum-varnames openapi generator vendor extension Support x-enum-varnames openapi generator vendor extension for better generated enum names Aug 14, 2023
@tfranzel
Copy link
Owner

Hi,

this has been an ongoing discussion for years. OpenAPI 3.1 does (afaik) support the construction mentioned in the issue:

OAI/OpenAPI-Specification#348 (comment)

Though we are still on 3.0.3 and since there was not one clear winner in representing this, we chose to encode the mapping in the description for now. We already had a couple of issues filed regarding this topic. I have to say, I am not that thrilled to introduce a x- extension into the codebase with limited ecosystem support.

@kavdev
Copy link
Author

kavdev commented Aug 15, 2023

@tfranzel gotcha, thanks for the heads up. Any chance you know what codegen would output with that proposed shape?

If there's an official way to accomplish this and it's just a matter of time until the upgrade to 3.1, this issue can be closed and I'll just wait until the announcement.

@tfranzel
Copy link
Owner

cheers.

Any chance you know what codegen would output with that proposed shape?

not exactly sure, but it would most likely be similar to your example

we will do that construction with 3.1, but exactly when we move to 3.1 is not yet decided. It will happen though eventually.

@gabn88
Copy link

gabn88 commented Oct 23, 2023

Sorry to reply on an old issue, but am I correct that the method mentioned here (OAI/OpenAPI-Specification#348 (comment)) is not supported in drf-spectacular? Having the descriptions of the choice in the general description field is not really handy for our frontend TypeScript engineers.

@tfranzel
Copy link
Owner

@gabn88, yes you are right. we do not currently "support" that. That particular comment is rather new, and I haven't tried what the generator or UI would do in that variation. There were a lot of ideas thrown around (with varying support) and it is an old (2015) issue. We chose to go the common enum route with a docstring because that seemed to be the most agreed on solution for 3.0.3. It also has to be considered that the oneOf/enum: [x] approach, although legal, may not have good support in the ecosystem.

It was and is a tradeoff in the absence of a properly supported/recommended solution to a rather basic problem. Potentially breaking people with that change this late is not that good of an idea imho. We should let that one rest till 3.1.

@gabn88
Copy link

gabn88 commented Oct 24, 2023

Thanks for the fast response! I love how quickly it was to set drf-spectacular up and how much works 'out-of-the-box'.

Our frontend team gave us the following feedback (they are using the TypeScript OpenAPI generator to create schemas, models and services):

  • The BlankEnum type should be removed.
  • The NullEnum type should be removed.
  • Arrays of string, number, etc should not have nullable items (see explanation below).
  • Some Enums that are based on Django IntegerChoices have the wrong descriptions (see explanation below).
  • Can we use this definition for enum documentation notation? This way the enums in TypeScript will have proper names. #bonus #would_be_awesome

Wrong Enum descriptions

If there are multiple IntegerChoices with different labels, they will be coerced into one. For example (simplified example):

class StatusChoices(IntegerChoices):
    ACTIVE = 1, 'active'
    INACTIVE = 2, 'inactive'

class IntensityChoices(IntegerChoices):
   LOW = 1, 'low'
   HIGH = 2, 'high'

will both be coerced into

IntensityEnum
      enum:
      - 1
      - 2
      type: integer
      description: |-
        * `1` - low
        * `2` - high

Nullable array items

Current:

roles:
type: array
items:
  type: string
  nullable: true

Should be like this, where the array itself is nullable:

roles:
type: array
nullable: true
items:
  type: string

I figured I could remove the NullEnum and BlankEnum by using ENUM_ADD_EXPLICIT_BLANK_NULL_CHOICE = False.
However, then the nullable: true is still on the items (the child) and not on the parent. Even with extra_kwargs = {': {'required': False, 'allow_null': True, 'allow_empty': True}} on the parent field. Is there a way to change this?

Same for the coercing behavior, can it be changed to keep the descriptions of different types even if the number of choices is equal?

@tfranzel
Copy link
Owner

glad you like it.

The BlankEnum type should be removed. The NullEnum type should be removed.

They do have a purpose. client generation sometimes chokes which is the reason we have "ENUM_ADD_EXPLICIT_BLANK_NULL_CHOICE": False. so far so good.

Arrays of string, number, etc should not have nullable items (see explanation below).

that has to be investigated. please create a new issue for it with a reproduction snippet. it is easier to discuss like that.

Some Enums that are based on Django IntegerChoices have the wrong descriptions (see explanation below).

This addresses your other #790 (comment). Yes, it is flaw that became a bug with the addition of the description string. First I thought is not fixable currently, but I realized yesterday that there might be a way. The django PR thing I was talking about is not going to happen either way.

Can we use this definition for enum documentation notation? This way the enums in TypeScript will have proper names. #bonus #would_be_awesome

Same problem with compatibility. not sure how much support we have for this solution. I find this even more problematic that the oneOf/enum: [x] approach.

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

No branches or pull requests

3 participants