Skip to content
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

Revert "Revert "Add pageable rule"" #169

Merged
merged 1 commit into from
May 10, 2018
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
134 changes: 134 additions & 0 deletions docs/pageable-operations.md
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();
}
}
```
1 change: 1 addition & 0 deletions src/typescript/azure-openapi-validator/index.ts
Original file line number Diff line number Diff line change
Expand Up @@ -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");
Expand Down
47 changes: 47 additions & 0 deletions src/typescript/azure-openapi-validator/rules/PageableOperation.ts
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';
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
Expand Up @@ -12,11 +12,24 @@ 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"() {
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 (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);
}
}
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": {}
}