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

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

Merged
merged 3 commits into from Nov 22, 2021

Conversation

rahul2393
Copy link
Contributor

@rahul2393 rahul2393 commented Nov 17, 2021

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

@rahul2393 rahul2393 requested review from hengfengli, skuruppu and a team as code owners Nov 17, 2021
@google-cla google-cla bot added the cla: yes This human has signed the Contributor License Agreement. label Nov 17, 2021
@product-auto-label product-auto-label bot added the api: spanner Issues related to the Cloud Spanner API. label Nov 17, 2021
@rahul2393 rahul2393 added the kokoro:force-run Add this label to force Kokoro to re-run the tests. label Nov 17, 2021
@kokoro-team kokoro-team removed the kokoro:force-run Add this label to force Kokoro to re-run the tests. label Nov 17, 2021
@git-hulk
Copy link

git-hulk commented Nov 18, 2021

Cool, many thanks to @rahul2393

@rahul2393 rahul2393 force-pushed the decode-struct-unequal-fields branch 2 times, most recently from 9c05613 to 77d0c1a Compare Nov 18, 2021
@rahul2393 rahul2393 requested review from codyoss and olavloite Nov 18, 2021
@rahul2393 rahul2393 force-pushed the decode-struct-unequal-fields branch 3 times, most recently from 391696e to c615922 Compare Nov 18, 2021
Copy link
Contributor

@olavloite olavloite left a comment

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

spanner/value.go Outdated
@@ -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)
Copy link
Contributor

@olavloite olavloite Nov 18, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Contributor Author

@rahul2393 rahul2393 Nov 18, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Done

spanner/value.go Outdated
for _, f := range fieldNames {
sf := fields.Match(f)
if sf == nil {
return errDupGoField(ptr)
Copy link
Contributor

@olavloite olavloite Nov 18, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Contributor Author

@rahul2393 rahul2393 Nov 18, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

spanner/value.go Outdated
@@ -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
Copy link
Contributor

@olavloite olavloite Nov 18, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why do we need this?

Copy link
Contributor Author

@rahul2393 rahul2393 Nov 18, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Contributor

@olavloite olavloite Nov 18, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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
Copy link
Contributor

olavloite commented Nov 18, 2021

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
Copy link
Contributor Author

rahul2393 commented Nov 18, 2021

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

@rahul2393 rahul2393 force-pushed the decode-struct-unequal-fields branch 2 times, most recently from b2fadc7 to 64e89cb Compare Nov 18, 2021
@olavloite
Copy link
Contributor

olavloite commented Nov 18, 2021

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

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

  1. 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)?
  2. 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.
  3. 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.
  4. 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 rahul2393 force-pushed the decode-struct-unequal-fields branch from 64e89cb to 6b72362 Compare Nov 19, 2021
@rahul2393 rahul2393 changed the title fix(spanner): allow toStruct to decode value into struct with unequal number of fields compared to Spanner row fields feat(spanner): add ToStructLenient method to decode to struct fields with no error return with un-matched row's column with struct's exported fields Nov 19, 2021
@rahul2393 rahul2393 changed the title feat(spanner): add ToStructLenient method to decode to struct fields with no error return with un-matched row's column with struct's exported fields 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 Nov 19, 2021
@rahul2393
Copy link
Contributor Author

rahul2393 commented Nov 19, 2021

@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.

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

@olavloite olavloite Nov 19, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

spanner/row.go Outdated
// 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
Copy link
Contributor

@olavloite olavloite Nov 19, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Contributor Author

@rahul2393 rahul2393 Nov 19, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

// 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 {
Copy link
Contributor

@olavloite olavloite Nov 19, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

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

@olavloite olavloite Nov 19, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

@@ -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 {
Copy link
Contributor

@olavloite olavloite Nov 19, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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 {

Copy link
Contributor Author

@rahul2393 rahul2393 Nov 19, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Contributor

@olavloite olavloite Nov 22, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ah, yeah, good point. I missed that.

spanner/value.go Outdated
@@ -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
Copy link
Contributor

@olavloite olavloite Nov 19, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.
@rahul2393 rahul2393 force-pushed the decode-struct-unequal-fields branch from 6b72362 to 92a0ed7 Compare Nov 19, 2021
@@ -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 {
Copy link
Contributor

@olavloite olavloite Nov 22, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ah, yeah, good point. I missed that.

@@ -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
Copy link
Contributor

@hengfengli hengfengli Nov 22, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

nit: return -> Return

@hengfengli hengfengli merged commit 899ffbf into googleapis:main Nov 22, 2021
4 checks passed
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. cla: yes This human has signed the Contributor License Agreement.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

spanner: ToStruct can ignore the missing fields instead of throwing error
5 participants