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

Remove catching of exceptions inside field resolver functions #86

Merged
merged 8 commits into from
Jun 27, 2017

Conversation

hlship
Copy link
Member

@hlship hlship commented Jun 23, 2017

Fixes #82 -- in the current form, the exception capturing is very aggressive, gets in the way of debugging when things go wrong.

@candid82
Copy link
Contributor

Would it be simpler to not catch exceptions at all and let users handle them however they want?

@hlship
Copy link
Member Author

hlship commented Jun 23, 2017

Given the mea culpa on decorators, that's worth considering. But still, this feels like something that will be applied uniformly across resolvers.

The existing code catches the exception, converts to an error map, and adds to the error map a number of keys to identify the source in the query document of the failure.

Perhaps we could see about stripping that out, so that an exception simply bubbles up, and probably gets caught back inside the Pedestal interceptor chain.

@candid82
Copy link
Contributor

The only potential problem I see here is async resolvers. Not sure how exceptions will propagate in async world.

@hlship
Copy link
Member Author

hlship commented Jun 23, 2017

The docs on async are already very specific about exceptions: catch them or your system will grind to a halt.

@hlship hlship changed the title Add hook for tracking exceptions thrown by field resolvers, and converting them into an error map for inclusion in the result Remove catching of exceptions inside field resolver functions Jun 23, 2017
@hlship
Copy link
Member Author

hlship commented Jun 23, 2017

Side note: the need to capture field resolver exceptions dates back to much earlier Lacinia, before the introduction of ResolverResult. With this change, the main execution flow has no try/catch at all (there's some in parsing and validation).

@bcarrell bcarrell merged commit e1f6c31 into master Jun 27, 2017
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.

4 participants