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(web): Relax GrpcWebService request body type #2016

Merged
merged 1 commit into from
Nov 7, 2024

Conversation

tottoto
Copy link
Collaborator

@tottoto tottoto commented Oct 19, 2024

Relaxes GrpcWebService's request body type.

@@ -113,7 +116,7 @@ where
debug!(kind = "other h2", content_type = ?req.headers().get(header::CONTENT_TYPE));
ResponseFuture {
case: Case::Other {
future: self.inner.call(req),
future: self.inner.call(req.map(tonic::body::boxed)),
Copy link
Collaborator

@tobz tobz Oct 22, 2024

Choose a reason for hiding this comment

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

Hmm, if we have to box the request no matter what, what are we gaining with this change?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

The benefit is the CorsGrpcWeb can handle any kind of body types which has the trait bounds.

Copy link
Collaborator

@tobz tobz left a comment

Choose a reason for hiding this comment

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

While the change itself seems fine, I'm still left with the thought: should we avoid all of the constraints that require a service that accepts Request<BoxBody>?

It seems like if we're going to bother to be totally generic over the request body type in GrpcWebService, we should also do so in CorsWebLayer? Maybe there's a very obvious reason why we can't, but looking at tower_http::cors::Cors shows that it's also generic over the request body... so it feels like we should be able to?

@tottoto
Copy link
Collaborator Author

tottoto commented Oct 23, 2024

While the change itself seems fine, I'm still left with the thought: should we avoid all of the constraints that require a service that accepts Request?

It seems like if we're going to bother to be totally generic over the request body type in GrpcWebService, we should also do so in CorsWebLayer? Maybe there's a very obvious reason why we can't, but looking at tower_http::cors::Cors shows that it's also generic over the request body... so it feels like we should be able to?

Since CorsWebLayer does not exist, I respond with information about other parts.

I assume your main question is whether we can avoid requiring a BoxBody in the internal service.

I guess so too. When I was working on this, I was also looking into the implementation of tower_http's Cors service to see if it is possible to avoid the BoxBody. However, it would require an internal design change to the implementation, so I opened this partial change which can be easily done for now.

@tottoto
Copy link
Collaborator Author

tottoto commented Nov 6, 2024

@tobz Are there any other concerns block this? If there are no problems with this, I will merge it at this stage.

Copy link
Collaborator

@tobz tobz left a comment

Choose a reason for hiding this comment

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

Sorry for the delay here.

I do think it's fine for the moment and we can always relax the bounds further in the future.

@tottoto tottoto added this pull request to the merge queue Nov 7, 2024
Merged via the queue into hyperium:master with commit d3da369 Nov 7, 2024
17 checks passed
@tottoto tottoto deleted the tonic-web-body branch November 7, 2024 16:41
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants