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

fix(swagger): added options in createEnumSchemaType #3307

Merged
merged 3 commits into from
Feb 19, 2025

Conversation

mag123c
Copy link
Contributor

@mag123c mag123c commented Feb 18, 2025

PR Checklist

Please check if your PR fulfills the following requirements:

PR Type

What kind of change does this PR introduce?

  • Bugfix
  • Feature
  • Code style update (formatting, local variables)
  • Refactoring (no functional changes, no api changes)
  • Build related changes
  • CI related changes
  • Other... Please describe:

What is the current behavior?

export enum StatusEnum {
  APPROVED = 1,
  PENDING,
  REJECTED,
}

export class Meeting {
  name: string;

  @ApiProperty({
    description: 'The status of the meeting',
    enum: StatusEnum,
    enumName: 'StatusEnum',
    'x-enumNames': ['APPROVED', 'PENDING', 'REJECTED'],
  })
  status: StatusEnum;
}
# current schemas
"components": {
    "schemas": {
      "StatusEnum": {
        "type": "number",
        "enum": [
          1,
          2,
          3
        ]
      },
      "Meeting": {
        "type": "object",
        "properties": {
          "status": {
            "description": "The status of the meeting",
            "x-enumNames": [
              "APPROVED",
              "PENDING",
              "REJECTED"
            ],
            "allOf": [
              {
                "$ref": "#/components/schemas/StatusEnum"
              }
            ]
          },
          "name": {
            "type": "string"
          }
        },
        "required": [
          "status",
          "name"
        ]
      }
    }

x-enumNames no longer works from v8.0.0

Previously, x-enumNames allowed customization of enum names in, but after the refactor in PR #3123, it was unintentionally removed. This breaks compatibility with certain API client generators that rely on x-enumNames for correct enum name mapping.

Issue Number: #3294

What is the new behavior?

Restores support for x-enumNames

"components": {
    "schemas": {
      "StatusEnum": {
        "type": "number",
        "description": "The status of the meeting",
        "enum": [
          1,
          2,
          3
        ],
        "x-enumNames": [
          "APPROVED",
          "PENDING",
          "REJECTED"
        ]
      },
      "Meeting": {
        "type": "object",
        "properties": {
          "status": {
            "$ref": "#/components/schemas/StatusEnum"
          },
          "name": {
            "type": "string"
          }
        },
        "required": [
          "status",
          "name"
        ]
      }
    }

Does this PR introduce a breaking change?

  • Yes
  • No

Other information

@mag123c
Copy link
Contributor Author

mag123c commented Feb 18, 2025

I've checked that this change does not break any existing tests, but I haven't added any new test cases specifically for x-enumNames. The fix ensures that x-enumNames is correctly handled again, but I'm not entirely sure if the current validation is sufficient.

Would it be better to add a dedicated test for createEnumSchemaType, or should I extend the existing tests? I'd appreciate any feedback on whether this is good to merge as-is or if there's anything I might have overlooked.

@mag123c
Copy link
Contributor Author

mag123c commented Feb 18, 2025

Oops, I'm sorry.
I just realized that I haven't run E2E tests yet. I'll check them and update the PR. Let me know if you have any feedback in the meantime!

Comment on lines +260 to +262
if ('$ref' in property) {
return omit(pick(property, '$ref'), keysToOmit);
}
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

why is this needed?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I thought I wasn't satisfied with the SchemaObject type if I didn't do this.
I checked again with your test code and it seems that it is unnecessary. Thanks for comment!!

@kamilmysliwiec kamilmysliwiec merged commit f231342 into nestjs:master Feb 19, 2025
1 check passed
@kamilmysliwiec
Copy link
Member

Pushed a few changes and updated the e2e test.

LGTM

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants