GODRIVER-1235 Skip embedded documents and arrays correctly in extJSONValueReader #544

benjirewis · 2020-11-30T15:26:31Z

Adds logic to correctly skip embedded documents and embedded arrays in extJSONValueReader by creating one function to skip both iteratively using the parser's advanceState() function. This addresses errors in unmarshalling extended JSON with undefined fields with nested documents or arrays.

Adds tests for unmarshalling extended JSON with undefined fields in unmarshal_test.go.

kevinAlbs

Looking good! I just have a question about whether we can use peekType to skip documents and request some additional tests.

bson/bsonrw/extjson_reader.go

kevinAlbs · 2020-12-03T04:50:09Z

bson/bsonrw/extjson_reader.go

-		_, err = ejvr.p.peekType()
+		typ, err = ejvr.p.peekType()
+		// account for embedded arrays
+		if typ == bsontype.Array {


Does this also need to check for bsontype.Document and call skipDocument? What if this is unmarshaling an array of documents?

Edit: it appears the thorough tests you wrote already check that! Looking a little deeper, I think extJSONParser.peekType, unlike extJSONParser.readValue will recurse into the document. If that is the case, could we use peekType to implement skipDocument? Maybe that is a little cheaper than readValue.

Great point! The skipObject() function now skips both documents and arrays and uses only peekType().

bson/unmarshal_test.go

kevinAlbs

The iterative approach is nicer! I think we can avoid peekType entirely. Let me know if you'd like to discuss.

kevinAlbs · 2020-12-05T01:13:43Z

bson/bsonrw/extjson_reader.go

 	for err == nil {
 		_, err = ejvr.p.peekType()
+		if err == ErrEOA || err == ErrEOD {
+			err = nil
+			if ejvr.p.depth == initialDepth-1 {


Gah, I pointed you in the wrong direction. Looking at advanceState, depth only applies to documents, not arrays. Sorry about that!

Ah, you're right, all good!

kevinAlbs · 2020-12-05T01:21:06Z

bson/bsonrw/extjson_parser.go

@@ -104,6 +104,13 @@ func (ejp *extJSONParser) peekType() (bsontype.Type, error) {
 	case jpsSawEndArray:
 		// this would only be a valid state if we were in array mode, so return end-of-array error
 		err = ErrEOA
+	case jpsSawEndObject:
+		if ejp.peekMode() == jpmObjectMode {


Looking over this again, I still feel like this is a little fragile. I think I'd prefer not modifying peekType if possible.

The comment on L105 justifies why the peekMode() is not checked for the array case. So, I thought it would be safe to remove this condition, but that led me astray.

Here is my rough understanding.

extJSONParser maintains a stack of modes for parsing nested arrays and objects. advanceState calls pushMode when encountering an [ or { token, and calls popMode when encountering a ] or } token to check that brackets are balanced and correct.

So when advanceState returns jpsSawEndObject, the mode stack for the object that was being read was just popped. Consequently, I think this condition is only true when popping a nested object. For example:

[ { "a" : { "b": 1 } } ]

When reading the first } this condition will be true.
But when reading the second }, we'll pop the jpmObjectMode. The mode stack will contain one jpmArrayMode. So this condition is false.

In summary, I don't quite understand the need for the check, and I am not sure why removing this check breaks tests.

There are other oddities about peekType (e.g. not all states are handled in this switch, so I think peekType just returns the 0 type in those cases...).

So unless we have a solid understanding, I'd like to vote for another approach. I think we can bypass peekType entirely by using advanceState (explained below).

Great points, and thank you for looking so in-depth into peekType(). I think I agree, and I've switched to using advanceState() with the algorithm you described below. The only thing we lose is the error-checking in peekType(); advanceState() advances forward blindly and does not return errors, so now neither does skipObject(). There are some disadvantages to this I may bring up offline...

Per offline discussion, I am not too concerned with losing some skip validation on malformed BSON. The previous behavior was incorrect on valid BSON.

kevinAlbs · 2020-12-05T01:27:57Z

bson/bsonrw/extjson_reader.go

-
-func (ejvr *extJSONValueReader) skipArray() error {
-	// read entire array until ErrEOA (using peekType)
+func (ejvr *extJSONValueReader) skipObject() error {


I think we can tweak this to only use advanceState.

The initial state, ejvr.p.s should to be jpsSawBeginArray or jpsSawBeginObject at the beginning of this function. I think we can do something like:

initialize count to 1

call advanceState

if the state is jpsSawBeginArray or jpsSawBeginObject, increment count

if the state is jpsSawEndArray or jpsSawEndObject, decrement count

repeat until count is 0

Sounds good! That algorithm seems to work perfectly for all existing examples.

kevinAlbs

LGTM!

bson/bsonrw/extjson_reader.go

kevinAlbs · 2020-12-08T15:00:43Z

bson/bsonrw/extjson_parser.go

@@ -104,6 +104,13 @@ func (ejp *extJSONParser) peekType() (bsontype.Type, error) {
 	case jpsSawEndArray:
 		// this would only be a valid state if we were in array mode, so return end-of-array error
 		err = ErrEOA
+	case jpsSawEndObject:
+		if ejp.peekMode() == jpmObjectMode {


Per offline discussion, I am not too concerned with losing some skip validation on malformed BSON. The previous behavior was incorrect on valid BSON.

divjotarora

This looks fantastic! The code changes all LGTM and I just have a couple of minor comments on the tests. I'll hold off on approving until those are done, but great work!

bson/unmarshal_test.go

divjotarora

LGTM. Great work!

…ValueReader (#544)

GODRIVER-1235 Add embedded document skip

3ebeff6

benjirewis requested review from kevinAlbs, iwysiu and divjotarora November 30, 2020 15:26

benjirewis marked this pull request as ready for review November 30, 2020 15:26

Add test for array skipping

439ce38

iwysiu approved these changes Dec 1, 2020

View reviewed changes

Benjamin Rewis added 2 commits December 2, 2020 14:53

Skip embedded arrays and add better tests

91d8742

Remove mArray from skip case

5d57bd1

benjirewis requested a review from iwysiu December 2, 2020 20:08

benjirewis changed the title ~~GODRIVER-1235 Skip embedded documents correctly in extJSONValueReader~~ GODRIVER-1235 Skip embedded documents and arrays correctly in extJSONValueReader Dec 2, 2020

kevinAlbs reviewed Dec 3, 2020

View reviewed changes

Benjamin Rewis added 3 commits December 3, 2020 15:25

Skip code with scope correctly and add tests

92ad20c

Use only one skipObject function

c39a6e7

Simplify skipObject and add type definition test

ed2dbca

benjirewis requested a review from kevinAlbs December 3, 2020 21:51

Account for empty embedded documents

c0b4bfc

kevinAlbs reviewed Dec 5, 2020

View reviewed changes

Use advanceState instead of peekType

75079c3

kevinAlbs self-requested a review December 7, 2020 16:33

kevinAlbs approved these changes Dec 8, 2020

View reviewed changes

divjotarora reviewed Dec 9, 2020

View reviewed changes

bson/unmarshal_test.go Outdated Show resolved Hide resolved

bson/unmarshal_test.go Outdated Show resolved Hide resolved

Move helper functions within test function

df65c2d

divjotarora approved these changes Dec 9, 2020

View reviewed changes

benjirewis merged commit cbd4540 into mongodb:master Dec 9, 2020

benjirewis deleted the godriver1235 branch December 9, 2020 16:40

benjirewis added a commit that referenced this pull request Dec 9, 2020

GODRIVER-1235 Skip embedded documents and arrays correctly in extJSON…

b10e639

…ValueReader (#544)

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

GODRIVER-1235 Skip embedded documents and arrays correctly in extJSONValueReader #544

GODRIVER-1235 Skip embedded documents and arrays correctly in extJSONValueReader #544

benjirewis commented Nov 30, 2020 •

edited

Loading

kevinAlbs left a comment

kevinAlbs Dec 3, 2020

benjirewis Dec 3, 2020

kevinAlbs left a comment

kevinAlbs Dec 5, 2020

benjirewis Dec 7, 2020

kevinAlbs Dec 5, 2020

benjirewis Dec 7, 2020

kevinAlbs Dec 8, 2020

kevinAlbs Dec 5, 2020

benjirewis Dec 7, 2020

kevinAlbs left a comment

kevinAlbs Dec 8, 2020

divjotarora left a comment

divjotarora left a comment

GODRIVER-1235 Skip embedded documents and arrays correctly in extJSONValueReader #544

GODRIVER-1235 Skip embedded documents and arrays correctly in extJSONValueReader #544

Conversation

benjirewis commented Nov 30, 2020 • edited Loading

kevinAlbs left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

kevinAlbs left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

kevinAlbs left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

divjotarora left a comment

Choose a reason for hiding this comment

divjotarora left a comment

Choose a reason for hiding this comment

benjirewis commented Nov 30, 2020 •

edited

Loading