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

Return Response<String> instead of Response<Body> in hyper integration #1096

Closed
LukasKalbertodt opened this issue Aug 25, 2022 · 1 comment · Fixed by #1101
Closed

Return Response<String> instead of Response<Body> in hyper integration #1096

LukasKalbertodt opened this issue Aug 25, 2022 · 1 comment · Fixed by #1101
Assignees
@LukasKalbertodt
Labels
enhancement Improvement of existing features or bugfix k::integration Related to integration with third-party libraries or systems
Milestone
0.16.0

Comments

@LukasKalbertodt
Copy link
Contributor

Is your feature request related to a problem? Please describe.
Sometimes it's useful to inspect the response returned by juniper_hyper::graphql. For example, in case of an error I would like to debug-log the whole JSON response. This is currently very tricky.

The function returns Response<Body> and the only way to read that Body is via the Stream trait (or the helper functions in hyper::body which also use that trait). But all those methods require Body by value or &mut Body. And in general that makes sense: Body can be many things and is not necessarily a byte buffer.

The best I could come up with to log the body is:

let (parts, body) = response.into_parts();
let body = hyper::body::to_bytes(body).await
    .expect("could not read API response body (this should never happen)");
debug!("Response body of failed API request: {}", String::from_utf8_lossy(&body));
response = hyper::Response::from_parts(parts, body.into());

I need to completely unpack the response to then assemble it again. And the await and expect are both not nice here.

Describe the solution you'd like
From a quick glance at the juniper_hyper code, it seems like the Body is always created via From<String>. But String implements the trait hyper::HttpBody directly! So my suggestion is that all functions return Response<String>. With that, my logging would be as simple as:

debug!("Response body of failed API request: {}", response.body());
@LukasKalbertodt LukasKalbertodt added the enhancement Improvement of existing features or bugfix label Aug 25, 2022
@tyranron tyranron added the k::integration Related to integration with third-party libraries or systems label Aug 25, 2022
@tyranron
Copy link
Member

@LukasKalbertodt makes sense! Would love to see a PR for this.

LukasKalbertodt added a commit to LukasKalbertodt/juniper that referenced this issue Sep 12, 2022
@LukasKalbertodt
Change body type of juniper-hyper functions from Body to String
2175920 
All responses were created from strings anyway (via `Body::from`), but
`String` implements the `Body` trait directly. With this change, it is
easy to inspect the response body, which is not true for `Body`.

Fixes graphql-rust#1096
@LukasKalbertodt LukasKalbertodt mentioned this issue Sep 12, 2022
Merged
LukasKalbertodt added a commit to LukasKalbertodt/juniper that referenced this issue Sep 12, 2022
@LukasKalbertodt
Change body type of juniper-hyper functions from Body to String
bf0242c 
All responses were created from strings anyway (via `Body::from`), but
`String` implements the `Body` trait directly. With this change, it is
easy to inspect the response body, which is not true for `Body`.

Fixes graphql-rust#1096
LukasKalbertodt added a commit to LukasKalbertodt/juniper that referenced this issue Sep 12, 2022
@LukasKalbertodt
Change body type of juniper-hyper functions from Body to String
49f5a6f 
All responses were created from strings anyway (via `Body::from`), but
`String` implements the `Body` trait directly. With this change, it is
easy to inspect the response body, which is not true for `Body`.

Fixes graphql-rust#1096
@tyranron tyranron added this to the 0.16.0 milestone Sep 19, 2022
tyranron pushed a commit that referenced this issue Sep 19, 2022
@LukasKalbertodt
Change body type of juniper_hyper functions from Body to String (…
e0ddeec 
…#1101, #1096)
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees

@LukasKalbertodt LukasKalbertodt

Labels
enhancement Improvement of existing features or bugfix k::integration Related to integration with third-party libraries or systems
Projects
None yet
Milestone
0.16.0
Development

Successfully merging a pull request may close this issue.

Change body type of juniper-hyper functions from Body to String LukasKalbertodt/juniper
2 participants
@tyranron @LukasKalbertodt