From c98a202ea4cd7aa1c0b02dc8a3d2f27b1358bbb8 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Fran=C3=A7ois=20Wouts?= Date: Fri, 22 Feb 2019 09:53:20 +1100 Subject: [PATCH] Add a `spot lint` command with two example rules (#106) The `lint` command will help ensure that an API follows a set of reasonable requirements. We're starting by defining linting rules based on Airtasker's API design guidelines, but the goal is to make it an extendable set over time. The command will be invoked with: ``` spot lint contract.ts ``` The plan is to later introduce a linting configuration file, e.g. `.spotlintrc` which would be automatically found based on the contract's path. Or we could make that provided via a flag. This will be defined later. As part of implementing the `lint` command, we need a way to access the `ContractNode`, which has useful debugging information such as the source location. The `safeParse()` function is therefore updated to return both the source node and the cleansed node. --- README.md | 21 +++ cli/src/commands/generate.ts | 2 +- cli/src/commands/lint.ts | 40 +++++ cli/src/commands/mock.ts | 2 +- cli/src/commands/validate.ts | 2 +- cli/src/common/safe-parse.ts | 8 +- lib/src/linting/linter.ts | 15 ++ lib/src/linting/rule.ts | 15 ++ lib/src/linting/rules.ts | 9 + .../linting/rules/has-request-payload.spec.ts | 146 +++++++++++++++ lib/src/linting/rules/has-request-payload.ts | 57 ++++++ .../rules/has-response-payload.spec.ts | 170 ++++++++++++++++++ lib/src/linting/rules/has-response-payload.ts | 52 ++++++ package.json | 2 + yarn.lock | 10 ++ 15 files changed, 546 insertions(+), 5 deletions(-) create mode 100644 cli/src/commands/lint.ts create mode 100644 lib/src/linting/linter.ts create mode 100644 lib/src/linting/rule.ts create mode 100644 lib/src/linting/rules.ts create mode 100644 lib/src/linting/rules/has-request-payload.spec.ts create mode 100644 lib/src/linting/rules/has-request-payload.ts create mode 100644 lib/src/linting/rules/has-response-payload.spec.ts create mode 100644 lib/src/linting/rules/has-response-payload.ts diff --git a/README.md b/README.md index f34d012dd..1a37c2df2 100644 --- a/README.md +++ b/README.md @@ -114,6 +114,7 @@ npx @airtasker/spot generate --contract api.ts * [`spot generate`](#spot-generate) * [`spot help [COMMAND]`](#spot-help-command) * [`spot init`](#spot-init) +* [`spot lint SPOT_CONTRACT`](#spot-lint-spot-contract) * [`spot mock SPOT_CONTRACT`](#spot-mock-spot-contract) * [`spot validate SPOT_CONTRACT`](#spot-validate-spot-contract) @@ -176,6 +177,26 @@ EXAMPLE _See code: [build/cli/src/commands/init.js](https://github.com/airtasker/spot/blob/v0.2.0/build/cli/src/commands/init.js)_ +## `spot lint SPOT_CONTRACT` + +Lint a Spot contract + +``` +USAGE + $ spot lint SPOT_CONTRACT + +ARGUMENTS + SPOT_CONTRACT path to Spot contract + +OPTIONS + -h, --help show CLI help + +EXAMPLE + $ spot lint api.ts +``` + +_See code: [build/cli/src/commands/lint.js](https://github.com/airtasker/spot/blob/v0.2.0/build/cli/src/commands/lint.js)_ + ## `spot mock SPOT_CONTRACT` Run a mock server based on a Spot contract diff --git a/cli/src/commands/generate.ts b/cli/src/commands/generate.ts index 6c7650551..6b3e68185 100644 --- a/cli/src/commands/generate.ts +++ b/cli/src/commands/generate.ts @@ -42,7 +42,7 @@ export default class Generate extends Command { const { flags } = this.parse(Generate); let { contract: contractPath, language, generator, out: outDir } = flags; const contractFilename = path.basename(contractPath, ".ts"); - const contract = safeParse.bind(this)(contractPath); + const contract = safeParse.call(this, contractPath).definition; if (!generator) { generator = (await prompt<{ Generator: string; diff --git a/cli/src/commands/lint.ts b/cli/src/commands/lint.ts new file mode 100644 index 000000000..ceed9f259 --- /dev/null +++ b/cli/src/commands/lint.ts @@ -0,0 +1,40 @@ +import { Command, flags } from "@oclif/command"; +import { lint } from "../../../lib/src/linting/linter"; +import { safeParse } from "../common/safe-parse"; + +const ARG_API = "spot_contract"; + +/** + * oclif command to lint a spot contract + */ +export default class Lint extends Command { + static description = "Lint a Spot contract"; + + static examples = ["$ spot lint api.ts"]; + + static args = [ + { + name: ARG_API, + required: true, + description: "path to Spot contract", + hidden: false + } + ]; + + static flags = { + help: flags.help({ char: "h" }) + }; + + async run() { + const { args } = this.parse(Lint); + const contractPath = args[ARG_API]; + const parsedContract = safeParse.call(this, contractPath).source; + // TODO: Make it possible to specify with a config file which lint rules to enable. + const lintingErrors = lint(parsedContract); + lintingErrors.forEach(error => { + this.error( + `${error.source.location}#${error.source.line}: ${error.message}` + ); + }); + } +} diff --git a/cli/src/commands/mock.ts b/cli/src/commands/mock.ts index 90c212421..cf9eb9416 100644 --- a/cli/src/commands/mock.ts +++ b/cli/src/commands/mock.ts @@ -40,7 +40,7 @@ export default class Mock extends Command { flags: { port, pathPrefix } } = this.parse(Mock); try { - const contract = safeParse.bind(this)(args[ARG_API]); + const contract = safeParse.call(this, args[ARG_API]).definition; await runMockServer(contract, { port, pathPrefix: pathPrefix || "", diff --git a/cli/src/commands/validate.ts b/cli/src/commands/validate.ts index 01f30b282..8d0c5207e 100644 --- a/cli/src/commands/validate.ts +++ b/cli/src/commands/validate.ts @@ -26,7 +26,7 @@ export default class Validate extends Command { async run() { const { args } = this.parse(Validate); - safeParse.bind(this)(args[ARG_API]); + const contract = safeParse.call(this, args[ARG_API]).definition; this.log("Contract is valid"); } } diff --git a/cli/src/common/safe-parse.ts b/cli/src/common/safe-parse.ts index ff70bb2dd..7254710b6 100644 --- a/cli/src/common/safe-parse.ts +++ b/cli/src/common/safe-parse.ts @@ -3,11 +3,12 @@ import { parse } from "../../../lib/src/parsers/parser"; import { verify } from "../../../lib/src/verifiers/verifier"; import { cleanse } from "../../../lib/src/cleansers/cleanser"; import { ContractDefinition } from "../../../lib/src/models/definitions"; +import { ContractNode } from "../../../lib/src/models/nodes"; export function safeParse( this: Command, contractPath: string -): ContractDefinition { +): { definition: ContractDefinition; source: ContractNode } { try { const parsedContract = parse(contractPath); const contractErrors = verify(parsedContract); @@ -17,7 +18,10 @@ export function safeParse( }); throw new Error("Contract is not valid"); } - return cleanse(parsedContract); + return { + definition: cleanse(parsedContract), + source: parsedContract + }; } catch (e) { throw this.error(e, { exit: 1 }); } diff --git a/lib/src/linting/linter.ts b/lib/src/linting/linter.ts new file mode 100644 index 000000000..1462a47ec --- /dev/null +++ b/lib/src/linting/linter.ts @@ -0,0 +1,15 @@ +import { unnest } from "ramda"; +import { ContractNode } from "../models/nodes"; +import { LintingRuleViolation } from "./rule"; +import { availableRules, RuleName } from "./rules"; + +export function lint( + contract: ContractNode, + ruleNames: RuleName[] = Object.keys(availableRules) as RuleName[] +): LintingRuleViolation[] { + return unnest( + ruleNames + .map(ruleName => availableRules[ruleName]) + .map(rule => rule(contract)) + ); +} diff --git a/lib/src/linting/rule.ts b/lib/src/linting/rule.ts new file mode 100644 index 000000000..6fe58489a --- /dev/null +++ b/lib/src/linting/rule.ts @@ -0,0 +1,15 @@ +import { Locatable } from "../models/locatable"; +import { ContractNode } from "../models/nodes"; + +/** + * A linting rule is a function that returns a list of violations, which will + * be empty when the rule is complied with. + */ +export interface LintingRule { + (contract: ContractNode): LintingRuleViolation[]; +} + +export interface LintingRuleViolation { + message: string; + source: Locatable; +} diff --git a/lib/src/linting/rules.ts b/lib/src/linting/rules.ts new file mode 100644 index 000000000..3d234d429 --- /dev/null +++ b/lib/src/linting/rules.ts @@ -0,0 +1,9 @@ +import { hasRequestPayload } from "./rules/has-request-payload"; +import { hasResponsePayload } from "./rules/has-response-payload"; + +export const availableRules = { + "has-request-payload": hasRequestPayload, + "has-response-payload": hasResponsePayload +}; + +export type RuleName = keyof typeof availableRules; diff --git a/lib/src/linting/rules/has-request-payload.spec.ts b/lib/src/linting/rules/has-request-payload.spec.ts new file mode 100644 index 000000000..469fc31a8 --- /dev/null +++ b/lib/src/linting/rules/has-request-payload.spec.ts @@ -0,0 +1,146 @@ +import { HttpMethod } from "../../models/http"; +import { + ApiNode, + BodyNode, + EndpointNode, + PathParamNode, + RequestNode +} from "../../models/nodes"; +import { TypeKind } from "../../models/types"; +import { fakeLocatable } from "../../test/fake-locatable"; +import { hasRequestPayload } from "./has-request-payload"; + +describe("rule: has-request-payload", () => { + test("valid for correct usage", () => { + const errors = hasRequestPayload({ + api: fakeLocatable({ + name: fakeLocatable("example-api") + }), + endpoints: [ + // GET endpoint with no request parameters at all. + fakeLocatable({ + name: fakeLocatable("listUsers"), + method: fakeLocatable("GET"), + path: fakeLocatable("/users"), + tests: [], + responses: [] + }), + // GET endpoint with a request path parameter but no body. + fakeLocatable({ + name: fakeLocatable("getUser"), + method: fakeLocatable("GET"), + path: fakeLocatable("/users/:userId"), + tests: [], + request: fakeLocatable({ + pathParams: fakeLocatable([ + fakeLocatable({ + name: fakeLocatable("userId"), + type: { + kind: TypeKind.STRING + } + }) + ]) + }), + responses: [] + }), + // POST endpoint with a request body. + fakeLocatable({ + name: fakeLocatable("createUser"), + method: fakeLocatable("POST"), + path: fakeLocatable("/users"), + tests: [], + request: fakeLocatable({ + body: fakeLocatable({ + type: { + kind: TypeKind.STRING + } + }) + }), + responses: [] + }) + ], + types: [] + }); + expect(errors).toEqual([]); + }); + + test("rejects GET endpoint with a request body", () => { + const errors = hasRequestPayload({ + api: fakeLocatable({ + name: fakeLocatable("example-api") + }), + endpoints: [ + fakeLocatable({ + name: fakeLocatable("createUser"), + method: fakeLocatable("GET"), + path: fakeLocatable("/users"), + tests: [], + request: fakeLocatable({ + body: fakeLocatable({ + type: { + kind: TypeKind.STRING + } + }) + }), + responses: [] + }) + ], + types: [] + }); + expect(errors).toMatchObject([ + { + message: + "createUser should not have a request payload as its method is GET" + } + ]); + }); + + test("rejects POST endpoint without a request", () => { + const errors = hasRequestPayload({ + api: fakeLocatable({ + name: fakeLocatable("example-api") + }), + endpoints: [ + fakeLocatable({ + name: fakeLocatable("createUser"), + method: fakeLocatable("POST"), + path: fakeLocatable("/users"), + tests: [], + responses: [] + }) + ], + types: [] + }); + expect(errors).toMatchObject([ + { + message: + "createUser should have a request payload as its method is POST" + } + ]); + }); + + test("rejects POST endpoint without a request body", () => { + const errors = hasRequestPayload({ + api: fakeLocatable({ + name: fakeLocatable("example-api") + }), + endpoints: [ + fakeLocatable({ + name: fakeLocatable("createUser"), + method: fakeLocatable("POST"), + path: fakeLocatable("/users"), + tests: [], + request: fakeLocatable({}), + responses: [] + }) + ], + types: [] + }); + expect(errors).toMatchObject([ + { + message: + "createUser should have a request payload as its method is POST" + } + ]); + }); +}); diff --git a/lib/src/linting/rules/has-request-payload.ts b/lib/src/linting/rules/has-request-payload.ts new file mode 100644 index 000000000..9866fe208 --- /dev/null +++ b/lib/src/linting/rules/has-request-payload.ts @@ -0,0 +1,57 @@ +import { Locatable } from "lib/src/models/locatable"; +import { EndpointNode } from "lib/src/models/nodes"; +import { complement } from "ramda"; +import { LintingRule } from "../rule"; + +/** + * Checks that the request payload is defined if and only if the method is POST/PUT/PATCH. + */ +export const hasRequestPayload: LintingRule = contract => { + return [ + ...mutationEndpointsHaveRequestPayload(contract), + ...nonMutationEndpointsDoNotHaveRequestPayload(contract) + ]; +}; + +const mutationEndpointsHaveRequestPayload: LintingRule = contract => { + return contract.endpoints + .filter(isMutationEndpoint) + .filter(complement(endpointHasRequestPayload)) + .map(endpoint => ({ + message: `${ + endpoint.value.name.value + } should have a request payload as its method is ${ + endpoint.value.method.value + }`, + source: endpoint + })); +}; + +const nonMutationEndpointsDoNotHaveRequestPayload: LintingRule = contract => { + return contract.endpoints + .filter(complement(isMutationEndpoint)) + .filter(endpointHasRequestPayload) + .map(endpoint => ({ + message: `${ + endpoint.value.name.value + } should not have a request payload as its method is ${ + endpoint.value.method.value + }`, + source: endpoint + })); +}; + +function isMutationEndpoint(endpoint: Locatable) { + switch (endpoint.value.method.value) { + case "POST": + case "PUT": + case "PATCH": + return true; + default: + return false; + } +} + +function endpointHasRequestPayload(endpoint: Locatable): boolean { + return Boolean(endpoint.value.request && endpoint.value.request.value.body); +} diff --git a/lib/src/linting/rules/has-response-payload.spec.ts b/lib/src/linting/rules/has-response-payload.spec.ts new file mode 100644 index 000000000..2a73495b8 --- /dev/null +++ b/lib/src/linting/rules/has-response-payload.spec.ts @@ -0,0 +1,170 @@ +import { HttpMethod } from "../../models/http"; +import { + ApiNode, + BodyNode, + DefaultResponseNode, + EndpointNode, + ResponseNode +} from "../../models/nodes"; +import { TypeKind } from "../../models/types"; +import { fakeLocatable } from "../../test/fake-locatable"; +import { hasResponsePayload } from "./has-response-payload"; + +describe("rule: has-response-payload", () => { + test("valid for correct usage", () => { + const errors = hasResponsePayload({ + api: fakeLocatable({ + name: fakeLocatable("example-api") + }), + endpoints: [ + // Endpoint with default response. + fakeLocatable({ + name: fakeLocatable("listUsers"), + method: fakeLocatable("GET"), + path: fakeLocatable("/users"), + tests: [], + responses: [], + defaultResponse: fakeLocatable({ + body: fakeLocatable({ + type: { + kind: TypeKind.STRING + } + }) + }) + }), + // Endpoint with one successful response. + fakeLocatable({ + name: fakeLocatable("listUsers"), + method: fakeLocatable("GET"), + path: fakeLocatable("/users"), + tests: [], + responses: [ + fakeLocatable({ + status: fakeLocatable(200), + body: fakeLocatable({ + type: { + kind: TypeKind.STRING + } + }) + }) + ] + }) + ], + types: [] + }); + expect(errors).toEqual([]); + }); + + test("rejects default response with no body", () => { + const errors = hasResponsePayload({ + api: fakeLocatable({ + name: fakeLocatable("example-api") + }), + endpoints: [ + // Endpoint with one successful response. + fakeLocatable({ + name: fakeLocatable("listUsers"), + method: fakeLocatable("GET"), + path: fakeLocatable("/users"), + tests: [], + responses: [], + defaultResponse: fakeLocatable({}) + }) + ], + types: [] + }); + expect(errors).toMatchObject([ + { + message: "default response is missing a body in endpoint listUsers" + } + ]); + }); + + test("rejects success response with no body", () => { + const errors = hasResponsePayload({ + api: fakeLocatable({ + name: fakeLocatable("example-api") + }), + endpoints: [ + // Endpoint with one successful response. + fakeLocatable({ + name: fakeLocatable("listUsers"), + method: fakeLocatable("GET"), + path: fakeLocatable("/users"), + tests: [], + responses: [ + fakeLocatable({ + status: fakeLocatable(200) + }) + ] + }) + ], + types: [] + }); + expect(errors).toMatchObject([ + { + message: + "response for status 200 is missing a body in endpoint listUsers" + } + ]); + }); + + test("rejects endpoint with no responses", () => { + const errors = hasResponsePayload({ + api: fakeLocatable({ + name: fakeLocatable("example-api") + }), + endpoints: [ + fakeLocatable({ + name: fakeLocatable("listUsers"), + method: fakeLocatable("GET"), + path: fakeLocatable("/users"), + tests: [], + responses: [] + }) + ], + types: [] + }); + expect(errors).toMatchObject([ + { + message: "endpoint listUsers does not declare any response" + } + ]); + }); + + test("rejects response without body even if other responses have bodies", () => { + const errors = hasResponsePayload({ + api: fakeLocatable({ + name: fakeLocatable("example-api") + }), + endpoints: [ + fakeLocatable({ + name: fakeLocatable("listUsers"), + method: fakeLocatable("GET"), + path: fakeLocatable("/users"), + tests: [], + responses: [ + fakeLocatable({ + status: fakeLocatable(200), + body: fakeLocatable({ + type: { + kind: TypeKind.STRING + } + }) + }), + fakeLocatable({ + status: fakeLocatable(404) + }) + ] + }) + ], + types: [] + }); + expect(errors).toMatchObject([ + { + message: + "response for status 404 is missing a body in endpoint listUsers" + } + ]); + }); +}); diff --git a/lib/src/linting/rules/has-response-payload.ts b/lib/src/linting/rules/has-response-payload.ts new file mode 100644 index 000000000..10f73a734 --- /dev/null +++ b/lib/src/linting/rules/has-response-payload.ts @@ -0,0 +1,52 @@ +import { Locatable } from "lib/src/models/locatable"; +import { + DefaultResponseNode, + EndpointNode, + ResponseNode +} from "lib/src/models/nodes"; +import { unnest } from "ramda"; +import { LintingRule } from "../rule"; + +/** + * Checks that the response payload is always defined. + */ +export const hasResponsePayload: LintingRule = contract => { + return unnest( + contract.endpoints.map(endpoint => { + const responses = findResponses(endpoint); + if (responses.length === 0) { + return [ + { + message: `endpoint ${ + endpoint.value.name.value + } does not declare any response`, + source: endpoint + } + ]; + } + return responses.filter(hasNoPayload).map(response => ({ + message: `${responseName(response)} is missing a body in endpoint ${ + endpoint.value.name.value + }`, + source: response + })); + }) + ); +}; + +function findResponses(endpoint: Locatable) { + return [ + ...endpoint.value.responses, + ...(endpoint.value.defaultResponse ? [endpoint.value.defaultResponse] : []) + ]; +} + +function hasNoPayload(response: Locatable) { + return !response.value.body; +} + +function responseName(response: Locatable) { + return "status" in response.value + ? `response for status ${response.value.status.value}` + : `default response`; +} diff --git a/package.json b/package.json index ad770c905..169fd63e8 100644 --- a/package.json +++ b/package.json @@ -12,6 +12,7 @@ "@oclif/plugin-help": "^2", "@types/cors": "^2.8.4", "@types/express": "^4.16.0", + "@types/ramda": "^0.25.50", "@types/randomstring": "^1.1.6", "ajv": "^6.9.1", "assert-never": "^1.1.0", @@ -21,6 +22,7 @@ "inquirer": "^6.2.0", "js-yaml": "^3.12.0", "lodash": "^4.17.11", + "ramda": "^0.26.1", "randomstring": "^1.1.5", "ts-morph": "^1.1.0", "tslib": "^1", diff --git a/yarn.lock b/yarn.lock index a3bd50329..983c43b91 100644 --- a/yarn.lock +++ b/yarn.lock @@ -329,6 +329,11 @@ resolved "https://registry.yarnpkg.com/@types/node/-/node-11.9.4.tgz#ceb0048a546db453f6248f2d1d95e937a6f00a14" integrity sha512-Zl8dGvAcEmadgs1tmSPcvwzO1YRsz38bVJQvH1RvRqSR9/5n61Q1ktcDL0ht3FXWR+ZpVmXVwN1LuH4Ax23NsA== +"@types/ramda@^0.25.50": + version "0.25.50" + resolved "https://registry.yarnpkg.com/@types/ramda/-/ramda-0.25.50.tgz#7f21d47c650f85b8a61c2b608adf8aa0f1a8fccf" + integrity sha512-hkGWVWyPEXkXSh7rxJIGg6ELG1/WlR2BB8JemZW8v2l50bfMUw6CjrETJpgd9i3jUxaxOE/X57DvclWd9wkZhg== + "@types/randomstring@^1.1.6": version "1.1.6" resolved "https://registry.yarnpkg.com/@types/randomstring/-/randomstring-1.1.6.tgz#45cdc060a6f043d610bcd46503a6887db2a209c3" @@ -3691,6 +3696,11 @@ qs@6.5.2, qs@~6.5.2: resolved "https://registry.yarnpkg.com/qs/-/qs-6.5.2.tgz#cb3ae806e8740444584ef154ce8ee98d403f3e36" integrity sha512-N5ZAX4/LxJmF+7wN74pUD6qAh9/wnvdQcjq9TZjevvXzSUo7bfmw91saqMjzGS2xq91/odN2dW/WOl7qQHNDGA== +ramda@^0.26.1: + version "0.26.1" + resolved "https://registry.yarnpkg.com/ramda/-/ramda-0.26.1.tgz#8d41351eb8111c55353617fc3bbffad8e4d35d06" + integrity sha512-hLWjpy7EnsDBb0p+Z3B7rPi3GDeRG5ZtiI33kJhTt+ORCd38AbAIjB/9zRIUoeTbE/AVX5ZkU7m6bznsvrf8eQ== + randomstring@^1.1.5: version "1.1.5" resolved "https://registry.yarnpkg.com/randomstring/-/randomstring-1.1.5.tgz#6df0628f75cbd5932930d9fe3ab4e956a18518c3"