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

Streamed response is not valid json (or: is this the expected format?) #581

Closed
colini opened this issue Mar 23, 2018 · 11 comments
Closed

Streamed response is not valid json (or: is this the expected format?) #581

colini opened this issue Mar 23, 2018 · 11 comments

Comments

@colini
Copy link

colini commented Mar 23, 2018

I've written a simple a simple proto that includes this rpc with a stream response:

rpc GetSomeData(GetSomeDataRequest) returns (stream SomeData) {
  option (google.api.http) = {
    get: "/v1beta1/getData:raw"
  };
};

My server replies with 3 SomeData records. The result from curl via my generated grpc-gateway is:

$ curl -i 'http://localhost:8080/v1beta1/getData:raw'
HTTP/1.1 200 OK
Content-Type: application/json
Grpc-Metadata-Content-Type: application/grpc
Grpc-Metadata-Grpc-Accept-Encoding: gzip
Date: Fri, 23 Mar 2018 12:30:50 GMT
Transfer-Encoding: chunked

{"result":{"fund":"FUND_0","value":123.99}}
{"result":{"fund":"FUND_1","value":123.99}}
{"result":{"fund":"FUND_2","value":123.99}}

This appears to be newline-delimited json, which is fine, but it's not valid json.

AFAIK I'm using the latest grpc-gateway code, which I installed per the instructions:

go get -u github.com/grpc-ecosystem/grpc-gateway/protoc-gen-grpc-gateway
go get -u github.com/grpc-ecosystem/grpc-gateway/protoc-gen-swagger
go get -u github.com/golang/protobuf/protoc-gen-go

With go 1.10

$ go version
go version go1.10 darwin/amd64

Is this the expected stream response format? I'd suggest enclosing the results in a list, which would add only 1 byte per line (the comma), and would then be valid json:

[
{"result":{"fund":"FUND_0","value":123.99}},
{"result":{"fund":"FUND_1","value":123.99}},
{"result":{"fund":"FUND_2","value":123.99}}
]

(Sorry if this is a repeat of #481, but it wasn't clear how that issue was resolved.)

@yugui
Copy link
Member

yugui commented Mar 24, 2018

Streaming requests and responses are mapped to JSON streaming by design.

https://en.m.wikipedia.org/wiki/JSON_streaming

@yugui yugui closed this as completed Mar 24, 2018
@colini
Copy link
Author

colini commented Mar 24, 2018

@yugui Thank you for the clarification.

@veqryn
Copy link

veqryn commented Sep 20, 2018

@yugui shouldn't the content type of the response be application/x-ndjson instead of application/json?

@johanbrandhorst
Copy link
Collaborator

@veqryn interesting, do you have a source for this? I couldn't find it in the Wikipedia article.

@veqryn
Copy link

veqryn commented Sep 20, 2018

@johanbrandhorst
Copy link
Collaborator

Is there an RFC? Is it well supported in browsers and clients? It seems a little bit bare bones, if you don't mind me saying. Do we risk breaking more consumers than not by introducing it?

@veqryn
Copy link

veqryn commented Sep 20, 2018

I am not sure honestly. I don't think there is an RFC yet...

@johanbrandhorst
Copy link
Collaborator

I think we might be best of waiting here, it doesn't seem clear to me that this is indeed appropriate. I'd be happy to be proved wrong, of course.

@veqryn
Copy link

veqryn commented Sep 20, 2018

K. They should probably get an RFC in order.

@seanlaff
Copy link
Contributor

seanlaff commented Jun 27, 2024

We're running into some issues with this because we have js clients that see content-type: json and blow up trying to parse it because it's not actually json. Having content-type: ndjson would be nice, but if there's hesitation to do that, maybe we could remove the content-type header on streaming responses? Or maybe set it to text/plain?

@johanbrandhorst
Copy link
Collaborator

I think it should be possible to customize this with a custom marshaler, such that we don't need to make a change for all users.

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

No branches or pull requests

5 participants