Skip to content

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

Conversation

HanaPearlman
Copy link
Contributor

@HanaPearlman HanaPearlman commented Nov 6, 2020

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 the CommandOperation constructor. So, we can get rid of the NO_INHERIT_OPTIONS aspect and simply not call resolveOptions before those commands. Similarly, the ReadPreference.resolve() function is no longer needed.

There is also a change to ReadPreference.fromOptions(), where if the options don't have a readPreference 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 optional session property.

NODE-2870

@@ -164,36 +164,6 @@ export class ReadPreference {
return readPreference as ReadPreference;
}

Copy link
Contributor Author

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.

@HanaPearlman HanaPearlman requested review from mbroadst, nbbeeken and emadum and removed request for mbroadst November 6, 2020 14:24
@HanaPearlman HanaPearlman force-pushed the NODE-2870/master/resole-inherited-options branch 2 times, most recently from a351b2c to a4d90a6 Compare November 6, 2020 20:01
@HanaPearlman HanaPearlman force-pushed the NODE-2870/master/resole-inherited-options branch from a4d90a6 to 34d8bba Compare November 6, 2020 20:19
@HanaPearlman HanaPearlman requested a review from mbroadst November 6, 2020 22:11
Copy link
Member

@mbroadst mbroadst left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM!

Copy link
Contributor

@nbbeeken nbbeeken left a 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.
Copy link
Contributor

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.

@HanaPearlman HanaPearlman merged commit 495e86b into mongodb:master Nov 9, 2020
@HanaPearlman HanaPearlman deleted the NODE-2870/master/resole-inherited-options branch November 9, 2020 18:14
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants