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): implement valuer and scanner interfaces #4936

Merged
merged 7 commits into from Oct 19, 2021

Conversation

olavloite
Copy link
Contributor

@olavloite olavloite commented Oct 1, 2021

Valuer / Scanner Interfaces

Adds implementations for the driver.Valuer and sql.Scanner interfaces for the Spanner Null* types. This makes it possible to use both the spanner.Null* types and the underlying native types in the Go sql driver.

That is, both the following will then be supported:

var r spanner.NullNumeric
rows.Scan(&r)

AND

var r big.Rat
rows.Scan(&r)

It is not possible to implement this directly in the Go sql driver, as these types are defined in the spanner package.

The interfaces are not implemented for the spanner.NullJSON type for two reasons:

  1. NullJSON already has a field called Value, which makes it technically impossible to add a method called Value().
  2. The underlying value of NullJSON is of type interface{}, which means that it can be anything. This means that there is no relevant other type than NullJSON that a user can use when calling sql.Row#Scan(dest ...interface{}) for a JSON column.

Gorm Data Type

Adds default data type mappings for the spanner.Null* types. That is; NullInt64 is for example mapped by default to an INT64 column. This allows structs that use spanner.Null* types for its fields to be used directly in Gorm migrations without the need to annotate them with the data type they should have in the database.
This feature is implemented by adding the func GormDataType() string to each of the spanner.Null* types.

@google-cla google-cla bot added the cla: yes This human has signed the Contributor License Agreement. label Oct 1, 2021
@olavloite olavloite changed the title [WIP] feat: implement valuer and scanner interfaces feat: implement valuer and scanner interfaces Oct 12, 2021
@olavloite olavloite marked this pull request as ready for review October 12, 2021 13:32
@olavloite olavloite requested review from skuruppu and a team as code owners October 12, 2021 13:32
@@ -554,6 +815,11 @@ func (n *NullJSON) UnmarshalJSON(payload []byte) error {
return nil
}

// GormDataType is used by gorm to determine the default data type for fields with this type.
func (n NullJSON) GormDataType() string {
return "JSON"
Copy link
Contributor

Choose a reason for hiding this comment

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

How about JSON? Why does it not implement Value and Scan interfaces?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I ran into a problem with JSON, and that is that spanner.NullJson has an exported field called Value. Go does not allow a struct to have a method and a field with the same name, so we cannot add the Value() method to NullJson. At least not without making a breaking change by renaming the Value field to something else.

Copy link
Contributor

Choose a reason for hiding this comment

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

Can we add this problem to the PR description? or the comment in the code? Just for records that why we don't implement valuer and scanner interfaces for JSON.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Good point, I've added it both to the comments for NullJSON and to the PR description.

@olavloite olavloite changed the title feat: implement valuer and scanner interfaces feat(spanner): implement valuer and scanner interfaces Oct 18, 2021
Copy link
Contributor

@hengfengli hengfengli left a comment

Choose a reason for hiding this comment

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

LGTM.

@olavloite olavloite added the automerge Merge the pull request once unit tests and other checks pass. label Oct 19, 2021
@gcf-merge-on-green gcf-merge-on-green bot merged commit 4537b45 into master Oct 19, 2021
@gcf-merge-on-green gcf-merge-on-green bot deleted the spanner-valuer-scanner branch October 19, 2021 07:44
@gcf-merge-on-green gcf-merge-on-green bot removed the automerge Merge the pull request once unit tests and other checks pass. label Oct 19, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
cla: yes This human has signed the Contributor License Agreement.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants