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

feat: propagate entity resolver original error details to response #3144

Conversation

bradleyoesch
Copy link
Contributor

@bradleyoesch bradleyoesch commented Oct 10, 2023

Error details from errors thrown in entity resolution are swallowed by a generic failure message that also leaks internal code paths, so instead we should propagate the original error.

Description

When an entity resolver throws an exception, add the thrown exception to the result. Turns out graphql-core handles mapping it to a GraphQLError with location/path info for us (link) so we don't want to raise our own GraphQLError.

If the exception is a TypeError, we assume the resolver was incorrectly implemented, which throws a TypeError under the hood (we check the type name explicitly so that clients can raise their own custom TypeErrors without losing data). The generic message now just outputs the type name rather than the full path, which includes file directory structure that shouldn't (imo) be exposed to the response.

To be honest, I don't love this type error approach, so I'm up for alternatives!

Types of Changes

  • Core
  • Bugfix
  • New feature
  • Enhancement/optimization
  • Documentation

Issues Fixed or Closed by This PR

Checklist

  • My code follows the code style of this project.
  • My change requires a change to the documentation.
  • I have updated the documentation accordingly.
  • I have read the CONTRIBUTING document.
  • I have added tests to cover my changes.
  • I have tested the changes and verified that they work and don't break anything (as well as I can manage).

@botberry
Copy link
Member

botberry commented Oct 10, 2023

Thanks for adding the RELEASE.md file!

Here's a preview of the changelog:


Update federation entity resolver exception handling to set the result to the original error instead of a GraphQLError, which obscured the original message and meta-fields.

Here's the tweet text:

🆕 Release (next) is out! Thanks to Bradley Oesch for the PR 👏

Get it here 👉 https://strawberry.rocks/release/(next)

Copy link

codecov bot commented May 20, 2024

Codecov Report

Attention: Patch coverage is 95.78947% with 4 lines in your changes missing coverage. Please review.

Project coverage is 96.57%. Comparing base (bea5cac) to head (3ef9b0d).
Report is 3 commits behind head on main.

Additional details and impacted files
@@            Coverage Diff             @@
##             main    #3144      +/-   ##
==========================================
- Coverage   96.58%   96.57%   -0.02%     
==========================================
  Files         524      525       +1     
  Lines       33632    33958     +326     
  Branches     5577     5603      +26     
==========================================
+ Hits        32485    32796     +311     
- Misses        912      925      +13     
- Partials      235      237       +2     

Copy link

codspeed-hq bot commented May 20, 2024

CodSpeed Performance Report

Merging #3144 will not alter performance

Comparing bradleyoesch:improvement-3135/handle-entity-errors (9fb6923) with bradleyoesch:improvement-3135/handle-entity-errors (3ef9b0d)

Summary

✅ 13 untouched benchmarks

@bradleyoesch bradleyoesch force-pushed the improvement-3135/handle-entity-errors branch from ff4ccb2 to 73e887a Compare July 8, 2024 14:08
@bradleyoesch bradleyoesch force-pushed the improvement-3135/handle-entity-errors branch from 6dd4271 to e6f8dc4 Compare July 11, 2024 01:33
@DoctorJohn DoctorJohn force-pushed the improvement-3135/handle-entity-errors branch from c587414 to 53d77a0 Compare July 23, 2024 02:20
Copy link
Member

@DoctorJohn DoctorJohn left a comment

Choose a reason for hiding this comment

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

This looks good to me now. This PR makes it easier for devs to spot code errors and adds additional well-writiten tests to the federation integration, which is awesome. Thanks again @bradleyoesch

@DoctorJohn DoctorJohn requested a review from patrick91 July 23, 2024 16:20
RELEASE.md Outdated Show resolved Hide resolved
@patrick91 patrick91 merged commit ff54e5b into strawberry-graphql:main Jul 23, 2024
62 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Messages from errors thrown in entity resolution are swallowed by a generic failure message
4 participants