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

Fix Rejection branching in warp filters (#1177) #1222

Merged
merged 13 commits into from
Nov 23, 2023
Merged

Conversation

tyranron
Copy link
Member

@tyranron tyranron commented Nov 21, 2023

Resolves #1177

Synopsis

See #1177:

Describe the bug
with juniper_warp = "0.7.0", this sequence can happen:

  1. request comes in with valid application/json body.
  2. post_json_filter runs; context_extractor fails for some transient reason (database error in my case), returns some Rejection that should produce a 5xx error.
  3. then post_graphql_filter runs. context_extractor happens to succeed this time. post_graphql_filter returns a 4xx rejection about a parse error because the body's actually in json format.

Solution

Seems like seanmonstar/warp#487 (comment) hits the exact issue we have here:

The problem is that or() currently swallows the error

or() is designed to swallow the error. If you need access to the error, use or_else()

So, we should replace or() filters branching with or_else() one.

See #1177 (comment):

  • Rework make_graphql_filter to execute context_extractor only once.
  • Provide a usage example with fallible context_extractor and mention rejection issue in API docs.
  • Re-check whether query, body and json filters used inside make_graphql_filter require recover()ing handle into hard errors.

Checklist

  • CHANGELOG entry is added
  • Covered with tests
  • Documentation is updated

@tyranron tyranron added enhancement Improvement of existing features or bugfix k::integration Related to integration with third-party libraries or systems lib::warp Related to `warp` crate integration k::refactor Refactoring, technical debt elimination and other improvements of existing code base k::testing Related to testing and/or automated tests labels Nov 21, 2023
@tyranron tyranron added this to the 0.16.0 milestone Nov 21, 2023
@tyranron tyranron self-assigned this Nov 21, 2023
@tyranron tyranron added k::documentation Related to project documentation k::example Related to usage examples feature New feature or request labels Nov 21, 2023
@tyranron tyranron marked this pull request as ready for review November 23, 2023 16:25
@tyranron tyranron enabled auto-merge (squash) November 23, 2023 17:11
@tyranron tyranron merged commit 4ef8cf7 into master Nov 23, 2023
174 checks passed
@tyranron tyranron deleted the 1177-fix-warp-filters branch November 23, 2023 17:39
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement Improvement of existing features or bugfix feature New feature or request k::documentation Related to project documentation k::example Related to usage examples k::integration Related to integration with third-party libraries or systems k::refactor Refactoring, technical debt elimination and other improvements of existing code base k::testing Related to testing and/or automated tests lib::warp Related to `warp` crate integration
Projects
None yet
Development

Successfully merging this pull request may close these issues.

juniper_warp::make_graphql_filter improperly returns 4xx on some context_extractor 5xx errors
1 participant