fix(spanner): use json.Number for decoding unknown values from spanner by rahul2393 · Pull Request #9054 · googleapis/google-cloud-go

rahul2393 · 2023-11-29T09:17:55Z

Go client library uses native json library(encoding/json) for marshalling/unmarshalling for decoding JSON spanner types.
Since native package maps values to float64 it automatically rounds off the values.

Example

package main

import (
	"log"
	"context"

	"cloud.google.com/go/spanner"
	"google.golang.org/api/iterator"
)

func getFloatValue(ctx context.Context, client *spanner.Client) {
	iter := client.Single().Query(ctx, spanner.NewStatement("select json '0.39240506000000003'"))
	for {
		row, err := iter.Next()
		if err == iterator.Done {
			break
		}
		if err != nil {
			panic(err)
		}
		var f spanner.NullJSON
		if err := row.Column(0, &f); err != nil {
			panic(err)
		}
		log.Printf("%v\n", f)
	}
}

func main() {
	ctx := context.Background()
	dsn := "projects/my-project/instances/my-instance/databases/my-database"
	client, err := spanner.NewClient(ctx, dsn)
	if err != nil {
		log.Fatal(err)
	}
	defer client.Close()
	getFloatValue(ctx, client)
}

will decode the value as 0.39240506.
Similarly when reading 145688415796432520 will be decoded as 145688415796432500 OR 1.456884157964325e+17

With this change Go client library will parse the JSON number values to same precision as stored in Spanner database using jsoniter library using json.Number when UseNumberWithJSONDecoderEncoder() is called from the application

NOTE

Package jsoniter implements encoding and decoding of JSON as defined in RFC 4627 and provides interfaces with identical syntax of standard lib encoding/json. Converting from encoding/json to jsoniter is no more than replacing the package with jsoniter and variable type declarations (if any). jsoniter interfaces gives 100% compatibility with code using standard lib.
"JSON and Go" (https://golang.org/doc/articles/json_and_go.html ) gives a description of how Marshal/Unmarshal operate between arbitrary or predefined json objects and bytes, and it applies to jsoniter.Marshal/Unmarshal as well.

# Conflicts: # spanner/value.go

olavloite · 2023-12-13T13:25:01Z

spanner/value.go

+	jsonProvider jsoniter.API
+
+	once sync.Once


If I understand this correctly, it means that jsonProvider is set once as a global variable the first time a client is created. That means that if you create a new client later with a different JSON decoding configuration, it will have no effect. Although it is not a very common use case, it is also very confusing. If we can only provide this as a global variable, then we should model it as such in the API as well, and clearly call out how it should be used.

olavloite · 2023-12-13T13:31:31Z

spanner/client.go

+	// UseNumber causes the Decoder to unmarshal a number into an interface{} as a
+	// Number instead of as a float64.


I'm only able to understand this comment because I know the context of this PR. Without that context, this comment contains too little information. Can we add:

Additional context that explains that this is for the JSON decoder. (the Decoder is not a clear reference to JSON decoding)

What it means that something is being unmarshalled as a Number instead of a float64? So what are the pros/cons of each?

So for example:

UseNumber specifies whether number values inside a Cloud Spanner JSON value should be decoded as a Number or a float64. Decoding to a Number guarantees that the precision used by Cloud Spanner is preserved. Decoding to a float64 can cause loss of precision. The default JSON decode function in Go uses float64. This is therefore also the default used by this client library. Change this value to true to prevent loss of precision.

olavloite · 2023-12-13T13:41:28Z

spanner/client.go

-
+	once.Do(func() {
+		// Initialize json provider.
+		jsonProvider = jsoniter.Config{


See also my comment below: This will be confusing for anyone who calls NewClientWithConfig first with UseNumber: false and then later again with UseNumber: true. The configuration appears to be for a single client, but is in reality a global variable. We need to change that so it is either clearer, or it is actually a client config. Possible solutions for that could be:

Add a global method that sets the preferred decode method. Clearly document that the value that is set will be used by all clients, including both clients that have already been created and clients that will be created in the future. Based on where the configuration option is needed, this is probably the best/only possible solution.

OR: Add overloaded functions that accept an argument that determines which decoding method should be used wherever possible (this would not have my preference).

OR (I don't think it is possible, but for completeness): Pass a reference to the config on to all objects that need to know, and use the value there.

I pushed changes for approach 1.

olavloite

I think we should make it possible for customers to both enable and disable the feature, as it is possible that they want to test their code with both options.

olavloite · 2023-12-14T11:21:27Z

spanner/integration_test.go

+	for idx := 0; idx < 2; idx++ {
+		if idx == 1 {
+			UseNumberWithJSONDecoderEncoder()
+			defer testSetJSONProviderNumberConfig(false)


nit: can we change this into

First get the current value of UseNumberWithJSONDecoderEncoder

A loop that loops through two boolean value true and false

Then calls UseNumberWithJSONDecoderEncoder(boolValue)

Then after the loop set the UseNumberWithJSONDecoderEncoder to the original value.

Also: Are we actually using this to check whether we get the expected value? In other words: I don't see any tests that are conditional on whether idx == 1 or idx == 0, and that has a different expected outcome depending on that.

There is condition on whether idx is 0/1, https://github.com/googleapis/google-cloud-go/pull/9054/files#diff-3303adad40b1fa3c6b0a92ea1f4ad28c32bc84c52000e7a5e9fc8b37e05eaa85R2226-R2229

olavloite · 2023-12-14T11:29:00Z

spanner/value.go

+// as Number (preserving precision) or float64 (risking loss).
+// Defaults to float64, call this method for lossless precision.
+// NOTE: This change affects all clients created by this library, both existing and future ones.
+func UseNumberWithJSONDecoderEncoder() {


This API means that you can never disable it after having enabled it. I don't think we have a good reason for that restriction, or? I can imagine that making it possible to both enable and disable the option will make it easier for users to use it in their own tests to determine what the behavior is.

So I think that we should add an argument to the method (true/false), or add a separate method for disabling the option.

Allowing users to enable/disable the feature means they can run into situation where Client 1(supposed to run without Number) running in thread1 get impacted by some thread2 which uses another Client2 and call UseNumberWithJSONDecoderEncoder(), are we ok with it?

That was already the case, however only in the direction default behavior => new behavior. Now we allow this to happen both ways.

What I mean is that the original implementation also allowed the following:

A client is created. This client is assumed to use the default Go encoding/decoding.

The flag is set to true and a new client is created.

Both the old client and the new client now use the new behavior.

It was however not possible to go back to the default behavior.

spanner/value.go

olavloite

LGTM, thanks for being patient with my many small requests :-)

fix(spanner): use json.Number for decoding unknown values from spanner

0a1b9d3

rahul2393 requested review from a team November 29, 2023 09:17

product-auto-label bot added size: m Pull request size is medium. api: spanner Issues related to the Spanner API. labels Nov 29, 2023

rahul2393 requested review from harshachinta and olavloite November 29, 2023 09:18

fix go vet

e127b4f

rahul2393 added the kokoro:force-run Add this label to force Kokoro to re-run the tests. label Nov 29, 2023

fix failing test

0477ce2

kokoro-team removed the kokoro:force-run Add this label to force Kokoro to re-run the tests. label Nov 29, 2023

Merge remote-tracking branch 'origin' into fix-number-decode

9608e01

# Conflicts: # spanner/value.go

rahul2393 force-pushed the fix-number-decode branch from fa2b944 to 9608e01 Compare December 5, 2023 07:07

rahul2393 added 2 commits December 6, 2023 06:56

fix tests

4c93151

add integration tests

2133585

rahul2393 force-pushed the fix-number-decode branch from be1e957 to 2133585 Compare December 12, 2023 18:00

product-auto-label bot added size: l Pull request size is large. and removed size: m Pull request size is medium. labels Dec 12, 2023

rahul2393 mentioned this pull request Dec 12, 2023

fix(spanner): use json.Number for decoding unknown values from spanner #9104

Closed

Merge branch 'main' into fix-number-decode

00fae7c

rahul2393 added the kokoro:force-run Add this label to force Kokoro to re-run the tests. label Dec 12, 2023

kokoro-team removed the kokoro:force-run Add this label to force Kokoro to re-run the tests. label Dec 12, 2023

rahul2393 and others added 5 commits December 12, 2023 23:33

fix go vet

75f73f7

fix race condition

ef8e494

fix failing tests

bc82a26

fix go vet

430c29f

Merge branch 'main' into fix-number-decode

43206f2

olavloite reviewed Dec 13, 2023

View reviewed changes

add global helper method to enable json.Number

286145f

rahul2393 force-pushed the fix-number-decode branch from 5291a3a to bb44f9b Compare December 14, 2023 10:38

rahul2393 force-pushed the fix-number-decode branch 2 times, most recently from 60903bb to a190f53 Compare December 14, 2023 10:55

incorporate suggestions

0c5fe88

rahul2393 force-pushed the fix-number-decode branch from a190f53 to 0c5fe88 Compare December 14, 2023 10:58

olavloite reviewed Dec 14, 2023

View reviewed changes

Incorporate suggestions

a6d1e54

rahul2393 force-pushed the fix-number-decode branch from 5724768 to a6d1e54 Compare December 14, 2023 11:56

rahul2393 requested a review from olavloite December 14, 2023 11:59

Merge branch 'main' into fix-number-decode

2775ea9

olavloite approved these changes Dec 14, 2023

View reviewed changes

rahul2393 merged commit 40d1392 into main Dec 14, 2023

rahul2393 deleted the fix-number-decode branch December 14, 2023 14:00

release-please bot mentioned this pull request Dec 14, 2023

chore(main): release spanner 1.54.0 #9129

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

fix(spanner): use json.Number for decoding unknown values from spanner#9054

fix(spanner): use json.Number for decoding unknown values from spanner#9054
rahul2393 merged 16 commits intomainfrom
fix-number-decode

rahul2393 commented Nov 29, 2023 •

edited

Loading

Uh oh!

olavloite Dec 13, 2023

Uh oh!

olavloite Dec 13, 2023

Uh oh!

olavloite Dec 13, 2023

Uh oh!

rahul2393 Dec 14, 2023

Uh oh!

olavloite left a comment

Uh oh!

olavloite Dec 14, 2023

Uh oh!

olavloite Dec 14, 2023

Uh oh!

rahul2393 Dec 14, 2023

Uh oh!

olavloite Dec 14, 2023

Uh oh!

rahul2393 Dec 14, 2023

Uh oh!

olavloite Dec 14, 2023

Uh oh!

Uh oh!

olavloite left a comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

		// UseNumber causes the Decoder to unmarshal a number into an interface{} as a
		// Number instead of as a float64.

Conversation

rahul2393 commented Nov 29, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

NOTE

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

olavloite left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

olavloite left a comment

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

rahul2393 commented Nov 29, 2023 •

edited

Loading