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

Table mapped structs do not support embedded structs #13

Open
silasdavis opened this issue Mar 7, 2014 · 15 comments
Open

Table mapped structs do not support embedded structs #13

silasdavis opened this issue Mar 7, 2014 · 15 comments

Comments

@silasdavis
Copy link
Contributor

If I attempt to map:

type CreatedUpdated struct {
    Created sql.NullInt64 //time.Time
    Updated sql.NullInt64 //time.Time
}

type ExternalSource struct {
    Uuid string
    Name string
    CreatedUpdated
}

With:

dbmap.AddTableWithName(ExternalSource{}, "external_sources")

It will fail with sql: converting Exec argument #2's type: unsupported type models.CreatedUpdated, a struct

This is because the embedded struct walking logic in dbmap.AddTable was removed with the intention to let sqlx handle the scanning.

Somehow we need to extract the logic in sqlx.StructScan and use it here, or if it does not generalise well add back in support for struct scanning in modl.

It also appears much of the bindPlan functionality from gorp has been abandoned, it would be nice to use prepared statements in the bind plan and also use the table mapping to remove as much reflection as possible when running a query. Since we have mapped the table up front I feel we should be able to use something leaner than sqlx.StructScan in Get or other planned queries...

@silasdavis
Copy link
Contributor Author

Looking a bit deeper, it seems that we are not making as much use of TableMap as we could, chiefly in Get. But I wonder if we could aslo could maintain a set of 'TableMaps' (or an improved 'QueryMap') for all queries, possibly indexed by the query string or prepared statement.

@jmoiron
Copy link
Owner

jmoiron commented Mar 7, 2014

It looks like I'm going to have to create a github.com/jmoiron/sqlx/reflect package so that the behavior of modl and sqlx can be kept in sync wrt handling more sophisticated object hierarchies. I had sort of planned on doing this anyway to get some reflect-type things (like sqlx.BaseSliceType, et al) out of the sqlx namespace. Doing this should make it easier to add this support back into the TableMap.

@silasdavis
Copy link
Contributor Author

Okay let me know if there is any independent work that I could help with. I like the idea of extending/improving TableMap to shift as much computation/reflection to mapping time rather than query time. I'd also like to bind arbitrary non-table queries. Any thoughts on that?

@sqs
Copy link
Contributor

sqs commented Apr 2, 2014

Any update on this? I would be interested in adding support for embedded structs to modl, as I did for gorp. I'm happy to help you finish/test it if you've already begun on the sqlx/reflect package, or take your direction (if you have any plans) and implement it from scratch.

@sqs
Copy link
Contributor

sqs commented Apr 22, 2014

FWIW, I implemented support for embedded structs in https://github.com/sqs/modl/commit/e801ca1fef9784285e9a36cde3a01f7287c85f69. If you would like to use this implementation instead of refactoring and creating sqlx/reflect, I can submit a PR.

@jmoiron
Copy link
Owner

jmoiron commented Apr 22, 2014

Please do, I'll almost certainly have time to look it over at Gophercon.

@DylanLukes
Copy link

Just sounding off, embedded structures would be great to have!

@mattbostock
Copy link

Likewise, I'd also like to see support for embedded structs.

@maxhawkins
Copy link

I'm also very interested in this feature.

Is there any way we can help, or is it dependent on new reflectx features?

@ferhatelmas
Copy link

Any news? Thanks!

@nwidger
Copy link

nwidger commented Jul 2, 2015

I'm interested in this as well. Any updates?

@robert-zaremba
Copy link

Any update here? We are also struggling with this.

@dabfleming
Copy link

Also just ran into this issue. Am currently using @sqs fork as a workaround.

Any chance of getting #17 merged?

@robert-zaremba
Copy link

It seams that this repo is not maintained any more. We also moved to @sqs 10 days ago.

@sqs
Copy link
Contributor

sqs commented Mar 11, 2016

FYI, we (@sourcegraph) just moved to go-gorp/gorp as we are not using embedded structs anymore. The @sqs fork has worked well for us, but I don't plan on maintaining it.

On Mar 11, 2016, at 01:39, Robert Zaremba notifications@github.com wrote:

It seams that this repo is not maintained any more. We also moved to @sqs 10 days ago.


Reply to this email directly or view it on GitHub.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

10 participants