-
Notifications
You must be signed in to change notification settings - Fork 14
fix(ipa): child path identifiers inherit parent path exceptions #896
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Changes from 2 commits
1d13d8f
dce329d
911bd0c
e2b25a0
c632cd6
b9d5607
150f63c
6baf706
ebee416
8f69c7a
ba48a6d
3d51c76
2bb010b
22447e8
0e1fba4
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -13,3 +13,32 @@ export function hasException(object, ruleName) { | |
} | ||
return false; | ||
} | ||
|
||
/** | ||
* Finds an exception in the path hierarchy of an OpenAPI Specification (OAS) document | ||
* for a specific rule name, starting from the current path and traversing up to parent paths. | ||
* | ||
* @param {object} oas - OpenAPI Specification document containing the paths. | ||
* @param {string} currentPath - Current path to check for exceptions. | ||
* @param {string} ruleName - Name of the rule for which the exception is being checked. | ||
* @return {string|null} Returns the path where the exception is found, | ||
* or null if no exception is found in the current path or its hierarchy. | ||
*/ | ||
export function findExceptionInPathHierarchy(oas, currentPath, ruleName) { | ||
// Check current path first | ||
if (hasException(oas.paths[currentPath], ruleName)) { | ||
return currentPath; | ||
} | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 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. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Yes! Added. Could you double-check? |
||
|
||
// 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. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Does this mean that if There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. No, it is the opposite, if There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Thanks for the clarifications. Why do we want to add this functionality? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 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. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Let's say that There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. This is only implemented for IPA-102 guidelines, which is covering the path formatting. It does not apply to other principles |
||
return null; | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can you update the rule descriptions for these to clarify the exception behaviour?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Rule documentation updated 👍