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

[rahul2393]

Fixes: #8669

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.

[olavloite]

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]

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:

1. Additional context that explains that this is for the JSON decoder. (the Decoder is not a clear reference to JSON decoding)
2. 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]

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:

1. 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.
2. OR: Add overloaded functions that accept an argument that determines which decoding method should be used wherever possible (this would not have my preference).
3. 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.

[rahul2393]

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]

nit: can we change this into

1. First get the current value of UseNumberWithJSONDecoderEncoder
2. A loop that loops through two boolean value true and false
3. Then calls UseNumberWithJSONDecoderEncoder(boolValue)
4. Then after the loop set the UseNumberWithJSONDecoderEncoder to the original value.

[olavloite]

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.

[rahul2393]

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

[olavloite]

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.

[rahul2393]

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?

[olavloite]

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:

1. A client is created. This client is assumed to use the default Go encoding/decoding.
2. The flag is set to true and a new client is created.
3. Both the old client and the new client now use the new behavior.
4. It was however not possible to go back to the default behavior.

[olavloite]

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