feat(ipa): implement IPA-132 ruleset and base Operations resource rules - #1435
feat(ipa): implement IPA-132 ruleset and base Operations resource rules#1435julius-jogela wants to merge 7 commits into
Conversation
Add the IPA-132 (Long-Running Operations) spectral ruleset with the three
rules governing the shape of the Operations resource, named after the
guideline IDs in the IPA-132 standard:
- xgen-IPA-132-operations-endpoint-must-not-be-global: rejects a root-level
Operations endpoint such as /api/atlas/v2/operations; every Operations
endpoint must have a parent resource in its path.
- xgen-IPA-132-operation-must-be-a-read-only-resource: Operations path items
may define only get, including custom method paths attached to an
Operations endpoint.
- xgen-IPA-132-operations-endpoint-must-be-a-leaf-resource: nothing may be
nested below operations/{operationId}. No guideline ID exists for this
constraint yet; it is implied by the path formats the standard defines.
All rules run at error severity with exceptions collected at the path-item
level. The path predicates live in the new utils/longRunningOperations.js;
the private helpers in the IPA-102 function and operationIdGeneration.js are
deliberately left untouched and will be consolidated in a follow-up.
The merged spec has no Operations endpoints yet, so the rules report nothing
today; the unit tests pin the contract for the first compliant long-running
operation.
| function checkViolationsAndReturnErrors(pathItem, path, ruleName) { | ||
| try { | ||
| const errors = []; | ||
| for (const method of FORBIDDEN_METHODS) { |
There was a problem hiding this comment.
You can re-use isReadOnlyResource util function
There was a problem hiding this comment.
JFYI isReadOnlyResource does not check the existence of other methods, it only looks at the schema fields. Existence of other methods is validated by other validation rules, such as IPA-xxx-ReadOnlyResourceShouldNotHave(create/update/delete)Method rules.
There was a problem hiding this comment.
Right — and one more gap on top of that: the xgen-IPA-106/107/108-readonly-resource-should-not-have-*-method rules never evaluate the collection-scoped Operations form at all, because isResourceCollectionIdentifier('/…/clusters/operations') is false (the /resource/resource shape isn't a valid collection identifier). They'd also depend on every OperationResponse field being readOnly: true, which isn't guaranteed. This rule matches on the Operations path shape directly, so both forms are covered regardless of schema annotations.
There was a problem hiding this comment.
Good catch! We might need to revisit those validation rules.
For now, using isReadOnlyResource together with the forbidden methods looks like a good idea, with the forbidden methods passed as functionOptions.
OperationResource fields being readOnly:true must be guaranteed
There was a problem hiding this comment.
Done in 07d5ad0 - forbidden methods are passed as functionOptions now, and the rule also validates the Operation resource with isReadOnlyResource, so every property of the GET response schema has to be readOnly: true. The schema check runs once per resource (on the collection path item) so the same schema issue doesn't get reported on both paths. One thing to keep in mind for when we revisit the readonly-resource rules: for the collection-scoped form isReadOnlyResource only finds the GET through its fallback loop, since isSingleResourceIdentifier doesn't recognize /resource/operations/{operationId}. Works fine today, just leaning on the fallback.
There was a problem hiding this comment.
Good call, done - dropped the functionOptions entirely, the rule now flags any HTTP method other than get on an Operations endpoint. While in there I also moved the readOnly schema check to run per path item on the get method's 2xx schemas (still via allPropertiesAreReadOnly): the resource-level isReadOnlyResource call turned out to depend on spec key order for the collection-scoped form and couldn't be suppressed by an exception on the path item that actually holds the offending get.
There was a problem hiding this comment.
Could you please create a ticket to note these limitations so we can work on them later?
There was a problem hiding this comment.
Will do - creating a ticket to track the resource evaluation util limitations (the collection-scoped Operations path gaps and the isReadOnlyResource fallback behaviour), will link it here.
- Set all three rules to warn severity while IPA-132 is experimental - Remove custom method handling from the Operations path predicates, rule descriptions and tests; a dedicated guideline and rule forbidding custom methods on Operations endpoints will follow separately - Check path item methods by key presence rather than truthiness so a declared but empty method (e.g. "post:") is still flagged by the read-only rule
Custom method paths yield no resource segments in the IPA-132 predicates, so none of the Operations rules evaluate them: flagging a post on :cancel would contradict the IPA-109 requirement that custom methods use POST, and a dedicated rule forbidding custom methods on Operations endpoints will own that check. Also align the read-only rule statement with the methods it actually flags.
- Export a generalized isRootLevelResource from resourceEvaluation.js (prefix and trailing path param stripped, a single segment remains) and use it in the must-not-be-global rule instead of the bespoke isRootLevelOperationsPath predicate - The read-only rule now takes its forbidden methods from functionOptions and additionally validates the Operation resource with isReadOnlyResource: all properties of the GET response schema must be readOnly, evaluated once per Operations resource on its collection path item
…32 rules
Per review, the read-only rule no longer takes a forbiddenMethods option:
any HTTP method other than get on an Operations endpoint is a violation.
Also fix defects found by deeper review of the rules:
- The readOnly schema condition is checked per path item on every 2xx
response schema of its get method, instead of once per resource via
isReadOnlyResource. The previous approach depended on spec key order
for collection-scoped resources, false-positived on resources carrying
an unrelated IPA-104 exception, never checked resources defining only
the single-operation path, and could not be suppressed by an exception
on the flagged path item.
- The leaf rule validates the first operations segment instead of the
path tail, so paths like .../operations/{id}/operations no longer pass.
- The Operations path predicates use the lenient isPathParam, so the
three rules agree on paths with non-camelCase parameters; parameter
casing is IPA-102's job.
- Null path items no longer crash the rules.
… endpoint Per review, the readOnly schema condition runs on the single Operation endpoint only, where the Get method is defined: the List method on the collection reuses the Operation resource schema and its response shape is owned by the must-return-operation-response rule. The method check still runs on every Operations path item. Also per review, drop the null path item and unnecessary-exception test cases from the must-not-be-global tests, and reuse shared path item examples across the read-only test cases.
| function checkViolationsAndReturnErrors(input, pathItem, path, ruleName) { | ||
| try { | ||
| const errors = []; | ||
| for (const method of HTTP_METHODS) { |
There was a problem hiding this comment.
So we are currently checking whether all the methods are present for the path, am I correct?
Instead, we can follow the same approach as the IPA113ResetMethodMustUsePost validation rule implementation: in that case, it checks if it is only post; for our case, it would be get.
There was a problem hiding this comment.
Done in 5880330 - same approach as IPA113ResetMethodMustUsePost, the defined method keys are extracted from the path item and anything other than get is flagged.
|
|
||
| // The readOnly condition is checked on the single Operation endpoint, where the Get method is | ||
| // defined. The List method on the collection reuses the Operation resource schema and its | ||
| // response shape is validated by xgen-IPA-132-operation-endpoints-must-return-operation-response. |
There was a problem hiding this comment.
Do we have xgen-IPA-132-operation-endpoints-must-return-operation-response rule in place?
| return; | ||
| } | ||
|
|
||
| const pathItem = oas.paths[input] ?? {}; |
There was a problem hiding this comment.
Could you clarify why we have this line of code?
There was a problem hiding this comment.
It was a fallback for null path items, but since that's not a case we see in practice I removed it in 5880330.
| } | ||
| } | ||
|
|
||
| function hasWritableGetResponseSchema(pathItem) { |
There was a problem hiding this comment.
[nit] How about hasReadOnlyGetResponseSchema and negating it? WritableGetResponseSchema sounds a bit awkward.
…item keys Per review, the read-only method check follows the IPA113ResetMethodMustUsePost approach: the HTTP methods defined on the path item are extracted from its keys and any method other than get is a violation. Also rename the schema helper to hasReadOnlyGetResponseSchema, drop the null path item fallbacks, and remove comments referencing rules that are not part of this PR.
2 participants
Proposed changes
This PR adds the IPA-132 (Long-Running Operations) spectral ruleset with the three rules governing the shape of the Operations resource. It is the first of a series implementing IPA-132 validation.
Jira ticket: CLOUDP-434369
Rules added
All at
warnseverity while IPA-132 is experimental, with exceptions collected at the path-item level:xgen-IPA-132-operations-endpoint-must-not-be-globalIPA-132-operations-endpoint-must-not-be-global/api/atlas/v2/operationsis a violationxgen-IPA-132-operation-must-be-a-read-only-resourceIPA-132-operation-must-be-a-read-only-resourcegetxgen-IPA-132-operations-endpoint-must-be-a-leaf-resourceIPA-132-operations-endpoint-must-be-a-leaf-resourceoperations/{operationId}The shared path predicates (identifying Operations endpoints in their collection-scoped, instance-scoped and
unauthforms) live in the newfunctions/utils/longRunningOperations.js.Testing
gen-ipa-docs, prettier and eslint clean.spectral lint openapi/.raw/v2.yamlwith the full ruleset: 0xgen-IPA-132-*findings — the spec has no Operations endpoints yet./operations,poston an Operations endpoint,/operations/{operationId}/logs): each rule fires once at the right location.Checklist
Changes to Spectral