PHPLIB-262: Implement iterator for BSON files

[kvwalker]

https://jira.mongodb.org/browse/PHPLIB-262

[kvwalker]

I ended up pretty much using @jmikola's BsonIterator. Locally, I tested this using mongodump on the phplib_test collection, but I'm not sure of the best way to write a consistent test for this.

[jmikola]

It should be easy to unit test this by constructing a few BSON documents as PHP values and running them all through fromPHP() to create a bunch of binary strings. You can then concatenate those strings and use the result as the input to this iterator. If implemented correctly, you should be able to use assertSameDocuments() (it accepts an iterator directly) and compare against the original list of documents.

Some inputs to handle would at least be basic PHP values (stdClass objects and arrays) and some library classes with a type map (BSONDocument and BSONArray).

[jmikola]

This looks like a dead import, since MongoDB\BSON isn't used below. Was this done in anticipation of callingthe BSON functions?

PHP 5.6 does support use function, although that won't work on 5.5, which we're still supporting. For now, I'd stick to calling \MongoDB\BSON\toPHP() and the like.

[jmikola]

I think this error message is actually incorrect. We're expecting at least 5 bytes (i.e. smallest possible document), which would include the 32-bit integer denoting the root document size and a trailing null byte.

PHP 5.5 won't allow us to define a constant relative to self::BSON_SIZE (constant expressions were introduced in 5.6), but I'd suggest a new MIN_DOCUMENT_LENGTH constant defined to 5, which we can reference in this error message and the conditional above (instead of using a magic number).

[jmikola]

* @see http://php.net/iterator.current
* @return mixed

[jmikola]

This doesn't look correct, as raw bytes from BSON data are being cast to an array and fed to the BSONDocument constructor. Try to var_dump() the elements of this iterator to see what this produces.

You're going to need to use the BSON functions to convert these bytes into a PHP value to assign to $this->current. Additionally, since MongoDB\BSON\toPHP() supports a type map it'd be reasonable if the constructor accepted an optional type map. In the interest of consistency, it should probably be through a new array $options = [] argument and parsed similar type maps for operation classes.

If there is a type map available, we can use it when calling toPHP(); otherwise, we can default to an empty array, which is equivalent to no type map for toPHP(). I'm not sure if null is accepted and ignored in the same fashion, but you could check the extension source to confirm.

[jmikola]

Ideas for a few additional test cases:

• Empty string should yield no documents during iteration
• Exception for remaining buffer length being < 5. This can be two cases: first and only "document", and second "document" following a valid document
• Exception for remaining buffer after 32-bit length field being too small, for both the first and second positions in the sequence.

[jmikola]

A link to toPHP() may be helpful, since that's the basis of this iterator:

* @see http://php.net/manual/en/function.mongodb.bson-tophp.php
* @param string $data    Concatenated, valid, BSON-encoded documents
* @param array  $options Iterator options

[jmikola]

Can we call this advance() or something to that effect? I realize this is private, but "get" implies that it has a return value.

[jmikola]

Can we consolidate all of the valid test cases into a single test that uses a data provider?

Consider the provideTypeMapOptionsAndExpectedDocuments() data provider from FindFunctionalTest. This can be altered to provide the binary string, type map (or null if default), and expected array of unserialized PHP values.

If you'd like to create the binary string within the return value from the provider, this convenient one-liner may help:

implode(array_map(
    'MongoDB\BSON\fromPHP',
    [ new stdClass, ['x' => 1] ]
));

[kvwalker]

I added tests for the first two cases you mentioned, but I'm confused by the last one. Maybe we can go over it tomorrow morning and then I can add that test case.

[jmikola]

For the exception tests, I'd suggest using the base document fromPHP([]), which should represent the smallest valid BSON document (in binary form). It contains a 32-bit integer with a value of 5, followed by a null byte (per the document definition in the spec where e_list is empty).

The first exception is when we don't have sufficient data in the buffer to read the 4-byte length header. For this, I'd like a test for both the very first document and a subsequent document:

• testCannotReadLengthFromFirstDocument(): start with one document as our input, and trim it to the first three bytes (anything less than five will suffice).
• testCannotReadLengthFromSubsequentDocument(): start with two documents as our input, but trim the second document to only three bytes

The second exception is when we were able to read the 4-byte length header, but there is not sufficient data in the buffer to read the remainder. Since the first check requires the document length to be at least five bytes, we'll need to use a document with at least one field.

• testCannotReadFirstDocument(): start with one document as our input and trim it to the first five bytes (this will cut it off in the key of the first field)
• testCannotReadSecondDocument(): start with two documents as our input, but trim the second document to the first five bytes

In PHPC, I use pack() to create these BSON documents. You may decide that's preferable (see: bson-toJSON_error-002.phpt).

[jmikola]

Alternatively, just change the first check to only require four bytes to read the length integer (instead of also requiring a null byte for min document size of five).

[jmikola]

I just remembered that assertSameDocuments() converts documents to JSON before it compares, so it's not useful for testing that type maps have been applied. See AggregateFunctionalTest::testTypeMapOption() and provideTypeMapOptionsAndExpectedDocuments() for a good approach. We can also re-use those data provider inputs.

[jmikola]

Let's make sure that one test passes null as the typeMap option, which will ensure code coverage of the constructor assigning [] in that case.

[jmikola]

Just a few straightforward test changes. LGTM otherwise.

[jmikola]

We can drop the object-casting from all of the fromPHP() inputs, unless you actually need an array like [ 0 => 'foo' ] to serialize as { "0": "foo" } instead of [ "foo" ].

[jmikola]

This should use assertEquals() to ensure we're checking expected types after type map conversion (as is done in AggregateFunctionalTest).

[jmikola]

Drop (object).

[jmikola]

Drop (object).