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

Marshalling of response object is bad for int64 properties #920

Closed
donutloop opened this issue Apr 5, 2019 · 6 comments
Closed

Marshalling of response object is bad for int64 properties #920

donutloop opened this issue Apr 5, 2019 · 6 comments
Labels

Comments

@donutloop
Copy link

donutloop commented Apr 5, 2019

What version of protobuf and what language are you using?

Version: libprotoc 3.7.0
Language: GOLANG
Version v1.3.1

What version of Go are you using (go version)?

go version go1.12 darwin/amd64

What operating system (Linux, Windows, …) and version?

MacOS Mojave 10.14.1 (18B75)

What did you do?

i had defined a message in Protobuffer this message contains a property called "property name: status, type:int64". if i send a response to the requester then i get instead of "int" a "string" as type back.

Input:

message HealthCheckResponse {
   repeated HealthStatus statusList = 1;
}

message HealthStatus {
    int64 status = 1;
    string service = 3;
}

What did you expect to see?

Output:

HTTP/1.1 200 OK
Content-Length: 52
Content-Type: application/json
Date: Fri, 05 Apr 2019 16:03:38 GMT
Grpc-Metadata-Content-Type: application/grpc

{"statusList":[{"status":1,"service":"Database"}]}

What did you see instead?

Output:

HTTP/1.1 200 OK
Content-Length: 52
Content-Type: application/json
Date: Fri, 05 Apr 2019 16:03:38 GMT
Grpc-Metadata-Content-Type: application/grpc

{"statusList":[{"status":"1","service":"Database"}]}
@achew22
Copy link
Collaborator

achew22 commented Apr 5, 2019

Hey @donutloop, thanks for sending in the issue. This has been discussed quite extensively in #219 (comment), #438, #735 and a few more. It's unlikely to change because there is a correctness issue, and we have no ability to change without dramatically changing the way JSON is handled in the core of protobuf (which would require an ecosystem wide migration).

Do you have any ideas on how could we make this behavior more obviously correct? Did you look in our documentation before filing an issue? If so, where so I can add more links and words. Thanks so much!

@donutloop
Copy link
Author

Have you opened a ticket on this protobuffer repository to discuss this issue in detail? It's definitely a major bug and it should be get fixed by this protobuffer team

@achew22
Copy link
Collaborator

achew22 commented Apr 6, 2019

@donutloop, thanks for the response. Based on this comment, do you really think it is a bug to handle things this way?

> (Math.pow(2,53) + 0) == (Math.pow(2,53) + 1)
true

is a sentiment that really scares me as an API designer and I'm generally in agreement with the protobuf team that this is the correct design even if it is unintuitive. Could you elaborate a bit on why you think it is a "major bug"?

mohammed90 added a commit to mohammed90/authn-server that referenced this issue May 16, 2019
…nverts (u)int64 to strings (see: grpc-ecosystem/grpc-gateway#920 ),

which is compliant with the Proto3 spec. However, this affects our response negatively because it breaks backwards compatibilty. Go's
JSON encoder/decoder doesn't have such surprises. This commit replaces runtime.JSONPb (from gRPC-Gateway) with a wrapper
around the stdlib JSON encoder/decoder.
@mgtest42
Copy link

I faced this behavior on the step of testing my rest service.
Can I access grpc-gateway internal JSON serializer?
Because of this, I can't properly compare the expected response with actual.
Golang JSON library marshals int64 as floats, and I can't force it to marshal it to strings.
And on the other hand, grpc-gateway returns a string, which can't be unmarshalled into original structs by the standard library.

@johanbrandhorst
Copy link
Collaborator

You can specify your own Marshaller with the runtime.WithMarshaler option.

@stale
Copy link

stale bot commented Sep 16, 2019

This issue has been automatically marked as stale because it has not had recent activity. It will be closed if no further activity occurs. Thank you for your contributions.

@stale stale bot added the wontfix label Sep 16, 2019
@stale stale bot closed this as completed Sep 23, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

No branches or pull requests

4 participants