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

spanner: Can not decode Null value to user defined custom type with DecodeSpanner #4552

Closed
NeoCN opened this issue Aug 4, 2021 · 7 comments · Fixed by #4558
Closed

spanner: Can not decode Null value to user defined custom type with DecodeSpanner #4552

NeoCN opened this issue Aug 4, 2021 · 7 comments · Fixed by #4558
Assignees
Labels
api: spanner Issues related to the Cloud Spanner API. priority: p2 Moderately-important priority. Fix may not be included in next release.

Comments

@NeoCN
Copy link

NeoCN commented Aug 4, 2021

Client

Spanner

Environment

macOS

Go Environment

$ go version
go version go1.16.6 darwin/amd64
$ go env

Code

https://github.com/googleapis/google-cloud-go/blob/v0.90.0/spanner/value.go#L1330-L1338

		// Check if the pointer is a custom type that implements spanner.Decoder
		// interface.
		if decodedVal, ok := ptr.(Decoder); ok {
			x, err := getGenericValue(v)
			if err != nil {
				return err
			}
			return decodedVal.DecodeSpanner(x)
		}

https://github.com/googleapis/google-cloud-go/blob/v0.90.0/spanner/value.go#L1911-L1923

// getGenericValue returns the interface{} value encoded in proto3.Value.
func getGenericValue(v *proto3.Value) (interface{}, error) {
	switch x := v.GetKind().(type) {
	case *proto3.Value_NumberValue:
		return x.NumberValue, nil
	case *proto3.Value_BoolValue:
		return x.BoolValue, nil
	case *proto3.Value_StringValue:
		return x.StringValue, nil
	default:
		return 0, errSrcVal(v, "Number, Bool, String")
	}
}

Expected behavior

With user defined custom type that implements spanner.Decoder interface, how the Null value is handled can be defined in the DecodeSpanner function, and the expected behavior is that DecodeSpanner function is invoked when decode Null value.

Actual behavior

As the code logic listed, getGenericValue function will return error when the value type is not Number, Bool, String.
And when the value is Null, the value type is *structpb.Value_NullValue, an error returned, so the DecodeSpanner in custom type will not be invoked and an error returned.

cannot use null_value:NULL_VALUE(Kind: *structpb.Value_NullValue) as Number, Bool, String Value
@NeoCN NeoCN added the triage me I really want to be triaged. label Aug 4, 2021
@product-auto-label product-auto-label bot added the api: spanner Issues related to the Cloud Spanner API. label Aug 4, 2021
@hengfengli hengfengli assigned olavloite and unassigned hengfengli Aug 5, 2021
@hengfengli hengfengli added priority: p2 Moderately-important priority. Fix may not be included in next release. and removed triage me I really want to be triaged. labels Aug 5, 2021
@hengfengli
Copy link
Contributor

hengfengli commented Aug 5, 2021

@olavloite Can you please take a look at this issue? Thanks.

olavloite added a commit that referenced this issue Aug 5, 2021
Allow NULL values from the database to be passed in to the DecodeSpanner method
of a struct that implements spanner.Decoder.

Fixes #4552
olavloite added a commit that referenced this issue Aug 5, 2021
Allow NULL values from the database to be passed in to the DecodeSpanner method
of a struct that implements spanner.Decoder.

Fixes #4552
olavloite added a commit that referenced this issue Aug 5, 2021
Allow NULL values from the database to be passed in to the DecodeSpanner method
of a struct that implements spanner.Decoder.

Fixes #4552
@NeoCN
Copy link
Author

NeoCN commented Aug 5, 2021

@hengfengli @olavloite Thanks for the quick fix.

Can you share the reasons for restrict the type that can be decoded by DecodeSpanner ?

@olavloite
Copy link
Contributor

olavloite commented Aug 5, 2021

@hengfengli @olavloite Thanks for the quick fix.

Can you share the reasons for restrict the type that can be decoded by DecodeSpanner ?

I'm not exactly sure what you mean in this case. Do you mean the restriction before this change? If so, I think the restriction was simply an accidental omission by the original author that did not realize that null values needed special handling.
Or do you mean that there is some restriction still in place that will prevent the decoding of some specific type also after this change? If so, would you mind giving an example of what type cannot be decoded?

@hengfengli
Copy link
Contributor

hengfengli commented Aug 5, 2021

@NeoCN Do you mean why we limit to proto3.Value_NumberValue, proto3.Value_BoolValue, proto3.Value_StringValue? This is because all spanner primitive types (I think Array and Struct are not primitive types) are encoded to these three types of values, refer to https://github.com/googleapis/googleapis/blob/master/google/spanner/v1/type.proto.

@NeoCN
Copy link
Author

NeoCN commented Aug 5, 2021

@hengfengli @olavloite Got it, thanks guys.

@NeoCN
Copy link
Author

NeoCN commented Aug 16, 2021

@hengfengli When will the new spanner package version with this bugfix be released ?

@hengfengli
Copy link
Contributor

hengfengli commented Aug 16, 2021

I can do a patch release (1.24.1) today.

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 Cloud Spanner API. priority: p2 Moderately-important priority. Fix may not be included in next release.
Projects
None yet
Development

Successfully merging a pull request may close this issue.

3 participants