Repository: nestjs/swagger
Pull request #2877 (2024-03-12 21:09:04)

fix(): Updated types for Specific ApiResponse decorator options to omit status
Decorators like ApiOkResponse always override the status to a specific code. 
Devs might think they are overriding the status but it's actually a no-op. 
Updated the types of the options to raise an error if status is passed.

## PR Checklist
Please check if your PR fulfills the following requirements:

- [X] The commit message follows our guidelines: https://github.com/nestjs/nest/blob/master/CONTRIBUTING.md
- [X] Tests for the changes have been added (for bug fixes / features)
- [ ] Docs have been added / updated (for bug fixes / features)


## PR Type
What kind of change does this PR introduce?

<!-- Please check the one that applies to this PR using "x". -->
- [X] Bugfix
- [ ] Feature
- [ ] Code style update (formatting, local variables)
- [ ] Refactoring (no functional changes, no api changes)
- [ ] Build related changes
- [ ] CI related changes
- [ ] Other... Please describe:

## What is the current behavior?
<!-- Please describe the current behavior that you are modifying, or link to a relevant issue. -->

Issue Number: N/A

Someone specifying a method like
```ts
@Controller('tests/:testId')
class TestAppController {
  @Get()
  @ApiOkResponse({ status: 201 })
  public get(@Param('testId') testId: string): string {
    return testId;
  }
}
```
Might think it will return a 201 response, But it's easy to miss the `Ok` in `ApiOkResponse` vs `ApiResponse`

## What is the new behavior?

The code above will not compile because `status` is no longer an accepted option for Response decorators that override the status

## Does this PR introduce a breaking change?
- [X] Yes
- [ ] No

<!-- If this PR contains a breaking change, please describe the impact and migration path for existing applications below. -->

Migration path is to remove `status` options on affected Response decorators as they were unused.
If the Response needed to have a different status, instead replace the decorator with `ApiResponse`

## Other information

An alternative would be to change the decorators to

```ts
export const ApiOkResponse = (options: ApiResponseOptions= {}) =>
  ApiResponse({
    status: HttpStatus.OK,
    ...options,
  });
```

Allowing to override the status, but that sounds even more dangerous because it would change the runtime of existing code instead of just having a compile error


Patch:
diff --git a/lib/decorators/api-response.decorator.ts b/lib/decorators/api-response.decorator.ts
index 092c0b76..27380900 100644
--- a/lib/decorators/api-response.decorator.ts
+++ b/lib/decorators/api-response.decorator.ts
@@ -24,6 +24,9 @@ export interface ApiResponseSchemaHost
 }
 
 export type ApiResponseOptions = ApiResponseMetadata | ApiResponseSchemaHost;
+export type ApiResponseNoStatusOptions =
+  | Omit<ApiResponseMetadata, 'status'>
+  | Omit<ApiResponseSchemaHost, 'status'>;
 
 export function ApiResponse(
   options: ApiResponseOptions,
@@ -81,150 +84,178 @@ export function ApiResponse(
   };
 }
 
-export const ApiOkResponse = (options: ApiResponseOptions = {}) =>
+export const ApiOkResponse = (options: ApiResponseNoStatusOptions = {}) =>
   ApiResponse({
     ...options,
     status: HttpStatus.OK
   });
 
-export const ApiCreatedResponse = (options: ApiResponseOptions = {}) =>
+export const ApiCreatedResponse = (options: ApiResponseNoStatusOptions = {}) =>
   ApiResponse({
     ...options,
     status: HttpStatus.CREATED
   });
 
-export const ApiAcceptedResponse = (options: ApiResponseOptions = {}) =>
+export const ApiAcceptedResponse = (options: ApiResponseNoStatusOptions = {}) =>
   ApiResponse({
     ...options,
     status: HttpStatus.ACCEPTED
   });
 
-export const ApiNoContentResponse = (options: ApiResponseOptions = {}) =>
+export const ApiNoContentResponse = (
+  options: ApiResponseNoStatusOptions = {}
+) =>
   ApiResponse({
     ...options,
     status: HttpStatus.NO_CONTENT
   });
 
-export const ApiMovedPermanentlyResponse = (options: ApiResponseOptions = {}) =>
+export const ApiMovedPermanentlyResponse = (
+  options: ApiResponseNoStatusOptions = {}
+) =>
   ApiResponse({
     ...options,
     status: HttpStatus.MOVED_PERMANENTLY
   });
 
-export const ApiFoundResponse = (options: ApiResponseOptions = {}) =>
+export const ApiFoundResponse = (options: ApiResponseNoStatusOptions = {}) =>
   ApiResponse({
     ...options,
     status: HttpStatus.FOUND
   });
 
-export const ApiBadRequestResponse = (options: ApiResponseOptions = {}) =>
+export const ApiBadRequestResponse = (
+  options: ApiResponseNoStatusOptions = {}
+) =>
   ApiResponse({
     ...options,
     status: HttpStatus.BAD_REQUEST
   });
 
-export const ApiUnauthorizedResponse = (options: ApiResponseOptions = {}) =>
+export const ApiUnauthorizedResponse = (
+  options: ApiResponseNoStatusOptions = {}
+) =>
   ApiResponse({
     ...options,
     status: HttpStatus.UNAUTHORIZED
   });
 
-export const ApiTooManyRequestsResponse = (options: ApiResponseOptions = {}) =>
+export const ApiTooManyRequestsResponse = (
+  options: ApiResponseNoStatusOptions = {}
+) =>
   ApiResponse({
     ...options,
     status: HttpStatus.TOO_MANY_REQUESTS
   });
 
-export const ApiNotFoundResponse = (options: ApiResponseOptions = {}) =>
+export const ApiNotFoundResponse = (options: ApiResponseNoStatusOptions = {}) =>
   ApiResponse({
     ...options,
     status: HttpStatus.NOT_FOUND
   });
 
 export const ApiInternalServerErrorResponse = (
-  options: ApiResponseOptions = {}
+  options: ApiResponseNoStatusOptions = {}
 ) =>
   ApiResponse({
     ...options,
     status: HttpStatus.INTERNAL_SERVER_ERROR
   });
 
-export const ApiBadGatewayResponse = (options: ApiResponseOptions = {}) =>
+export const ApiBadGatewayResponse = (
+  options: ApiResponseNoStatusOptions = {}
+) =>
   ApiResponse({
     ...options,
     status: HttpStatus.BAD_GATEWAY
   });
 
-export const ApiConflictResponse = (options: ApiResponseOptions = {}) =>
+export const ApiConflictResponse = (options: ApiResponseNoStatusOptions = {}) =>
   ApiResponse({
     ...options,
     status: HttpStatus.CONFLICT
   });
 
-export const ApiForbiddenResponse = (options: ApiResponseOptions = {}) =>
+export const ApiForbiddenResponse = (
+  options: ApiResponseNoStatusOptions = {}
+) =>
   ApiResponse({
     ...options,
     status: HttpStatus.FORBIDDEN
   });
 
-export const ApiGatewayTimeoutResponse = (options: ApiResponseOptions = {}) =>
+export const ApiGatewayTimeoutResponse = (
+  options: ApiResponseNoStatusOptions = {}
+) =>
   ApiResponse({
     ...options,
     status: HttpStatus.GATEWAY_TIMEOUT
   });
 
-export const ApiGoneResponse = (options: ApiResponseOptions = {}) =>
+export const ApiGoneResponse = (options: ApiResponseNoStatusOptions = {}) =>
   ApiResponse({
     ...options,
     status: HttpStatus.GONE
   });
 
-export const ApiMethodNotAllowedResponse = (options: ApiResponseOptions = {}) =>
+export const ApiMethodNotAllowedResponse = (
+  options: ApiResponseNoStatusOptions = {}
+) =>
   ApiResponse({
     ...options,
     status: HttpStatus.METHOD_NOT_ALLOWED
   });
 
-export const ApiNotAcceptableResponse = (options: ApiResponseOptions = {}) =>
+export const ApiNotAcceptableResponse = (
+  options: ApiResponseNoStatusOptions = {}
+) =>
   ApiResponse({
     ...options,
     status: HttpStatus.NOT_ACCEPTABLE
   });
 
-export const ApiNotImplementedResponse = (options: ApiResponseOptions = {}) =>
+export const ApiNotImplementedResponse = (
+  options: ApiResponseNoStatusOptions = {}
+) =>
   ApiResponse({
     ...options,
     status: HttpStatus.NOT_IMPLEMENTED
   });
 
 export const ApiPreconditionFailedResponse = (
-  options: ApiResponseOptions = {}
+  options: ApiResponseNoStatusOptions = {}
 ) =>
   ApiResponse({
     ...options,
     status: HttpStatus.PRECONDITION_FAILED
   });
 
-export const ApiPayloadTooLargeResponse = (options: ApiResponseOptions = {}) =>
+export const ApiPayloadTooLargeResponse = (
+  options: ApiResponseNoStatusOptions = {}
+) =>
   ApiResponse({
     ...options,
     status: HttpStatus.PAYLOAD_TOO_LARGE
   });
 
-export const ApiPaymentRequiredResponse = (options: ApiResponseOptions = {}) =>
+export const ApiPaymentRequiredResponse = (
+  options: ApiResponseNoStatusOptions = {}
+) =>
   ApiResponse({
     ...options,
     status: HttpStatus.PAYMENT_REQUIRED
   });
 
-export const ApiRequestTimeoutResponse = (options: ApiResponseOptions = {}) =>
+export const ApiRequestTimeoutResponse = (
+  options: ApiResponseNoStatusOptions = {}
+) =>
   ApiResponse({
     ...options,
     status: HttpStatus.REQUEST_TIMEOUT
   });
 
 export const ApiServiceUnavailableResponse = (
-  options: ApiResponseOptions = {}
+  options: ApiResponseNoStatusOptions = {}
 ) =>
   ApiResponse({
     ...options,
@@ -232,7 +263,7 @@ export const ApiServiceUnavailableResponse = (
   });
 
 export const ApiUnprocessableEntityResponse = (
-  options: ApiResponseOptions = {}
+  options: ApiResponseNoStatusOptions = {}
 ) =>
   ApiResponse({
     ...options,
@@ -240,14 +271,14 @@ export const ApiUnprocessableEntityResponse = (
   });
 
 export const ApiUnsupportedMediaTypeResponse = (
-  options: ApiResponseOptions = {}
+  options: ApiResponseNoStatusOptions = {}
 ) =>
   ApiResponse({
     ...options,
     status: HttpStatus.UNSUPPORTED_MEDIA_TYPE
   });
 
-export const ApiDefaultResponse = (options: ApiResponseOptions = {}) =>
+export const ApiDefaultResponse = (options: ApiResponseNoStatusOptions = {}) =>
   ApiResponse({
     ...options,
     status: 'default'


Test patch:
diff --git a/test/decorators/api-response.decorator.spec.ts b/test/decorators/api-response.decorator.spec.ts
new file mode 100644
index 00000000..0db528c1
--- /dev/null
+++ b/test/decorators/api-response.decorator.spec.ts
@@ -0,0 +1,154 @@
+import { Controller, Get, HttpStatus, Param } from '@nestjs/common';
+import { DECORATORS } from '../../lib/constants';
+import {
+  ApiAcceptedResponse,
+  ApiBadGatewayResponse,
+  ApiBadRequestResponse,
+  ApiConflictResponse,
+  ApiCreatedResponse,
+  ApiDefaultResponse,
+  ApiForbiddenResponse,
+  ApiFoundResponse,
+  ApiGatewayTimeoutResponse,
+  ApiGoneResponse,
+  ApiInternalServerErrorResponse,
+  ApiMethodNotAllowedResponse,
+  ApiMovedPermanentlyResponse,
+  ApiNoContentResponse,
+  ApiNotAcceptableResponse,
+  ApiNotFoundResponse,
+  ApiNotImplementedResponse,
+  ApiOkResponse,
+  ApiPayloadTooLargeResponse,
+  ApiPaymentRequiredResponse,
+  ApiPreconditionFailedResponse,
+  ApiRequestTimeoutResponse,
+  ApiResponse,
+  ApiServiceUnavailableResponse,
+  ApiTooManyRequestsResponse,
+  ApiUnauthorizedResponse,
+  ApiUnprocessableEntityResponse,
+  ApiUnsupportedMediaTypeResponse
+} from '../../lib/decorators';
+
+describe('ApiResponse', () => {
+  describe('when applied on the method level', () => {
+    @Controller('tests/:testId')
+    class TestAppController {
+      @Get()
+      @ApiResponse({ status: 204 })
+      public get(@Param('testId') testId: string): string {
+        return testId;
+      }
+    }
+
+    it('should attach metadata to a given method', () => {
+      const controller = new TestAppController();
+      expect(
+        Reflect.hasMetadata(DECORATORS.API_RESPONSE, controller.get)
+      ).toBeTruthy();
+      expect(
+        Reflect.getMetadata(DECORATORS.API_RESPONSE, controller.get)
+      ).toEqual({
+        '204': { description: '', isArray: undefined, type: undefined }
+      });
+    });
+
+    it.each([
+      { decorator: ApiOkResponse, status: HttpStatus.OK },
+      { decorator: ApiCreatedResponse, status: HttpStatus.CREATED },
+      { decorator: ApiAcceptedResponse, status: HttpStatus.ACCEPTED },
+      { decorator: ApiNoContentResponse, status: HttpStatus.NO_CONTENT },
+      {
+        decorator: ApiMovedPermanentlyResponse,
+        status: HttpStatus.MOVED_PERMANENTLY
+      },
+      { decorator: ApiFoundResponse, status: HttpStatus.FOUND },
+      { decorator: ApiBadRequestResponse, status: HttpStatus.BAD_REQUEST },
+      { decorator: ApiUnauthorizedResponse, status: HttpStatus.UNAUTHORIZED },
+      {
+        decorator: ApiTooManyRequestsResponse,
+        status: HttpStatus.TOO_MANY_REQUESTS
+      },
+      { decorator: ApiNotFoundResponse, status: HttpStatus.NOT_FOUND },
+      {
+        decorator: ApiInternalServerErrorResponse,
+        status: HttpStatus.INTERNAL_SERVER_ERROR
+      },
+      { decorator: ApiBadGatewayResponse, status: HttpStatus.BAD_GATEWAY },
+      { decorator: ApiConflictResponse, status: HttpStatus.CONFLICT },
+      { decorator: ApiForbiddenResponse, status: HttpStatus.FORBIDDEN },
+      {
+        decorator: ApiGatewayTimeoutResponse,
+        status: HttpStatus.GATEWAY_TIMEOUT
+      },
+      { decorator: ApiGoneResponse, status: HttpStatus.GONE },
+      {
+        decorator: ApiMethodNotAllowedResponse,
+        status: HttpStatus.METHOD_NOT_ALLOWED
+      },
+      {
+        decorator: ApiNotAcceptableResponse,
+        status: HttpStatus.NOT_ACCEPTABLE
+      },
+      {
+        decorator: ApiNotImplementedResponse,
+        status: HttpStatus.NOT_IMPLEMENTED
+      },
+      {
+        decorator: ApiPreconditionFailedResponse,
+        status: HttpStatus.PRECONDITION_FAILED
+      },
+      {
+        decorator: ApiPayloadTooLargeResponse,
+        status: HttpStatus.PAYLOAD_TOO_LARGE
+      },
+      {
+        decorator: ApiPaymentRequiredResponse,
+        status: HttpStatus.PAYMENT_REQUIRED
+      },
+      {
+        decorator: ApiRequestTimeoutResponse,
+        status: HttpStatus.REQUEST_TIMEOUT
+      },
+      {
+        decorator: ApiServiceUnavailableResponse,
+        status: HttpStatus.SERVICE_UNAVAILABLE
+      },
+      {
+        decorator: ApiUnprocessableEntityResponse,
+        status: HttpStatus.UNPROCESSABLE_ENTITY
+      },
+      {
+        decorator: ApiUnsupportedMediaTypeResponse,
+        status: HttpStatus.UNSUPPORTED_MEDIA_TYPE
+      },
+      { decorator: ApiDefaultResponse, status: 'default' }
+    ] as const)(
+      'should not allow to override status of $decorator.name[$status]',
+      ({ decorator, status }) => {
+        @Controller('tests/:testId')
+        class TestAppController {
+          @Get()
+          @decorator({
+            // @ts-expect-error -- Should error if user tries to override status
+            status: 2010
+          })
+          public get(@Param('testId') testId: string): string {
+            return testId;
+          }
+        }
+
+        const controller = new TestAppController();
+        expect(
+          Reflect.hasMetadata(DECORATORS.API_RESPONSE, controller.get)
+        ).toBeTruthy();
+        expect(
+          Reflect.getMetadata(DECORATORS.API_RESPONSE, controller.get)
+        ).toEqual({
+          [status]: { description: '', isArray: undefined, type: undefined }
+        });
+      }
+    );
+  });
+});