-
Notifications
You must be signed in to change notification settings - Fork 14
CLOUDP-288860: Support IPA exception and validate exception extensions #321
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
Conversation
{ | ||
name: 'invalid paths with exceptions', | ||
document: { | ||
paths: { | ||
'/api/atlas/v2/unauth/resourceName1/resourceName2': { | ||
'x-xgen-IPA-exception': { | ||
'xgen-IPA-102-path-alternate-resource-name-path-param': { | ||
reason: 'test', | ||
}, | ||
}, | ||
}, | ||
'/api/atlas/v2/resourceName/{pathParam1}/{pathParam2}': { | ||
'x-xgen-IPA-exception': { | ||
'xgen-IPA-102-path-alternate-resource-name-path-param': { | ||
reason: 'test', | ||
}, | ||
}, | ||
}, | ||
}, | ||
}, | ||
errors: [], |
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.
Expect no errors when component has exception
{ | ||
name: 'invalid method with exception', | ||
document: { | ||
paths: { | ||
'/standard': { | ||
post: {}, | ||
get: {}, | ||
'x-xgen-IPA-exception': { | ||
'xgen-IPA-104-resource-has-GET': { | ||
reason: 'test', | ||
}, | ||
}, | ||
}, | ||
'/standard/{exampleId}': { | ||
patch: {}, | ||
delete: {}, | ||
}, | ||
}, | ||
}, | ||
errors: [], | ||
}, |
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.
Expect no errors when component has exception
* @param ruleName the name of the exempted rule | ||
* @returns {boolean} true if the object has an exception named ruleName, otherwise false | ||
*/ | ||
export function hasException(object, ruleName) { |
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.
Q: Do we accept if the exception is not in the correct format?
Because the exception format will be evaluated within the scope of another rule, and it will not affect the acceptance of the rule exception. WDYT?
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.
Yeah I decided not to check the format here since we have a rule that covers it already, so here we assume the extension has keys with the rule names
function isValidException(ruleName, exception) { | ||
const exceptionObjectKeys = Object.keys(exception); | ||
return ( | ||
ruleName.startsWith(RULE_NAME_PREFIX) && |
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.
Q: Should we check if the ruleName
is among the rules defined?
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.
Yeah I was thinking about it but couldn't figure out how to do it without having to introduce a list of some sorts with the rules names, that would be manually updated any time there are new rules, I'll have a think if we can achieve this in some way
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.
Yes, I think you would need another helper function which loads ipa-spectral.yaml
to a Spectral object and fetch the rule names from there
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.
I think it is fine by now, it can be an improvement for later
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.
Cool, I can create a ticket for consideration as a nice to have later
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.
LGTM 🚀
Proposed changes
Adds ability to ignore API components with IPA exception extensions to existing IPA rules:
xgen-IPA-104-resource-has-GET
xgen-IPA-102-path-alternate-resource-name-path-param
Also added a IPA rule validation
xgen-IPA-005-exception-extension-format
to check the format of anyx-xgen-IPA-exception
extensions. This rule does not accept exceptions, and should not.Jira ticket: CLOUDP-288860
Testing
x-xgen-IPA-exception
extension is present with the corresponding rule namexgen-IPA-005-exception-extension-format
rule to validate that it catches invalid exception extensionshasException
helper function