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.

Sign up for GitHub

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

Jump to bottom

Add support for Request<T> where T is a type that implements hyper::body::Body trait #1263

Merged
merged 12 commits into from
Sep 26, 2024
Merged

Add support for Request<T> where T is a type that implements hyper::body::Body trait #1263

merged 12 commits into from
Sep 26, 2024

Conversation

elcharitas
Copy link
Contributor

@elcharitas elcharitas commented Jun 15, 2024 •
edited
Loading

Should resolve #1102

Problem

Attempts to use any other hyper request type aside Request<hyper::body::Incoming> fails with a compile-time error of type mismatch

Solution

Add generic support for valid supported hyper Request or any custom Request that has a body which implements hyper::body::Body

Checklist

  • Tests are added
  • CHANGELOG entry is added
  • Documentation is updated (not required)

@elcharitas elcharitas changed the title Support multiple hyper request types Add support for Request<T> where T is a type that implements hyper::body::Body trait Jun 15, 2024
@elcharitas elcharitas marked this pull request as ready for review June 15, 2024 12:51
@elcharitas
Copy link
Contributor Author

CI Checks seem to fail and this is weird since the issues arise from the juniper crate.

Any pointers?

LegNeato
LegNeato requested changes Aug 22, 2024
View reviewed changes
Copy link
Member

@LegNeato LegNeato left a comment

Choose a reason for hiding this comment

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

Thank you for the PR, sorry for the slow response! Are there backward compat considerations here? I see you have a default type but I am not sure. Also, can you add a test so we can be sure this doesn't regress?

@elcharitas
Copy link
Contributor Author

Thank you for the PR, sorry for the slow response! Are there backward compat considerations here? I see you have a default type but I am not sure. Also, can you add a test so we can be sure this doesn't regress?

Thanks for checking in. To answer your concerns..

Are there backward compat considerations here?

Yes, this PR is fully backward compatible. How do we know? There's a test already included here and it passes with no need for further configuration.

can you add a test so we can be sure this doesn't regress

Yes I can add a test for custom Request<T> type. However, I must point out that this PR makes no algo change whatsoever, it only allows passing generic request types making juniper compatible with different request types.

@elcharitas elcharitas requested a review from LegNeato August 23, 2024 07:16
LegNeato
LegNeato previously requested changes Sep 18, 2024
View reviewed changes
juniper_hyper/src/lib.rs Outdated Show resolved Hide resolved
@elcharitas elcharitas requested a review from LegNeato September 18, 2024 16:47
@tyranron
Copy link
Member

@elcharitas @LegNeato I'll look into it too in the next few working days.

@tyranron tyranron added enhancement Improvement of existing features or bugfix k::api Related to API (application interface) k::integration Related to integration with third-party libraries or systems lib::hyper Related to `hyper` crate integration labels Sep 26, 2024
@tyranron tyranron added this to the 0.17.0 milestone Sep 26, 2024
@tyranron tyranron added the semver::breaking Breaking change in terms of SemVer label Sep 26, 2024
tyranron
tyranron approved these changes Sep 26, 2024
View reviewed changes
Copy link
Member

@tyranron tyranron left a comment

Choose a reason for hiding this comment

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

@elcharitas I've somewhat relaxed trait bounds. Anything else seems fine. Thanks! 🍻

@tyranron tyranron merged commit 257bc69 into graphql-rust:master Sep 26, 2024
180 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@tyranron tyranron tyranron approved these changes

@LegNeato LegNeato Awaiting requested review from LegNeato

Assignees

@elcharitas elcharitas

Labels
enhancement Improvement of existing features or bugfix k::api Related to API (application interface) k::integration Related to integration with third-party libraries or systems lib::hyper Related to `hyper` crate integration semver::breaking Breaking change in terms of SemVer
Projects
None yet
Milestone
0.17.0
Development

Successfully merging this pull request may close these issues.

Possible API improvements to juniper_hyper
3 participants
@elcharitas @tyranron @LegNeato