go sqlgen: Add support for converting "Zero" value fields to NULL in the db #181

willhug · 2018-10-04T02:41:48Z

Summary: Adds support to sqlgen to convert "Zero" value model fields to NULL in
the database. This only needs to be applied when inserting a value to the
database because when we read from the db NULL values are noop (leaving the
zero value).

I put some cleanups in the PR as well, would be easy to pull them out if we want.

Additionally, I wrote tests for the "Main" use cases, but I'm very open
to add tests for other edge cases, let me know and I'll add them.

coveralls · 2018-10-04T02:44:12Z

Pull Request Test Coverage Report for Build 1135

46 of 53 (86.79%) changed or added relevant lines in 4 files are covered.
5 unchanged lines in 2 files lost coverage.
Overall coverage increased (+0.2%) to 65.761%

Changes Missing Coverage	Covered Lines	Changed/Added Lines	%
internal/fields/reflect.go	20	21	95.24%
sqlgen/reflect.go	2	4	50.0%
internal/fields/sql.go	4	8	50.0%

Files with Coverage Reduction	New Missed Lines	%
reactive/graph.go	1	94.07%
graphql/schemabuilder/reflect.go	4	71.81%

Totals
Change from base Build 1112:	0.2%
Covered Lines:	3509
Relevant Lines:	5336

💛 - Coveralls

berfarah · 2018-10-04T06:19:00Z

internal/fields/sql.go

@@ -115,8 +119,7 @@ var _ driver.Valuer = &Valuer{}
 // into the type dictated by our descriptor.
 type Scanner struct {
 	*Descriptor
-	value   reflect.Value
-	isValid bool


Was this not used anywhere? If so, hooray, goodbye allocation!

berfarah · 2018-10-04T06:19:12Z

internal/fields/sql.go

@@ -77,6 +77,10 @@ func (f Valuer) Value() (driver.Value, error) {
 			return iface.MarshalJSON()
 		}
 		return json.Marshal(i)
+	case f.Tags.Contains("zeroIsNull"):


nullable? Trying to think of a better name

As per offline convo: implicitnull?

changing to implicitNull

I would prefer all lowercase since that's how go's stdlib does it (omitempty).

berfarah · 2018-10-04T06:21:45Z

sqlgen/zeronull_test.go

@@ -0,0 +1,187 @@
+package sqlgen
+
+import (


This seems fine, though it'd be nice to have tests in the fields package given we test tons of other values there 🤷‍♂️. At this point it might be worth consolidating the fields package with sqlgen anyway.

Yeah, adding some tests here and seeing edge cases, will update PR

willhug · 2018-10-04T21:29:07Z

Adding some tests for filters to make sure they still work properly.

willhug · 2018-10-04T22:28:10Z

Updated! Also added a db test thing I've been using locally to get tests to run without an EnvVar every time.

berfarah · 2018-10-04T22:36:36Z

internal/testfixtures/db.go

@@ -28,6 +28,8 @@ var DefaultDBConfig = DBConfig{
 func init() {
 	if pw := os.Getenv("DB_PASSWORD"); pw != "" {
 		DefaultDBConfig.Password = pw
+	} else {


We can change the password to just always be dev and update the docker container on here?

Once this section is removed and the DefaultDBConfig's password reflects the docker container's password this PR is good to go! Make sure to update the CHANGELOG.md with the changes, too!

berfarah · 2018-10-04T22:37:14Z

sqlgen/integration_test.go

+		Name:         "Bob",
+		Uuid:         testfixtures.CustomType{'1', '1', '2', '3', '8', '4', '9', '1', '2', '9', '3'},
+		Mood:         &mood,
+		ImplicitNull: "test",


Hmm - do we want this in our benchmark?

stephen

i'm excited with this change.

stephen · 2018-10-04T23:33:27Z

sqlgen/implicitnull_test.go

+	}, rows)
+}
+
+func GetCount(db *sql.DB, countQuery string) (int, error) {


nit: don't export this?

ooc: can you do stuff with exported functions in a test?

Only from other test files I believe.

stephen · 2018-10-04T23:35:54Z

internal/fields/sql.go

@@ -77,6 +77,10 @@ func (f Valuer) Value() (driver.Value, error) {
 			return iface.MarshalJSON()
 		}
 		return json.Marshal(i)
+	case f.Tags.Contains("implicitnull"):
+		if IsZero(f.value) {


i'm excited by how small this change is that i missed it in my first read.

stephen · 2018-10-04T23:36:12Z

internal/fields/reflect.go

+// Mostly copied from https://github.com/golang/go/issues/7501
+// Works for mostly everything except zero initialized arrays (e.g.
+// `var ray [5]string` will not be "Zero")
+func IsZero(v reflect.Value) bool {


nit: should we make this private?

@stephen given that it's in an internal package, it's going to be private API one way or another - but I think that's a good point

stephen · 2018-10-04T23:37:24Z

sqlgen/implicitnull_test.go

+		NullByte:  []byte("hello there"),
+		NullTime:  timeField,
+	}
+	if _, err := db.InsertRow(context.Background(), newRow); err != nil {


could we add a test for UpdateRow as well?

stephen · 2018-10-04T23:39:42Z

internal/fields/sql.go

@@ -77,6 +77,10 @@ func (f Valuer) Value() (driver.Value, error) {
 			return iface.MarshalJSON()
 		}
 		return json.Marshal(i)
+	case f.Tags.Contains("implicitnull"):


nit: what is implicit about this null? i kind of like zeroisnull, but i could be convinced otherwise

No strong feelings either way, @berfarah @stephen whichever preference you guys want.

stephen · 2018-10-04T23:44:40Z

sqlgen/implicitnull_test.go

+			assert.Equal(t, []*ImplicitNull{
+				newRow,
+			}, rows)
+		})


could we also add a test for the reading that a null in the db reads out as the zero value?

stephen

from @berfarah:
let's verify that we have:

integration test for livesql (binlog)
integration test for sqlgen CRUD
integration test for batching in sqlgen

willhug · 2018-10-05T03:36:11Z

Not to self: binlog and Delete still need tests.

willhug · 2018-10-05T23:21:22Z

Updated! should be good to review now!

Summary: Changes the default test fixture to use "dev" as the password for databases.

Summary: Noticed isValid wasn't used outside of the Scan function, so removed it from the field and just initialized it in the Scan function.

Summary: Adds a function that can take a reflect.Value and return whether the provided value is the "Zero" value of that type. We'll use this to determine whether a provided type should be treated as "null" for the new "zeroIsNull" tag.

Summary: Adds a new tag `sql:",implicitnull"` we can use on thunder fields to mark that the "Zero" value of a field should be treated as NULL in the database.

willhug requested review from berfarah and stephen October 4, 2018 02:41

willhug force-pushed the wh/protoStuff/part-1.0 branch from 4be26e4 to 350b7bb Compare October 4, 2018 02:42

berfarah reviewed Oct 4, 2018

View reviewed changes

willhug force-pushed the wh/protoStuff/part-1.0 branch from 350b7bb to 62f12b4 Compare October 4, 2018 22:26

willhug force-pushed the wh/protoStuff/part-1.0 branch from 62f12b4 to e392a12 Compare October 4, 2018 22:32

berfarah reviewed Oct 4, 2018

View reviewed changes

stephen reviewed Oct 4, 2018

View reviewed changes

stephen reviewed Oct 5, 2018

View reviewed changes

willhug force-pushed the wh/protoStuff/part-1.0 branch from e392a12 to 7cbb86a Compare October 5, 2018 03:35

willhug force-pushed the wh/protoStuff/part-1.0 branch from 7cbb86a to 69f376d Compare October 5, 2018 23:19

berfarah approved these changes Oct 8, 2018

View reviewed changes

willhug force-pushed the wh/protoStuff/part-1.0 branch from 69f376d to ada61ed Compare October 9, 2018 19:49

Will Hughes added 6 commits October 9, 2018 12:50

go db: Change default database to use "dev" password (same as local)

1f9f0ef

Summary: Changes the default test fixture to use "dev" as the password for databases.

go fields/Scanner: Move "isValid" field to be function variable

e87864a

Summary: Noticed isValid wasn't used outside of the Scan function, so removed it from the field and just initialized it in the Scan function.

go fields/reflect: Add IsZero function

970ac98

Summary: Adds a function that can take a reflect.Value and return whether the provided value is the "Zero" value of that type. We'll use this to determine whether a provided type should be treated as "null" for the new "zeroIsNull" tag.

go implicitNull: Add support for converting zero value fields to NULL

51bbf7b

Summary: Adds a new tag `sql:",implicitnull"` we can use on thunder fields to mark that the "Zero" value of a field should be treated as NULL in the database.

go livesql: Add implicitnull binlog test

fff3d87

CHANGELOG: Add changelog entry for implicitnull

80cb3cc

willhug force-pushed the wh/protoStuff/part-1.0 branch from ada61ed to 80cb3cc Compare October 9, 2018 19:50

willhug merged commit 29e1b32 into master Oct 9, 2018

willhug deleted the wh/protoStuff/part-1.0 branch October 9, 2018 20:28

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

go sqlgen: Add support for converting "Zero" value fields to NULL in the db #181

go sqlgen: Add support for converting "Zero" value fields to NULL in the db #181

willhug commented Oct 4, 2018

coveralls commented Oct 4, 2018 •

edited

berfarah Oct 4, 2018

willhug Oct 4, 2018

berfarah Oct 4, 2018

berfarah Oct 4, 2018

willhug Oct 4, 2018

berfarah Oct 4, 2018

berfarah Oct 4, 2018

willhug Oct 4, 2018

willhug commented Oct 4, 2018

willhug commented Oct 4, 2018

berfarah Oct 4, 2018

willhug Oct 5, 2018 •

edited

berfarah Oct 8, 2018

berfarah Oct 4, 2018

stephen left a comment

stephen Oct 4, 2018

berfarah Oct 5, 2018

willhug Oct 5, 2018

willhug Oct 5, 2018

stephen Oct 4, 2018

stephen Oct 4, 2018

berfarah Oct 5, 2018

willhug Oct 5, 2018

stephen Oct 4, 2018

stephen Oct 4, 2018

willhug Oct 5, 2018

stephen Oct 4, 2018

willhug Oct 5, 2018

stephen left a comment

willhug commented Oct 5, 2018

willhug commented Oct 5, 2018

go sqlgen: Add support for converting "Zero" value fields to NULL in the db #181

go sqlgen: Add support for converting "Zero" value fields to NULL in the db #181

Conversation

willhug commented Oct 4, 2018

coveralls commented Oct 4, 2018 • edited

Pull Request Test Coverage Report for Build 1135

💛 - Coveralls

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

willhug commented Oct 4, 2018

willhug commented Oct 4, 2018

Choose a reason for hiding this comment

willhug Oct 5, 2018 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

stephen 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

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

stephen left a comment

Choose a reason for hiding this comment

willhug commented Oct 5, 2018

willhug commented Oct 5, 2018

coveralls commented Oct 4, 2018 •

edited

willhug Oct 5, 2018 •

edited