From 993550b65c7a67ffba6fe6fd404695522e2251b7 Mon Sep 17 00:00:00 2001 From: mcardosos Date: Thu, 26 Apr 2018 19:07:12 -0700 Subject: [PATCH 1/6] Add pageable operation rule --- docs/pageable-operations.md | 134 ++++++++++++++++++ .../azure-openapi-validator/index.ts | 1 + .../rules/PageableOperation.ts | 39 +++++ .../tests/composite-azure-test.ts | 7 + .../tests/resources/PageableOperation.json | 60 ++++++++ 5 files changed, 241 insertions(+) create mode 100644 docs/pageable-operations.md create mode 100644 src/typescript/azure-openapi-validator/rules/PageableOperation.ts create mode 100644 src/typescript/azure-openapi-validator/tests/resources/PageableOperation.json diff --git a/docs/pageable-operations.md b/docs/pageable-operations.md new file mode 100644 index 000000000..40513b113 --- /dev/null +++ b/docs/pageable-operations.md @@ -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 GetMultiplePages(this IPagingOperations operations, string clientRequestId = default(string), PagingGetMultiplePagesOptions pagingGetMultiplePagesOptions = default(PagingGetMultiplePagesOptions)) +{ + return operations.GetMultiplePagesAsync(clientRequestId, pagingGetMultiplePagesOptions).GetAwaiter().GetResult(); +} +… +public static IPage GetMultiplePagesNext(this IPagingOperations operations, string nextPageLink, string clientRequestId = default(string), PagingGetMultiplePagesOptions pagingGetMultiplePagesOptions = default(PagingGetMultiplePagesOptions)) +{ + return operations.GetMultiplePagesNextAsync(nextPageLink, clientRequestId, pagingGetMultiplePagesOptions).GetAwaiter().GetResult(); +} +} +``` \ No newline at end of file diff --git a/src/typescript/azure-openapi-validator/index.ts b/src/typescript/azure-openapi-validator/index.ts index cf1766517..3b17c96e9 100644 --- a/src/typescript/azure-openapi-validator/index.ts +++ b/src/typescript/azure-openapi-validator/index.ts @@ -7,6 +7,7 @@ import { Message } from "../jsonrpc/types"; import { rules, OpenApiTypes, MergeStates } from "./rule"; // register rules +require("./rules/PageableOperation"); require("./rules/DescriptionMustNotBeNodeName"); require("./rules/ControlCharactersAreNotAllowed"); require("./rules/ArraySchemaMustHaveItems"); diff --git a/src/typescript/azure-openapi-validator/rules/PageableOperation.ts b/src/typescript/azure-openapi-validator/rules/PageableOperation.ts new file mode 100644 index 000000000..3ef0d7ca3 --- /dev/null +++ b/src/typescript/azure-openapi-validator/rules/PageableOperation.ts @@ -0,0 +1,39 @@ +/*--------------------------------------------------------------------------------------------- + * 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'; +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'; + }); + + const schemaRef = node[getKey]['responses']['200']['schema']['$ref']; + const schemaPath: string[] = (schemaRef).split('/'); + const schemaProperties = doc.definitions[schemaPath[2]].properties; + + const valueKey = Object.keys(schemaProperties).find(key => { + return key.toLowerCase() === 'value'; + }); + + if (valueKey != undefined) { + if (schemaProperties[valueKey].type === 'array') { + yield { message: `Operation '${node[getKey].operationId}' might be pageable. Consider adding the x-ms-pageable swagger extension.`, location: path.concat(getKey) }; + + } + } + } +}); diff --git a/src/typescript/azure-openapi-validator/tests/composite-azure-test.ts b/src/typescript/azure-openapi-validator/tests/composite-azure-test.ts index dfa190892..95ea8b780 100644 --- a/src/typescript/azure-openapi-validator/tests/composite-azure-test.ts +++ b/src/typescript/azure-openapi-validator/tests/composite-azure-test.ts @@ -12,6 +12,7 @@ import { collectTestMessagesFromValidator } from './utilities/tests-helper'; import { DescriptionMustNotBeNodeName } from '../rules/DescriptionMustNotBeNodeName'; +import { PageableOperation } from '../rules/PageableOperation'; @suite class CompositeAzureTests { @test async "description should not be property name"() { @@ -19,4 +20,10 @@ import { DescriptionMustNotBeNodeName } from '../rules/DescriptionMustNotBeNodeN 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"() { + const fileName: string = 'PageableOperation.json'; + const messages: Message[] = await collectTestMessagesFromValidator(fileName, OpenApiTypes.arm, MergeStates.composed); + assertValidationRuleCount(messages, PageableOperation, 1); + } } diff --git a/src/typescript/azure-openapi-validator/tests/resources/PageableOperation.json b/src/typescript/azure-openapi-validator/tests/resources/PageableOperation.json new file mode 100644 index 000000000..fc54c6e9c --- /dev/null +++ b/src/typescript/azure-openapi-validator/tests/resources/PageableOperation.json @@ -0,0 +1,60 @@ +{ + "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", + "properties": { + "value": { + "type": "array", + "description": "List of plants", + "items": { + "$ref": "#/definitions/Plant" + } + }, + "nextLink": { + "type": "string", + "description": "Link to next page of results" + } + } + }, + "Plant": { + "type": "object", + "description": "A plant", + "properties": { + "name": { + "type": "string", + "description": "The plant's name" + }, + "edible": { + "type": "boolean", + "description": "Is the plant edible?" + } + } + } + }, + "parameters": {} +} \ No newline at end of file From ad69c6be5f42608cb2fd7ef383c311ed5f01f0b4 Mon Sep 17 00:00:00 2001 From: mcardosos Date: Thu, 26 Apr 2018 19:08:56 -0700 Subject: [PATCH 2/6] Writing --- .../azure-openapi-validator/rules/PageableOperation.ts | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/typescript/azure-openapi-validator/rules/PageableOperation.ts b/src/typescript/azure-openapi-validator/rules/PageableOperation.ts index 3ef0d7ca3..c58d30970 100644 --- a/src/typescript/azure-openapi-validator/rules/PageableOperation.ts +++ b/src/typescript/azure-openapi-validator/rules/PageableOperation.ts @@ -31,7 +31,7 @@ rules.push({ if (valueKey != undefined) { if (schemaProperties[valueKey].type === 'array') { - yield { message: `Operation '${node[getKey].operationId}' might be pageable. Consider adding the x-ms-pageable swagger extension.`, location: path.concat(getKey) }; + 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) }; } } From 666ab06cb6d9f57670e2042e212aeee85d3c9279 Mon Sep 17 00:00:00 2001 From: mcardosos Date: Fri, 27 Apr 2018 14:34:21 -0700 Subject: [PATCH 3/6] Feedback --- .../rules/PageableOperation.ts | 28 +++++--- .../rules/utilities/rules-helper.ts | 11 ++++ .../tests/composite-azure-test.ts | 8 ++- .../tests/resources/PageableOperation.json | 7 +- .../happyPath/PageableOperation.json | 66 +++++++++++++++++++ 5 files changed, 107 insertions(+), 13 deletions(-) create mode 100644 src/typescript/azure-openapi-validator/rules/utilities/rules-helper.ts create mode 100644 src/typescript/azure-openapi-validator/tests/resources/happyPath/PageableOperation.json diff --git a/src/typescript/azure-openapi-validator/rules/PageableOperation.ts b/src/typescript/azure-openapi-validator/rules/PageableOperation.ts index c58d30970..83ce2f56b 100644 --- a/src/typescript/azure-openapi-validator/rules/PageableOperation.ts +++ b/src/typescript/azure-openapi-validator/rules/PageableOperation.ts @@ -3,6 +3,7 @@ * Licensed under the MIT License. See License.txt in the project root for license information. *--------------------------------------------------------------------------------------------*/ import { MergeStates, OpenApiTypes, rules } from '../rule'; +import { getResponseSchema } from './utilities/rules-helper'; export const PageableOperation: string = "PageableOperation"; const jp = require('jsonpath'); @@ -21,18 +22,25 @@ rules.push({ return key.toLowerCase() === 'get'; }); - const schemaRef = node[getKey]['responses']['200']['schema']['$ref']; - const schemaPath: string[] = (schemaRef).split('/'); - const schemaProperties = doc.definitions[schemaPath[2]].properties; + var schemaProperties = getResponseSchema(node[getKey], doc) - const valueKey = Object.keys(schemaProperties).find(key => { - return key.toLowerCase() === 'value'; - }); - - 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) }; + 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 (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) }; } } } diff --git a/src/typescript/azure-openapi-validator/rules/utilities/rules-helper.ts b/src/typescript/azure-openapi-validator/rules/utilities/rules-helper.ts new file mode 100644 index 000000000..58a35b580 --- /dev/null +++ b/src/typescript/azure-openapi-validator/rules/utilities/rules-helper.ts @@ -0,0 +1,11 @@ +/*--------------------------------------------------------------------------------------------- + * Copyright (c) Microsoft Corporation. All rights reserved. + * Licensed under the MIT License. See License.txt in the project root for license information. + *--------------------------------------------------------------------------------------------*/ + +export function getResponseSchema(node, doc): any { + const schemaRef = node['responses']['200']['schema']['$ref']; + const schemaPath: string[] = (schemaRef).split('/'); + const schemaProperties = doc.definitions[schemaPath[2]].properties; + return schemaProperties; +} \ No newline at end of file diff --git a/src/typescript/azure-openapi-validator/tests/composite-azure-test.ts b/src/typescript/azure-openapi-validator/tests/composite-azure-test.ts index 95ea8b780..03a6c3fd6 100644 --- a/src/typescript/azure-openapi-validator/tests/composite-azure-test.ts +++ b/src/typescript/azure-openapi-validator/tests/composite-azure-test.ts @@ -21,9 +21,15 @@ import { PageableOperation } from '../rules/PageableOperation'; assertValidationRuleCount(messages, DescriptionMustNotBeNodeName, 2); } - @test async "operations returning a model including an array might be pageable"() { + @test async "operations returning a model including an array might be pageable (sad path)"() { const fileName: string = 'PageableOperation.json'; const messages: Message[] = await collectTestMessagesFromValidator(fileName, OpenApiTypes.arm, MergeStates.composed); assertValidationRuleCount(messages, PageableOperation, 1); } + + @test async "operations returning a model including an array might be pageable (happy path)"() { + const fileName: string = 'happypath/PageableOperation.json'; + const messages: Message[] = await collectTestMessagesFromValidator(fileName, OpenApiTypes.arm, MergeStates.composed); + assertValidationRuleCount(messages, PageableOperation, 0); + } } diff --git a/src/typescript/azure-openapi-validator/tests/resources/PageableOperation.json b/src/typescript/azure-openapi-validator/tests/resources/PageableOperation.json index fc54c6e9c..b7e8b718e 100644 --- a/src/typescript/azure-openapi-validator/tests/resources/PageableOperation.json +++ b/src/typescript/azure-openapi-validator/tests/resources/PageableOperation.json @@ -27,17 +27,20 @@ "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" + "description": "Link to next page of results", + "readOnly": true } } }, diff --git a/src/typescript/azure-openapi-validator/tests/resources/happyPath/PageableOperation.json b/src/typescript/azure-openapi-validator/tests/resources/happyPath/PageableOperation.json new file mode 100644 index 000000000..4f5e33a10 --- /dev/null +++ b/src/typescript/azure-openapi-validator/tests/resources/happyPath/PageableOperation.json @@ -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": {} +} \ No newline at end of file From c6817ae738ab2c55cfdd79ab90abbfc4f6404729 Mon Sep 17 00:00:00 2001 From: mcardosos Date: Fri, 27 Apr 2018 15:53:06 -0700 Subject: [PATCH 4/6] More feedback --- .../rules/PageableOperation.ts | 6 +-- .../rules/utilities/rules-helper.ts | 38 ++++++++++++++++--- 2 files changed, 36 insertions(+), 8 deletions(-) diff --git a/src/typescript/azure-openapi-validator/rules/PageableOperation.ts b/src/typescript/azure-openapi-validator/rules/PageableOperation.ts index 83ce2f56b..ee4e58d29 100644 --- a/src/typescript/azure-openapi-validator/rules/PageableOperation.ts +++ b/src/typescript/azure-openapi-validator/rules/PageableOperation.ts @@ -3,7 +3,7 @@ * Licensed under the MIT License. See License.txt in the project root for license information. *--------------------------------------------------------------------------------------------*/ import { MergeStates, OpenApiTypes, rules } from '../rule'; -import { getResponseSchema } from './utilities/rules-helper'; +import { getSuccessfulResponseSchema } from './utilities/rules-helper'; export const PageableOperation: string = "PageableOperation"; const jp = require('jsonpath'); @@ -22,7 +22,7 @@ rules.push({ return key.toLowerCase() === 'get'; }); - var schemaProperties = getResponseSchema(node[getKey], doc) + var schemaProperties = getSuccessfulResponseSchema(node[getKey], doc) function hasArrayProperty(schema) { let arrayPresent: boolean = false; @@ -38,7 +38,7 @@ rules.push({ // 1 - Array // 2 - NextLink // 3 - Count - if (Object.keys(schemaProperties).length <= 3) { + 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) }; } diff --git a/src/typescript/azure-openapi-validator/rules/utilities/rules-helper.ts b/src/typescript/azure-openapi-validator/rules/utilities/rules-helper.ts index 58a35b580..211086c9b 100644 --- a/src/typescript/azure-openapi-validator/rules/utilities/rules-helper.ts +++ b/src/typescript/azure-openapi-validator/rules/utilities/rules-helper.ts @@ -3,9 +3,37 @@ * Licensed under the MIT License. See License.txt in the project root for license information. *--------------------------------------------------------------------------------------------*/ -export function getResponseSchema(node, doc): any { - const schemaRef = node['responses']['200']['schema']['$ref']; - const schemaPath: string[] = (schemaRef).split('/'); - const schemaProperties = doc.definitions[schemaPath[2]].properties; - return schemaProperties; +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 ('$ref' in schema) { + const schemaRef = response['schema']['$ref']; + const schemaPath: string[] = (schemaRef).split('/'); + const schemaProperties = doc.definitions[schemaPath[2]].properties; + return schemaProperties; + } + return schema.properties; } \ No newline at end of file From 4b72a484bbba9993cc4f38358d46a1b533d9aa59 Mon Sep 17 00:00:00 2001 From: mcardosos Date: Tue, 1 May 2018 11:00:07 -0700 Subject: [PATCH 5/6] Check for nil --- .../azure-openapi-validator/rules/utilities/rules-helper.ts | 3 +++ 1 file changed, 3 insertions(+) diff --git a/src/typescript/azure-openapi-validator/rules/utilities/rules-helper.ts b/src/typescript/azure-openapi-validator/rules/utilities/rules-helper.ts index 211086c9b..6d2f1b7a8 100644 --- a/src/typescript/azure-openapi-validator/rules/utilities/rules-helper.ts +++ b/src/typescript/azure-openapi-validator/rules/utilities/rules-helper.ts @@ -29,6 +29,9 @@ function getMostSuccessfulResponseKey(responses: string[]): string { 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[] = (schemaRef).split('/'); From 14aa7ce1749732d6ca1739dd917c2c44d6b4f274 Mon Sep 17 00:00:00 2001 From: mcardosos Date: Tue, 1 May 2018 12:06:55 -0700 Subject: [PATCH 6/6] Fix CI --- .../azure-openapi-validator/tests/composite-azure-test.ts | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/typescript/azure-openapi-validator/tests/composite-azure-test.ts b/src/typescript/azure-openapi-validator/tests/composite-azure-test.ts index 03a6c3fd6..4054d1013 100644 --- a/src/typescript/azure-openapi-validator/tests/composite-azure-test.ts +++ b/src/typescript/azure-openapi-validator/tests/composite-azure-test.ts @@ -28,7 +28,7 @@ import { PageableOperation } from '../rules/PageableOperation'; } @test async "operations returning a model including an array might be pageable (happy path)"() { - const fileName: string = 'happypath/PageableOperation.json'; + const fileName: string = 'happyPath/PageableOperation.json'; const messages: Message[] = await collectTestMessagesFromValidator(fileName, OpenApiTypes.arm, MergeStates.composed); assertValidationRuleCount(messages, PageableOperation, 0); }