Google-cloud-go: Spanner: `NullString` returning quoted strings

[arminm]

The NullString's String() function is returning quoted strings instead of just the string value. The implementation is quite strange where it casts strings into a %q format instead of just a %s, or not using fmt.Sprintf at all!

NullString.StringVal is already a string type:

// NullString represents a Cloud Spanner STRING that may be NULL.
type NullString struct {
  StringVal string
  Valid     bool // Valid is true if StringVal is not NULL.
}

but its String() function is using a quoted format to return the value:

// String implements Stringer.String for NullString
func (n NullString) String() string {
  if !n.Valid {
    return fmt.Sprintf("%v", "<null>")
  }
  return fmt.Sprintf("%q", n.StringVal)
}

NullString reference: https://github.com/googleapis/google-cloud-go/blob/master/spanner/value.go#L67-L73

There's also NullTime that is also returning a quoted string when it doesn't need to format the response:
NullTime: https://github.com/googleapis/google-cloud-go/blob/master/spanner/value.go#L109-L115

and NullDate where probably %s should be used.

NullDate: https://github.com/googleapis/google-cloud-go/blob/master/spanner/value.go#L123-L129

[olavloite]

@arminm Thanks for your detailed report.

You're observation is correct, but I don't think this is something we would want to change. The String() method should return a descriptive string of the value, but is not guaranteed to return the raw string value if the underlying value _could_ be a string. The decision to return a quoted string for NullString, NullTime and NullDate was apparently made in the initial version the client library, and others might be relying on that.

Could you elaborate a little bit on why you would want this changed and/or what other means might also mitigate any (potential) problems that you are running into with this?

[arminm]

String() is supposed to return a string representation of the value, which is a simple string, but it's adding new characters to it. That's quite counter-intuitive, and as it caught me by surprise in a weird bug I had to spend hours to find, it can affect others. I expected the field to just work when it's not null, so i can pass it to functions that call String() on an object like fmt.Sprintf or loggers.

I also can't pass a variable with NullString type to an interface that accepts Stringer.String and have to wrap it in another struct to return the StringVal instead now.

It's just quite a strange decision made by Google here, to assume that the String() function is only used for logging and it's ok to add quotes... Spanner's lack of support for nullable fields has been quite annoying for me in migrations (i've been using Spanner since January in production), and i was excited to see there's a type to handle nulls, but now it has this annoying issue.

I personally think a better solution to handle nullable fields is to fix the ToStruct method to be able to handle nulls instead of returning errors. The struct descriptions could have another option as well like:

type Model struct {
  ID       string  `spanner:"id"`
  Name string  `spanner:"name,nullable"`
}

and just handle that to return "" when Name is null, or just null if it's a pointer.

[olavloite]

@arminm After an internal discussion we are inclined to agree with you that the current implementation of String() for NullString might be worth changing, even if it will change the current behavior that others might be relying on. Adding characters to the underlying string really isn't very intuitive. I also like your suggestion for letting ToStruct handle null values better.

So we'll look into two separate changes:

1. Changing the current behavior of the String() methods of NullString, NullTime and NullDate to return a non-quoted string.
2. Let the ToStruct method accept null values for fields that are explicitly marked as nullable.

[arminm]

Thank you for being so receptive of my feedback. I'll keep an eye open for the changes in the upcoming versions. 🙏

[skuruppu]

@arminm just wanted to check whether you think the work in #1735 helped you or do you think there's more that we could do here?

[larkee]

Given that the work seems to have been done, I am closing this issue. If you feel that there is more that needs to be done, please feel free to reopen :+1:

[arminm]

@skuruppu it definitely helped! I quickly updated to the version that added the native support for JSON, and refactored my code to use that instead of my custom translations. 🙏