PHPLIB-1627: BulkWriteCommand API

[jmikola]

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

[GromNaN]

Do you allow extending this class? In that case, you should use static here and in all return types. Otherwise the class should be final.

[jmikola]

Introducing it as final seems preferable.

[GromNaN]

When is null returned? I think this should be mentioned in the comment.

[jmikola]

Currently, executeBulkWriteCommand() returns null when an unacknowledged write concern is used.

The alternative would be to return BulkWriteCommandResult object where isAcknowledged() returns false and no other methods can be called. That's what we do in the legacy executeBulkWrite() API.

I'm leaning towards a null object since it's consistent with the spec's instruction on not populating partialResult on BulkWriteCommandException when zero successful writes have occurred. In the legacy API, the result object could just be empty.

[jmikola]

I'm now wondering if the current design of having executeBulkWriteCommand() return a nullable BulkWriteCommandResult is optimal. Interestingly, the spec doesn't explicitly suggest that when it presents a signature for Client::bulkWrite(), but the section on modeling unacknowledged results ends up suggesting this when it links to the CRUD spec's guidance on modeling unacknowledged results. I was also influenced by libmongoc, which leaves the result null in these cases.

The spec is clear about BulkWriteCommandException having an optional value for the partial write result, so I was planning to use nullable BulkWriteCommandResult for its $partialResult property (and getter); however, I'm curious if a BulkWriteCommandResult object in an unacknowledged state would also be preferable there.

In PHPC-2144, we changed PHPC 2.0 to remove the nullable return types for fields on WriteResult when the write was unacknowledged. Instead, we just throw if getters are accessed and the result object is empty (i.e. isAcknowledged() returns false). That made PHPC consistent with the PHPLIB API. My memory is a bit fuzzy, but I remember the nullable return values being more of an annoyance for static analysis. And in the general case where users don't use w:0, they can probably ignore isAcknowledged() and just assume that exceptions won't be thrown when they access the result.

[alcaeus]

In general, I think it would be preferable to return something other than null in all cases, as long as users can deduce what happen (e.g. unacknowledged writes). Returning null in some cases creates one "special" condition that users need to check for instead of handling a result in all cases that didn't error.

[GromNaN]

The method Collection::bulkWrite() always returns an instance of BulkWriteResult, I would do the same for Client::bulkWrite().

[jmikola]

I think it would be preferable to return something other than null in all cases

How do you feel about the verbose result fields (e.g. BulkWriteCommandResult::getInsertResults()) returning a nullable Document? If we have the regular getters (e.g. getInsertedCount()) throw when isAcknowledged() is false, we can do the same with the others in relation to a hasVerboseResults() field. That is one approach suggested in the bulkWrite spec, although it's this is a level removed from the Client::bulkWrite() API so it's less important.

I also didn't have qualms about using a nullable BulkWriteCommandResult for BulkWriteCommandException::getPartialResult(), which is directly suggested in the spec.