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.

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

Don't swallow root server exceptions #1921

Closed
wants to merge 1 commit into from

Conversation

dakriy
Copy link

@dakriy dakriy commented Jan 29, 2024

Let root level bootstrap exceptions go to the ktor exception handler rather than being swallowed.

📝 Description

Exceptions that happen when building the context are swallowed obscuring what actually went wrong and making it hard to fix.

🔗 Related Issues

#1920

Let root level bootstrap exceptions go to the ktor exception handler rather than being swallowed.
@curtiscook
Copy link
Contributor

Hi @samuelAndalon can we get this merged?

@dariuszkuc
Copy link
Collaborator

Server should not respond with exception stacktraces. Exceptions should be logged instead.

@curtiscook
Copy link
Contributor

Server should not respond with exception stacktraces. Exceptions should be logged instead.

I don't understand your point here and it doesn't read constructively.

Currently, any exception throws a 400 error with no message and I have no control over the error code or messaging.

As you know... Some errors are 400, some are 401, 403, 500, or 503. Technically, since you're catching a base exception, it should be a 500 error. If you look through all the code it looks like maybe people should be catching errors and rethrowing a GraphQLError that can provide a message, but I'm still not seeing where I can set the status code.

Honestly, the "correct" way to handle this should be to let KTor (or Spring) handle the exception and map any unhandled exceptions to a 500 status code, which could easily be part of the example.

Or at the very least, I think there could be some documentation on suggested error handling.

@dariuszkuc
Copy link
Collaborator

Superseded by #1936

@dariuszkuc dariuszkuc closed this Apr 9, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Development

Successfully merging this pull request may close these issues.

3 participants