-
Notifications
You must be signed in to change notification settings - Fork 1.8k
refactor: consolidate options inheritance logic #2614
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
refactor: consolidate options inheritance logic #2614
Conversation
@@ -164,36 +164,6 @@ export class ReadPreference { | |||
return readPreference as ReadPreference; | |||
} | |||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Note: ReadPreference.fromOptions
should not default to primary
if it cannot find a read pref in the options. In resolveOptions
when ReadPreference.fromOptions
is undefined, we default to the parent read pref. If fromOptions
always defaults to primary, we would never use the parent read pref.
a351b2c
to
a4d90a6
Compare
a4d90a6
to
34d8bba
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM!
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM-ing, my only comment is that we can do some documenting, the behavior isn't new so it's not critical but could be a good improvement. 👍
|
||
// Intentionally, we do not inherit options from parent for this operation. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think we should add this to the function's docs for the ops that do this. Particularly with BSON options the return value could be unexpectedly different.
This PR introduces a new helper
resolveOptions
which includes the logic for inheriting some options from a parent. NOTE: It only merges in inherited properties (read/write concern, read preference, and bson options) from the parent, and does not add to or modify any other properties of options.Options are resolved before a command is constructed. This means the options passed to
CommandOperation
are already properly inherited, and inheritance is no longer needed in theCommandOperation
constructor. So, we can get rid of theNO_INHERIT_OPTIONS
aspect and simply not callresolveOptions
before those commands. Similarly, theReadPreference.resolve()
function is no longer needed.There is also a change to
ReadPreference.fromOptions()
, where if the options don't have areadPreference
property and a session is present on the options, we attempt to get the read preference from the session. This might have been intended all along, since the type of the options passed to this function,ReadPreferenceFromOptions
, already had an optionalsession
property.NODE-2870