Skip to content

Conversation

@yelizhenden-mdb
Copy link
Collaborator

@yelizhenden-mdb yelizhenden-mdb commented Apr 2, 2025

Proposed changes

Jira ticket: CLOUDP-306578

   xgen-IPA-123-allowable-enum-values-should-not-exceed-20:
    description: |
      Allowable enum values should not exceed 20 entries.

      ##### Implementation details
      Rule checks for the following conditions:
        - Applies to inline enum values
        - Validates that each enum array has 20 or fewer values
        - Reusable enum schemas will be ignored
        - Skips validation if the schema has an exception defined for this rule

Found 61 violations to fix

Checklist

  • I have signed the MongoDB CLA
  • I have added tests that prove my fix is effective or that my feature works

Changes to Spectral

  • I have read the README file for Spectral Updates

Further comments

- Skips validation if the schema has an exception defined for this rule
message: '{{error}} https://mdb.link/mongodb-atlas-openapi-validation#xgen-IPA-123-enum-values-must-be-upper-snake-case'
severity: error
resolved: false
Copy link
Collaborator Author

@yelizhenden-mdb yelizhenden-mdb Apr 2, 2025

Choose a reason for hiding this comment

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

FYI: drive-by fix, not to duplicate the check for referenced schemas. The rule validates only the root component

@yelizhenden-mdb yelizhenden-mdb marked this pull request as ready for review April 2, 2025 16:19
@yelizhenden-mdb yelizhenden-mdb requested a review from a team as a code owner April 2, 2025 16:19
import { resolveObject } from './utils/componentUtils.js';

const RULE_NAME = 'xgen-IPA-123-enum-values-should-not-exceed-20';
const ERROR_MESSAGE = 'Enum arrays should not exceed 20 values. Current count: ';
Copy link
Collaborator

Choose a reason for hiding this comment

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

Suggested change
const ERROR_MESSAGE = 'Enum arrays should not exceed 20 values. Current count: ';
const ERROR_MESSAGE = 'Enum arrays should not exceed 20 values, consider opting for a string instead. Current count: ';

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

I also thought about including it, but not sure if producers would understand the difference, because type is already string. What do you think?

Copy link
Collaborator

Choose a reason for hiding this comment

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

Yeah I took it right from the IPAs, so if it's unclear we should probably update the IPA too to explain. No string opinion though, I think in the context it makes sense

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Until the investigation for the reusable enum schemas are finalized, I think we should not give any suggestions here. I will note that the relevant IPA and error message here should be refined according to the investigation results

Copy link
Member

Choose a reason for hiding this comment

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

IMHO it is premature to put recomendation for using string. We moving problem to area we also want to fix (long strings).

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Copy link
Member

@wtrocki wtrocki Apr 3, 2025

Choose a reason for hiding this comment

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

To be clear - I agree that each IPA should have a clear message how to workaround the problem.
For this IPA we agreed that we're going to delegate it outside this PR as more investigation is required.

@wtrocki wtrocki requested a review from Copilot April 2, 2025 18:20
Copy link

Copilot AI left a comment

Choose a reason for hiding this comment

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

Pull Request Overview

This pull request implements a new validation rule ensuring that enum arrays in the OpenAPI schema do not exceed 20 values. The key changes include:

  • Adding a new rule function (IPA123EnumValuesShouldNotExceed20.js) that checks the enum value count.
  • Updating documentation (README.md) to include the new rule.
  • Modifying the Spectral ruleset configuration (IPA-123.yaml) to register the new rule.
  • Providing comprehensive tests for the new rule in IPA123EnumValuesShouldNotExceed20.test.js.

Reviewed Changes

Copilot reviewed 4 out of 4 changed files in this pull request and generated no comments.

File Description
tools/spectral/ipa/rulesets/functions/IPA123EnumValuesShouldNotExceed20.js New function to enforce the maximum allowed enum values
tools/spectral/ipa/rulesets/README.md Documentation update for the new rule
tools/spectral/ipa/rulesets/IPA-123.yaml Rule configuration update to include the new rule
tools/spectral/ipa/tests/IPA123EnumValuesShouldNotExceed20.test.js Added tests covering various scenarios for the new rule

errors: [],
},
{
name: 'valid on with reusable schemas',
Copy link
Collaborator Author

@yelizhenden-mdb yelizhenden-mdb Apr 3, 2025

Choose a reason for hiding this comment

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

FYI: Reusable enum schemas are ignored now

message: '{{error}} https://mdb.link/mongodb-atlas-openapi-validation#xgen-IPA-123-allowable-enum-values-should-not-exceed-20'
severity: warn
resolved: false
given: '$..enum'
Copy link
Member

Choose a reason for hiding this comment

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

❤️

Copy link
Member

@wtrocki wtrocki left a comment

Choose a reason for hiding this comment

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

LGTM althought strong recomendation to keep enum size as parameter for rule inputs

Copy link
Member

@wtrocki wtrocki left a comment

Choose a reason for hiding this comment

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

LGTM!

@wtrocki
Copy link
Member

wtrocki commented Apr 3, 2025

In driveby next time cover parameter in rule description

@yelizhenden-mdb yelizhenden-mdb merged commit c4bec38 into main Apr 3, 2025
8 checks passed
@yelizhenden-mdb yelizhenden-mdb deleted the CLOUDP-306578 branch April 3, 2025 10:54
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.

4 participants