[Major Change] Introduce `types` package for type handling in sqlgen #142

berfarah · 2018-08-14T19:59:11Z

Goals

Support protobuf struct (de)serialization
Support custom scalars. For real. Like, types that aren't natively supported by SQL.
Keep supporting scalar aliases (string -> custom string etc).

Changes

This PR adds a types package with the intention of replacing Scannables / TypeConverters. This unifies all of our logic in one place for determining how values are converted, while also supporting native interfaces.

Implementation

Replace scannables with fields. Ensure that Empty/Interface is being called when scanning values.
Add integration tests for both native and custom types.
Deprecate MustRegisterCustomScalar and MustRegisterSimpleScalar.
???
Profit

Usage

type Model struct {
  Foo ProtoModel `sql:",binary"` // (de)serializes to a binary blob via `Marshal`/`MarshalBinary`
  IP IP `sql:",primary,string"` // (de)serializes into a string via `MarshalText`
  UUID UUID // serializes via `sql/driver.Valuer` and deserializes via `sql.Scanner`
  Bar CustomString // (de)serializes the same way a native string would
  // int8, int16, int32 are converted to int64
  // uint8, uint16, uint32, uint64 are converted to int64
  // float32 is converted to float64
  // bool, float64, int64, string, []byte, time.Time and nil are natively supported
}

(but really, after this is implemented for SQL, look at where we could use it on the GraphQL side as well)

stephen

overall comments:

feel like there are package/high level comments needed in the early commits.
think the write path is not there yet.

stephen · 2018-08-14T23:18:42Z

types/field.go

+// Blank creates a blank copy of a field. This _must_ be used in order to create a
+// sql.Scanner from a field.
+// Example:
+// f := FieldFromValue(map[string]string{"sql": "binary", i)


nit: add indent so that this shows up as a code block

also, i think this example code is out of date

This won't be needed any more

stephen · 2018-08-14T23:20:36Z

types/sql.go

+		if iface, ok := i.(encoding.BinaryMarshaler); ok {
+			return iface.MarshalBinary()
+		}
+	case "string", "text":


is there a reason to support both tags? can we support only one?

The idea here is that you might have varchar or a text blob - though I'd be fine with changing it to just string

stephen · 2018-08-14T23:20:56Z

types/sql.go

+	}
+
+	// Handle sql type overrides as special cases.
+	switch f.TagType["sql"] {


is it safe to reuse the sql struct field tag in this way?

I've changed this

stephen · 2018-08-14T23:21:33Z

types/sql.go

+	// Handle native types.
+	switch i.(type) {
+	case *[]byte:
+		fmt.Println("bytes")


nit: remove

stephen · 2018-08-15T21:18:21Z

types/field.go

+	"reflect"
+)
+
+type TagSet map[string]struct{}


can you add a comment for this

think all of the exported symbols in this file could use an explanation, and a package-level comment explaining how this is going to be used would be helpful. I have trouble understanding the higher level intent of this code by looking at this commit.

stephen · 2018-08-15T21:43:41Z

sqlgen/reflect.go

 	ptr := reflect.New(table.Type)
 	elem := ptr.Elem()

 	for i, column := range table.Columns {
-		value, _ := scannables[i].(driver.Valuer).Value()
+		// These values are all non-pointers and have been copied.


what does this imply?

Added a comment here

stephen · 2018-08-15T21:44:28Z

types/field.go

@@ -68,5 +68,21 @@ func (f *Field) Blank() *Field {
 	return field
 }

+// CopyTo copies the reference onto the given value. Note: this uses pointers, so maybe this is a
+// bad name?
+func (f *Field) CopyTo(val reflect.Value) {


maybe SetIn or SetValue would help?

stephen · 2018-08-15T21:46:08Z

sqlgen/reflect.go

 		return nil, err
 	}

-	return BuildStruct(table, scannables), nil
+	// Coerce fields into their actual type.


almost think we should remove this comment. at first i thought it meant something more than "satisfy scanner.Scan" was happening

stephen · 2018-08-15T22:10:34Z

testfixtures/custom_type.go

@@ -5,25 +5,28 @@ import (
 	"fmt"
 )

-type CustomType []byte
+type CustomType [16]byte


could you explain this fixture change?

Yep: I wanted this to no longer be a natively supported SQL type

stephen · 2018-08-15T22:16:54Z

types/field.go

+	IsNil bool
+}
+
+func FieldFromValue(in interface{}, tags []string) *Field {


ok, having read the whole PR now, I think I'd like to see this in _test.go only - it doesn't seem like there is a reason to use this outside of test.

I've removed this entirely

berfarah · 2018-08-16T23:30:22Z

Discounting tests, comments and test data, this only adds ~200 LOC. 😬

coveralls · 2018-08-17T01:41:18Z

Pull Request Test Coverage Report for Build 922

207 of 279 (74.19%) changed or added relevant lines in 6 files are covered.
2 unchanged lines in 1 file lost coverage.
Overall coverage increased (+0.4%) to 64.289%

Changes Missing Coverage	Covered Lines	Changed/Added Lines	%
sqlgen/reflect.go	38	39	97.44%
fields/descriptor.go	27	41	65.85%
sqlgen/deprecated.go	0	25	0.0%
fields/sql.go	127	159	79.87%

Files with Coverage Reduction	New Missed Lines	%
sqlgen/reflect.go	2	78.49%

Totals
Change from base Build 918:	0.4%
Covered Lines:	3244
Relevant Lines:	5046

💛 - Coveralls

stephen · 2018-08-17T18:39:09Z

fields/sql.go

+		if iface, ok := i.(encoding.BinaryMarshaler); ok {
+			return iface.MarshalBinary()
+		}
+	case f.Tags.Contains("string"), f.Tags.Contains("text"):


nit: think one of your comment responses mentions removing text. i think having only one here would be nice.

stephen · 2018-08-20T18:25:33Z

fields/sql.go

+func (f Valuer) Value() (driver.Value, error) {
+	// Ideally, all the interfaces below would be implemented as normal methods, not indirect
+	// methods. However, in order to be safe we take the address of pointers where we can.
+	// This specifically supports protobuf.


is this comment referring to the next line? i am having trouble understanding what this implies for my read here

I should move this into the descriptor.Valuer method

stephen · 2018-08-20T18:28:28Z

fields/sql.go

+	// }
+	switch {
+	case f.Tags.Contains("binary"):
+		if iface, ok := i.(marshaler); ok {


could you help me understand the difference between marshaler and binary marshaler? why would one implement one vs. the other?

I've attempted to clarify this by the interface. This specifically supports protobuf and other libraries that implement Marshal() (I know I've seen it before but I can't remember one off the top of my head)

stephen · 2018-08-20T18:29:14Z

fields/sql.go

+	case f.Tags.Contains("json"):
+		if b, ok := i.([]byte); ok {
+			return b, nil
+		}


why would one use the json tag if the value is a string? i.e. there is nothing for the marshaler to do

I can remove this

stephen · 2018-08-20T18:30:27Z

fields/interface.go

@@ -0,0 +1,9 @@
+package fields
+
+type marshaler interface {


could you point to some places in the standard library where this is used but not exported?

There are similar but not exactly the same patterns in the stdlib. We looked at gogo/protobuf#28 together. I'm still not clear on why it wasn't implemented - the issue was fairly confusing and from over 3 years ago.

github.com/golang/protobuf/proto.Marshaler and github.com/golang/protobuf/proto.Unmarshaler?

stephen · 2018-08-20T18:36:57Z

fields/sql.go

+			s.value.Set(reflect.ValueOf(src))
+			return nil
+		}
+	}


could you comment on why these need to be handled in this codepath, but not in the valuer path?

Since these are native types, we don't need to do any special handling. Because scanner returns an error for unhandled types, we need to explicitly handle them (as opposed to how valuer works where SQL will pick up our slack)

This should be handled before Scanner

Thanks, I think that helps.

stephen · 2018-08-20T18:44:17Z

example/main.go

@@ -6,6 +6,7 @@ import (
 	"net/http"
 	"sort"

+	_ "github.com/go-sql-driver/mysql"


what is this for?

there is a lot going on in this last commit. it'd be great to split it up into a few different changes - maybe by valuer/scanner/livesql.

This is in order to "install" the mysql driver.

berfarah · 2018-08-20T20:28:30Z

@stephen Hopefully these commits are a little cleaner

stephen

Think one other thing that would have helped my understanding here is a high level comment about the API surface area, i.e. this work does help in cases where:

we control the package and can add additional interfaces to a type
the package itself implements standard text/binary marshalling interfaces

it doesn't help with things like:

types outside of our controlled packages that don't support marshalling interfaces, e.g. arbitrary conversion for time.Time

stephen · 2018-08-20T20:41:06Z

sqlgen/reflect.go

@@ -319,7 +332,7 @@ func makeWhere(table *Table, filter Filter) (*SimpleWhere, error) {
 			return nil, fmt.Errorf("unknown column %s", name)
 		}

-		l = append(l, whereElem{column: column, value: value})
+		l = append(l, whereElem{column: column, value: column.Descriptor.Valuer(reflect.ValueOf(value))})


to verify - this is a departure from the existing behavior because the filters values are now converted vs. used directly.

stephen · 2018-08-20T20:46:10Z

sqlgen/reflect_test.go

-	}) {
-		t.Error("bad select")
-	}
+	assert.NoError(t, err)


did the tests from here down in this file change due to api changes, or as cleanup? if the latter, it'd be nice to have them separate/noted that way for reviewing.

😬 it was admittedly cleanup. I tried to extract these all into a separate PR but some of them got away from me

stephen · 2018-08-20T20:49:07Z

fields/sql.go

+		if iface, ok := i.(encoding.TextMarshaler); ok {
+			return iface.MarshalText()
+		}
+	case f.Tags.Contains("json"):


mentioned irl, but i think it would have been nice to see string/json support as a follow up vs. in this initial implementation.

stephen · 2018-08-20T20:51:42Z

fields/sql.go

+type Scanner struct {
+	*Descriptor
+	value   reflect.Value
+	IsValid bool


does IsValid need to be persisted on the struct? I see that it is only referenced within Scan

Yep, since we treat the value as a non-pointer value, IsValid tells us whether or not something is nil

could you point to where that is used outside of Scan?

Good point - this should be internal API

(it's used specifically in CopyTo)

ah, thanks i missed that. making it package private sounds good

stephen · 2018-08-20T20:52:54Z

fields/sql.go

+			s.value.Set(reflect.ValueOf(src))
+			return nil
+		}
+	}


Thanks, I think that helps.

stephen · 2018-08-20T20:55:33Z

sqlgen/reflect.go

-		}
-		if !ok {
-			return nil, fmt.Errorf("bad type %s: field %s has unsupported type %s", typ, field.Name, field.Type)
+		d := fields.New(field.Type, tags[1:])


is it safe for us to reuse the sql tag in this manner? should we use a different tag?

The sql tag is explicitly being used by only us in sqlgen. I think we should just use sql everywhere.

ah, yep, that makes sense. i had it in my head that other software used that tag too

Other ORMs do AFAIK, but nothing in the go stdlib (and the driver doesn't either)

berfarah · 2018-08-21T22:45:57Z

Includes #150

Allocations went up:

goos: darwin
goarch: amd64
pkg: github.com/samsarahq/thunder/sqlgen
Benchmark/Read-8         	    5000	    291292 ns/op	    2239 B/op	      60 allocs/op
Benchmark/Create-8       	    2000	    683282 ns/op	    1568 B/op	      39 allocs/op
Benchmark/Update-8       	    2000	    576854 ns/op	    1976 B/op	      49 allocs/op
Benchmark/Delete-8       	    2000	    643999 ns/op	    1000 B/op	      28 allocs/op

changpingc · 2018-08-22T22:34:13Z

fields/interface.go

@@ -0,0 +1,9 @@
+package fields
+
+type marshaler interface {


github.com/golang/protobuf/proto.Marshaler and github.com/golang/protobuf/proto.Unmarshaler?

changpingc · 2018-08-22T22:38:16Z

fields/sql.go

+//    bool
+//    []byte
+//    string
+//    time.Time


@berfarah time.Time is annoying here because mysql timestamp stores 6 digits after decimal point (microseconds), but time.Time stores nanoseconds. This means I can't write a time.Now() and expect to read back the same value.

That's where we can use Scan/Value to convert stuff more effectively

changpingc · 2018-08-22T22:41:49Z

fields/sql.go

+//    time.Time
+//    nil - for NULL values
+func (f Valuer) Value() (driver.Value, error) {
+	i := f.value.Interface()


should we check f.value.IsValid() first?
reflect.ValueOf(interface{}(nil)).Interface() panics, although I don't see if we can even define a sqlgen model struct with interface{} type field

changpingc · 2018-08-22T22:43:43Z

fields/sql.go

+	// If our interface supports driver.Valuer we can immediately short-circuit as this is what the
+	// MySQL driver would do.
+	if valuer, ok := i.(driver.Valuer); ok {
+		return valuer.Value()


should we have Valuer handle nil case itself?

For valuer, the nil case will panic (I checked). In scanner we can and do handle it.

changpingc · 2018-08-22T22:53:15Z

fields/sql.go

+}
+
+var _ driver.Valuer = Valuer{}
+var _ driver.Valuer = &Valuer{}


do we need this?

Nope, but it's nice to know that we are fulfilling the interface and the Go stdlib introduces this pattern. It only runs once so allocations are constant time (and presumably garbage collected)

changpingc · 2018-08-22T22:57:16Z

fields/sql.go

+//    time.Time
+//    nil - for NULL values
+func (s *Scanner) Scan(src interface{}) error {
+	// Get a value of the pointer of the pointer of our type. The Scanner and Unmarshalers should


Can you explain why this is returning pointer to pointer? For a field like type User struct { Name string }, I think it's type.Kind() is reflect.String?

I think this comment is actually just wrong. of the pointer should be there once. Will update.

changpingc · 2018-08-22T22:59:12Z

fields/sql.go

+type Scanner struct {
+	*Descriptor
+	value   reflect.Value
+	isValid bool


do we need this? Can we make it never write valid reflect.Value if not isValid?

isValid is more following the style of NullBytes/NullBool/etc where it means "is not nil"

The fields package abstracts away dealing with and coercing types for struct fields. A descriptor is responsible for keeping track of all type information associated with a struct field. It automatically dereferences the value and remembers that we're dealing with pointers in order to reduce the permutations of value assignment in dealing with the reflected values. Now rather than having to check both values if they are pointers when assigning (2*2 permutations, 4 code branches) you can simply know that this side is always dereferenced and only assign pointers when

It seems like this is used by a lot of packages but not explicitly referenced in the standard library. Adding it to this package as a native interface for handling it.

Fields can be converted to SQL by creating a Valuer from their descriptor.

SQL values can be serialized into fields by creating a Scanner from their corresponding field's descriptor.

This is a registration-time validation that the type is in fact a valid SQL type.

Ensure we fully support Protobuf by adding a test for it.

This is for more pleasant debugging/diff output

MySQL needs to be imported in order for the driver to be registered (AFAIK it's registered via `init()`)

Gives us better diffing

berfarah · 2018-08-23T16:51:24Z

@stephen since this is a pretty big breaking change, I'm thinking we should tag this with a 1.0 release. What do you think?

changpingc · 2018-08-23T23:46:51Z

fields/sql_test.go

+		{Out: float32(5), In: float64(5)},
+		{Out: likeFloat(5), In: float64(5)},
+		// Interfaces without tags:
+		{Out: ifaceScanner{[]byte("scan me")}, In: []byte("scan me")},


pointer ifaceScanner test case?

berfarah requested a review from stephen August 14, 2018 19:59

berfarah force-pushed the bf/types-package branch from be49296 to 4d25354 Compare August 15, 2018 21:07

berfarah changed the title ~~[proposal]: add types package for handling types~~ [Major Change] Introduce types package for type handling in sqlgen Aug 15, 2018

berfarah force-pushed the bf/types-package branch from 4d25354 to 7d8e175 Compare August 15, 2018 21:12

stephen reviewed Aug 15, 2018

View reviewed changes

berfarah force-pushed the bf/types-package branch from 864ff0d to 1193918 Compare August 16, 2018 23:22

berfarah force-pushed the bf/types-package branch 5 times, most recently from 49c2702 to 7c32ad3 Compare August 17, 2018 01:38

stephen reviewed Aug 20, 2018

View reviewed changes

berfarah force-pushed the bf/types-package branch 4 times, most recently from 996d78c to f73398c Compare August 20, 2018 20:28

stephen reviewed Aug 20, 2018

View reviewed changes

berfarah force-pushed the bf/types-package branch 7 times, most recently from 77cfe65 to 9ce377a Compare August 21, 2018 22:42

berfarah requested review from changpingc and willhug August 21, 2018 23:01

stephen approved these changes Aug 22, 2018

View reviewed changes

changpingc reviewed Aug 22, 2018

View reviewed changes

berfarah force-pushed the bf/types-package branch 2 times, most recently from 1a21b03 to e4aaee1 Compare August 23, 2018 16:34

berfarah added 14 commits August 23, 2018 09:34

go testfixtures: test non-native SQL type in sqlgen/livesql

47a33a0

go fields: add Marshal interface

9624e88

It seems like this is used by a lot of packages but not explicitly referenced in the standard library. Adding it to this package as a native interface for handling it.

go fields: Valuer satisfies sql/driver.Valuer interface

e539bf2

Fields can be converted to SQL by creating a Valuer from their descriptor.

go fields: Scanner satisfies sql.Scanner interface

ccebbe9

SQL values can be serialized into fields by creating a Scanner from their corresponding field's descriptor.

go fields: Add validation of SQL types

b0b8295

This is a registration-time validation that the type is in fact a valid SQL type.

go internal: add proto example for testing

d28de94

go fields: add test specifically for protobuf

b6c72ac

Ensure we fully support Protobuf by adding a test for it.

go testfixtures: add stringer to CustomType

3ea4e02

This is for more pleasant debugging/diff output

go sqlgen: implement read path

09653ad

sqlgen: add filter and write paths

c82abd4

go livesql: use new type system

63aa30e

go {livesql,sqlgen,example}: import mysql

ed07816

MySQL needs to be imported in order for the driver to be registered (AFAIK it's registered via `init()`)

go sqlgen: Use assert.Equal over pretty.Compare

5f08128

Gives us better diffing

berfarah force-pushed the bf/types-package branch from e4aaee1 to 5f08128 Compare August 23, 2018 16:34

berfarah mentioned this pull request Aug 23, 2018

sqlgen: support custom types #136

Closed

berfarah merged commit 4d17995 into master Aug 23, 2018

berfarah deleted the bf/types-package branch August 23, 2018 20:56

berfarah mentioned this pull request Aug 23, 2018

sqlgen: introduce TypeConverter, support proto fields #99

Closed

changpingc reviewed Aug 23, 2018

View reviewed changes

changpingc mentioned this pull request Sep 12, 2018

revert sqlgen changes #167

Merged

This pull request was closed.

[Major Change] Introduce types package for type handling in sqlgen #142

[Major Change] Introduce types package for type handling in sqlgen #142

Conversation

berfarah commented Aug 14, 2018

Goals

Changes

Implementation

Usage

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

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

berfarah commented Aug 16, 2018

coveralls commented Aug 17, 2018 • edited Loading

Pull Request Test Coverage Report for Build 922

💛 - 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

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

berfarah commented Aug 20, 2018

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

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

berfarah commented Aug 21, 2018

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

[Major Change] Introduce `types` package for type handling in sqlgen #142

[Major Change] Introduce `types` package for type handling in sqlgen #142

coveralls commented Aug 17, 2018 •

edited

Loading