stephenh / ts-proto

An idiomatic protobuf generator for TypeScript
Apache License 2.0
2.13k stars 345 forks source link

grpc-web int64 always send in Long format #134

Closed PhilipMantrov closed 4 years ago

PhilipMantrov commented 4 years ago

Repo steps: get object with int64 field, edit that field (forceLong = number / string doesn't matter), save. Enjoy how you submit Long format to the backend.

stephenh commented 4 years ago

@PhilipMantrov can you clarify what is put on the wire vs. what you expect?

In general, the forceLong long is not about "what do things like look on the wire", b/c that is dictated by the protobuf spec and/or grpc-web spec. Instead, forceLong is just about how you'd like to have int64 represented to your client TypeScript code.

Also, for grpc-web functionality, ts-proto heavily leverages the improbable-eng/grpc-web runtime library to do all of the encoding/decoding, so I don't know that there is really much ts-proto can control here.

Unless it's something like ts-proto should always give grpc-web's serializeBinary method specific / different types (i.e. not number or Long or what not?) for int64 fields, but again you'll have to clarify and/or investigate that a bit because I'm not really sure how it works at this point.

PhilipMantrov commented 4 years ago

It takes time to research, i notice you when find problem (and better describe it).

PhilipMantrov commented 4 years ago

Bingo. Its protobufjs issue (https://github.com/protobufjs/protobuf.js/issues/1109, https://github.com/protobufjs/protobuf.js/issues/1253).

stephenh commented 4 years ago

@PhilipMantrov I don't think that's the same issue; that issue is talking about (afaict) regular/non-grpc-web proto methods like FooMessage.decode. For those, ts-proto generates its own decode methods that "know" the protobuf.js runtime is going to return a long and converts it over to the right thing, i.e. here for long to string:

https://github.com/stephenh/ts-proto/blob/master/integration/simple-long-string/simple.ts#L1683

And here for long to number:

https://github.com/stephenh/ts-proto/blob/master/integration/simple/simple.ts#L1980

I haven't exhaustively checked the behavior, so I could be missing something, but my current thinking is that ts-proto isn't affected by that bug.

Fwiw your ticket in general is very vague, i.e. I'm still not following what the problem is. I.e. your description of "Enjoy how you submit Long format to the backend." --- isn't that what's supposed to happen with int64 fields, i.e. regardless of whether the TS code uses numbers/strings/longs, when put on the wire, it always has to be a long.

Given that, I'm not sure there is a bug here, pending further clarification.

PhilipMantrov commented 4 years ago

Before sending request on backend: image After sending request on backend we see this: image Its problem starting from grpc.unary method, and probably our point of interes this string in encode method writer.uint32(88).int64(message.openTime);

PhilipMantrov commented 4 years ago

When request encoding all uint64 and int64 values pass method LongBits from protobufjs and this method create object image

stephenh commented 4 years ago

After sending request on backend we see this:

What is your backend? Is it also TypeScript using the ts-proto-generated code?

writer.uint32(88).int64(message.openTime);

I don't think that is the problem, because the protobuf spec says that 64-bit numbers must go on the wire like that (the hi/lo thing). ts-proto can't change the protobuf wire format, so we have to call int64 and it has to put the hi/lo data structure on the wire.

What ts-proto can do, is turn that hi/lo Long back into a number, like this line in simple.ts / Numbers.decode method:

          message.int64 = longToNumber(reader.int64() as Long);

You should have a similar line for message.openTime = longToNumber(...) in your ts-proto generated decode methods.

If your backend is TS, please verify you're going through a ts-proto-generated decode method that would longToNumber that openTime value.

stephenh commented 4 years ago

I put a few tests here that show more explicitly that ts-proto's decode method converts the longs back to numbers:

https://github.com/stephenh/ts-proto/blob/master/integration/simple/numbers-test.ts#L41

PhilipMantrov commented 4 years ago

What is your backend? Is it also TypeScript using the ts-proto-generated code?

Yes. I use NestJS implementation, and in this implementation we have just interfaces and we dont do longToNumber() on the backend side. But main thing this about frontend part and what request he send.

stephenh commented 4 years ago

we dont do longToNumber() on the backend side

So that is your "bug", right? In the protobuf+javascript world, you have to treat 64-bits as longs/that high/low format. Technically ts-proto let's you pretend to use numbers, but is really still putting longs on the wire, because it has to, to follow the protobuf wire format.

And so if your backend is not calling longToNumber, then of course you're getting a long?

I still don't see where the bug is.

Please take more time to write up what is happening.

PhilipMantrov commented 4 years ago

Yes, this is not a bug, I finally figured how is work, thanks for the clarification.