Skip to content

Conversation

vicb
Copy link
Contributor

@vicb vicb commented Nov 21, 2020

Thank you for opening a Pull Request! Before submitting your PR, there are a few things you can do to make sure it goes smoothly:

  • Make sure to open an issue as a bug/issue before writing your code! That way we can discuss the change, evaluate designs, and agree on the general idea
  • Ensure the tests and linter pass
  • Code coverage does not decrease (if any source code was changed)
  • Appropriate docs were updated (if necessary)

As discussed in #755 datastore can not currently store an empty buffer due to a bug in protobufjs - see protobufjs/protobuf.js#1500.

However protobufs supports 2 way of passing a buffer, as seen in converter.js:

            case "bytes": gen
                ("if(typeof d%s===\"string\")", prop)
                    ("util.base64.decode(d%s,m%s=util.newBuffer(util.base64.length(d%s)),0)", prop, prop, prop)
                ("else if(d%s.length)", prop)
                    ("m%s=d%s", prop, prop);

This PR switch from passing a Buffer (the else branch in the snippet above) to passing a base64 encoded string (the if branch in the snippet above).

Encoding/Decoding a non empty buffer was already tested and I added a test for encoding/decoding an empty buffer. As expected the test fails with the current code:

  1) Datastore
       create, retrieve and delete
         should save/get/delete an empty buffer:
     Error: 3 INVALID_ARGUMENT: The value "buf" does not contain a value.

I am not sure if #755 should be marked as fixed or if we should wait for protobufs to be fixed ?

@vicb vicb requested a review from a team as a code owner November 21, 2020 20:05
@google-cla google-cla bot added the cla: yes This human has signed the Contributor License Agreement. label Nov 21, 2020
@product-auto-label product-auto-label bot added the api: datastore Issues related to the googleapis/nodejs-datastore API. label Nov 21, 2020
@vicb vicb force-pushed the empty-buffer branch 2 times, most recently from cf4a2b8 to 075011e Compare November 21, 2020 20:27
@codecov
Copy link

codecov bot commented Nov 21, 2020

Codecov Report

Merging #767 (398b614) into master (96ecdfa) will increase coverage by 0.00%.
The diff coverage is 100.00%.

Impacted file tree graph

@@           Coverage Diff           @@
##           master     #767   +/-   ##
=======================================
  Coverage   98.94%   98.94%           
=======================================
  Files          11       11           
  Lines        7893     7896    +3     
  Branches      469      469           
=======================================
+ Hits         7810     7813    +3     
  Misses         81       81           
  Partials        2        2           
Impacted Files Coverage Δ
src/entity.ts 100.00% <100.00%> (ø)

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 96ecdfa...398b614. Read the comment docs.

@crwilcox
Copy link
Contributor

Thanks for the contribution!

@crwilcox crwilcox added the kokoro:run Add this label to force Kokoro to re-run the tests. label Nov 25, 2020
@yoshi-kokoro yoshi-kokoro removed the kokoro:run Add this label to force Kokoro to re-run the tests. label Nov 25, 2020
@crwilcox crwilcox merged commit cf88927 into googleapis:master Nov 25, 2020
@vicb
Copy link
Contributor Author

vicb commented Nov 25, 2020

Thanks for merging the PR Chris.

@vicb vicb deleted the empty-buffer branch November 25, 2020 17:10
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

api: datastore Issues related to the googleapis/nodejs-datastore API. cla: yes This human has signed the Contributor License Agreement.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants