CLOUDP-302982: IPA-104 - Get object should have suffix "Response" #465
Conversation
| if (!schema.$ref.endsWith('Request')) { | ||
| return collectAndReturnViolation(path, RULE_NAME, ERROR_MESSAGE_SCHEMA_NAME); | ||
| } | ||
| const schemaRef = getSchemaRef(schema); |
There was a problem hiding this comment.
Created a shared function since this logic is pretty much the same for the new rule as well
There was a problem hiding this comment.
I was looking for that function as well :)
Nice!
| const contentMediaType = path[path.length - 1]; | ||
|
|
||
| if (isCustomMethodIdentifier(resourcePath)) { | ||
| if (isCustomMethodIdentifier(resourcePath) || !contentMediaType.endsWith('json')) { |
There was a problem hiding this comment.
Added check to only apply rule to json content types, since csv and gzip don't really have a schema, it's alright if they don't reference one
There was a problem hiding this comment.
Good call as this might cause failures for new content types. Good to have short spec for it?
There was a problem hiding this comment.
Sure! I'll add to the spec tab
| * @param {object} schema the schema object | ||
| * @returns {string} the schema ref | ||
| */ | ||
| export function getSchemaRef(schema) { |
tools/spectral/ipa/rulesets/functions/getMethodReturnsResponseSuffixedObject.js
Show resolved
Hide resolved
| }, | ||
| 'application/vnd.atlas.2024-01-01+json': { | ||
| schema: { | ||
| $ref: '#/components/schemas/SchemaResponse', |
There was a problem hiding this comment.
What would be our approach for inline schemas (properties defined here).
IMHO we should allow them and skip reporting success
There was a problem hiding this comment.
Currently the rule will fail and tell the API producer to implement a schema ref, same approach as we did in #473
|
|
||
| const componentSchemas = { | ||
| schemas: { | ||
| SchemaResponse: { |
There was a problem hiding this comment.
Technical note to add:
- We going to cover only top response object.
- No oneOf support
- No inline schemas
- No checks for children
- No checks for allOf
There was a problem hiding this comment.
Yup, I'm good with that, can document when merged
| schema: { | ||
| type: 'array', | ||
| items: { | ||
| $ref: '#/components/schemas/Schema', |
There was a problem hiding this comment.
example to ignore
| $ref: '#/components/schemas/Schema', | |
| oneOf: [ {$ref: '#/components/schemas/Schema'}, {$ref: '#/components/schemas/Schema'}} |
| '/resource/{id}/singleton': { | ||
| get: { | ||
| responses: { | ||
| 200: { |
There was a problem hiding this comment.
Optional: Test for two 2xx response codes.
There was a problem hiding this comment.
That would be an invalid json since we cannot have two keys of the same value
3 participants
Proposed changes
Adds rule to check for
Responsesuffix of responses from get methods. Additionally did a little refactor to add a common function as this rule is very similar toxgen-IPA-106-create-method-request-body-is-request-suffixed-object. Also added check so that we validate json content types only.Testing:
Jira ticket: CLOUDP-302982