Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

🐛 If the response body is empty, return an empty JSON instead #59

Merged
merged 8 commits into from
Apr 24, 2020
Merged

🐛 If the response body is empty, return an empty JSON instead #59

Show file tree
Hide file tree
Changes from 3 commits
Commits
Show all changes
8 commits
Select commit Hold shift + click to select a range
0526bf8
:memo: Change response status definition 204 to 201
shogo-mitomo Apr 24, 2020
50da197
:shell: npx nest generate interceptor shared/created-response
shogo-mitomo Apr 24, 2020
5d2cf68
:recycle: If the response body is empty, return an empty JSON instead
shogo-mitomo Apr 24, 2020
ff64d08
:memo: Change response status definition 201 to 200
shogo-mitomo Apr 24, 2020
8c34059
:recycle: Change response status 201 to 200
shogo-mitomo Apr 24, 2020
e7e94f6
:recycle: Move created-response.interceptor.* directory
shogo-mitomo Apr 24, 2020
96ef4f7
:shell: npx nest generate class shared/classes/created-response.class
shogo-mitomo Apr 24, 2020
7aa82fd
:recycle: Return empty object as a response of POST endpoints
shogo-mitomo Apr 24, 2020
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
3 changes: 3 additions & 0 deletions src/admins/admins.controller.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -2,6 +2,7 @@ import {
Controller,
Get,
UseGuards,
UseInterceptors,
Post,
Body,
UsePipes,
Expand All @@ -23,11 +24,13 @@ import { FirebaseAdminUserValidateGuard } from '../auth/guards/firebase-admin-us
import { CreateAdminRequestDto, SetPositiveFlagDto } from './dto/create-admin.dto'
import { VALIDATION_PIPE_OPTIONS } from '../constants/validation-pipe'
import { Admin } from './classes/admin.class'
import { CreatedResponseInterceptor } from '../shared/created-response.interceptor'

@ApiTags('admin')
@ApiBearerAuth()
@ApiUnauthorizedResponse()
@UseGuards(FirebaseAdminUserValidateGuard)
@UseInterceptors(CreatedResponseInterceptor)
@Controller('admins')
export class AdminsController {
constructor(private adminsService: AdminsService) {}
Expand Down
9 changes: 6 additions & 3 deletions src/auth/auth.controller.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -4,14 +4,15 @@ import {
Request,
UseGuards,
UsePipes,
UseInterceptors,
ValidationPipe,
Body,
} from '@nestjs/common'
import {
ApiOperation,
ApiTags,
ApiBearerAuth,
ApiNoContentResponse,
ApiCreatedResponse,
ApiUnauthorizedResponse,
ApiBadRequestResponse,
} from '@nestjs/swagger'
Expand All @@ -20,17 +21,19 @@ import { AuthService } from './auth.service'
import { FirebaseAdminUserLoginGuard } from './guards/firebase-admin-user-login.guard'
import { VALIDATION_PIPE_OPTIONS } from '../constants/validation-pipe'
import { LoginNormalUserRequestDto } from './dto/login-normal-user.dto'
import { CreatedResponseInterceptor } from '../shared/created-response.interceptor'

@ApiTags('auth')
@ApiBearerAuth()
@ApiUnauthorizedResponse()
@UseInterceptors(CreatedResponseInterceptor)
@Controller('auth')
export class AuthController {
constructor(private authService: AuthService) {}

@UsePipes(new ValidationPipe(VALIDATION_PIPE_OPTIONS))
@ApiOperation({ summary: 'Login endpoint for normal user' })
@ApiNoContentResponse()
@ApiCreatedResponse()
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@yashmurty
Oh, I understood. Then all of our POST endpoints should return 200, right?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@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! 🙇 🙏

@ApiBadRequestResponse()
@UseGuards(FirebaseNormalUserLoginGuard)
@Post('login')
Expand All @@ -42,7 +45,7 @@ export class AuthController {
}

@ApiOperation({ summary: 'Login endpoint for admin user' })
@ApiNoContentResponse()
@ApiCreatedResponse()
@ApiBadRequestResponse()
@UseGuards(FirebaseAdminUserLoginGuard)
@Post('admin/login')
Expand Down
7 changes: 7 additions & 0 deletions src/shared/created-response.interceptor.spec.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,7 @@
import { CreatedResponseInterceptor } from './created-response.interceptor'

describe('CreatedResponseInterceptor', () => {
it('should be defined', () => {
expect(new CreatedResponseInterceptor()).toBeDefined()
})
})
10 changes: 10 additions & 0 deletions src/shared/created-response.interceptor.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,10 @@
import { CallHandler, ExecutionContext, Injectable, NestInterceptor } from '@nestjs/common'
import { Observable } from 'rxjs'
import { map } from 'rxjs/operators'

@Injectable()
export class CreatedResponseInterceptor implements NestInterceptor {
intercept(context: ExecutionContext, next: CallHandler): Observable<any> {
return next.handle().pipe(map((data) => (data === undefined ? {} : data)))
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Awesome! ✨

Copy link
Member

@yashmurty yashmurty Apr 24, 2020 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@yashmurty

I guess we need to use this value as type: {} somehow for swagger documentation?

Oops, I'll add it soon 🙇‍♂️ Thank you.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@yashmurty

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?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

}
}
3 changes: 3 additions & 0 deletions src/users/users.controller.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -4,6 +4,7 @@ import {
Request,
UseGuards,
UsePipes,
UseInterceptors,
ValidationPipe,
Post,
Body,
Expand All @@ -21,11 +22,13 @@ import { TempID } from './classes/temp-id.class'
import { FirebaseNormalUserValidateGuard } from '../auth/guards/firebase-normal-user-validate.guard'
import { VALIDATION_PIPE_OPTIONS } from '../constants/validation-pipe'
import { CreateCloseContactsRequestDto } from './dto/create-close-contact.dto'
import { CreatedResponseInterceptor } from '../shared/created-response.interceptor'

@ApiTags('app')
@ApiBearerAuth()
@ApiUnauthorizedResponse()
@UseGuards(FirebaseNormalUserValidateGuard)
@UseInterceptors(CreatedResponseInterceptor)
@Controller('users')
export class UsersController {
constructor(private usersService: UsersService) {}
Expand Down