Add pageable rule #161
Add pageable rule #161
Conversation
|
|
||
| @suite class CompositeAzureTests { | ||
| @test async "description should not be property name"() { | ||
| const fileName: string = 'DescriptionSameAsPropertyName.json'; | ||
| const messages: Message[] = await collectTestMessagesFromValidator(fileName, OpenApiTypes.arm, MergeStates.composed); | ||
| assertValidationRuleCount(messages, DescriptionMustNotBeNodeName, 2); | ||
| } | ||
|
|
||
| @test async "operations returning a model including an array might be pageable"() { |
There was a problem hiding this comment.
Can you add a positive test case too?
|
|
||
| const schemaRef = node[getKey]['responses']['200']['schema']['$ref']; | ||
| const schemaPath: string[] = (<string>schemaRef).split('/'); | ||
| const schemaProperties = doc.definitions[schemaPath[2]].properties; |
There was a problem hiding this comment.
What will happen if the model is defined inline?
There was a problem hiding this comment.
I think we can run with this assumption since there are linter rules in place to flag inline definitions
On the other hand though, moving this to a utility method could be helpful
| const schemaProperties = doc.definitions[schemaPath[2]].properties; | ||
|
|
||
| const valueKey = Object.keys(schemaProperties).find(key => { | ||
| return key.toLowerCase() === 'value'; |
There was a problem hiding this comment.
Is the key - 'value' a mandatory field? Please confirm if an array could be defined without this key
| return key.toLowerCase() === 'value'; | ||
| }); | ||
|
|
||
| if (valueKey != undefined) { |
There was a problem hiding this comment.
Will this be true if the valueKey is null? If so, I am fine
|
The logic behind linter rule IDs is decided by PMs |
| }, | ||
| "nextLink": { | ||
| "type": "string", | ||
| } |
There was a problem hiding this comment.
nit: Let's be diligent, mark this as readonly
| return key.toLowerCase() === 'get'; | ||
| }); | ||
|
|
||
| const schemaRef = node[getKey]['responses']['200']['schema']['$ref']; |
There was a problem hiding this comment.
Most operations should have a 200 response but what happens here if they don't?
|
|
||
| const schemaRef = node[getKey]['responses']['200']['schema']['$ref']; | ||
| const schemaPath: string[] = (<string>schemaRef).split('/'); | ||
| const schemaProperties = doc.definitions[schemaPath[2]].properties; |
There was a problem hiding this comment.
I think we can run with this assumption since there are linter rules in place to flag inline definitions
On the other hand though, moving this to a utility method could be helpful
|
|
||
| if (valueKey != undefined) { | ||
| if (schemaProperties[valueKey].type === 'array') { | ||
| yield { message: `Based on the response model schema, operation '${node[getKey].operationId}' might be pageable. Consider adding the x-ms-pageable swagger extension.`, location: path.concat(getKey) }; |
There was a problem hiding this comment.
nit: let's not call it swagger 😄
| * Copyright (c) Microsoft Corporation. All rights reserved. | ||
| * Licensed under the MIT License. See License.txt in the project root for license information. | ||
| *--------------------------------------------------------------------------------------------*/ | ||
| import { MergeStates, OpenApiTypes, rules } from '../rule'; |
There was a problem hiding this comment.
If I remember correctly, we decided to flag this only if a model had less than 3 properties with one being the value array. There might be models which have a value array and a whole lot of other properties, should we flag this rule for such cases too?
There was a problem hiding this comment.
I had not heard the 3 properties thing, but it makes sense
There was a problem hiding this comment.
It would be great if you can talk to one of the PMs regarding this before going through with the implementation
This reverts commit 4cd01da.
Big question, what is the logic behind the linter rules IDs?