-
Notifications
You must be signed in to change notification settings - Fork 234
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
allow ptr in inline structs #146
Conversation
Thanks for the PR - we'll take a proper look but seems like a great change! Dom |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks good!
bson/encode.go
Outdated
@@ -229,7 +229,12 @@ func (e *encoder) addStruct(v reflect.Value) { | |||
if info.Inline == nil { | |||
value = v.Field(info.Num) | |||
} else { | |||
value = v.FieldByIndex(info.Inline) | |||
field, errField := safeFieldByIndex(v, info.Inline) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
this change will silently skip encoding errors, where they used to panic before.
Not very familiar with the bson encoder anymore, @domodwyer thoughts?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Encoder has a structInfo
object, based only on type, not on value.
As we allow pointers to structs here, we can have chain of inline indices (info.Inline
) and e.g. element behind a n-1
index might be nil
, so trying to get element by index n
will panic because of reflect: indirection through nil pointer to embedded struct
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think this is probably fine either way - relying on the encoder to panic because the input was invalid is a bit of a stretch in terms of backwards-compatibility.
Would you pull the latest development into this branch and we'll get it merged! Thanks for taking the time to open a PR - it's really appreciated. Dom |
Branch is updated. Also a commit with some extra comments is added |
Thanks for taking the time to contribute 👍 Dom |
This only works when transforming data to bson to be written into mongo. When reading (populating the struct with) a record I still get the "reflect: indirection through nil pointer to embedded struct" error. |
@bojangavrovski hm. Ok, I'm looking into it |
BSON encoder doesn't allow to use pointers to struct in
inline
mode.There was an issue on the same topic on old repo:
https://github.com/go-mgo/mgo/issues/346
Such structure:
is assumed to become
{"a":0, "m":0}
(withoutmstruct
structure) - the same way asjson
encoder does itwithout fix we get: "reflect: indirection through nil pointer to embedded struct"