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

@@ -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

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;

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}`
+      );
+    });
+  }
+}

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 || "",

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");
   }
 }

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 });
   }

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))
+  );
+}

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<unknown>;
+}

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;

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<ApiNode>({
+        name: fakeLocatable("example-api")
+      }),
+      endpoints: [
+        // GET endpoint with no request parameters at all.
+        fakeLocatable<EndpointNode>({
+          name: fakeLocatable("listUsers"),
+          method: fakeLocatable<HttpMethod>("GET"),
+          path: fakeLocatable("/users"),
+          tests: [],
+          responses: []
+        }),
+        // GET endpoint with a request path parameter but no body.
+        fakeLocatable<EndpointNode>({
+          name: fakeLocatable("getUser"),
+          method: fakeLocatable<HttpMethod>("GET"),
+          path: fakeLocatable("/users/:userId"),
+          tests: [],
+          request: fakeLocatable<RequestNode>({
+            pathParams: fakeLocatable([
+              fakeLocatable<PathParamNode>({
+                name: fakeLocatable("userId"),
+                type: {
+                  kind: TypeKind.STRING
+                }
+              })
+            ])
+          }),
+          responses: []
+        }),
+        // POST endpoint with a request body.
+        fakeLocatable<EndpointNode>({
+          name: fakeLocatable("createUser"),
+          method: fakeLocatable<HttpMethod>("POST"),
+          path: fakeLocatable("/users"),
+          tests: [],
+          request: fakeLocatable<RequestNode>({
+            body: fakeLocatable<BodyNode>({
+              type: {
+                kind: TypeKind.STRING
+              }
+            })
+          }),
+          responses: []
+        })
+      ],
+      types: []
+    });
+    expect(errors).toEqual([]);
+  });
+
+  test("rejects GET endpoint with a request body", () => {
+    const errors = hasRequestPayload({
+      api: fakeLocatable<ApiNode>({
+        name: fakeLocatable("example-api")
+      }),
+      endpoints: [
+        fakeLocatable<EndpointNode>({
+          name: fakeLocatable("createUser"),
+          method: fakeLocatable<HttpMethod>("GET"),
+          path: fakeLocatable("/users"),
+          tests: [],
+          request: fakeLocatable<RequestNode>({
+            body: fakeLocatable<BodyNode>({
+              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<ApiNode>({
+        name: fakeLocatable("example-api")
+      }),
+      endpoints: [
+        fakeLocatable<EndpointNode>({
+          name: fakeLocatable("createUser"),
+          method: fakeLocatable<HttpMethod>("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<ApiNode>({
+        name: fakeLocatable("example-api")
+      }),
+      endpoints: [
+        fakeLocatable<EndpointNode>({
+          name: fakeLocatable("createUser"),
+          method: fakeLocatable<HttpMethod>("POST"),
+          path: fakeLocatable("/users"),
+          tests: [],
+          request: fakeLocatable<RequestNode>({}),
+          responses: []
+        })
+      ],
+      types: []
+    });
+    expect(errors).toMatchObject([
+      {
+        message:
+          "createUser should have a request payload as its method is POST"
+      }
+    ]);
+  });
+});

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<EndpointNode>) {
+  switch (endpoint.value.method.value) {
+    case "POST":
+    case "PUT":
+    case "PATCH":
+      return true;
+    default:
+      return false;
+  }
+}
+
+function endpointHasRequestPayload(endpoint: Locatable<EndpointNode>): boolean {
+  return Boolean(endpoint.value.request && endpoint.value.request.value.body);
+}