feat(spanner): add ToStructLenient method to decode to struct fields with no error return in case of un-matched row's column with struct's exported fields #5153

rahul2393 · 2021-11-17T07:46:17Z

The PR adds method ToStructLenient which decodes the row's column into the struct's exported fields, the difference with the existing ToStruct methods are:

Decoding from row's column to struct with no matching exported fields is allowed.

ToStructLenient should pass all existing and future test cases for ToStruct method so tests are updated to make sure same test cases run for both methods.

ToStructLenient is created to handle below example:

DDL:

CREATE TABLE Singers (
    SingerId	INT64 NOT NULL,
    FirstName	STRING(1024)
) PRIMARY KEY (SingerId)

and client code looks like

struct Singer {
  SingerId     int64
  FirstName string
}

func GetSinger(ID int64) (*Singer, error) {
  ...
  spannerCli.Single().Query(ctx, spanner.Statement {
      SQL: 'SELECT * FROM singers WHERE SingerId=@id',
      Params: map[string]string{"id": ID},
  ).Do(func(r *spanner.Row) error {
       singer := new(Singer)
       r.ToStruct(singer)
  )
}

Now, let's say we update the singers table to have new column LastName, ToStruct will start returning errNoOrDupGoField after schema update to fix this ToStructLenient can be used instead of ToStruct.

Fixes: #5131

git-hulk · 2021-11-18T04:04:57Z

Cool, many thanks to @rahul2393

olavloite

There seems to be some integration tests that are failing. Would you mind taking a look at those?

olavloite · 2021-11-18T07:49:51Z

spanner/value.go

@@ -2871,6 +2871,11 @@ func errNilSpannerStructType() error {
 	return spannerErrorf(codes.FailedPrecondition, "unexpected nil StructType in decoding Cloud Spanner STRUCT")
 }

+// errDupGoField returns error for duplicated Go STRUCT field names
+func errDupGoField(s interface{}) error {
+	return spannerErrorf(codes.InvalidArgument, "Go struct %+v(type %T) has duplicate fields for GO STRUCT field", s, s)


nit: Could this error also include the field that is duplicate? I think that would be helpful to any user who needs to debug code that returns this error.

olavloite · 2021-11-18T07:51:56Z

spanner/value.go

+	for _, f := range fieldNames {
+		sf := fields.Match(f)
+		if sf == nil {
+			return errDupGoField(ptr)


Is this really a duplicate field? Or is it a missing field? I would expect fields.Match(f) to return nil if the field name is not in the list, and that this scenario is that the field is missing and not duplicate.

Here it is duplicate field, it would be missing if we would have tried matching the spanner.Row field name, but here the same GO struct field names are matched from which the fields object was created.

olavloite · 2021-11-18T07:55:57Z

spanner/value.go

@@ -2995,6 +3007,38 @@ func decodeStructArray(ty *sppb.StructType, pb *proto3.ListValue, ptr interface{
 	return nil
 }

+func getAllFieldNames(v reflect.Value) []string {
+	var names []string
+	s := v


Why do we need this?

We need this for cases when GO struct has more than one common spanner tags, In that case we want to return duplicate error. There was no way to get all exported struct field names so created one here. please let me know if you think there is another good way to do this.

Sorry, I should have been more specific. I meant literally why do we need this specific line:

s := v

Why can't the method just use v?

olavloite · 2021-11-18T08:01:18Z

Fixes: #5131

Also, would you mind extending the PR description to describe what the PR does. I understand that it solves the issue, but adding a short description of what/how makes it easier to review and will automatically be included in the release notes.

rahul2393 · 2021-11-18T08:09:12Z

Thanks @olavloite, Integration tests passed and updated the PR description.

olavloite · 2021-11-18T09:25:46Z

@rahul2393 Thanks for the changes. The implementation itself looks good to me.

I however have a couple of general concerns / questions regarding this:

First of all, I think this PR should be labelled feat and not fix. This is not fixing a bug, but relaxing a restriction that appears to have been added quite deliberately when the feature was created. Would you mind extending the PR description a little bit more to explain what is possible now that was not possible before (preferably with an example)?
I'm also wondering whether we need to mark this as a breaking change, or otherwise at least flag it more prominently in the release notes than a 'normal' feature, as we are changing the behavior of the method. The ToStruct method would previously return an error in certain cases, and after this change, it will stop returning an error. In theory, there could be users who rely on an error being returned in those cases, and whose code would break after this change.
We should update the documentation of the ToStruct method here. As far as I can tell, this change does not contradict with anything in the documentation of that method, but that is because the documentation is not explicit in what happens if the result set contains more/less columns than there are fields in the struct. The documentation should be extended to explicitly state what the method does if the exported fields of the struct and the columns in the result set do not match.
The documentation also does not explicitly state that it will only fill exported fields. From what I can tell, both the current implementation and the one in this PR will only fill exported fields, and I think the documentation should reflect that.

rahul2393 · 2021-11-19T10:45:04Z

@olavloite I have updated the PR to add new method instead of updating the existing ToStruct method so it won't be a breaking change, also updated the PR description. same tests should be run for ToStructLenient which were created for ToStruct so tests are updated, this will make sure that functionality wise ToStruct and ToStructLenient works same with just one difference of relaxing restriction on matching row's column with struct exported field.
Please help review again.

olavloite · 2021-11-19T10:55:24Z

spanner/examples_test.go

+	if err != nil {
+		// TODO: Handle error.
+	}
+	row, err := client.Single().ReadRow(ctx, "Accounts", spanner.Key{"alice"}, []string{"name", "balance"})


nit: I think this example would be clearer if we either added/removed an extra column in the read operation, and/or in the Account struct, as to show that the two do not need to have the exact same fields.

olavloite · 2021-11-19T10:57:28Z

spanner/row.go

-// If ToStruct returns an error, the contents of p are undefined. Some fields may
+// If ToStruct returns an error, the contents of p are undefined or if the there is un-matching
+// column from row's columns into struct's exported fields. Some fields may


I think you should undo this change. The original 'if there is an error...' statement is true regardless of the type of error. Instead, the documentation above should clearly state when the method will return an error.

I would suggest that we add a point 3 above with something like this:

// 3. The number of columns in the row must match the number of exported fields in the struct. There must be exactly one match for each column in the row. The method will return an error if a column in the row cannot be assigned to a field in the struct. The method will also return an error if an exported field in the struct cannot not be assigned a value from the row.

if an exported field in the struct cannot not be assigned a value from the row.

I think this is not the case with ToStruct now, it won't return error.

olavloite · 2021-11-19T11:04:26Z

spanner/row.go

+// Slice and pointer fields will be set to nil if the source column is NULL, and a
+// non-nil value if the column is not NULL. To decode NULL values of other types, use
+// one of the spanner.NullXXX types as the type of the destination field.
+func (r *Row) ToStructLenient(p interface{}) error {


Suggested change

func (r *Row) ToStructLenient(p interface{}) error {

// If ToStruct returns an error, the contents of p are undefined. Some fields may

// have been successfully populated, while others were not; you should not use any of

// the fields.

func (r *Row) ToStructLenient(p interface{}) error {

This warning is still true for this method. Although this method is less likely to return an error, it can still fail if for example the type of one of the fields is not compatible with the datatype of the corresponding column.

olavloite · 2021-11-19T11:06:32Z

spanner/row.go

+//
+//   2. Otherwise, if the name of a field matches the name of a column (ignoring case),
+//      decode the column into the field.
+//


Suggested change

//

//

// 3. The number of columns in the row and exported fields in the struct do not need to match.

// Any field in the struct that cannot not be assigned a value from the row is assigned its default value.

// Any column in the row that does not have a corresponding field in the struct is ignored.

olavloite · 2021-11-19T11:09:54Z

spanner/value.go

@@ -2986,7 +3003,7 @@ func decodeStructArray(ty *sppb.StructType, pb *proto3.ListValue, ptr interface{
 			return errDecodeArrayElement(i, pv, "STRUCT", err)
 		}
 		// Decode proto3.ListValue l into struct referenced by s.Interface().
-		if err = decodeStruct(ty, l, s.Interface()); err != nil {
+		if err = decodeStruct(ty, l, s.Interface(), false); err != nil {


Shouldn't this be:

Suggested change

if err = decodeStruct(ty, l, s.Interface(), false); err != nil {

if err = decodeStruct(ty, l, s.Interface(), lenient); err != nil {

No, this is called from decodeStructArray and we don't want to change that function.

Ah, yeah, good point. I missed that.

olavloite · 2021-11-19T11:10:32Z

spanner/value.go

@@ -2921,13 +2926,25 @@ func decodeStruct(ty *sppb.StructType, pb *proto3.ListValue, ptr interface{}) er
 	if err != nil {
 		return ToSpannerError(err)
 	}
+	// return error if linent is true and destination has duplicate exported columns


nit:

Suggested change

// return error if linent is true and destination has duplicate exported columns

// return error if lenient is true and destination has duplicate exported columns

…with no error return with un-matched row's column with struct's exported fields.

olavloite · 2021-11-22T08:17:30Z

spanner/value.go

@@ -2986,7 +3003,7 @@ func decodeStructArray(ty *sppb.StructType, pb *proto3.ListValue, ptr interface{
 			return errDecodeArrayElement(i, pv, "STRUCT", err)
 		}
 		// Decode proto3.ListValue l into struct referenced by s.Interface().
-		if err = decodeStruct(ty, l, s.Interface()); err != nil {
+		if err = decodeStruct(ty, l, s.Interface(), false); err != nil {


Ah, yeah, good point. I missed that.

hengfengli · 2021-11-22T11:01:33Z

spanner/value.go

@@ -2921,13 +2926,25 @@ func decodeStruct(ty *sppb.StructType, pb *proto3.ListValue, ptr interface{}) er
 	if err != nil {
 		return ToSpannerError(err)
 	}
+	// return error if lenient is true and destination has duplicate exported columns


nit: return -> Return

rahul2393 requested review from hengfengli, skuruppu and a team as code owners November 17, 2021 07:46

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

product-auto-label bot added the api: spanner Issues related to the Spanner API. label Nov 17, 2021

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

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

rahul2393 force-pushed the decode-struct-unequal-fields branch 2 times, most recently from 9c05613 to 77d0c1a Compare November 18, 2021 05:30

rahul2393 requested review from codyoss and olavloite November 18, 2021 05:30

rahul2393 force-pushed the decode-struct-unequal-fields branch 3 times, most recently from 391696e to c615922 Compare November 18, 2021 07:53

olavloite reviewed Nov 18, 2021

View reviewed changes

rahul2393 force-pushed the decode-struct-unequal-fields branch 2 times, most recently from b2fadc7 to 64e89cb Compare November 18, 2021 08:21

rahul2393 force-pushed the decode-struct-unequal-fields branch from 64e89cb to 6b72362 Compare November 19, 2021 10:27

olavloite reviewed Nov 19, 2021

View reviewed changes

feat(spanner): add ToStructLenient method to decode to struct fields …

92a0ed7

…with no error return with un-matched row's column with struct's exported fields.

rahul2393 force-pushed the decode-struct-unequal-fields branch from 6b72362 to 92a0ed7 Compare November 19, 2021 11:29

rahul2393 added 2 commits November 19, 2021 22:23

Merge branch 'main' into decode-struct-unequal-fields

f20e554

Merge branch 'main' into decode-struct-unequal-fields

3041555

olavloite approved these changes Nov 22, 2021

View reviewed changes

hengfengli reviewed Nov 22, 2021

View reviewed changes

hengfengli approved these changes Nov 22, 2021

View reviewed changes

hengfengli merged commit 899ffbf into googleapis:main Nov 22, 2021

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat(spanner): add ToStructLenient method to decode to struct fields with no error return in case of un-matched row's column with struct's exported fields #5153

feat(spanner): add ToStructLenient method to decode to struct fields with no error return in case of un-matched row's column with struct's exported fields #5153

rahul2393 commented Nov 17, 2021 •

edited

git-hulk commented Nov 18, 2021

olavloite left a comment

olavloite Nov 18, 2021

rahul2393 Nov 18, 2021

olavloite Nov 18, 2021

rahul2393 Nov 18, 2021

olavloite Nov 18, 2021

rahul2393 Nov 18, 2021

olavloite Nov 18, 2021

olavloite commented Nov 18, 2021

rahul2393 commented Nov 18, 2021 •

edited

olavloite commented Nov 18, 2021

rahul2393 commented Nov 19, 2021

olavloite Nov 19, 2021

olavloite Nov 19, 2021

rahul2393 Nov 19, 2021 •

edited

olavloite Nov 19, 2021

olavloite Nov 19, 2021

olavloite Nov 19, 2021

rahul2393 Nov 19, 2021

olavloite Nov 22, 2021

olavloite Nov 19, 2021

olavloite Nov 22, 2021

hengfengli Nov 22, 2021

-//
+//
+// 3. The number of columns in the row and exported fields in the struct do not need to match.
+// Any field in the struct that cannot not be assigned a value from the row is assigned its default value.
+// Any column in the row that does not have a corresponding field in the struct is ignored.

	if err = decodeStruct(ty, l, s.Interface(), false); err != nil {
	if err = decodeStruct(ty, l, s.Interface(), lenient); err != nil {

	// return error if linent is true and destination has duplicate exported columns
	// return error if lenient is true and destination has duplicate exported columns

feat(spanner): add ToStructLenient method to decode to struct fields with no error return in case of un-matched row's column with struct's exported fields #5153

feat(spanner): add ToStructLenient method to decode to struct fields with no error return in case of un-matched row's column with struct's exported fields #5153

Conversation

rahul2393 commented Nov 17, 2021 • edited

git-hulk commented Nov 18, 2021

olavloite left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

olavloite commented Nov 18, 2021

rahul2393 commented Nov 18, 2021 • edited

olavloite commented Nov 18, 2021

rahul2393 commented Nov 19, 2021

Choose a reason for hiding this comment

Choose a reason for hiding this comment

rahul2393 Nov 19, 2021 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

rahul2393 commented Nov 17, 2021 •

edited

rahul2393 commented Nov 18, 2021 •

edited

rahul2393 Nov 19, 2021 •

edited