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

feat: add support for JSON data type #1368

Merged
merged 17 commits into from
Aug 26, 2021

Conversation

olavloite
Copy link
Contributor

@olavloite olavloite commented May 12, 2021

Adds support for JSON data type.

@olavloite olavloite added the do not merge Indicates a pull request not ready for merge, due to either quality or timing. label May 12, 2021
@olavloite olavloite requested review from a team as code owners May 12, 2021 14:06
@google-cla google-cla bot added the cla: yes This human has signed the Contributor License Agreement. label May 12, 2021
@product-auto-label product-auto-label bot added the api: spanner Issues related to the googleapis/nodejs-spanner API. label May 12, 2021
@snippet-bot
Copy link

snippet-bot bot commented May 12, 2021

Here is the summary of changes.

You are about to add 3 region tags.

This comment is generated by snippet-bot.
If you find problems with this result, please file an issue at:
https://github.com/googleapis/repo-automation-bots/issues.
To update this comment, add snippet-bot:force-run label or use the checkbox below:

  • Refresh this comment

// array of objects that should be inserted into a JSON column. If we were
// to specify this value as an array instead of a string, the client
// library would encode this value as ARRAY<JSON> instead of JSON.
VenueDetails: `[
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@zoercai Here's an example of the issue with top-level JSON arrays. The client library does not know whether this should be encoded as JSON or ARRAY<JSON> if it is specified as an actual array instead of a string. Specifying it as a string will cause the client library to just encode it as a string, which is the default encoding for JSON values.

Comment on lines +425 to +427
if (is.object(value)) {
return JSON.stringify(value);
}
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is where the most important part of this PR is: It ensures that any value that has been specified as an object will be converted to a valid JSON string. An array of objects will be converted to an array of valid JSON strings.

@codecov
Copy link

codecov bot commented May 17, 2021

Codecov Report

Merging #1368 (52b8cf5) into master (ecaaf3b) will decrease coverage by 0.43%.
The diff coverage is 100.00%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master    #1368      +/-   ##
==========================================
- Coverage   98.60%   98.16%   -0.44%     
==========================================
  Files          23       23              
  Lines       21950    21964      +14     
  Branches     1238     1219      -19     
==========================================
- Hits        21644    21562      -82     
- Misses        297      388      +91     
- Partials        9       14       +5     
Impacted Files Coverage Δ
src/codec.ts 99.70% <100.00%> (-0.30%) ⬇️
src/v1/database_admin_client.ts 98.50% <0.00%> (-1.00%) ⬇️
src/v1/instance_admin_client.ts 98.15% <0.00%> (-0.98%) ⬇️
src/v1/spanner_client.ts 98.40% <0.00%> (-0.85%) ⬇️
src/transaction-runner.ts 97.77% <0.00%> (-0.84%) ⬇️
src/backup.ts 99.10% <0.00%> (-0.72%) ⬇️
src/common-grpc/service-object.ts 99.34% <0.00%> (-0.66%) ⬇️
src/session-pool.ts 99.34% <0.00%> (-0.50%) ⬇️
src/database.ts 99.75% <0.00%> (-0.22%) ⬇️
... and 2 more

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 ecaaf3b...52b8cf5. Read the comment docs.

@skuruppu
Copy link
Contributor

@olavloite I just merged in the codegen PR so you can rebase this PR now.

@olavloite olavloite added do not merge Indicates a pull request not ready for merge, due to either quality or timing. and removed do not merge Indicates a pull request not ready for merge, due to either quality or timing. labels Jun 24, 2021
@olavloite
Copy link
Contributor Author

I added the Do not merge label back as the system test is failing with

  1) Spanner
       types
         "before all" hook for "should throw an error for incorrect value types":
     Error: 12 UNIMPLEMENTED: JSON type is not enabled.
      at Object.callErrorFromStatus (node_modules/@grpc/grpc-js/build/src/call.js:31:26)
          -> /workspace/node_modules/@grpc/grpc-js/src/call.ts:81:24
      at Object.onReceiveStatus (node_modules/@grpc/grpc-js/build/src/client.js:179:52)
          -> /workspace/node_modules/@grpc/grpc-js/src/client.ts:338:36
      at /workspace/node_modules/@grpc/grpc-js/build/src/call-stream.js:80:35
          -> /workspace/node_modules/@grpc/grpc-js/src/call-stream.ts:170:27
      at Object.onReceiveStatus (node_modules/grpc-gcp/build/src/index.js:73:29)
          -> /workspace/node_modules/grpc-gcp/src/index.ts:129:15
      at InterceptingListenerImpl.onReceiveStatus (node_modules/@grpc/grpc-js/build/src/call-stream.js:75:23)
          -> /workspace/node_modules/@grpc/grpc-js/src/call-stream.ts:166:19
      at Object.onReceiveStatus (node_modules/@grpc/grpc-js/build/src/client-interceptors.js:336:141)
          -> /workspace/node_modules/@grpc/grpc-js/src/client-interceptors.ts:426:34
      at Object.onReceiveStatus (node_modules/@grpc/grpc-js/build/src/client-interceptors.js:299:181)
          -> /workspace/node_modules/@grpc/grpc-js/src/client-interceptors.ts:389:48
      at /workspace/node_modules/@grpc/grpc-js/build/src/call-stream.js:145:78
          -> /workspace/node_modules/@grpc/grpc-js/src/call-stream.ts:276:24
      at processTicksAndRejections (internal/process/task_queues.js:79:11)

@bcoe bcoe added the owlbot:run Add this label to trigger the Owlbot post processor. label Jul 2, 2021
@gcf-owl-bot gcf-owl-bot bot removed the owlbot:run Add this label to trigger the Owlbot post processor. label Jul 2, 2021
@generated-files-bot
Copy link

Warning: This pull request is touching the following templated files:

test/codec.ts Outdated Show resolved Hide resolved
@zoercai
Copy link
Contributor

zoercai commented Aug 23, 2021

Hi @olavloite, we're getting ready to merge the JSON PRs now for Node, Java, and Python. You can merge this PR as soon as all the tests pass. Could you please rebase and rerun CI? Thanks!

@olavloite olavloite removed the do not merge Indicates a pull request not ready for merge, due to either quality or timing. label Aug 23, 2021
@olavloite olavloite added the kokoro:force-run Add this label to force Kokoro to re-run the tests. label Aug 23, 2021
@yoshi-kokoro yoshi-kokoro removed the kokoro:force-run Add this label to force Kokoro to re-run the tests. label Aug 23, 2021
@skuruppu skuruppu removed the owlbot:ignore instruct owl-bot to ignore a PR label Aug 24, 2021
@skuruppu skuruppu added automerge Merge the pull request once unit tests and other checks pass. owlbot:run Add this label to trigger the Owlbot post processor. and removed automerge Merge the pull request once unit tests and other checks pass. labels Aug 24, 2021
@gcf-owl-bot gcf-owl-bot bot removed the owlbot:run Add this label to trigger the Owlbot post processor. label Aug 25, 2021
@skuruppu skuruppu added the owlbot:ignore instruct owl-bot to ignore a PR label Aug 25, 2021
@skuruppu skuruppu added the automerge Merge the pull request once unit tests and other checks pass. label Aug 25, 2021
@skuruppu
Copy link
Contributor

@olavloite if you can please take a look at the test failure, that would be great. I ran it a couple of times and got the same error so I gathered it must be due to the new tests we added in this PR.

ERROR: Error: 6 ALREADY_EXISTS: Row [81] in table Venues already exists.

@skuruppu skuruppu removed the automerge Merge the pull request once unit tests and other checks pass. label Aug 25, 2021
@olavloite
Copy link
Contributor Author

@olavloite if you can please take a look at the test failure, that would be great. I ran it a couple of times and got the same error so I gathered it must be due to the new tests we added in this PR.

ERROR: Error: 6 ALREADY_EXISTS: Row [81] in table Venues already exists.

It was caused by a merge/rebase error. The previously added test runs for the tagging feature were also added once more by this PR, which meant that the tagging sample tests were executed twice. I've removed the duplicate test runs, and hopefully that should fix it.

@skuruppu skuruppu added owlbot:ignore instruct owl-bot to ignore a PR and removed owlbot:ignore instruct owl-bot to ignore a PR labels Aug 25, 2021
@skuruppu
Copy link
Contributor

Now asked for help from the Node.js team on how to unblock the OwlBot check.

@skuruppu skuruppu added the automerge Merge the pull request once unit tests and other checks pass. label Aug 26, 2021
@olavloite olavloite merged commit b8d6fe5 into googleapis:master Aug 26, 2021
@gcf-merge-on-green gcf-merge-on-green bot removed the automerge Merge the pull request once unit tests and other checks pass. label Aug 26, 2021
@olavloite olavloite deleted the json-data-type branch August 26, 2021 12:22
asthamohta pushed a commit to asthamohta/nodejs-spanner that referenced this pull request Sep 21, 2021
* feat: support JSON data type

* fix: remove unnecessary changes

* fix: linting + skip tests on emulator

* docs: simplify samples

* feat: decorate commit error with additional information

Decorate the error returned by a commit RPC in the specific case that
the application tries to insert an array of objects into a JSON column.

* test: add unit tests for decorateError

* test: add test for deeply nested objects

* test: add test for decoding complex JSON string

* fix: typo

* fix: run linter

* fix: update expected query output

* chore: run linter

* chore: run linter

* fix: remove duplicate tag test execution

* fix: remove duplicate transaction tag test run

Co-authored-by: skuruppu <skuruppu@google.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
api: spanner Issues related to the googleapis/nodejs-spanner API. cla: yes This human has signed the Contributor License Agreement. owlbot:ignore instruct owl-bot to ignore a PR
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

5 participants