fix(ipa): preserve resource scope in operation IDs for trailing /operations paths - #1413
fix(ipa): preserve resource scope in operation IDs for trailing /operations paths#1413julius-jogela wants to merge 2 commits into
Conversation
…ations paths
A collection-scoped Operations resource and an instance-scoped Operations
resource under the same parent produced the same operation ID, because
generateOperationID removes path parameters before building the noun list
and then singularizes every non-final noun.
Keep the parent noun in plural form when the trailing 'operations' section
is attached to a resource collection, so the two resources get distinct IDs:
/groups/{groupId}/clusters/operations -> listGroupClustersOperations
/groups/{groupId}/clusters/{clusterName}/operations -> listGroupClusterOperations
The change is gated on a trailing 'operations' section with a non-parameter
parent, so no other operation IDs are affected.
…ation IDs
The sibling item-level Operations paths collided for the same reason as the
resource collection paths: path parameters are removed before the noun list is
built, so both reduced to the same nouns and the parent was singularized in
both cases.
Extend isCollectionScopedOperationsPath to drop a trailing path parameter
before the existing structural check, so the same collection- versus
instance-scope distinction applies to a single Operations resource:
/groups/{groupId}/clusters/operations/{operationId} -> getGroupClustersOperation
/groups/{groupId}/clusters/{clusterName}/operations/{operationId} -> getGroupClusterOperation
Document the exception in the IPA-104 valid-operation-id description, which
governs the Get method on a single resource.
|
Extended this PR to also cover the item-level sibling collision I flagged in the original description.
Re-ran the compatibility replay of The open review question from the original description is now resolved, so nothing is outstanding. |
Problem
Operations resources with different scopes under the same parent generate the same operationId, so the collection-scoped and instance-scoped variants cannot coexist. This affects both the resource collection and the single resource:
/groups/{groupId}/clusters/operationslistGroupClusterOperationslistGroupClustersOperations/groups/{groupId}/clusters/{clusterName}/operationslistGroupClusterOperationslistGroupClusterOperations(unchanged)/groups/{groupId}/clusters/operations/{operationId}getGroupClusterOperationgetGroupClustersOperation/groups/{groupId}/clusters/{clusterName}/operations/{operationId}getGroupClusterOperationgetGroupClusterOperation(unchanged)Root cause
Verified in
tools/spectral/ipa/rulesets/functions/utils/operationIdGeneration.js.generateOperationIDfilters path parameters out of the path before building the noun list, so both scopes reduce to the same nouns (groups,clusters,operations). The loop over non-final nouns then singularizesclustersunconditionally, erasing the only thing that distinguished them. The scope information exists only in the raw path, which the noun list has already discarded.Fix
Keep the parent noun in plural form when the
operationssection is attached to a resource collection rather than to a single resource, following the generator's existing convention that plural versus singular parent nouns communicate collection versus instance scope. The existing singular form is retained for instance-scoped paths.One helper,
isCollectionScopedOperationsPath, covers both endpoints: it drops a trailing path parameter and then applies the same structural check. The noun index is unaffected by that strip because path parameters are already filtered out of the noun list.Why the fix is intentionally narrow
The helper requires all of:
operations, ignoring a single trailing path parameter (so.../operations/logsis untouched);operationsis not a path parameter (this is what distinguishes the two scopes);/operationsat the API root is unaffected;General singularization behaviour is unchanged, and no rule function, fixture, or snapshot was modified to accommodate it.
Compatibility audit
Zero change to existing operationIds. I replayed
main's generator against this branch's generator over all 319 paths inopenapi/.raw/v2.yaml× 10 method names (3190 combinations), including the legacy empty-method form, and diffed the results: no differences. The only changed operationIds are the two intended ones in the table above.There are currently no paths ending in
/operationsin the spec, so this only affects operations being introduced. A repo-wide grep forClustersOperationandclusters/operationsfound no other occurrence — no fixtures, snapshots, or generated documentation encode the affected IDs.Atlas CLI: Atlas CLI derives command names from operationId, but it lives in
mongodb/mongodb-atlas-cli, not this repo, and this repo owns no Atlas CLI fixtures. Since no existing operationId changes, there is no command rename for it to absorb. The Go CLI intools/clionly splits specs and passes operationIds through verbatim; it is unaffected.IPA-104 / IPA-105 impact
Both
xgen-IPA-105-valid-operation-idandxgen-IPA-104-valid-operation-idcompare the spec's operationId against this shared generator, so they pick up the new behaviour automatically — no rule function or validation semantics changed.Both published descriptions stated that only the last noun may be plural, which is now inaccurate for this case, so each gained one sentence documenting the exception (IPA-105 for the resource collection, IPA-104 for the single resource) and
rulesets/README.mdwas regenerated vianpm run gen-ipa-docs.Tests
__tests__/utils/operationIdGeneration.test.jscovers all four paths, explicit assertions that each scoped pair differs, negative cases for every narrowing condition, and a guard that unrelated paths (nested collection, single resource, multi-word custom method, legacy custom method) keep their existing IDs.npx jest .../operationIdGeneration.test.jsnpx jest IPA104 IPA105npx jest(full suite)npm run lint-jsnpm run format-checknpm run ipa-validationNote for reviewers
Unrelated and not addressed here:
generateOperationIDthrowsTypeError: Cannot read properties of undefinedon paths that reduce to zero nouns (for example/api/atlas/v2itself), becausesingularizeis called onundefined. This is pre-existing onmainand my compatibility harness had to guard against it. Happy to file it separately.