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

int64 serialization/deserialization not working properly #534

Closed
paralin opened this Issue Dec 9, 2016 · 6 comments

Comments

Projects
None yet
2 participants
@paralin

paralin commented Dec 9, 2016

var protobufjs = require('protobufjs');
var ro = protobufjs.loadSync('./test.proto');

var test = ro.lookup('test.Test');
var enc = test.encode({int_64: '314159265358979'}).finish();
var dec = test.decode(enc);

if (dec.int_64 === '314159265358979') {
  console.log('Test passed.');
} else {
  console.log(dec.int_64 + ' != 314159265358979 (expected)');
}
syntax = "proto3";
package test;

message Test {
  fixed64 int_64 = 1;
}

Output:

16675180715352071609 != 314159265358979 (expected)

Extracted from a failing test case in grpc. long is not installed. Node v6.9.1

@dcodeIO

This comment has been minimized.

Owner

dcodeIO commented Dec 9, 2016

var enc = test.encode({int_64: 314159265358979}).finish(); // not a string

Still wondering where the other number comes from.

@paralin

This comment has been minimized.

paralin commented Dec 9, 2016

Yeah maybe, but these guys at grpc seem to want to pass it in as a string. For backwards compat would it be possible to accept this and convert it in Protobuf.JS?

@dcodeIO

This comment has been minimized.

Owner

dcodeIO commented Dec 9, 2016

Will see what I can do, need to setup this test first!

dcodeIO added a commit that referenced this issue Dec 9, 2016

@dcodeIO

This comment has been minimized.

Owner

dcodeIO commented Dec 9, 2016

Alright, most of this should be working now. Without a long-lib, parseInt is used for strings now.

However, I doubt that the following line

if (dec.int_64 === '314159265358979') {

would actually work with v5, because v5 always returns a Long instance when fixed64 is supported. Hence, I assume that grpc is using long actually (if it has protobufjs in its dependencies, it even must because it is a non-optional dep on node prior to v6).

What actually works with a long lib is this:

if (dec.int_64 == '314159265358979') {

because there is Long#toString, which is called implicitly here (at the cost of some usually unnecessary overhead btw - converting hence and forth between longs and strings is suboptimal).

Test case

Let me know if this is sufficient!

@paralin

This comment has been minimized.

paralin commented Dec 10, 2016

I'm testing it still and it doesn't seem to want to work properly, I have long installed as a dependency. Investigating some more.

@paralin

This comment has been minimized.

paralin commented Dec 10, 2016

Okay, two of the test cases are fixed, see #536 for the next issue :)

@paralin paralin closed this Dec 10, 2016

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment