Skip to content

allow ptr in inline structs #146

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.

Sign up for GitHub

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

Merged
domodwyer merged 3 commits into from
Apr 30, 2018
Merged

allow ptr in inline structs #146

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
3381f10
allow ptr in inline structs
Apr 15, 2018
3fb5f17
Merge branch 'development' into development
larrycinnabar Apr 29, 2018
46c4b47
inline pointer_to_struce mode: update comments. return error on point…
Apr 29, 2018
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
8 changes: 8 additions & 0 deletions bson/bson.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -746,6 +746,14 @@ func getStructInfo(st reflect.Type) (*structInfo, error) {
return nil, errors.New("Option ,inline needs a map with string keys in struct " + st.String())
}
inlineMap = info.Num
case reflect.Ptr:
// allow only pointer to struct
if field.Type.Elem().Kind() != reflect.Struct {
break
}

field.Type = field.Type.Elem()
fallthrough
case reflect.Struct:
sinfo, err := getStructInfo(field.Type)
if err != nil {
Expand Down
49 changes: 47 additions & 2 deletions bson/bson_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -271,6 +271,42 @@ func (s *S) TestMarshalBuffer(c *C) {
c.Assert(data, DeepEquals, buf[:len(data)])
}

func (s *S) TestPtrInline(c *C) {
cases := []struct {
In interface{}
Out bson.M
}{
{
In: inlinePtrStruct{A: 1, MStruct: &MStruct{M: 3}},
Out: bson.M{"a": 1, "m": 3},
},
{ // go deeper
In: inlinePtrPtrStruct{B: 10, inlinePtrStruct: &inlinePtrStruct{A: 20, MStruct: &MStruct{M: 30}}},
Out: bson.M{"b": 10, "a": 20, "m": 30},
},
{
// nil embed struct
In: &inlinePtrStruct{A: 3},
Out: bson.M{"a": 3},
},
{
// nil embed struct
In: &inlinePtrPtrStruct{B: 5},
Out: bson.M{"b": 5},
},
}

for _, cs := range cases {
data, err := bson.Marshal(cs.In)
c.Assert(err, IsNil)
var dataBSON bson.M
err = bson.Unmarshal(data, &dataBSON)
c.Assert(err, IsNil)

c.Assert(dataBSON, DeepEquals, cs.Out)
}
}

// --------------------------------------------------------------------------
// Some one way marshaling operations which would unmarshal differently.

Expand Down Expand Up @@ -713,8 +749,6 @@ var marshalErrorItems = []testItemType{
"Attempted to marshal empty Raw document"},
{bson.M{"w": bson.Raw{Kind: 0x3, Data: []byte{}}},
"Attempted to marshal empty Raw document"},
{&inlineCantPtr{&struct{ A, B int }{1, 2}},
"Option ,inline needs a struct value or map field"},
{&inlineDupName{1, struct{ A, B int }{2, 3}},
"Duplicated key 'a' in struct bson_test.inlineDupName"},
{&inlineDupMap{},
Expand Down Expand Up @@ -1174,6 +1208,17 @@ type inlineUnexported struct {
M map[string]interface{} `bson:",inline"`
unexported `bson:",inline"`
}
type MStruct struct {
M int `bson:"m,omitempty"`
}
type inlinePtrStruct struct {
A int
*MStruct `bson:",inline"`
}
type inlinePtrPtrStruct struct {
B int
*inlinePtrStruct `bson:",inline"`
}
type unexported struct {
A int
}
Expand Down
23 changes: 22 additions & 1 deletion bson/encode.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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)
Copy link

@tadukurow tadukurow Apr 19, 2018

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?

Copy link
Author

@larrycinnabar larrycinnabar Apr 19, 2018 •
edited
Loading

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

Copy link

@domodwyer domodwyer Apr 23, 2018

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.

if errField != nil {
continue
}

value = field
}
if info.OmitEmpty && isZero(value) {
continue
Expand All @@ -238,6 +243,22 @@ func (e *encoder) addStruct(v reflect.Value) {
}
}

func safeFieldByIndex(v reflect.Value, index []int) (result reflect.Value, err error) {
defer func() {
if recovered := recover(); recovered != nil {
switch r := recovered.(type) {
case string:
err = fmt.Errorf("%s", r)
case error:
err = r
}
}
}()

result = v.FieldByIndex(index)
return
}

func isZero(v reflect.Value) bool {
switch v.Kind() {
case reflect.String:
Expand Down