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

Optional specification of stream content type #4926

Merged
merged 3 commits into from
Nov 13, 2024

Conversation

huin
Copy link
Contributor

@huin huin commented Nov 8, 2024

We have grpc-gateway based services that include both unary and streamed responses, using JSONPb as the marshaler. There is a niggle where the response type for the streamed response is application/json, but we'd like it to be application/x-ndjson in the streamed response case.

References to other Issues or PRs

This looks like it would have provided a solution for at least one comment on #581 (#581 (comment) in particular).

Have you read the Contributing Guidelines?

Yes. This does not appear to need file regeneration.

Brief description of what is fixed or changed

Support specifying streamed response content types, without changing the existing behaviour of grpc-gateway unless consumers opt in by providing their own Marshaler to set the stream content type.

Other comments

@johanbrandhorst
Copy link
Collaborator

johanbrandhorst commented Nov 9, 2024

Hi, thanks for your PR. I don't think we should need any new functionality for this. Marshalers in the grpc-gateway are registered by incoming Accept: header contents. Could you do something like

type myJSONPbWrapper struct {
    *runtime.JSONPb
}

func (myJSONPbWrapper) ContentType() string {
    return "application/x-ndjson"
}

and

mux := runtime.NewServeMux(
    runtime.WithMarshalerOption("application/json", &runtime.JSONPb{}),
    runtime.WithMarshalerOption("application/x-ndjson", myJSONPbWrapper{&runtime.JSONPb{}}),
)

?

@huin
Copy link
Contributor Author

huin commented Nov 11, 2024

I don't think that quite captures the solution I was seeking.

Using the Accept header requires that the client provides the content type that it will receive in the response Content-Type header, and the server will blindly repeat back whichever of the two options it is given in code, regardless of the actual content type that it sends back.

I was aiming for something where the server would authoritatively state the content type on endpoints that have a streaming result.

For more context: in our situation, the client is not under our control, as we provide the serving grpc-gateway service, but not the client that is consuming a grpc-gateway service.

Copy link
Collaborator

@johanbrandhorst johanbrandhorst left a comment

Choose a reason for hiding this comment

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

OK, fair enough, I do think there's a pretty good use case for this. It sorta sucks that we can introduce this retroactively though since it might break users. What do you think about adding an example of an ndjson stream to our docs page? It could wrap the JSONPb marshaler to implement the streaming content type and to set the delimiter. Maybe a new doc in https://github.com/grpc-ecosystem/grpc-gateway/tree/main/docs/docs/mapping?

runtime/handler_test.go Outdated Show resolved Hide resolved
@huin huin force-pushed the stream-content-type branch from d232cba to 6f90bf8 Compare November 12, 2024 08:30
@huin
Copy link
Contributor Author

huin commented Nov 12, 2024

OK, fair enough, I do think there's a pretty good use case for this. It sorta sucks that we can introduce this retroactively though since it might break users. What do you think about adding an example of an ndjson stream to our docs page? It could wrap the JSONPb marshaler to implement the streaming content type and to set the delimiter. Maybe a new doc in https://github.com/grpc-ecosystem/grpc-gateway/tree/main/docs/docs/mapping?

Willdo, I'll take a crack at that now.

@huin huin force-pushed the stream-content-type branch from 9c0e65b to 92b736b Compare November 12, 2024 09:14
@huin
Copy link
Contributor Author

huin commented Nov 12, 2024

I've added another commit to add another page of documentation, and refer to it from an existing page.

Copy link
Collaborator

@johanbrandhorst johanbrandhorst left a comment

Choose a reason for hiding this comment

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

Excellent, I love the new docs page!

@johanbrandhorst johanbrandhorst enabled auto-merge (squash) November 13, 2024 18:37
@johanbrandhorst johanbrandhorst merged commit 371dddb into grpc-ecosystem:main Nov 13, 2024
14 checks passed
@johanbrandhorst
Copy link
Collaborator

Thanks for your contribution!

@huin
Copy link
Contributor Author

huin commented Nov 14, 2024

No problem, thanks for reviewing!

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