Skip to content

GODRIVER-2017 Skip empty extJSON objects correctly in Unmarshal #677

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

Merged
merged 3 commits into from
May 28, 2021
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
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
17 changes: 17 additions & 0 deletions bson/bsonrw/extjson_reader.go
Original file line number Diff line number Diff line change
Expand Up @@ -164,6 +164,23 @@ func (ejvr *extJSONValueReader) skipObject() {
depth := 1
for depth > 0 {
ejvr.p.advanceState()

// If object is empty, raise depth and continue. When emptyObject is true, the
// parser has already read both the opening and closing brackets of an empty
// object ("{}"), so the next valid token will be part of the parent document,
// not part of the nested document.
//
// If there is a comma, there are remaining fields, emptyObject must be set back
// to false, and comma must be skipped with advanceState().
if ejvr.p.emptyObject {
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Optional: Add a description of the expected state of the parser/buffer to help people reading the code understand why we need different behavior for when ejvr.p.emptyObject is true.

E.g.

When emptyObject is true, the parser has already read both the opening and closing brackets of an empty object ("{}"), so the next valid token will be part of the parent document, not part of the nested document.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Good idea! Added a description of the state of the parser (the exact text you gave).

if ejvr.p.s == jpsSawComma {
ejvr.p.emptyObject = false
ejvr.p.advanceState()
}
depth--
continue
}

switch ejvr.p.s {
case jpsSawBeginObject, jpsSawBeginArray:
depth++
Expand Down
40 changes: 35 additions & 5 deletions bson/unmarshal_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -234,7 +234,7 @@ func TestUnmarshalExtJSONWithUndefinedField(t *testing.T) {
// When unmarshalling extJSON, fields that are undefined in the destination struct are skipped.
// This process must not skip other, defined fields and must not raise errors.
type expectedResponse struct {
DefinedField string
DefinedField interface{}
}

unmarshalExpectedResponse := func(t *testing.T, extJSON string) *expectedResponse {
Expand All @@ -246,98 +246,128 @@ func TestUnmarshalExtJSONWithUndefinedField(t *testing.T) {
}

testCases := []struct {
name string
testJSON string
name string
testJSON string
expectedValue interface{}
}{
{
"no array",
`{
"UndefinedField": {"key": 1},
"DefinedField": "value"
}`,
"value",
},
{
"outer array",
`{
"UndefinedField": [{"key": 1}],
"DefinedField": "value"
}`,
"value",
},
{
"embedded array",
`{
"UndefinedField": {"keys": [2]},
"DefinedField": "value"
}`,
"value",
},
{
"outer array and embedded array",
`{
"UndefinedField": [{"keys": [2]}],
"DefinedField": "value"
}`,
"value",
},
{
"embedded document",
`{
"UndefinedField": {"key": {"one": "two"}},
"DefinedField": "value"
}`,
"value",
},
{
"doubly embedded document",
`{
"UndefinedField": {"key": {"one": {"two": "three"}}},
"DefinedField": "value"
}`,
"value",
},
{
"embedded document and embedded array",
`{
"UndefinedField": {"key": {"one": {"two": [3]}}},
"DefinedField": "value"
}`,
"value",
},
{
"embedded document and embedded array in outer array",
`{
"UndefinedField": [{"key": {"one": [3]}}],
"DefinedField": "value"
}`,
"value",
},
{
"code with scope",
`{
"UndefinedField": {"logic": {"$code": "foo", "$scope": {"bar": 1}}},
"DefinedField": "value"
}`,
"value",
},
{
"embedded array of code with scope",
`{
"UndefinedField": {"logic": [{"$code": "foo", "$scope": {"bar": 1}}]},
"DefinedField": "value"
}`,
"value",
},
{
"type definition embedded document",
`{
"UndefinedField": {"myDouble": {"$numberDouble": "1.24"}},
"DefinedField": "value"
}`,
"value",
},
{
"empty embedded document",
`{
"UndefinedField": {"empty": {}},
"UndefinedField": {"empty": {}, "key": 1},
"DefinedField": "value"
}`,
"value",
},
{
"empty object before",
`{
"UndefinedField": {},
"DefinedField": {"value": "a"}
}`,
D{{"value", "a"}},
},
{
"empty object after",
`{
"DefinedField": {"value": "a"},
"UndefinedField": {}
}`,
D{{"value", "a"}},
},
}
for _, tc := range testCases {
t.Run(tc.name, func(t *testing.T) {
responseDoc := unmarshalExpectedResponse(t, tc.testJSON)
assert.Equal(t, "value", responseDoc.DefinedField, "expected DefinedField to be 'value', got %q", responseDoc.DefinedField)
assert.Equal(t, tc.expectedValue, responseDoc.DefinedField, "expected DefinedField to be %v, got %q",
tc.expectedValue, responseDoc.DefinedField)
})
}
}
Expand Down