GODRIVER-3049 Replace bsoncore.DocumentSequence with a bson type-agnostic analogue #1492
Conversation
prestonvasquez
requested review from
matthewdale
and removed request for
a team
API Change Report./x/bsonx/bsoncoreincompatible changesArrayStyle: removed compatible changesIterator: added ./x/mongo/driverincompatible changes##(*BatchCursor).Batch: changed from func() *./x/bsonx/bsoncore.DocumentSequence to func() *./x/bsonx/bsoncore.Iterator compatible changesNewBatchCursorFromList: added |
|
@prestonvasquez this has a conflict now. |
x/bsonx/bsoncore/iterator.go
Outdated
| docs := make([]Document, 0, len(vals)) | ||
| for _, v := range vals { | ||
| if v.Type != bsontype.EmbeddedDocument { | ||
| continue |
There was a problem hiding this comment.
The method documentation says this will return an error, but it actually ignores non-document values. We should return an error here instead.
x/bsonx/bsoncore/iterator.go
Outdated
| if len(iter.List[iter.pos:]) == 1 && iter.List[iter.pos] == 0x00 { | ||
| return nil, io.EOF // At the end of the document | ||
| } | ||
|
|
||
| elem, _, ok := ReadElement(iter.List[iter.pos:]) |
There was a problem hiding this comment.
Optional: Consider clarifying the logic here by storing the remaining data as a variable.
| if len(iter.List[iter.pos:]) == 1 && iter.List[iter.pos] == 0x00 { | |
| return nil, io.EOF // At the end of the document | |
| } | |
| elem, _, ok := ReadElement(iter.List[iter.pos:]) | |
| rem := iter.List[iter.pos:] | |
| if len(rem) == 1 && rem[0] == 0x00 { | |
| return nil, io.EOF // At the end of the document | |
| } | |
| elem, _, ok := ReadElement(rem) |
x/bsonx/bsoncore/iterator.go
Outdated
|
|
||
| // ErrCorruptedDocument is returned when a full document couldn't be read from | ||
| // the sequence. | ||
| var ErrCorruptedDocument = errors.New("invalid DocumentSequence: corrupted document") |
There was a problem hiding this comment.
This error variable isn't used outside of this package and doesn't need to be exported.
x/bsonx/bsoncore/iterator.go
Outdated
| // ErrNonDocument is returned when a DocumentSequence contains a non-document | ||
| // BSON value. | ||
| var ErrNonDocument = errors.New("invalid DocumentSequence: a non-document value was found in sequence") | ||
|
|
||
| // ErrInvalidDocumentSequenceStyle is returned when an unknown | ||
| // DocumentSequenceStyle is set on a DocumentSequence. | ||
| var ErrInvalidDocumentSequenceStyle = errors.New("invalid DocumentSequenceStyle") |
There was a problem hiding this comment.
These error variables are unused and should be removed.
prestonvasquez
dismissed stale reviews from qingyang-hu and matthewdale
via
e9399ac
GODRIVER-3049
Summary
The goal of bsoncore.Iterator is to abstract the arithmetic (i.e. constant time) retrieval of elements of a BSON array. The iterator does this by maintaining the “current” position of which element is “next-to-be-retrieved” in a type called List, which would likely be loaded from the cursor.nextBatch on a server response. This solution is more simple than it's predecessor and is more powerful, in that the List data is not restricted to documents.
Background & Motivation
The DocumentSequence pattern was introduced in GODRIVER-800 to avoid repetitively copying bytes into a document sequence.
The 800 pattern assumes an enum-like type for categorizing data as either a sequence: i.e. {x:1}{x:2}{x:3}, or a BSON array: {1:{x:1},2:{x:2},3:{x:3}}. 3049 proposes we simplify this pattern using bsoncore.Array, which will handle both cases.