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

fix: empty buffers submitted to a repeated bytes field were converted to undefined #1500

Closed
wants to merge 2 commits into from

Conversation

jawid-h
Copy link

@jawid-h jawid-h commented Oct 28, 2020

I believe it is related to the following issue (#885). We're experiencing the same problem.

When we're having following message in .proto file:

message SomeMessage {
  repeated bytes objects = 1;
}

and trying to submit an empty Buffer in objects list field, the code compiled by protobufjs ignores it. I believe by having following statement:

else if (objects[i].length) ...

This PR fixes that by including zero-length buffers in the check.

@jawid-h jawid-h changed the title fix: empty buffers submitted to a reapeated bytes field were converted to undefined fix: empty buffers submitted to a repeated bytes field were converted to undefined Oct 28, 2020
@alexander-fenster
Copy link
Contributor

@jawid-h The change looks good to us, could you add a test that would cover this situation? Thank you!

@vicb
Copy link
Contributor

vicb commented Nov 20, 2020

Hey @alexander-fenster

I would like to move this issue forward as it is causing some problems with the google datastore node client (googleapis/nodejs-datastore#755).

This is the test case I have tried to add to this PR:

            test.test(test.name + " - Message.toObject with empty buffers", function(test) {
                var msg = Message.create({
                    bytesVal: protobuf.util.newBuffer(0),
                });

                test.equal(Message.toObject(msg, { bytes: String }).bytesVal, "", "bytes to base64 strings");

                if (protobuf.util.isNode) {
                    const bytesVal = Message.toObject(msg, { bytes: Buffer }).bytesVal; 
                    test.ok(Buffer.isBuffer(bytesVal), "bytes to buffers");
                    test.equal(bytesVal.length, 0, "empty buffer");
                }

                test.end();
            });            

However it seems to be causing unrelated troubles.

Would you have any hints on the best way to test this PR ?

Thanks

@alexander-fenster
Copy link
Contributor

Taken care of by #1514.

bcoe added a commit that referenced this pull request Dec 7, 2020
closes #1500
fixes #885

Co-authored-by: Benjamin E. Coe <[email protected]>
This was referenced Jul 8, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants