🐛 If the response body is empty, return an empty JSON instead #59
Conversation
src/auth/auth.controller.ts
Outdated
| @Controller('auth') | ||
| export class AuthController { | ||
| constructor(private authService: AuthService) {} | ||
|
|
||
| @UsePipes(new ValidationPipe(VALIDATION_PIPE_OPTIONS)) | ||
| @ApiOperation({ summary: 'Login endpoint for normal user' }) | ||
| @ApiNoContentResponse() | ||
| @ApiCreatedResponse() |
There was a problem hiding this comment.
@shogo-mitomo
After thinking a bit, I think finally it would be better to just use 200.
Because 201 spec states a new resource is created, and must return the resource in the response payload.
https://developer.mozilla.org/en-US/docs/Web/HTTP/Status/201
The HTTP 201 Created success status response code indicates that the request has succeeded and has led to the creation of a resource. The new resource is effectively created before this response is sent back and the new resource is returned in the body of the message, its location being either the URL of the request, or the content of the Location header.
There was a problem hiding this comment.
@yashmurty
Oh, I understood. Then all of our POST endpoints should return 200, right?
There was a problem hiding this comment.
@shogo-mitomo
You are absolutely right.
Since we never return the created object, and we want to return a non empty response {}, I think 200 would be the most appropriate response code! 🙇 🙏
shogo-mitomo
added
⚠️ DO NOT MERGE
| @Injectable() | ||
| export class CreatedResponseInterceptor implements NestInterceptor { | ||
| intercept(context: ExecutionContext, next: CallHandler): Observable<any> { | ||
| return next.handle().pipe(map((data) => (data === undefined ? {} : data))) |
There was a problem hiding this comment.
@shogo-mitomo I guess we need to use this value as type: {} somehow for swagger documentation?
Also, let's use this interceptor as backup, but also start explicitly returning {} in the response body from controllers as well? 🙇
There was a problem hiding this comment.
I guess we need to use this value as type: {} somehow for swagger documentation?
Oops, I'll add it soon 🙇♂️ Thank you.
There was a problem hiding this comment.
Also, let's use this interceptor as backup, but also start explicitly returning {} in the response body from controllers as well? 🙇
I'm sorry, I didn't quite understand. What are the benefits of this?
There was a problem hiding this comment.
@shogo-mitomo I mean to say, if we new team mate looks at controller file, he will see just return now and guess return response should be empty.
So to make things clear, in controller file as well, let's add return {} explicitly.
But in situation where we forget to add, the interceptor that you added will still take care of it. 🙇
|
@yashmurty I updated them all. Please check it again 🙇♂️ |
| @ApiBadRequestResponse() | ||
| @Post('/users') | ||
| async postAdminUser(@Request() req, @Body() createAdminRequest: CreateAdminRequestDto) { | ||
| @HttpCode(200) |
There was a problem hiding this comment.
@shogo-mitomo I believe we do not need this?
If we were using @ApiResponse, we would need it I guess.
But since we use @ApiOkResponse, it will automatically add 200 code.
Please correct me if I'm wrong? :bow
There was a problem hiding this comment.
@yashmurty
This is not for Swagger.
NestJS returns 201 for POST method by default, so I have to overwrite it 😅
https://docs.nestjs.com/controllers#status-code
There was a problem hiding this comment.
@shogo-mitomo
Oh I See! I understand now. 🙏
Thanks for the clarification! 🙇
| @HttpCode(200) | ||
| async setPositiveFlag(@Body() setPositiveFlag: SetPositiveFlagDto): Promise<CreatedResponse> { | ||
| this.adminsService.setPositiveFlag(setPositiveFlag) | ||
| return {} |
| async adminUserLoginFirebase(@Request() req): Promise<void> { | ||
| return this.authService.adminUserlogin(req.user) | ||
| @HttpCode(200) | ||
| async adminUserLoginFirebase(@Request() req): Promise<CreatedResponse> { |
There was a problem hiding this comment.
Promise<CreatedResponse>
This is great!
Even the return type is explicit now! 🎉
Overview
Closes https://github.com/codeforjapan/contact-tracing-bug-tracking/issues/1
Closes https://github.com/codeforjapan/contact-tracing-bug-tracking/issues/2
Closes https://github.com/codeforjapan/contact-tracing-bug-tracking/issues/3