StructScan infinite loop

[elwinar]

I had a case earlier which made me pull my hair for a full day and half :

Let's say we have the following structs :

type Entity struct {
    ID int64 `db:"id"`
    Service *Service
}

type Service struct {
    Entity *Entity
}

func NewEntity(id int64) *Entity {
    e := &Entity{}
    e.Service = &Service{}
    e.Service.Entity = e
    return e
}

When StructScaning, the function tries to establish a field map by exploring the tree of embedded types of the struct. I my case, the exploration never end because of the pointers.

This issue bring the following question to my mind : what is the point of exploring non-embedded fields ? It seem rather non-semantic to me…
And independantly, why making a field map and not simply look for the fields as needed when scanning ?

[jmoiron]

Thanks for the bug report; I only saw the title on my way into work this morning and thought right away "hmm, structscan would choke on circular data structures".

1. Scanning non-embedded structs was a mistake I am going to be fixing in a "API Breaking" release coming up.
2. Getting the reflect introspection work out of the way up front saves a lot of time. I'd like to change it so that the maps provide a direct way of getting the field. Right now they contain the BFS position in the tree, which means it still must be traversed. It'd be better to store a list positions at each depth.

Does this need to be fixed for embedded structs as well, or does the compiler prevent you from doing embedded struct loops? If not, it'l be fixed soon with the api cleanup release.

[elwinar]

If you don't scan non-embedded structs, I don't think the BFS will be necessary. I didn't researched much the way reflection work in Go, but I think that one way or another, each exported field of the struct should be accessible right away with the right RTTI function.

It doesn't need to be fixed for embedded structs, as the compiler complains about recursive embedded (see http://play.golang.org/p/Nkz7n-1W6m).

[jmoiron]

Okay, excellent. I suspected the compiler might complain.

BFS is still sadly required to emulate Go's shadowing rules. reflect.FieldByName works on embedded fields, but it won't utilize tags, so it has to be somewhat re-implemented.

Removing the descent into non-embedded fields is going to simplify things quite a lot. There is a pull request for adding embedded struct support to modl and the code is way simpler because it doesn't have to check for Scanner or Valuer or do any weird stuff.

[elwinar]

I was trying at the same time, but for me the Tags are embedded at the same time… http://play.golang.org/p/PbEtNt_JOh

[jmoiron]

Yes but it's backwards; when you scan, you only have the tag name, and you must go and find the field that fits it.

[elwinar]

I think that the StructScan function should just iterate over the given struct fields and populate a map[string]string with the correspondences sql -> struct. No need to do something fancy…

[jmoiron]

I'll do some benchmarking to see if Value.FieldByName carries a big penalty compared to just accessing by offset. If not, then you're right, each reachable name in a struct is unique and should get me there. It's still nice to cache the field mapping, they are actually expensive to build.

[elwinar]

Access the value's fields by offset should be better, that's right.
Here is an example of the field mapping loop : http://play.golang.org/p/UzZzrOr1AU

[jmoiron]

That doesn't do embedded structs (eg: 'age' is not accessible). More and more I'm thinking that sqlx's reflect stuff can be genericized and pushed into an sqlx/reflect package. I need to reuse some of it in modl anyway.

[elwinar]

You're right, it doesn't do embedded struct. I didn't saw it… but the Field.Anonymous field can differentiate embedded field from others, so it can be resolved with either a recursion or a stack (as you did).

[jmoiron]

Yes, I think it will end up looking a bit like https://github.com/sqs/modl/commit/dae428523dbab11adfed450f45782f5c7152027b

[elwinar]

Exactly.
I think that you should externalize your extended reflection methods into another package. That would simplify both sqlx and modl…

[freeeve]

And you could call it reflectx. :)

[jmoiron]

@wfreeman that was probably a joke but I can't think of a better way to make it live with the regular reflect package ...

[freeeve]

I was actually entirely serious. And willing to help--I'd use it.