fix: handle NaN and Infinity float values in gRPC #4631
Conversation
2fa5e78 to
fdd70b2
Compare
|
Couple of things:
|
There was a problem hiding this comment.
Thank you for the PR and sorry for the slow reply 🙇
I have left some comments around the code.
On the whole idea I am still not particularly certain this is ... what should be done, if anything.
Trying to figure out how this works in other cases I found protocolbuffers/protobuf-javascript#49 which seems to suggest that this isn't really supported. But maybe I am getting something wrong.
From the test it seems that this just works on the way back which seems awfully convient.
| b, err := req.ToObject(c.vu.Runtime()).MarshalJSON() | ||
|
|
||
| object := req.ToObject(c.vu.Runtime()) | ||
| normalized, err := normalizeNumberStrings(object, c.vu.Runtime()) |
There was a problem hiding this comment.
As you are effectively changing the input I would expect that you will first copy it as otherwiser this is breaking change and likely a very strange one, where after calling some grpc method suddently a bunch of your floats are strings.
There was a problem hiding this comment.
But the object is built based on the req, which is a sobek.Value passed by value, so we shouldn't worry about it, no? Or am I misunderstanding you? (Probably this, my brain is fried today, sorry 😞 )
|
Hey @mstoykov, thanks a lot for the review! I’ve made some changes based on your comments. There’s one part I didn't change yet because I’m not fully sure I understand it correctly, but we can keep discussing it in the open conversation. Regarding the issue you mentioned: it looks like an older open issue. From what I saw, the code has since changed and now properly asserts both I also tested it locally using the dummy.proto from the issue, for both client and server (wire format). Additionally, the ProtoJSON documentation confirms that both values are officially supported. Just a heads-up: I might be a bit slower next week if any further changes are needed — apologies in advance. Thanks again for the great feedback. 🙏 EDIT: Tests are failing, don't know if related to my changes but the |
Fixes (?): #3990
The problem was in this specific line:
k6/internal/js/modules/k6/grpc/client.go
Line 363 in a74d803
This
MarshalJSONimplemented in Sobek uses a copy of JSON.stringify() under the hood, and per JSON RFC, neitherInfinityorNaNas values are accepted as valid numbers.However, since we rely on ProtoJSON, which does support special strings
"Infinity"and"NaN", I guess that the idea would be to "normalize" them as a string format before marshaling, which is what this PR implements.