-
Notifications
You must be signed in to change notification settings - Fork 46
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
Add pageable rule #161
Add pageable rule #161
Changes from all commits
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 |
---|---|---|
@@ -0,0 +1,134 @@ | ||
# PageableOperations | ||
|
||
## Description | ||
|
||
This rule appears when you define a get operation, and its response schema has a property that is an array. The operation might be pageable. | ||
|
||
## How to fix | ||
|
||
Add an `x-ms-pageable` extension to the operation. | ||
|
||
## Effect on generated code | ||
|
||
### Before | ||
|
||
#### Spec | ||
|
||
```json5 | ||
… | ||
"paths": { | ||
"/plants": { | ||
"get": { | ||
"operationId": "Stuff_List", | ||
"responses": { | ||
"200": { | ||
"schema": { | ||
"$ref": "#/definitions/StuffList" | ||
} | ||
} | ||
} | ||
} | ||
} | ||
} | ||
… | ||
"definitions": { | ||
"StuffList": { | ||
"type": "object", | ||
"properties": { | ||
"value": { | ||
"type": "array", | ||
"items": { | ||
"$ref": "#/definitions/Stuff" | ||
} | ||
}, | ||
"nextLink": { | ||
"type": "string", | ||
} | ||
} | ||
}, | ||
"Stuff": { | ||
"type": "object", | ||
"properties": { | ||
"name": { | ||
"type": "string", | ||
}, | ||
} | ||
} | ||
} | ||
… | ||
``` | ||
|
||
#### Generated code | ||
|
||
```csharp | ||
public static ProductResult GetMultiplePages(this IPagingOperations operations, string clientRequestId = default(string), PagingGetMultiplePagesOptions pagingGetMultiplePagesOptions = default(PagingGetMultiplePagesOptions)) | ||
{ | ||
return operations.GetMultiplePagesAsync(clientRequestId, pagingGetMultiplePagesOptions).GetAwaiter().GetResult(); | ||
} | ||
``` | ||
|
||
### After | ||
|
||
#### Spec | ||
|
||
```json5 | ||
… | ||
"paths": { | ||
"/plants": { | ||
"get": { | ||
"operationId": "Stuff_List", | ||
"responses": { | ||
"200": { | ||
"schema": { | ||
"$ref": "#/definitions/StuffList" | ||
} | ||
} | ||
} | ||
"x-ms-pageable": { | ||
"nextLinkName": "nextLink" | ||
} | ||
} | ||
} | ||
} | ||
… | ||
"definitions": { | ||
"StuffList": { | ||
"type": "object", | ||
"properties": { | ||
"value": { | ||
"type": "array", | ||
"items": { | ||
"$ref": "#/definitions/Stuff" | ||
} | ||
}, | ||
"nextLink": { | ||
"type": "string", | ||
} | ||
} | ||
}, | ||
"Stuff": { | ||
"type": "object", | ||
"properties": { | ||
"name": { | ||
"type": "string", | ||
}, | ||
} | ||
} | ||
} | ||
… | ||
``` | ||
|
||
#### Generated code | ||
|
||
```csharp | ||
public static IPage<Product> GetMultiplePages(this IPagingOperations operations, string clientRequestId = default(string), PagingGetMultiplePagesOptions pagingGetMultiplePagesOptions = default(PagingGetMultiplePagesOptions)) | ||
{ | ||
return operations.GetMultiplePagesAsync(clientRequestId, pagingGetMultiplePagesOptions).GetAwaiter().GetResult(); | ||
} | ||
… | ||
public static IPage<Product> GetMultiplePagesNext(this IPagingOperations operations, string nextPageLink, string clientRequestId = default(string), PagingGetMultiplePagesOptions pagingGetMultiplePagesOptions = default(PagingGetMultiplePagesOptions)) | ||
{ | ||
return operations.GetMultiplePagesNextAsync(nextPageLink, clientRequestId, pagingGetMultiplePagesOptions).GetAwaiter().GetResult(); | ||
} | ||
} | ||
``` |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,47 @@ | ||
/*--------------------------------------------------------------------------------------------- | ||
* 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. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 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. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I had not heard the 3 properties thing, but it makes sense 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. It would be great if you can talk to one of the PMs regarding this before going through with the implementation |
||
import { getSuccessfulResponseSchema } from './utilities/rules-helper'; | ||
export const PageableOperation: string = "PageableOperation"; | ||
|
||
const jp = require('jsonpath'); | ||
|
||
rules.push({ | ||
id: "R2029", | ||
name: PageableOperation, | ||
severity: "warning", | ||
category: "SDKViolation", | ||
mergeState: MergeStates.composed, | ||
openapiType: OpenApiTypes.arm | OpenApiTypes.dataplane, | ||
appliesTo_JsonQuery: "$.paths[?(@.get)]", | ||
run: function* (doc, node, path) { | ||
const operations = Object.keys(node); | ||
const getKey = operations.find(key => { | ||
return key.toLowerCase() === 'get'; | ||
}); | ||
|
||
var schemaProperties = getSuccessfulResponseSchema(node[getKey], doc) | ||
|
||
function hasArrayProperty(schema) { | ||
let arrayPresent: boolean = false; | ||
Object.keys(schema).forEach(function (key) { | ||
if (schema[key].type === 'array') { | ||
arrayPresent = true; | ||
} | ||
}); | ||
return arrayPresent; | ||
} | ||
|
||
// Why length 3? | ||
// 1 - Array | ||
// 2 - NextLink | ||
// 3 - Count | ||
if (schemaProperties != undefined && schemaProperties != null && Object.keys(schemaProperties).length <= 3) { | ||
if (hasArrayProperty(schemaProperties) && !('x-ms-pageable' in node[getKey])) { | ||
yield { message: `Based on the response model schema, operation '${node[getKey].operationId}' might be pageable. Consider adding the x-ms-pageable extension.`, location: path.concat(getKey) }; | ||
} | ||
} | ||
} | ||
}); |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,42 @@ | ||
/*--------------------------------------------------------------------------------------------- | ||
* Copyright (c) Microsoft Corporation. All rights reserved. | ||
* Licensed under the MIT License. See License.txt in the project root for license information. | ||
*--------------------------------------------------------------------------------------------*/ | ||
|
||
export function getSuccessfulResponseSchema(node, doc): any { | ||
const responses = Object.keys(node['responses']); | ||
const response = getMostSuccessfulResponseKey(responses) | ||
return getResponseSchema(node['responses'][response], doc) | ||
} | ||
|
||
function getMostSuccessfulResponseKey(responses: string[]): string { | ||
var response: string = 'default'; | ||
if (responses.includes('200')) { | ||
response = '200' | ||
} else { | ||
var twoHundreds = []; | ||
responses.forEach(function (value) { | ||
if (value.startsWith('2')) { | ||
twoHundreds.push(value); | ||
} | ||
}) | ||
if (twoHundreds.length > 0) { | ||
response = twoHundreds[0]; | ||
} | ||
} | ||
return response; | ||
} | ||
|
||
function getResponseSchema(response: Object, doc): any { | ||
const schema = response['schema']; | ||
if (schema === undefined || schema === null) { | ||
return; | ||
} | ||
if ('$ref' in schema) { | ||
const schemaRef = response['schema']['$ref']; | ||
const schemaPath: string[] = (<string>schemaRef).split('/'); | ||
const schemaProperties = doc.definitions[schemaPath[2]].properties; | ||
return schemaProperties; | ||
} | ||
return schema.properties; | ||
} |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,63 @@ | ||
{ | ||
"swagger": "2.0", | ||
"info": { | ||
"title": "LinterTests", | ||
"description": "Use this swagger to test the linter to detect response types with arrays as possible pageable operations", | ||
"version": "2016-10-10" | ||
}, | ||
"host": "management.azure.com", | ||
"paths": { | ||
"/plants": { | ||
"get": { | ||
"description": "Get a list of plants.", | ||
"operationId": "Plants_List", | ||
"parameters": [], | ||
"responses": { | ||
"200": { | ||
"description": "OK.", | ||
"schema": { | ||
"$ref": "#/definitions/PlantsList" | ||
} | ||
} | ||
} | ||
} | ||
} | ||
}, | ||
"definitions": { | ||
"PlantsList": { | ||
"type": "object", | ||
"description": "Response to a Pageable operation. This one lists plants", | ||
"readOnly": true, | ||
"properties": { | ||
"value": { | ||
"type": "array", | ||
"description": "List of plants", | ||
"items": { | ||
"$ref": "#/definitions/Plant" | ||
}, | ||
"readOnly": true | ||
}, | ||
"nextLink": { | ||
"type": "string", | ||
"description": "Link to next page of results", | ||
"readOnly": true | ||
} | ||
} | ||
}, | ||
"Plant": { | ||
"type": "object", | ||
"description": "A plant", | ||
"properties": { | ||
"name": { | ||
"type": "string", | ||
"description": "The plant's name" | ||
}, | ||
"edible": { | ||
"type": "boolean", | ||
"description": "Is the plant edible?" | ||
} | ||
} | ||
} | ||
}, | ||
"parameters": {} | ||
} |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,66 @@ | ||
{ | ||
"swagger": "2.0", | ||
"info": { | ||
"title": "LinterTests", | ||
"description": "Use this swagger to test the linter to detect response types with arrays as possible pageable operations", | ||
"version": "2016-10-10" | ||
}, | ||
"host": "management.azure.com", | ||
"paths": { | ||
"/plants": { | ||
"get": { | ||
"description": "Get a list of plants.", | ||
"operationId": "Plants_List", | ||
"x-ms-pageable": { | ||
"nextLinkName": "nextLink" | ||
}, | ||
"parameters": [], | ||
"responses": { | ||
"200": { | ||
"description": "OK.", | ||
"schema": { | ||
"$ref": "#/definitions/PlantsList" | ||
} | ||
} | ||
} | ||
} | ||
} | ||
}, | ||
"definitions": { | ||
"PlantsList": { | ||
"type": "object", | ||
"description": "Response to a Pageable operation. This one lists plants", | ||
"readOnly": true, | ||
"properties": { | ||
"value": { | ||
"type": "array", | ||
"description": "List of plants", | ||
"items": { | ||
"$ref": "#/definitions/Plant" | ||
}, | ||
"readOnly": true | ||
}, | ||
"nextLink": { | ||
"type": "string", | ||
"description": "Link to next page of results", | ||
"readOnly": true | ||
} | ||
} | ||
}, | ||
"Plant": { | ||
"type": "object", | ||
"description": "A plant", | ||
"properties": { | ||
"name": { | ||
"type": "string", | ||
"description": "The plant's name" | ||
}, | ||
"edible": { | ||
"type": "boolean", | ||
"description": "Is the plant edible?" | ||
} | ||
} | ||
} | ||
}, | ||
"parameters": {} | ||
} |
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: Let's be diligent, mark this as
readonly