feat: add support for JSON data type in Spanner #6390

olavloite · 2021-04-28T19:08:27Z

Introduces support for the new JSON data type to Spanner. JSON is represented as plain strings in the client library, and no parsing or checking of the JSON is done by the client. As strings are also used to represent the STRING data type in Spanner, there is no default mapping from one Clr type to the JSON data type. That means that a client application must set SpannerDbType.Json as the type of a parameter that should be interpreted and sent as JSON to Spanner. (This is equal to the behavior of the DATE data type that also does not have a corresponding Clr type, as DateTime is already used for TIMESTAMP.)

jskeet · 2021-04-29T07:14:11Z

I will have a look at this when I get the chance, but it may not be before Tuesday (due to vacation and a public holiday).

skuruppu

Looks good from my perspective but I will let @jskeet review first before approving.

jskeet

Mostly fine - I'd just like to see valid values in the tests.

jskeet · 2021-05-04T13:36:19Z

.../Google.Cloud.Spanner.Data/Google.Cloud.Spanner.Data.IntegrationTests/GetSchemaTableTests.cs

@@ -45,11 +44,15 @@ public async Task GetSchemaTable_WithFlagEnabled_ReturnsNull()
        {
            using (var connection = new SpannerConnection($"{_fixture.ConnectionString};EnableGetSchemaTable=true"))
            {
-                var command = connection.CreateSelectCommand($"SELECT Int64Value, BytesArrayValue FROM {_fixture.TableName}");
+                var sql = _fixture.RunningOnEmulator


I think it would be cleaner to have a separate to for JSON schema values. Most of what's tested is here doesn't really correspond with "GetSchemaTable with the flag enabled returns null".

I think that this test case more in general could need some work:

The _ReturnsNull suffix indicates that the method should return null with the flag enabled, but it actually does the opposite: Enabling the flag will ensure that the method does not return null.

The current method only checks the return values of Int64 and BytesArray columns. I think it would be valuable to cover all the data types in this test case. It already uses a table that contains columns with all supported data types, so it would only be a matter of adding assertions for the other columns as well. (This is the reason I chose to add the assertions for JSON here)

WDYT?

Agreed about the suffix being odd. (I can't remember the history of this test.)

I think if we want to handle all types, we probably want a complete rewrite of the test, ideally separately from this PR. I could probably do that in the next week or so, if this PR can wait for that?

Yes, that shouldn't be a problem, it will probably take a little longer before this PR can be merged.

(I'm also happy to take it on, if that would be easier for you)

If you've got the time, that would be great, thanks. Feel free to change your mind and push it back to me :)

...le.Cloud.Spanner.Data/Google.Cloud.Spanner.Data.Tests/SpannerDbTypeTests.ValueConversions.cs

jskeet

Out of interest, do we expect the emulator to support JSON at the same time as the production service? If so, I think I'd prefer to see the PR with unconditional tests that fail on the emulator... the code will be so much simpler.

jskeet · 2021-05-06T14:59:42Z

.../Google.Cloud.Spanner.Data/Google.Cloud.Spanner.Data.IntegrationTests/GetSchemaTableTests.cs

@@ -45,11 +44,15 @@ public async Task GetSchemaTable_WithFlagEnabled_ReturnsNull()
        {
            using (var connection = new SpannerConnection($"{_fixture.ConnectionString};EnableGetSchemaTable=true"))
            {
-                var command = connection.CreateSelectCommand($"SELECT Int64Value, BytesArrayValue FROM {_fixture.TableName}");
+                var sql = _fixture.RunningOnEmulator


Agreed about the suffix being odd. (I can't remember the history of this test.)

I think if we want to handle all types, we probably want a complete rewrite of the test, ideally separately from this PR. I could probably do that in the next week or so, if this PR can wait for that?

olavloite · 2021-05-07T07:08:38Z

Out of interest, do we expect the emulator to support JSON at the same time as the production service? If so, I think I'd prefer to see the PR with unconditional tests that fail on the emulator... the code will be so much simpler.

No, unfortunately the emulator will normally implement new features of Cloud Spanner with a delay of 2-3 months (but could be more, could be less). For the NUMERIC data type, which was the previous similar addition, it took a little more than 3 months between the official release and before it was available on the emulator.

amanda-tarafa

I think you might be missing an update to BindingTests

snippet-bot · 2021-05-12T16:22:48Z

No region tags are edited in this PR.

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

olavloite · 2021-05-12T16:29:08Z

I think you might be missing an update to BindingTests

Good point, thanks. I've added tests for JSON to the BindingTests as well.

I've also removed the special handling of NUMERIC from the tests here: #6456

skuruppu · 2021-06-09T04:36:48Z

@olavloite if you could handle the conflicts on this PR, that would be awesome.

@jskeet and @amanda-tarafa if you could please take another look and approve, that would be great. We're getting close to being able to merge this PR.

jskeet

Just re-reviewed - looks fine to me, thanks. Will merge after collisions have been resolved.

jskeet · 2021-06-09T06:41:32Z

Sorry, just re-read - I won't merge until given the green light explicitly :)

jskeet · 2021-06-24T06:04:35Z

#6659 now includes the JSON type - does that mean this can be rebased and merged?

skuruppu · 2021-06-24T06:08:44Z

Since the tests are passing, yes absolutely :)

olavloite · 2021-06-24T08:50:59Z

#6659 now includes the JSON type - does that mean this can be rebased and merged?

It's now been rebased and cleaned up. As far as I'm concerned, it is ready to be merged.

jskeet · 2021-06-24T09:25:11Z

Hmm... it's showing conflicts at the moment. Could you rebase against master and force push again? I'll then review (which may well be tomorrow rather than today) and merge if I have no further comments.

olavloite · 2021-06-24T09:35:07Z

Hmm... it's showing conflicts at the moment. Could you rebase against master and force push again? I'll then review (which may well be tomorrow rather than today) and merge if I have no further comments.

Huh, I'm not seeing any conflicts at the moment... Could it be that your browser is loading the PR from cache?

jskeet · 2021-06-24T09:41:13Z

Could it be that your browser is loading the PR from cache?

Nope, force reloaded. This is what I'm seeing at the bottom of the PR - which may be because I've got the "Rebase and merge" option selects (and that's what I'd want to use):

olavloite · 2021-06-24T09:42:53Z

Ah, I see. (The Squash and merge which is the default I'm seeing does not indicate any merge problems). I'll have a look.

test: use valid JSON values in tests test: add binding tests for JSON fix: remove unused sample tags fix: fix and add back tests losts in rebase

olavloite · 2021-06-24T10:28:06Z

I've rebased and squashed it down to one commit. PTAL when you have time.

jskeet · 2021-06-24T11:30:37Z

Great, thanks - have been advised to hold off actually merging temporarily anyway, but I'll review when I can just to remind myself of it, and be ready to merge when given the go-ahead.

jskeet

Basically still fine - a few comments that could be cleaned up. I'm going to hold off merging as advised even after that though.

...Google.Cloud.Spanner.Data/Google.Cloud.Spanner.Data.IntegrationTests/AllTypesTableFixture.cs

.../Google.Cloud.Spanner.Data/Google.Cloud.Spanner.Data.IntegrationTests/GetSchemaTableTests.cs

apis/Google.Cloud.Spanner.Data/Google.Cloud.Spanner.Data.IntegrationTests/WriteTests.cs

jskeet · 2021-06-25T13:14:42Z

apis/Google.Cloud.Spanner.Data/Google.Cloud.Spanner.Data/SpannerDbType.ValueConversion.cs

+                case TypeCode.Json:
+                    if (value is string jsonString)
+                    {
+                        return new Value { StringValue = jsonString };


Note to self: after this is merged, I'll rewrite all of these "new Value" to "Value.FromString" etc.

skuruppu · 2021-08-23T11:10:14Z

If everyone is ok with it, and since the tests are passing, let's merge this PR :)

Changes in Google.Cloud.Spanner.Data version 3.12.0: - [Commit 7c6a6f1](googleapis@7c6a6f1): feat: add support for JSON data type in Spanner ([issue 6390](googleapis#6390)) - [Commit ac367e2](googleapis@ac367e2): feat: Regenerate all APIs to support self-signed JWTs - [Commit 61938b6](googleapis@61938b6): - feat(Spanner): Support comments and statement hints in untyped commands. - Alternative to [issue 6848](googleapis#6848) - Closes [issue 6847](googleapis#6847) - [Commit d26b04c](googleapis@d26b04c): fix: address review comments - [Commit d2025be](googleapis@d2025be): fix: use logger from SpannerSettings - [Commit b34f6f4](googleapis@b34f6f4): cleanup: fix comment + remove unnecessary import - [Commit fc7a41b](googleapis@fc7a41b): test: remove connectionstring tests and add settings test - [Commit 6016ef0](googleapis@6016ef0): - feat: support custom SpannerSettings in SessionPoolManager - Support setting custom SpannerSettings when creating a SessionPoolManager. - [Commit 0ab6b8b](googleapis@0ab6b8b): - feat: allow adding an additional version header - Allows adding an additional version header to the connection string. This will be - added to the `x-goog-api-client` header that is used by the underlying Spanner client. - Only a fixed set of values may be set for the header (currently only 'efcore' is - allowed), and the property is not intended for public use. - [Commit 250124f](googleapis@250124f): - fix: synchronize access to the underlying transaction for ambient transactions ([issue 6616](googleapis#6616)) - * fix: synchronize access to the underlying transaction for ambient transactions - Synchronizes access to the underlying Spanner transaction to prevent - multiple transactions from being created if parallel commands are - executed on the ambient transaction. - * test: add integration test - * fix: use Lazy for initialization - * chore: address review comments - * fix: remove unnecessary read call Packages in this release: - Release Google.Cloud.Spanner.Admin.Database.V1 version 3.12.0 - Release Google.Cloud.Spanner.Admin.Instance.V1 version 3.12.0 - Release Google.Cloud.Spanner.Common.V1 version 3.12.0 - Release Google.Cloud.Spanner.Data version 3.12.0 - Release Google.Cloud.Spanner.V1 version 3.12.0

Changes in Google.Cloud.Spanner.Data version 3.12.0: - [Commit 7c6a6f1](7c6a6f1): feat: add support for JSON data type in Spanner ([issue 6390](#6390)) - [Commit ac367e2](ac367e2): feat: Regenerate all APIs to support self-signed JWTs - [Commit 61938b6](61938b6): - feat(Spanner): Support comments and statement hints in untyped commands. - Alternative to [issue 6848](#6848) - Closes [issue 6847](#6847) - [Commit d26b04c](d26b04c): fix: address review comments - [Commit d2025be](d2025be): fix: use logger from SpannerSettings - [Commit b34f6f4](b34f6f4): cleanup: fix comment + remove unnecessary import - [Commit fc7a41b](fc7a41b): test: remove connectionstring tests and add settings test - [Commit 6016ef0](6016ef0): - feat: support custom SpannerSettings in SessionPoolManager - Support setting custom SpannerSettings when creating a SessionPoolManager. - [Commit 0ab6b8b](0ab6b8b): - feat: allow adding an additional version header - Allows adding an additional version header to the connection string. This will be - added to the `x-goog-api-client` header that is used by the underlying Spanner client. - Only a fixed set of values may be set for the header (currently only 'efcore' is - allowed), and the property is not intended for public use. - [Commit 250124f](250124f): - fix: synchronize access to the underlying transaction for ambient transactions ([issue 6616](#6616)) - * fix: synchronize access to the underlying transaction for ambient transactions - Synchronizes access to the underlying Spanner transaction to prevent - multiple transactions from being created if parallel commands are - executed on the ambient transaction. - * test: add integration test - * fix: use Lazy for initialization - * chore: address review comments - * fix: remove unnecessary read call Packages in this release: - Release Google.Cloud.Spanner.Admin.Database.V1 version 3.12.0 - Release Google.Cloud.Spanner.Admin.Instance.V1 version 3.12.0 - Release Google.Cloud.Spanner.Common.V1 version 3.12.0 - Release Google.Cloud.Spanner.Data version 3.12.0 - Release Google.Cloud.Spanner.V1 version 3.12.0

olavloite added the do not merge Indicates a pull request not ready for merge, due to either quality or timing. label Apr 28, 2021

olavloite requested a review from zoercai April 28, 2021 19:08

olavloite requested a review from a team as a code owner April 28, 2021 19:08

google-cla bot added the cla: yes This human has signed the Contributor License Agreement. label Apr 28, 2021

skuruppu reviewed Apr 29, 2021

View reviewed changes

jskeet reviewed May 4, 2021

View reviewed changes

jskeet reviewed May 6, 2021

View reviewed changes

amanda-tarafa reviewed May 11, 2021

View reviewed changes

jskeet approved these changes Jun 9, 2021

View reviewed changes

olavloite force-pushed the spanner-json branch from 01cb8e4 to c52b531 Compare June 24, 2021 08:43

olavloite removed the do not merge Indicates a pull request not ready for merge, due to either quality or timing. label Jun 24, 2021

olavloite force-pushed the spanner-json branch from 52914df to 8081996 Compare June 24, 2021 10:05

feat: support JSON data type

19b97de

test: use valid JSON values in tests test: add binding tests for JSON fix: remove unused sample tags fix: fix and add back tests losts in rebase

olavloite force-pushed the spanner-json branch from 8081996 to 19b97de Compare June 24, 2021 10:22

jskeet approved these changes Jun 25, 2021

View reviewed changes

chore: address review comments

01b93b5

amanda-tarafa mentioned this pull request Aug 13, 2021

feat: add samples for JSON data type GoogleCloudPlatform/dotnet-docs-samples#1315

Merged

Merge branch 'master' into spanner-json

cd25e2a

skuruppu merged commit 7c6a6f1 into googleapis:master Aug 23, 2021

amanda-tarafa mentioned this pull request Aug 24, 2021

Release Spanner libraries version 3.12.0 #7032

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat: add support for JSON data type in Spanner #6390

feat: add support for JSON data type in Spanner #6390

olavloite commented Apr 28, 2021 •

edited

Loading

jskeet commented Apr 29, 2021

skuruppu left a comment

jskeet left a comment

jskeet May 4, 2021

olavloite May 6, 2021

jskeet May 6, 2021

olavloite May 7, 2021

jskeet May 7, 2021

jskeet left a comment

jskeet May 6, 2021

olavloite commented May 7, 2021

amanda-tarafa left a comment

snippet-bot bot commented May 12, 2021 •

edited

Loading

olavloite commented May 12, 2021

skuruppu commented Jun 9, 2021

jskeet left a comment

jskeet commented Jun 9, 2021

jskeet commented Jun 24, 2021

skuruppu commented Jun 24, 2021

olavloite commented Jun 24, 2021

jskeet commented Jun 24, 2021

olavloite commented Jun 24, 2021

jskeet commented Jun 24, 2021

olavloite commented Jun 24, 2021

olavloite commented Jun 24, 2021

jskeet commented Jun 24, 2021

jskeet left a comment

jskeet Jun 25, 2021

skuruppu commented Aug 23, 2021

feat: add support for JSON data type in Spanner #6390

feat: add support for JSON data type in Spanner #6390

Conversation

olavloite commented Apr 28, 2021 • edited Loading

jskeet commented Apr 29, 2021

skuruppu left a comment

Choose a reason for hiding this comment

jskeet left a comment

Choose a reason for hiding this comment

jskeet May 4, 2021

Choose a reason for hiding this comment

olavloite May 6, 2021

Choose a reason for hiding this comment

jskeet May 6, 2021

Choose a reason for hiding this comment

olavloite May 7, 2021

Choose a reason for hiding this comment

jskeet May 7, 2021

Choose a reason for hiding this comment

jskeet left a comment

Choose a reason for hiding this comment

jskeet May 6, 2021

Choose a reason for hiding this comment

olavloite commented May 7, 2021

amanda-tarafa left a comment

Choose a reason for hiding this comment

snippet-bot bot commented May 12, 2021 • edited Loading

No region tags are edited in this PR.

olavloite commented May 12, 2021

skuruppu commented Jun 9, 2021

jskeet left a comment

Choose a reason for hiding this comment

jskeet commented Jun 9, 2021

jskeet commented Jun 24, 2021

skuruppu commented Jun 24, 2021

olavloite commented Jun 24, 2021

jskeet commented Jun 24, 2021

olavloite commented Jun 24, 2021

jskeet commented Jun 24, 2021

olavloite commented Jun 24, 2021

olavloite commented Jun 24, 2021

jskeet commented Jun 24, 2021

jskeet left a comment

Choose a reason for hiding this comment

jskeet Jun 25, 2021

Choose a reason for hiding this comment

skuruppu commented Aug 23, 2021

olavloite commented Apr 28, 2021 •

edited

Loading

snippet-bot bot commented May 12, 2021 •

edited

Loading