-
Notifications
You must be signed in to change notification settings - Fork 547
v1::read_preference (CXX-3237, CXX-3238) #1505
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
v1::read_preference (CXX-3237, CXX-3238) #1505
Conversation
| /// | ||
| /// @deprecated Use @ref mode instead. | ||
| /// | ||
| MONGOCXX_DEPRECATED MONGOCXX_ABI_EXPORT_CDECL() read_preference(read_mode mode); |
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 expect inlining the previously ABI exported methods is an ABI breaking change to the (unstable) v_noabi. I think that is OK. Consider noting ABI removals in the changelog (if not already planned) as done with previous ABI removal notes.
A generic note may be appropriate:
v_noabiABI exports have been made inline. Consumers must recompile`
I expect many v_noabi classes will be re-implemented in terms of v1. And this may just be a useful heads up that a recompile is needed.
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.
Rather than enumerating all of the individual exported symbols removed in the 4.2.0 release, added an "Important" admonition at the top of the 4.2.0 release section indicating to users that recompilation is required to link against the new library release. Users should be recompiling against every release given the lack of a stable ABI guarantee, but nevertheless, this release deserves a reminder.
| } | ||
|
|
||
| /// | ||
| /// Sets or updates the tag set list for this read_preference. |
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.
Not introduced in this PR, but suggested drive-by fix of incorrect comment:
| /// Sets or updates the tag set list for this read_preference. | |
| /// Gets the tag set list for this read_preference. |
| {mode::k_secondary_preferred, mode::k_secondary_preferred}, | ||
| {mode::k_nearest, mode::k_nearest}, | ||
| {static_cast<mode>(-1), mode::k_primary}, | ||
| {static_cast<mode>(5), mode::k_primary}, |
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.
Prior to this PR, using a value outside of the enum hits a BSON_UNREACHABLE:
// Prior to this PR:
TEST_CASE("Bad read mode", "[read_preference]") {
read_preference rp;
rp.mode(static_cast<mongocxx::v_noabi::read_preference::read_mode>(-1)); // Abort!
}Suggest restoring this behavior (or throwing an exception?). That may help add a future read_mode enumerator without concern for breaking consumers. E.g. if consumers are relying on 5 meaning primary, adding a new read mode might later break that assumption.
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.
Fixed/updated the read preference setter documentation for consistency with other enum field setters: users should not be passing any inputs which are not explicitly permitted in the list (precondition violation). Rather than termination or exceptions, the intent is to have undocumented well-defined behavior as permitted by precondition violation. Changing the behavior of e.g. the value of 5 in a subsequent release does not constitute an API breaking change, as a user using said value prior to that release is violating the function contract.
Resolves CXX-3237 and CXX-3238 for the
v1::read_preferencecomponent.The deprecated v_noabi ctors and the
.hedge()accessors are left as-is, to be removed in an upcoming major release. Note that this PR proposes v1 includes.hedge()in the definition ofoperator==(), as "hedge" is still "a supported field" despite its deprecation. This can be removed in favor of v_noabi's behavior which excludes "hedge" if that is preferable.