Fix:If attachment token expires, it throws a 500 error instead of Unauthenticated #8939
Conversation
…uthenticated This fixes this issue by throwing a descriptive error instead of a server error 500
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
There was a problem hiding this comment.
PR Summary
Implements a new ExpiredAttachmentTokenError class to handle expired attachment tokens with proper 401 Unauthorized responses instead of 500 Internal Server Error.
- Added
ExpiredAttachmentTokenErrorclass in/packages/twenty-server/src/engine/core-modules/graphql/utils/graphql-errors.util.ts - Missing
nameproperty definition inExpiredAttachmentTokenErrorclass unlike other error classes - Error class not added to
graphQLPredefinedExceptionsmapping in/packages/twenty-server/src/engine/utils/global-exception-handler.util.ts - Modified
convertHttpExceptionToGraphqlfunction to handle expired token messages
💡 (1/5) You can manually trigger the bot by mentioning @greptileai in a comment!
2 file(s) reviewed, 4 comment(s)
Edit PR Review Bot Settings | Greptile
| export class ExpiredAttachmentTokenError extends BaseGraphQLError { | ||
| constructor(message = 'The attachment token has expired') { | ||
| super(message, ErrorCode.UNAUTHENTICATED); | ||
| } |
There was a problem hiding this comment.
logic: Missing Object.defineProperty(this, 'name', { value: 'ExpiredAttachmentTokenError' }); to match pattern of other error classes
| constructor(message = 'The attachment token has expired') { | ||
| super(message, ErrorCode.UNAUTHENTICATED); |
There was a problem hiding this comment.
style: Consider reusing AuthenticationError since this is using the same UNAUTHENTICATED code, or document why a separate class is needed
| } else if (message.includes('attachment token expired')) { | ||
| // Check if the error message indicates an expired attachment token | ||
| error = new ExpiredAttachmentTokenError(message); |
There was a problem hiding this comment.
style: String matching on error messages is fragile. Consider using a specific error code or type instead.
| // Check if the error message indicates an expired attachment token | ||
| error = new ExpiredAttachmentTokenError(message); | ||
| } | ||
| else { |
There was a problem hiding this comment.
syntax: Extra whitespace before else clause breaks consistency with the rest of the file
Welcome!
Hello there, congrats on your first PR! We're excited to have you contributing to this project. |
There was a problem hiding this comment.
@giiger Thanks for opening a PR, I agree we should improve error handling.
-
About the functional behavior: I'm a bit surprise this has to deal with graphql errors. To me files are serve over controllers (file.controller.ts) and I don't see what it has to do with controllers.
-
About the strategy: For rest controllers and for GraphQL API, we want to keep the number of possible errors low (here we should re-use the Authentication error I think) but have a flexible message which is already currently possible. We want services (here token validation service) to throw business specific errors (with dedicated error_code) and then the caller here (a rest controller to convert it to a more standard exception)
Let's first discuss 1) could you help me understand why this has to deal with GraphQL, I'm a bit lost
|
Yes, I apologize I am new to contributing on open source and I am not too familiar with the file structure or the code norms of this project. I see your point about the separation between controllers and GraphQL, and I may have misunderstood how the error handling should be implemented in this context. Could you possibly give me some guidance about my next steps I should take? I would like to continue working towards dealing with error handling |
|
@giiger sure ! First:
|
|
@giiger I'm going to close this PR as it's staled, feel free to re-open it if you still want to work on it |
This fixes this issue by throwing a descriptive error instead of a server error 500