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..ee4e58d29 --- /dev/null +++ b/src/typescript/azure-openapi-validator/rules/PageableOperation.ts @@ -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'; +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) }; + } + } + } +}); 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..6d2f1b7a8 --- /dev/null +++ b/src/typescript/azure-openapi-validator/rules/utilities/rules-helper.ts @@ -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[] = (schemaRef).split('/'); + const schemaProperties = doc.definitions[schemaPath[2]].properties; + return schemaProperties; + } + return schema.properties; +} \ 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 dfa190892..4054d1013 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,16 @@ 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 (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 new file mode 100644 index 000000000..b7e8b718e --- /dev/null +++ b/src/typescript/azure-openapi-validator/tests/resources/PageableOperation.json @@ -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": {} +} \ No newline at end of file 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