-
Couldn't load subscription status.
- Fork 14
CLOUDP-304943: IPA rule for Update request schema suffix #569
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
| * @param {string} ruleName the rule name | ||
| * @returns {[{path, message: string}]} the errors found, or an empty array in case of no errors | ||
| */ | ||
| export function checkSchemaRefSuffixAndReturnErrors(path, contentPerMediaType, expectedSuffix, 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.
To keep things consistent, I think we should keep the checkViolationsAndReturnErrors function and call these helper functions within its try-catch block. I also noticed another similar function: checkResponseCodeAndReturnErrors.
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'd generally prefer to not have functions that basically only call another function. But if you have a strong opinion of always having the checkViolationsAndReturnErrors I can update all current rules in a follow-up
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 understand your concern. I think it is fine, we keep the naming convention similar at least, and it is a kind of best-practice suggestion. No need for update. Thanks!
| #### xgen-IPA-107-update-method-request-body-is-update-request-suffixed-object | ||
|
|
||
|  | ||
| The Update method request should be a `UpdateRequest` suffixed object. |
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.
Nit: This description reads strangely.
| The Update method request should be a `UpdateRequest` suffixed object. | |
| The Update operation request body should reference model with `UpdateRequest` suffix. |
| Rule checks for the following conditions: | ||
| - Applies to PUT/PATCH methods on single resource paths and singleton resources | ||
| - Applies only to JSON content types | ||
| - Validation only applies to schema references to a predefined schema (not inline) |
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.
Unrelated: Would we have linting to prevent inline schemas (we do not have IPA for that)
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.
Not sure if we expect all json schemas to have schema refs, or if there are issues with allowing inline schemas
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.
SGTM. There are issues relevant to the code gen tools. Will take this offline. Thank you for confirming
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 rule
xgen-IPA-107-update-method-request-body-is-update-request-suffixed-objectand also refactor to use a common validation function for all suffix related rules.Jira ticket: CLOUDP-304943