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"