Merge pull request #4 from darraghoriordan/feat/fixes-and-add-new-swa…

…gger-rules

feat: add some new rules and fixes

.vscode/settings.json

@@ -3,6 +3,7 @@
     "xo.format.enable": true,
     "cSpell.words": [
         "TSES",
+        "darraghor",
         "injectables"
     ],
     "prettier.configPath": ".prettierrc.json",

README.md

@@ -1,5 +1,34 @@
 # Some eslint rules for working with NestJs projects
 
+## Why
+
+### Nest Dependency Injection
+
+There are some things you don't want to forget when working with Nest dependency injection.
+
+The Nest DI is declarative and if you forget to provide an injectable you wont see an error until run time.
+
+If you're using custom providers the errors can be really tricky to figure out because they won't explicitly error about mismatched injected items, you will just get unexpected operation.
+
+### Open Api / Swagger and automatically generating a client for front end
+
+When working with NestJS I generate my front end models using the swagger generated from the nest controllers and models. I have a bunch of rules that are mostly for strict typing for those controllers and models.
+
+They are somewhat opinionated but necessary for clean model generation if using an Open Api model generator.
+
+## Rule list (more details for each rule below)
+
+Nest Modules
+
+-   provided-injected-should-match-factory-parameters
+-   injectable-should-be-provided
+
+Nest Swagger
+
+-   api-property-matches-property-optionality
+-   controllers-should-supply-api-tags
+-   api-method-should-specify-api-operation
+
 ## To install
 
 ```
@@ -86,7 +115,9 @@ There is some additional configuration you can provide for this rule. This is th
 
 ### Rule: api-property-matches-property-optionality
 
-This checks that you have added the correct api property decorator for your swagger documents. There are specific decorators for optional properties
+This checks that you have added the correct api property decorator for your swagger documents.
+
+There are specific decorators for optional properties and using the correct one affects Open Api generation.
 
 The following FAILS because this is an optional property and should have `@ApiPropertyOptional`
 
@@ -107,3 +138,50 @@ class TestClass {
     thisIsAStringProp!: string;
 }
 ```
+
+### Rule: controllers-should-supply-api-tags
+
+If you have more than a handful of api methods the swagger UI is difficult to navigate. It's easier to group api methods by using tags.
+
+This PASSES because it has api tags
+
+```ts
+@ApiTags("my-group-of-methods")
+@Controller("my-controller")
+class TestClass {}
+```
+
+The following FAILS because it's missing api tags
+
+```ts
+@Controller("my-controller")
+class TestClass {}
+```
+
+### Rule: api-method-should-specify-api-operation
+
+If you have an api method like @Get() you should specify the return status code (and type!) by using @ApiOkResponse and the other expected responses. I often leave out 400s and 500s because it's kind of assumed but they should be used if the return type changes!
+
+This PASSES
+
+```ts
+class TestClass {
+    @Get()
+    @ApiOkResponse({type: String, isArray: true})
+    @ApiBadRequestResponse({description: "Bad Request"})
+    public getAll(): Promise<string[]> {
+        return [];
+    }
+}
+```
+
+The following FAILS because it's missing api operation decorators
+
+```ts
+class TestClass {
+    @Get()
+    public getAll(): Promise<string[]> {
+        return [];
+    }
+}
+```

TODO.md

@@ -2,13 +2,8 @@ A list of things to work on
 
 ## swagger rules
 
-- Controller decorator should have api tags
-- methods wit hpost, put etc should have an api operation tag
-- if method returns array or promise array then the api response should have isArray:true
-- objects returned or in the body input should have attributes for swagger
-- optional properties should specify optional. except excluded properties in class transformer.
-- enum properties should specify enums
+-   enum properties should specify enum param, not specify a type and should specify an enum name
+-   if method returns array or promise array then the api response should have isArray:true or a type of [type]
+-   objects returned or in the body input should have attributes for swagger
 
 ## nest js rules
-
-- injectable class must be included as a provider in a module.

src/configs/noSwagger.ts

@@ -5,5 +5,8 @@ export = {
     rules: {
         "@darraghor/nestjs-typed/api-property-matches-property-optionality":
             "off",
+        "@darraghor/nestjs-typed/api-method-should-specify-api-operation":
+            "off",
+        "@darraghor/nestjs-typed/controllers-should-supply-api-tags": "off",
     },
 };

src/configs/recommended.ts

@@ -14,5 +14,8 @@ export = {
         ],
         "@darraghor/nestjs-typed/api-property-matches-property-optionality":
             "error",
+        "@darraghor/nestjs-typed/api-method-should-specify-api-operation":
+            "error",
+        "@darraghor/nestjs-typed/controllers-should-supply-api-tags": "error",
     },
 };

src/rules/apiMethodsShouldSpecifyApiOperation/apiMethodsShouldSpecifyApiOperation.test-data.ts

@@ -0,0 +1,33 @@
+export const testCases = [
+    {
+        moduleCode: `class TestClass {
+            @Get()
+            @ApiOkResponse({ type: String, isArray: true })
+            @ApiBadRequestResponse({ description: "Bad Request" })
+            public getAll(): Promise<string[]> {
+                return [];
+            }
+        }`,
+        shouldUseDecorator: false,
+        message: "all good",
+    },
+    {
+        moduleCode: `class TestClass {
+            @Get()
+            public getAll(): Promise<string[]> {
+                return [];
+            }
+        }`,
+        shouldUseDecorator: true,
+        message: "no api operation decorator",
+    },
+    {
+        moduleCode: `class TestClass {
+            public getAll(): Promise<string[]> {
+                return [];
+            }
+        }`,
+        shouldUseDecorator: false,
+        message: "not an http method",
+    },
+];

src/rules/apiMethodsShouldSpecifyApiOperation/apiMethodsShouldSpecifyApiOperation.test.ts

@@ -0,0 +1,33 @@
+import {typedTokenHelpers} from "../../utils/typedTokenHelpers";
+import {
+    fakeContext,
+    fakeFilePath,
+} from "../../utils/nestModules/nestProvidedInjectableMapper.test-date";
+import {TSESTree} from "@typescript-eslint/types";
+import {testCases} from "./apiMethodsShouldSpecifyApiOperation.test-data";
+import {shouldUseApiOperationDecorator} from "./apiMethodsShouldSpecifyApiOperation";
+
+// should probably be split up into multiple tests
+describe("apiMethodsShouldSpecifyApiOperation", () => {
+    test.each(testCases)(
+        "is an expected response for %#",
+        (testCase: {
+            moduleCode: string;
+            shouldUseDecorator: boolean;
+            message: string;
+        }) => {
+            const ast = typedTokenHelpers.parseStringToAst(
+                testCase.moduleCode,
+                fakeFilePath,
+                fakeContext
+            );
+
+            const shouldUseOptional = shouldUseApiOperationDecorator(
+                // eslint-disable-next-line @typescript-eslint/no-explicit-any
+                (ast.body[0] as TSESTree.ClassDeclaration).body
+                    .body[0] as TSESTree.MethodDefinition
+            );
+            expect(shouldUseOptional).toEqual(testCase.shouldUseDecorator);
+        }
+    );
+});

src/rules/apiMethodsShouldSpecifyApiOperation/apiMethodsShouldSpecifyApiOperation.ts

@@ -0,0 +1,85 @@
+// Import { getParserServices } from "@typescript-eslint/experimental-utils/dist/eslint-utils";
+// import * as tsutils from "tsutils";
+// import { getParserServices } from "@typescript-eslint/experimental-utils/dist/eslint-utils";
+import {TSESTree} from "@typescript-eslint/types";
+import {createRule} from "../../utils/createRule";
+import {typedTokenHelpers} from "../../utils/typedTokenHelpers";
+
+export const shouldUseApiOperationDecorator = (
+    node: TSESTree.MethodDefinition
+): boolean => {
+    const hasApiMethodDecorator = typedTokenHelpers.nodeHasDecoratorsNamed(
+        node,
+        ["Get", "Post", "Put", "Delete", "Patch", "Options", "Head", "All"]
+    );
+
+    const hasApiOperationDecorator = typedTokenHelpers.nodeHasDecoratorsNamed(
+        node,
+        [
+            "ApiResponse",
+            "ApiOkResponse",
+            "ApiCreatedResponse",
+            "ApiAcceptedResponse",
+            "ApiNoContentResponse",
+            "ApiMovedPermanentlyResponse",
+            "ApiFoundResponse",
+            "ApiBadRequestResponse",
+            "ApiUnauthorizedResponse",
+            "ApiTooManyRequestsResponse",
+            "ApiNotFoundResponse",
+            "ApiInternalServerErrorResponse",
+            "ApiBadGatewayResponse",
+            "ApiConflictResponse",
+            "ApiForbiddenResponse",
+            "ApiGatewayTimeoutResponse",
+            "ApiGoneResponse",
+            "ApiMethodNotAllowedResponse",
+            "ApiNotAcceptableResponse",
+            "ApiNotImplementedResponse",
+            "ApiPreconditionFailedResponse",
+            "ApiPayloadTooLargeResponse",
+            "ApiRequestTimeoutResponse",
+            "ApiServiceUnavailableResponse",
+            "ApiUnprocessableEntityResponse",
+            "ApiUnsupportedMediaTypeResponse",
+            "ApiDefaultResponse",
+        ]
+    );
+
+    return hasApiMethodDecorator && !hasApiOperationDecorator;
+};
+
+const rule = createRule({
+    name: "api-method-should-specify-api-operation",
+    meta: {
+        docs: {
+            description:
+                "Api methods should at least specify the expected OK response with @ApiOkResponse. But also add any error responses that might not be expected (e.g. 429)",
+            category: "Best Practices",
+            recommended: false,
+            requiresTypeChecking: true,
+        },
+        messages: {
+            shouldSpecifyApiOperation: `A method decorated with @Get, @Post etc. should specify the expected ApiOperation e.g. @ApiOkResponse(type: MyType)`,
+        },
+        schema: [],
+        type: "suggestion",
+    },
+    defaultOptions: [],
+
+    create(context) {
+        return {
+            // eslint-disable-next-line @typescript-eslint/naming-convention
+            MethodDefinition(node: TSESTree.MethodDefinition): void {
+                if (shouldUseApiOperationDecorator(node)) {
+                    context.report({
+                        node: node,
+                        messageId: "shouldSpecifyApiOperation",
+                    });
+                }
+            },
+        };
+    },
+});
+
+export default rule;

src/rules/apiPropertyMatchesPropertyOptionality/apiPropertyMatchesPropertyOptionality.ts

@@ -8,9 +8,9 @@ import {typedTokenHelpers} from "../../utils/typedTokenHelpers";
 export const shouldUseRequiredDecorator = (
     node: TSESTree.ClassProperty
 ): boolean => {
-    const hasOptionalDecorator = typedTokenHelpers.nodeHasDecoratorNamed(
+    const hasOptionalDecorator = typedTokenHelpers.nodeHasDecoratorsNamed(
         node,
-        "ApiPropertyOptional"
+        ["ApiPropertyOptional"]
     );
 
     const isOptionalPropertyValue =
@@ -22,9 +22,9 @@ export const shouldUseRequiredDecorator = (
 export const shouldUseOptionalDecorator = (
     node: TSESTree.ClassProperty
 ): boolean => {
-    const hasRequiredDecorator = typedTokenHelpers.nodeHasDecoratorNamed(
+    const hasRequiredDecorator = typedTokenHelpers.nodeHasDecoratorsNamed(
         node,
-        "ApiProperty"
+        ["ApiProperty"]
     );
 
     const isOptionalPropertyValue =
@@ -37,7 +37,8 @@ const rule = createRule({
     name: "api-property-matches-property-optionality",
     meta: {
         docs: {
-            description: "Public api methods should have documentation",
+            description:
+                "Properties should have correct @ApiProperty decorators",
             category: "Best Practices",
             recommended: false,
             requiresTypeChecking: true,

src/rules/controllerDecoratedHasApiTags/controllerDecoratedHasApiTags.test-data.ts

@@ -0,0 +1,26 @@
+export const testCases = [
+    {
+        moduleCode: `
+        @ApiTags("my-tag")
+        @Controller("my-controller")
+        class TestClass {
+      }`,
+        shouldUseApiTagsDecorator: false,
+        message: "all good state",
+    },
+    {
+        moduleCode: `
+        @Controller("my-controller")
+        class TestClass {
+      }`,
+        shouldUseApiTagsDecorator: true,
+        message: "missing tag",
+    },
+    {
+        moduleCode: `
+        class TestClass {
+      }`,
+        shouldUseApiTagsDecorator: false,
+        message: "not a controller",
+    },
+];

src/rules/controllerDecoratedHasApiTags/controllerDecoratedHasApiTags.test.ts

@@ -0,0 +1,34 @@
+import {typedTokenHelpers} from "../../utils/typedTokenHelpers";
+import {
+    fakeContext,
+    fakeFilePath,
+} from "../../utils/nestModules/nestProvidedInjectableMapper.test-date";
+import {TSESTree} from "@typescript-eslint/types";
+import {testCases} from "./controllerDecoratedHasApiTags.test-data";
+import {shouldUseApiTagDecorator} from "./controllerDecoratedHasApiTags";
+
+// should probably be split up into multiple tests
+describe("controllerDecoratedHasApiTags", () => {
+    test.each(testCases)(
+        "is an expected response for %#",
+        (testCase: {
+            moduleCode: string;
+            shouldUseApiTagsDecorator: boolean;
+            message: string;
+        }) => {
+            const ast = typedTokenHelpers.parseStringToAst(
+                testCase.moduleCode,
+                fakeFilePath,
+                fakeContext
+            );
+
+            const shouldUseOptional = shouldUseApiTagDecorator(
+                // eslint-disable-next-line @typescript-eslint/no-explicit-any
+                ast.body[0] as TSESTree.ClassDeclaration
+            );
+            expect(shouldUseOptional).toEqual(
+                testCase.shouldUseApiTagsDecorator
+            );
+        }
+    );
+});