Support embedded structs

[sqs]

Adds support for working with embedded structs.

For example, consider the WithEmbeddedStruct type (which is part of the associated test):

type WithEmbeddedStruct struct {
    Id int64
    Names
 }

 type Names struct {
    FirstName string
    LastName  string
 }

It maps to a DB table with id, firstname, and lastname columns. (The embedded struct is flattened, as is the default behavior in most Go encoders (incl. encoding/json) and gorp). Structs of type *WithEmbeddedStruct can be Inserted or Selected into, and the embedded struct fields will be written to.

The tests pass on MySQL, PostgreSQL, and SQLite3. (Incidentally, TestWithTime fails for me on master and with this PR; I think that's an sqlx issue.)

[jmoiron]

None of the tests fail on master for me. Can you paste me the test output?

• are we skipping unexported fields? (f.PkgPath != "")
• what about structs which implement Scanner?
• edit: what about name shadowing? this is DFS and Go's access rules are BFS

sqlx has some weird behavior wrt structs in structs: it treats embedded and non-embedded structs the same, and it skips over any structs implementing Scanner or Valuer (since it does writes and reads, via Select and its named param support). I don't know why it was done this way originally, and seeing your code now makes me want to change it.

I somewhat hastily accepted a patch for embedded structs on sqlx in the past, so I want to accept this but I want to be ab it more diligent about it first.

[sqs]

Thanks for reviewing this. It doesn't address any of those 3 bullet points you raised; good catches. I'll revise the patch.

---

BTW, the master test failure (which, to be clear, seems unrelated to this) is:

$ ./test-all  -test.v -test.run=TestWithTime
Testing MySQL
=== RUN TestWithTime-8
--- FAIL: TestWithTime-8 (0.06 seconds)
    modl_test.go:844: {1 2013-08-09 21:30:43 +0000 UTC} != {1 0001-01-01 00:00:00 +0000 UTC}
FAIL
exit status 1
FAIL    github.com/sqs/modl 0.066s

The correct value is written to the MySQL table, but it fails to be scanned into the time value correctly. My go version is go version devel +824f981dd4b7 Tue Apr 29 21:41:54 2014 -0400 linux/amd64 and my MySQL version is 5.5.37-0ubuntu0.13.10.1 (Ubuntu).

[jmoiron]

Ah, it's failing for MySQL only.

I believe this is related to not having the ?parseTime=true flag on your MySQL DSN. I should put this in the README. The behavior of time.Time is tricky to get right across databases, because lib/pq supports it natively.

[jmoiron]

It should be possible to support this a little more easily now that github.com/jmoiron/sqlx/reflectx exists. Part of the design of that was to allow reuse here. I'll look into this sometime later this week when I have time.

[cryptix]

hi, is there any progress on this?

[cryptix]

I just noticed the following: If your autoIncr Id isn't the first Field in the struct, you get the following panic: reflect: Field index out of range

You can trigger it really simple by changing the test:

diff --git a/modl_test.go b/modl_test.go
index 52e5694..e670815 100644
--- a/modl_test.go
+++ b/modl_test.go
@@ -66,8 +66,8 @@ type WithStringPk struct {
 type CustomStringType string

 type WithEmbeddedStruct struct {
-       Id int64
        Names
+       Id int64
 }

[cryptix@planc ~GOPATH/src/github.com/jmoiron/modl:embedded-structs*] ./test-all
Skipping MySQL, $MODL_MYSQL_DSN=
Skipping PostgreSQL, $MODL_POSTGRES_DSN=
Testing SQLite
--- FAIL: TestWithEmbeddedStruct (0.00 seconds)
panic: reflect: Field index out of range [recovered]
    panic: reflect: Field index out of range

goroutine 53 [running]:
runtime.panic(0x6de920, 0xc2080c4b00)
    /mnt/fast/go/src/pkg/runtime/panic.c:279 +0xf5
testing.func·006()
    /mnt/fast/go/src/pkg/testing/testing.go:416 +0x176
runtime.panic(0x6de920, 0xc2080c4b00)
    /mnt/fast/go/src/pkg/runtime/panic.c:248 +0x18d
reflect.Value.Field(0x75b6c0, 0xc2080c1230, 0x0, 0x196, 0x2, 0x0, 0x0, 0x0, 0x0)
    /mnt/fast/go/src/pkg/reflect/value.go:870 +0x25f
github.com/jmoiron/modl.insert(0xc2080198b0, 0x7fe40d9a2a50, 0xc2080198b0, 0x7fe40bef1ee8, 0x1, 0x1, 0x0, 0x0)
    /home/cryptix/go/src/github.com/jmoiron/modl/modl.go:335 +0x3ff
github.com/jmoiron/modl.(*DbMap).Insert(0xc2080198b0, 0x7fe40bef1ee8, 0x1, 0x1, 0x0, 0x0)
    /home/cryptix/go/src/github.com/jmoiron/modl/dbmap.go:270 +0x90
github.com/jmoiron/modl.TestWithEmbeddedStruct(0xc2080b1950)
    /home/cryptix/go/src/github.com/jmoiron/modl/modl_test.go:602 +0x2e0
testing.tRunner(0xc2080b1950, 0xb5be80)
    /mnt/fast/go/src/pkg/testing/testing.go:422 +0x8b
created by testing.RunTests
    /mnt/fast/go/src/pkg/testing/testing.go:504 +0x8db

goroutine 16 [chan receive]:
testing.RunTests(0x844908, 0xb5bd00, 0x16, 0x16, 0x1)
    /mnt/fast/go/src/pkg/testing/testing.go:505 +0x923
testing.Main(0x844908, 0xb5bd00, 0x16, 0x16, 0xb566a0, 0x2, 0x2, 0xb76480, 0x0, 0x0)
    /mnt/fast/go/src/pkg/testing/testing.go:435 +0x84
main.main()
    github.com/jmoiron/modl/_test/_testmain.go:93 +0x9c

goroutine 19 [finalizer wait]:
runtime.park(0x48e9b0, 0xb71828, 0xb5e9c9)
    /mnt/fast/go/src/pkg/runtime/proc.c:1369 +0x89
runtime.parkunlock(0xb71828, 0xb5e9c9)
    /mnt/fast/go/src/pkg/runtime/proc.c:1385 +0x3b
runfinq()
    /mnt/fast/go/src/pkg/runtime/mgc0.c:2644 +0xcf
runtime.goexit()
    /mnt/fast/go/src/pkg/runtime/proc.c:1445

goroutine 31 [chan receive]:
database/sql.(*DB).connectionOpener(0xc208044800)
    /mnt/fast/go/src/pkg/database/sql/sql.go:583 +0x48
created by database/sql.Open
    /mnt/fast/go/src/pkg/database/sql/sql.go:442 +0x27c

goroutine 17 [syscall]:
runtime.goexit()
    /mnt/fast/go/src/pkg/runtime/proc.c:1445

goroutine 54 [runnable]:
database/sql.(*DB).connectionOpener(0xc208045f80)
    /mnt/fast/go/src/pkg/database/sql/sql.go:582
created by database/sql.Open
    /mnt/fast/go/src/pkg/database/sql/sql.go:442 +0x27c
exit status 2
FAIL    github.com/jmoiron/modl 0.013s

[sqs]

OK, I've rebased the patch against current master and addressed @cryptix's issue.

Responses below to the points @jmoiron raised. Also, @jmoiron, let me know if you think the reflection code could be replaced by reflectx.

are we skipping unexported fields? (f.PkgPath != "")

No. It seems useful and consistent with expectations to be able to have:

type myEmbeddedStruct struct { B string }
type myTableMappedStruct struct {
  ID int
  myEmbeddedStruct
}

where B is mapped. If we only considered exported embedded structs, then modl's behavior would differ from that of Go stdlib packages (encoding/json, text/template, etc.).

what about structs which implement Scanner?

encoding/json and Go convention would imply that we should map an embedded struct implementing Scanner to a single column (whereas if it did not implement Scanner it would map to one column per field). This sounds right to me. It definitely would deserve a mention in the final docs for struct embedding.

what about name shadowing? this is DFS and Go's access rules are BFS

Is there a way to use reflectx to make proper name shadowing easier to implement? I could implement it here and write tests for it, but if there is an easy way to use reflectx to do it, or if you plan to add such functionality to reflectx, I don't want to duplicate it here.

[jmoiron]

Reflectx has had some time in production now that it is in charge of doing field maps for sqlx. I believe it skips unexported embedded fields, which looks like it may be a mistake. I think this should be easily changeable, as changing this couldn't interfere with working code.

It does however properly handle the tree search and it implements the fastest possible attribute access code (recursively fetching fields by index rather than doing one fetch by name is much faster).

I suspect that reflectx will make this simpler, but that I might have to tweak a thing or two with it. It would be a good second system to run with it on.

[tonyhb]

+1. @jmoiron happy to help wherever we need it. would you like me to take a look at sqlx and implement new reflect logic to recursively map structs?