CLOUDP-373281: support consumer-provided IPA word lists#1383
CLOUDP-373281: support consumer-provided IPA word lists#1383mongodb-sage-bot[bot] wants to merge 3 commits into
Conversation
## Proposed changes Lets consumers of the IPA validation ruleset (e.g. MMS) supply their own additions to the shared `ignoreList` and `grammaticalWords` lists used by the casing rules `xgen-IPA-117-operation-summary-format` and `xgen-IPA-126-tag-names-should-use-title-case`, without requiring a new release of the `@mongodb-js/ipa-validation-ruleset` package. Investigation findings: - The lists were never hardcoded in the rule functions — they are already passed via Spectral `functionOptions` in `IPA-117.yaml` / `IPA-126.yaml`. Spectral's function API does support consumer-provided configuration, but overriding `functionOptions` requires a consumer to re-declare the whole rule, which is the source of the friction described in the ticket. - Recommended approach (implemented here as the PoC): a consumer points the `IPA_WORD_LISTS` environment variable at a JSON file they own (living in their repository, e.g. MMS). A new `resolveWordLists` helper merges those words, de-duplicated, with the package-provided defaults. Consumers can only add words, never remove package-provided ones. - The openapi repository's own CI does not set `IPA_WORD_LISTS`, so its validation behaviour is unchanged; there are no CI or test implications for this repo, and the lists remain owned by the package by default. Changes: - Add `rulesets/functions/utils/wordLists.js` with `loadExtraWordLists` and `resolveWordLists`, reading and caching the optional JSON file. - Wire `resolveWordLists(options)` into the IPA-117 and IPA-126 rule functions. - Document the `IPA_WORD_LISTS` option for consumers in the IPA README. _Jira ticket:_ CLOUDP-373281 ## Checklist - [ ] I have signed the [MongoDB CLA](https://www.mongodb.com/legal/contributor-agreement) - [x] I have added tests that prove my fix is effective or that my feature works ### Changes to Spectral - [x] I have read the [README](../tools/spectral/README.md) file for Spectral Updates ## Further comments Behaviour is fully backwards compatible: when `IPA_WORD_LISTS` is unset, the package-provided lists from `functionOptions` are used unchanged. Unit tests cover the loader and merge/de-duplication logic, and an end-to-end test runs both rules through Spectral to prove a consumer-supplied acronym (`ignoreList`) and grammatical word (`grammaticalWords`) change validation output only when the env var is set.
|
Sage Bot can help with this pull request. Just |
| * @param {NodeJS.ProcessEnv} env the environment to read consumer additions from | ||
| * @returns {{ignoreList: Array<string>, grammaticalWords: Array<string>}} the resolved lists | ||
| */ | ||
| export function resolveWordLists(options = {}, env = process.env) { |
There was a problem hiding this comment.
The main challenge and research to be peform is to validate how we can have spectral validation passing on the openapi repository which is our second layer of defense to check if OpenAPI do not have any issues. That have been easy with hardcoded values but becomes synchronization problem once rules are defined at the consumer level.
Areas to explore:
- Continuance of pattern: Duplication of definitions in the OpenAPI repository
- Cost of synchronizing rules after
- Simplicity: Disabling validation for those rules in second (openaAPI validation)
- Anything else you can think of
| * The openapi repository's own CI does not set this variable, so the package-provided | ||
| * lists in the rulesets are used unchanged. | ||
| */ | ||
| export const WORD_LISTS_ENV_VAR = 'IPA_WORD_LISTS'; |
There was a problem hiding this comment.
What is the benefit of Env variable?
Can we usenative way of passing arguments to spectral via rule functionOptions or any other natively supported way?
| * new release of the ruleset package. | ||
| * | ||
| * The referenced JSON file may define `ignoreList` and/or `grammaticalWords` arrays: | ||
| * { "ignoreList": ["MMS"], "grammaticalWords": ["per"] } |
There was a problem hiding this comment.
Let's remove MMS mentions from PR.
|
@mongodb-sage-bot please address comments by performing agentic research. |
|
All clean. The changes address all three unresolved threads from @wtrocki:
Tests pass and formatting conforms to the repo's prettier config. Reworked the consumer word-list mechanism for |
Replace the custom IPA_WORD_LISTS environment-variable file loader with Spectral's native functionOptions mechanism for the ignoreList and grammaticalWords configuration used by xgen-IPA-117-operation-summary-format and xgen-IPA-126-tag-names-should-use-title-case. Research finding (addressing @wtrocki's feedback): the rules already receive their lists through functionOptions, and consumers extending the ruleset can override them natively by redeclaring the rule with their own functionOptions in .spectral.yaml. No bespoke env-var loader is required, which avoids the definition-duplication and synchronization concern for the openapi repository's own validation — the lists in IPA-117.yaml and IPA-126.yaml stay the canonical package defaults used by this repo's CI and are unaffected by consumer overrides. Changes: - Remove rulesets/functions/utils/wordLists.js and its unit tests. - Revert the IPA-117 and IPA-126 rule functions to destructure ignoreList and grammaticalWords directly from functionOptions, matching the existing pattern used by other rule functions. - Rewrite the README "Consumer-Provided Word Lists" section to document the native functionOptions override approach and remove MMS-specific mentions. - Rework the end-to-end test to prove consumers can override the lists via functionOptions without editing the package.
|
@mongodb-sage-bot canonical package defaults used by this repo's own CI. I think we have option to disable second line of validation in this repository for those vobaculary based IPA rules. This way we can still keep test cases develop rules in future but do not need to keep the list in this repository to match current openapi files |
|
❌ Sage Bot - Unexpected Error Sage Bot ran into an unexpected error processing your request. If you need help, reach out in #ask-devprod. |
|
@mongodb-sage-bot canonical package defaults used by this repo's own CI. I think we have option to disable second line of validation in this repository for those vobaculary based IPA rules. This way we can still keep test cases develop rules in future but do not need to keep the list in this repository to match current openapi files |
|
I've completed the changes. I was unable to run the test suite because dependency installation is blocked in this sandbox (no Disabled the two consumer-owned vocabulary rules ( |
The vocabulary-based rules xgen-IPA-117-operation-summary-format and xgen-IPA-126-tag-names-should-use-title-case rely on ignoreList and grammaticalWords word lists that are driven by the OpenAPI specs of the downstream repositories consuming this ruleset. Keeping copies of those lists in this repository in sync just to satisfy its own second layer of validation is a maintenance burden: once a consumer updates its lists, this repository's validation would miss the new entries and fail. Address the feedback from @wtrocki and @julius-jogela by turning both rules off in ipa-spectral.yaml, so this repository no longer runs them as its second layer of validation. The rule definitions, their default word lists, and their unit tests remain in IPA-117.yaml and IPA-126.yaml for future development and for consumers who redeclare the rules with their own functionOptions. - Turn off both rules in ipa-spectral.yaml with an explanatory comment. - Update the README "Consumer-Provided Word Lists" section to document that the rules are disabled here and why. - Add a test asserting the full ipa-spectral.yaml ruleset does not run either rule.
|
Approach LGTM. Great job bot!
Let's check if that is the case and we have capacity to develop and test changes. |
2 participants
Proposed changes
Lets consumers of the IPA validation ruleset (e.g. MMS) supply their own
additions to the shared
ignoreListandgrammaticalWordslists used by thecasing rules
xgen-IPA-117-operation-summary-formatandxgen-IPA-126-tag-names-should-use-title-case, without requiring a new releaseof the
@mongodb-js/ipa-validation-rulesetpackage.Investigation findings:
passed via Spectral
functionOptionsinIPA-117.yaml/IPA-126.yaml.Spectral's function API does support consumer-provided configuration, but
overriding
functionOptionsrequires a consumer to re-declare the whole rule,which is the source of the friction described in the ticket.
IPA_WORD_LISTSenvironment variable at a JSON file they own (living in theirrepository, e.g. MMS). A new
resolveWordListshelper merges those words,de-duplicated, with the package-provided defaults. Consumers can only add
words, never remove package-provided ones.
IPA_WORD_LISTS, so itsvalidation behaviour is unchanged; there are no CI or test implications for
this repo, and the lists remain owned by the package by default.
Changes:
rulesets/functions/utils/wordLists.jswithloadExtraWordListsandresolveWordLists, reading and caching the optional JSON file.resolveWordLists(options)into the IPA-117 and IPA-126 rule functions.IPA_WORD_LISTSoption for consumers in the IPA README.Jira ticket: CLOUDP-373281
Checklist
Changes to Spectral
Further comments
Behaviour is fully backwards compatible: when
IPA_WORD_LISTSis unset, thepackage-provided lists from
functionOptionsare used unchanged. Unit testscover the loader and merge/de-duplication logic, and an end-to-end test runs
both rules through Spectral to prove a consumer-supplied acronym
(
ignoreList) and grammatical word (grammaticalWords) change validationoutput only when the env var is set.
Important
MongoDB Contribution Guidelines
This pull request was generated by sage-bot on behalf of julius.jogela@mongodb.com (ticket assignee). The assignee must self-review the changes before requesting review from another engineer.
The assignee is not allowed to merge their own PR without approval from another engineer.