fix(ipa): child path identifiers inherit parent path exceptions #896
Conversation
yelizhenden-mdb
changed the title
yelizhenden-mdb
changed the title
| // Check parent paths by removing segments from the end | ||
| const pathSegments = currentPath.split('/').filter((segment) => segment !== ''); | ||
|
|
||
| for (let i = pathSegments.length - 1; i > 0; i--) { | ||
| const parentPath = '/' + pathSegments.slice(0, i).join('/'); | ||
| if (oas.paths[parentPath] && hasException(oas.paths[parentPath], ruleName)) { | ||
| return parentPath; | ||
| } | ||
| } | ||
|
|
There was a problem hiding this comment.
Does this mean that if atlas/api/v2.0/groups/{groupId}/cluster/{clusterName} has an exception also that exception is applied to atlas/api/v2.0/groups/{groupId}/?
There was a problem hiding this comment.
No, it is the opposite, if atlas/api/v2.0/groups/{groupId}/ has an exception for the rule, atlas/api/v2.0/groups/{groupId}/cluster/{clusterName} will inherit it
There was a problem hiding this comment.
Thanks for the clarifications. Why do we want to add this functionality? atlas/api/v2.0/groups/{groupId} and atlas/api/v2.0/groups/{groupId}/cluster/{clusterName} are two separated resources
There was a problem hiding this comment.
Currently, when API producers define new API endpoints using Swagger annotations in Java, they specify only the child path segment to append to a parent path. They are not responsible for, nor can they modify, the parent path itself and exceptions on it.
This way, any new method extending the parent path does not require separate exceptions. API producers won’t need to add exceptions each time, though we will still collect metrics as if the child paths had defined exceptions
There was a problem hiding this comment.
Let's say that atlas/api/v2.0/groups/{groupId}/ has an exception to xgen-IPA-104-resource-has-GET which means that the GET method is missing. Why should we applied this exception to all the child resources?
There was a problem hiding this comment.
This is only implemented for IPA-102 guidelines, which is covering the path formatting. It does not apply to other principles
| if (hasException(oas.paths[currentPath], ruleName)) { | ||
| return currentPath; | ||
| } |
There was a problem hiding this comment.
Could we return a spectral error if the current path has an exception as well as the parent? So we can identify the unnecessary ones?
There was a problem hiding this comment.
Yes! Added. Could you double-check?
| const violations = checkViolations(pathKey, path, ignoredValues); | ||
|
|
||
| return evaluateAndCollectAdoptionStatus(violations, RULE_NAME, oas.paths[input], path); | ||
| // Check for exceptions in path hierarchy |
There was a problem hiding this comment.
Can you update the rule descriptions for these to clarify the exception behaviour?
There was a problem hiding this comment.
Rule documentation updated 👍
| * Evaluates and collects adoptions, exceptions and violations based on the rule, evaluated object and the validation errors. | ||
| * If the object is violating the rule, but has an exception, the validation error is ignored | ||
| * If the object is adopting the rule, but has an exception, a validation error will be returned | ||
| * If the object is adopting the rule, but has an exception, an unnecessary exception error is returned, but the object is counted as adopting the rule |
There was a problem hiding this comment.
FYI: Behavior change. IMHO we don't need to collect "unnecessary exceptions" as violations.
Let me know if you have strong opinions
There was a problem hiding this comment.
Sure, makes sense. Thanks!
3 participants
Proposed changes
If an exception is defined for a parent path identifier, all child paths inherit it—no need to add separate exceptions.
The validation rules for path identifier formats have been updated accordingly.
Jira ticket: CLOUDP-308270
Checklist
Changes to Spectral
Further comments