JavaScript: Use strings for 64-bit ints to preserve precision

[seishun]

Currently the following happens:

C:\Users\Nikolai\Downloads\protobuf\js>node
> var u = require('./testbinary_pb')
undefined
> var t = new u.TestAllTypes();
undefined
> t.setOptionalSfixed64('76561197988007571')
undefined
> var bytes = t.serializeBinary()

> var t2 = u.TestAllTypes.deserializeBinary(bytes)

> t2.getOptionalSfixed64()
76561197988007570
>

This fixes it by representing 64-bit ints as decimal strings:

$ node
> var u = require('./testbinary_pb')
undefined
> var t = new u.TestAllTypes();
undefined
> t.setOptionalSfixed64('76561197988007571')
undefined
> var bytes = t.serializeBinary()

> var t2 = u.TestAllTypes.deserializeBinary(bytes)

> t2.getOptionalSfixed64()
'76561197988007571'
>

[grpc-kokoro]

Thanks for your pull request. The automated tests will run as soon as one of the admins verifies this change is ok to test.

[haberman]

Thanks for the fix. This is an issue we need to address, but it's not 100% clear what the right way forward is. Converting from string to integer all the time could be annoying. There is a js_type option in descriptor.proto (https://github.com/google/protobuf/blob/master/src/google/protobuf/descriptor.proto#L484) but we don't currently respect it. There is a slightly different version of that option internally. We need to figure out which direction to go with this.

Additionally, since we're released 3.0 as GA, we need changes to start being backward-compatible. That means providing a migration path for a non-backward-compatible change like this. We would probably need to introduce it as an option that initially defaults to false, then flip it to defaulting to true, then remove it.

[seishun]

Converting from string to integer all the time could be annoying.

Well, to be fair, you just stick a + in front of it.

The js_type option needs to be present in the .proto file, right? In that case, it's not a good solution for those who are using existing .proto files.

We could add an option to deserializeBinary or to protoc. I think the latter is better since you'd only specify it once rather than all over the code.

Another option is to return a string only if it would lose precision otherwise. But I don't like this solution because it would make the returned type unpredictable. And it would also be technically non-backward-compatible too (although the code it would break is kinda already broken anyway).

[haberman]

The js_type option needs to be present in the .proto file, right? In that case, it's not a good solution for those who are using existing .proto files.

True. It may be better for this option to default to string. Then people can set the option on fields where it is safe to return integer (because the field is expected to always be less than 2^54).

Another option is to return a string only if it would lose precision otherwise. But I don't like this solution because it would make the returned type unpredictable.

I agree, I think the return type should be consistent.

[seishun]

Makes sense. The commit I just pushed reverts to the old behavior if jstype is specified as JS_NUMBER.

[seishun]

Any update on this?

[zean00]

Any solution or workaround on this issue?

[grpc-kokoro]

Thanks for your pull request. The automated tests will run as soon as one of the admins verifies this change is ok for us to run on our infrastructure.

[seishun]

@haberman I see in 39f9b43 that you've implemented some required changes in writer.js, reader.js, encoder.js and decoder.js internally, but it looks like the new functions aren't used anywhere. So I've just changed the PR to make use of the new functions.

[haberman]

@seishun Thanks for the update.

Our current thinking is that we may need to preserve the current default (int64s as JavaScript numbers) for the sake of backward compatibility. We released 3.0 as GA and we try not to break existing code or workflows.

We should definitely be able to add support for js_type=JS_STRING. But you mentioned before that you might have existing .proto files that you can't easily change?

[seishun]

Our current thinking is that we may need to preserve the current default (int64s as JavaScript numbers) for the sake of backward compatibility.

I see. Could this be added as an option to protoc then? I still hope that int64s as strings can eventually become the default though.

But you mentioned before that you might have existing .proto files that you can't easily change?

This is correct, and I guess it's the same for most users. Having to modify or pre-process a lot of existing .proto files because of a specific protobuf implementation might be annoying. And I believe it should generally be up to the implementation to decide how to represent protobuf types, not the .proto file.

[haberman]

I see. Could this be added as an option to protoc then?

Yes, that seems like a possibility. I agree it would also be a better default in the long term.

And I believe it should generally be up to the implementation to decide how to represent protobuf types, not the .proto file.

I can see that argument. I think the argument for allowing this to be set in the .proto file is: some fields might be marked int64 but the person writing the .proto file knows they will never actually exceed 2**53, so representing them as a number is safe (since JavaScript numbers can exactly represent all integers in this range).

The best example of this is timestamps that are in milliseconds or microseconds. 2**53 microseconds is 285 years and 2**53 milliseconds is 285,427 years. So a person who puts a timestamp in a .proto file can write [js_type=JS_NUMBER] to signal that storing the value as a JavaScript number is always safe.

[seishun]

Done, the representation of int64s in case of jstype=JS_NORMAL is now governed by a protoc option int64_default_type which defaults to number.

The tests now fail because I'm not sure how to test an option. It seems there is no existing infrastructure for testing options. I can just revert the changes in tests for now.

I think the argument for allowing this to be set in the .proto file is: some fields might be marked int64 but the person writing the .proto file knows they will never actually exceed 2**53, so representing them as a number is safe (since JavaScript numbers can exactly represent all integers in this range).

I agree about the use case for jstype=JS_NUMBER. But it seems that in most cases you can't sacrifice precision in int64s, which suggests that jstype=JS_STRING should be the default. Or rather, the semantic distinction between jstype=JS_STRING and jstype=JS_NORMAL (the current default) is unclear.

[cmoad]

Does anyone know the status of this problem with respect to v3.2.1? I'm having precision issues and resorted to a sed trick to rewrite all instances of readInt64/writeInt64 to readInt64String/writeInt64String in the generated JS file. This works, but isn't ideal.

I added the field option (ex. int64 id = 1 [jstype = JS_STRING];), but it doesn't appear to be respected.

Thanks!

[bazel-io]

Can one of the admins verify this patch?

[acozzette]

@cmoad I believe @updogliu is working on this as part of #2908. @updogliu, do you have any updates?

[haleyw]

Hi there, any updates?

[updogliu]

Sorry for the delay. Just returned from a month-long vacation.
I am working on adding support of js_type. The plan is to use JS_NUMBER as the default, and change the comment of js_type in descriptor.proto accordingly [link].

[longdivision]

Hey there - is this still planned to go ahead? We have some int64s which are losing precision and having the int64_default_type protoc option to preserve them as strings would be incredibly useful.

[bazel-io]

Can one of the admins verify this patch?

[updogliu]

The support of js_type option has been added to the Google internal codebase. It will be exported to the open source repo later. By then one can specify the JS type of a 64-bit integer field to be string in the generated code. The default JS type will be still number.

[seishun]

Using the js_type option requires modifying the .proto file so it's not really a solution as explained here.

[longdivision]

Agreed with @seishun. We're hoping to set the behaviour for all of our protos and ideally want to avoid having to add the annotation to every int64 field we have.

[cmoad]

I'd like to throw in a vote for releasing the field option so there is at least 1 option in the near term. Thanks for the support on this.

[xfxyjwf]

For everyone who is interested in this, please help create an issue (or comment on an existing issue) instead. This is a significant semantic change and we need to carefully implement and rollout the change in both google internally and opensource. It's not something we can accept in a pull request.