refactor: consolidate options inheritance for bulk ops #2617
Conversation
| if (options.readPreference) { | ||
| options.readPreference = ReadPreference.primary; | ||
| } | ||
|
|
||
| const commandOptions = Object.assign( |
There was a problem hiding this comment.
We need to check for this case. If a read preference is in the options, it will be included in the command, even if that command is a write.
We are already doing the equivalent check for write operations which pass through CommandOperation, but for those that just subclass OperationBase, we needed a separate check.
If this approach is too buried/hacky for us, an alternative would be manually setting the read preference to primary in the options for each write command subclassing OperationBase, either before we construct the write command (which is what we do for RunCommandOperations) or in the constructor of the command (which is what we do in CommandOperation).
There was a problem hiding this comment.
My inclination is that we would want to do the hard thing here and explicitly codify a fixed primary read preference at the point that operations are constructed/executed, if only just for readability of the codebase. The paths for "write commands" in the wire protocol layer are symbolic, and ideally would go away if we were doing command construction correctly: in a 3.6+ world we would always just use the command wire protocol method.
This is another form of options resolution, but now with restrictions. We try to handle this during construction of the CommandOperation (as you note) by use of a WRITABLE aspect, codifying the restriction into the "type system". It actually looks to me that the resolution in CommandOperation could just be moved to OperationBase (I believe you're also proposing this), that seems to be where it belongs. A readPreference is a fundamental aspect of an operation as it is used to determine the server selected, so I think it does belong on the OperationBase class - and you should be able to determine if the operation is writable there as well.
Another (more functional) approach might be to encode this through a chain of functions:
new InsertOneOperation(topology, ..., fixedPrimaryReadPreference(resolveOptions(this, options)))`)
but that might get a little overwhelming as we introduce more restrictions in resolution
HanaPearlman
changed the title
HanaPearlman
changed the title
HanaPearlman
requested review from
mbroadst,
nbbeeken and
emadum
and removed request for
mbroadst and
nbbeeken
src/bulk/common.ts
Outdated
| finalOptions.writeConcern = bulkOperation.s.writeConcern; | ||
| } | ||
| const finalOptions = resolveOptions(bulkOperation, { | ||
| ordered: bulkOperation.isOrdered, |
There was a problem hiding this comment.
I think you want to invert these, since this means that ordered provided in options would take precedence in the assignment. My rule of thumb is to always spread the "defaults" first (options or some builtOptions)
| if (options.readPreference) { | ||
| options.readPreference = ReadPreference.primary; | ||
| } | ||
|
|
||
| const commandOptions = Object.assign( |
There was a problem hiding this comment.
My inclination is that we would want to do the hard thing here and explicitly codify a fixed primary read preference at the point that operations are constructed/executed, if only just for readability of the codebase. The paths for "write commands" in the wire protocol layer are symbolic, and ideally would go away if we were doing command construction correctly: in a 3.6+ world we would always just use the command wire protocol method.
This is another form of options resolution, but now with restrictions. We try to handle this during construction of the CommandOperation (as you note) by use of a WRITABLE aspect, codifying the restriction into the "type system". It actually looks to me that the resolution in CommandOperation could just be moved to OperationBase (I believe you're also proposing this), that seems to be where it belongs. A readPreference is a fundamental aspect of an operation as it is used to determine the server selected, so I think it does belong on the OperationBase class - and you should be able to determine if the operation is writable there as well.
Another (more functional) approach might be to encode this through a chain of functions:
new InsertOneOperation(topology, ..., fixedPrimaryReadPreference(resolveOptions(this, options)))`)
but that might get a little overwhelming as we introduce more restrictions in resolution
HanaPearlman
deleted the
NODE-2870/master/bulk-resolve-inherited-options
branch
This PR extends the work from #2614 to bulk operations. Now, bulk ops use
resolveOptionsto inherit options from the parent. Also, since no bson options or read preference resolving needs to be done in subclasses ofOperationBase, assigning thebsonOptionsandreadPreferenceproperties now occurs in theOperationBaseconstructor.The changes to
find_and_modify.ts,insert.ts, andcommon_functions.tsshould have been done in the last PR :(NODE-2870