refactor(api-service): remove ApiException in favour of BadRequestException #8066
Conversation
✅ Deploy Preview for dashboard-v2-novu-staging ready!
To edit notification comments on pull requests, go to your Netlify site configuration. |
| @@ -70,7 +69,7 @@ export class AuthController { | |||
| Logger.verbose('Checking Github Auth'); | |||
|
|
|||
| if (!process.env.GITHUB_OAUTH_CLIENT_ID || !process.env.GITHUB_OAUTH_CLIENT_SECRET) { | |||
| throw new ApiException( | |||
| throw new BadRequestException( | |||
There was a problem hiding this comment.
The usage of BadRequestException is questionable here, it should be probably InternalServerErrorException
but for now this PR doesn't focus on this - it would be a breaking change, at least now we can see that the exceptions are wrong since they're not being hidden behind ApiException
| @@ -147,7 +153,7 @@ export class CommunityAuthService implements IAuthService { | |||
| ); | |||
|
|
|||
| const dbUser = await this.userRepository.findById(user._id); | |||
| if (!dbUser) throw new ApiException('User not found'); | |||
| if (!dbUser) throw new BadRequestException('User not found'); | |||
There was a problem hiding this comment.
Same here should be NotFoundException
| @@ -151,7 +151,7 @@ export class GetChanges { | |||
| try { | |||
| if (process.env.NOVU_ENTERPRISE === 'true' || process.env.CI_EE_TEST === 'true') { | |||
| if (!require('@novu/ee-shared-services')?.TranslationsService) { | |||
| throw new ApiException('Translation module is not loaded'); | |||
| throw new BadRequestException('Translation module is not loaded'); | |||
There was a problem hiding this comment.
Same, InternalServerErrorException
ChmaraX
requested review from
djabarovgeorge,
SokratisVidros,
LetItRock and
Copilot
| @@ -147,7 +153,7 @@ export class CommunityAuthService implements IAuthService { | |||
| ); | |||
|
|
|||
| const dbUser = await this.userRepository.findById(user._id); | |||
| if (!dbUser) throw new ApiException('User not found'); | |||
| if (!dbUser) throw new BadRequestException('User not found'); | |||
There was a problem hiding this comment.
Consider using NotFoundException here instead of BadRequestException to better indicate that the user was not located in the database.
| if (!dbUser) throw new BadRequestException('User not found'); | |
| if (!dbUser) throw new NotFoundException('User not found'); |
Copilot is powered by AI, so mistakes are possible. Review output carefully before use.
There was a problem hiding this comment.
True, but user might have a code that checks if (error.status === 400) - that would be a breaking change.
That said we can still make it and bump a release version.
There was a problem hiding this comment.
This feels like a safe change, affecting only the V0 built-in auth. It should be a 404.
| @@ -147,7 +153,7 @@ export class CommunityAuthService implements IAuthService { | |||
| ); | |||
|
|
|||
| const dbUser = await this.userRepository.findById(user._id); | |||
| if (!dbUser) throw new ApiException('User not found'); | |||
| if (!dbUser) throw new BadRequestException('User not found'); | |||
There was a problem hiding this comment.
This feels like a safe change, affecting only the V0 built-in auth. It should be a 404.
The
ApiExceptionis just an empty extension ofBadRequestException:The project was using something like 60%
BadRequestExceptionand 40%ApiExceptioninterchangeably.The
ApiExceptionwas also copied toapi,workerandwsor imported from@novu/application-generic- not consistent.I think using
BadRequestExceptioneverywhere is better and simpler approach since it comes fromnestjs/commondirectly and it doesn't seems like we need this abstraction.Caution
Could this be potentially a breaking change? I don't think so because exception type/instance is not being passed over HTTP and I think SDK has its own type for errors.
Like if someone would check in their code for
error instanceof ApiExceptionor something like that