-
Notifications
You must be signed in to change notification settings - Fork 547
CXX-3236 add mongocxx v1 declarations #1482
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
base: master
Are you sure you want to change the base?
Conversation
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.
Posting initial comments.
- Should
std::unique_ptr<impl> _implbe madevoid* _implinstead (even when aclass implis used) to leave open the possibility of replacingimplwith a mongoc equivalent or vise versa as an ABI compatible change? Or is this "premature optimization" and usingstd::unique_ptr<T>is sufficient?
I am slightly in favor of switching to void *, but also OK with the current proposal. I expect saving one pointer dereference may not make an observable performance difference in most cases (especially when much of mongocxx API includes network calls), but this may help to future-proof the ABI.
- Regardless of the question above, should classes which can currently be implemented entirely in terms of a
mongoc_*_ttype usevoid* _impl; // mongoc_*_t, e.g. to avoid double-indirection? (Initial changes propose that only the view-likev1::events::*classes do this.)
Similar: Slightly in favor, but OK with current proposal.
- Is providing
char const*+std::string+string_viewset of overloads premature optimization? Should they be simplified to just a singlestring_viewoverload with an internalstd::stringallocation?
IIUC this only appears to impact four methods: read_concern::acknowledge_string(), server_error::has_error_label(), uri::uri(), and write_concern::tag.
Some libbson API was added accept a length argument (e.g. bson_iter_init_find_w_len). If libmongoc adds functions accepting a string length (suggested in comment), that may avoid a need for the char const* / std::string overloads. Filed CDRIVER-6128 to propose this API in the C driver.
IMO: I am slightly in favor of reducing the overloads since this could be eventually improved by libmongoc additions.
- Is removing the deprecated
v1::read_preference::hedge()premature? (CXX-3241)
I am in favor of adding to v1. Hedged reads was only deprecated in server 8.0. And only deprecated in the C++ driver v4.1.
I am in favor of keepint these out. CXX-1939 deprecated 5 years ago, and CXX-1978 deprecated 8 years ago, I expect those have low usage. If that assumption is wrong, they can be added to v1 on request.
- Should
mongoc_cursor_clone()be implemented by mongocxx by making cursors copyable? Or shouldmongoc_cursor_clone()be deprecated given its confusing behavior (re-execute the underlying query)?
I also think mongoc_cursor_clone behavior is surprising (I would expect "clone" means to clone the current state). mongoc_cursor_clone documents the behavior. And this appears to be the requested behavior in CDRIVER-204 for a language binding. I would rather not continue the pattern in C++ driver unless there is a known need. But if there are users wanting the C API, I think it is fine as-is.
- Should
v1::change_stream::batch_size()supportstd::uint32_tfor consistency withmongoc_cursor_set_batch_size()? Or doesmongoc_cursor_set_batch_size()need to be updated to supportstd::int64_tinstead?
The spec suggests both should be std::int32_t. I expect mongoc_cursor_set_batch_size should instead be int32_t to match the spec.
src/mongocxx/include/mongocxx/v1/events/server_heartbeat_failed.hpp
Outdated
Show resolved
Hide resolved
src/mongocxx/include/mongocxx/v1/events/server_heartbeat_started.hpp
Outdated
Show resolved
Hide resolved
src/mongocxx/include/mongocxx/v1/events/server_heartbeat_succeeded.hpp
Outdated
Show resolved
Hide resolved
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 am slightly in favor of reducing the [NTBS] overloads since this could be eventually improved by libmongoc additions.
Done. This means we are accepting a performance penalty (due to unconditional std::string allocations) until mongoc improves support for length-based string API.
I am in favor of adding [
.hedge()] to v1. Hedged reads was only deprecated in server 8.0.
Done. v1::read_preference also has deprecated .hedge() accessors.
I would rather not continue the [
mongoc_cursor_clone()] pattern in C++ driver unless there is a known need.
Will leave v1::cursor as-is then. Both v1::cursor and v_noabi::cursor will remain non-copyable.
I am slightly in favor of switching to
void *... this may help to future-proof the ABI.
Done. All _impl data members are now annotated with the underlying type (class impl; is assumed and implied by the presence of the declaration).
The following classes are defined entirely in terms of the mongoc API:
v1::events::*(all event types are fully backed by mongoc equivalents)v1::client_encryption->mongoc_client_encryption_tv1::client_session::options->mongoc_session_opt_tv1::read_concern->mongoc_read_concern_tv1::read_preference->mongoc_read_prefs_tv1::transaction->mongoc_transaction_opt_tv1::uri->mongoc_uri_tv1::write_concern->mongoc_write_concern_t
The following classes are defined entirely in terms of other mongocxx API:
v1::change_stream::iterator->v1::change_stream(shared state iterators)v1::cursor::iterator->v1::cursor(shared state iterators)
The following classes still use class impl; despite an apparent mongoc equivalent being available:
v1::apm(mongoc_apm_callbacks_t)- Needs to support
std::function<T>getters.
- Needs to support
v1::auto_encryption(mongoc_auto_encryption_opts_t)- Needs to support
.key_vault_client() -> v1::client*and.key_vault_pool() -> v1::pool*. - Missing
mongoc_auto_encryption_opts_get_*()for getters.
- Needs to support
v1::change_stream(mongoc_change_stream_t)- Needs to support shared state for iterators (i.e. the event document).
v1::client_encryption::options(mongoc_client_encryption_opts_t)- Needs to support mongocxx getters (no
mongoc_client_encryption_opts_get_*()).
- Needs to support mongocxx getters (no
v1::client_session(mongoc_client_session_t)- Needs to support
.client() -> v1::client const&and.options() -> v1::client_session::options const&.
- Needs to support
v1::client(mongoc_client_t)- Needs to extend the lifetime of
v1::apm.
- Needs to extend the lifetime of
v1::collection(mongoc_collection_t)- Needs to store a reference to the associated
v1::clientobject (possibly avoidable if we usemongoc_collection_create_indexes_with_opts()inv1::indexes::create_many()?).
- Needs to store a reference to the associated
v1::cursor(mongoc_cursor_t)- Needs to support shared state for iterators (i.e. the result document).
v1::database(mongoc_database_t)- Needs to store a reference to the associated
v1::clientobject. - Missing
mongoc_database_command_simple_with_server_id().
- Needs to store a reference to the associated
v1::encrypt(mongoc_client_encryption_encrypt_opts_t)- Missing
mongoc_client_encryption_encrypt_opts_get_*()for getters.
- Missing
v1::indexes::model(mongoc_index_model_t)- Missing
mongoc_index_model_get_*()for getters.
- Missing
v1::range(mongoc_client_encryption_encrypt_range_opts_t)- Missing
mongoc_client_encryption_encrypt_range_opts_get_*()for getters.
- Missing
v1::pool(mongoc_client_pool_t)- Needs to extend the lifetime of
v1::apm.
- Needs to extend the lifetime of
src/mongocxx/include/mongocxx/v1/events/server_heartbeat_failed.hpp
Outdated
Show resolved
Hide resolved
|
Applied the decision made in #1487 to |
Summary
Resolves CXX-3236. Followup to #1462 and companion PR to #1469 and #1470. Contains cherry-pick of proposed changes in #1469 (the "cpx: cxx-abi-v1-instance" commit).
This is 5 out of an estimated 7 major PRs which in total are expected to resolve CXX-2745 and CXX-3320.
Related tickets affected by the changes in this PR (applicable only to the v1 API) include (not exhaustive, but tried my best):
document::viewinstead ofdocument::view_or_valuefrom options getters"options::client::ssl_optswithout "ssl=true" in URI"pool::entry::operator->()"bsoncxx::string::view_or_valueingridfs::bucket"#includehygiene in header files"mongocxx::v_noabi::options::index::storage_options"Relative Changelog (v_noabi -> v1)
is_open()tov1::gridfs::downloaderandv1::gridfs::uploaderto extend behavior ofoperator bool().flush()tov1::gridfs::uploader(exposingv_noabi::gridfs::uploader::flush_chunks()) + documenting the existence of underlying buffers for better control over its behavior.chunks_written()tov1::gridfs::uploader(useful information)..upserted_ids()forv1::update_many_resultfor consistency withv1::bulk_write::result(vs.v1::update_one_resultwhich uses the old.upserted_id()).k_unknowntov1::write_concernfor symmetry withv1::read_concern.v1::indexes,v1::pipeline,v1::bulk_write::single, etc.v1::bulk_write::result::raw()to allow access to the raw server response.v1::bulk_write::insert_one::valueis made public.v1::pool::entry::operator->,v1::client::database(), etc. (by removing deleted overloads and ref-qualifiers; premature pessimization).v_noabi::*::view_or_valueis replaced with eithervieworvalue(std::stringforv_noabi::string::view_or_value).value.view.char const*to allow avoiding forced allocations. (We could really use acstring_view...)view, with a few exceptions for "simple" classes wrapping the underlying value with no additional invariants (e.g.v1::bulk_write::insert_one).Optional<view_or_value>->Optional<View>(view_or_valueimplies internal ownership).T const&orOptional<T> const&: returnTorOptional<T>instead.Optional<T> const&) which severely restrict implementation freedom.v1::client_session::client(): return the associated client (must be a mongocxx reference).v1::bulk_write::single::get_*(): straightforward union-like API.constqualifier from member functions and parameters which are not logically const.v1::collection::list_indexes()usesmongoc_collection_t*.v1::databaseparameters inv1::client_encryptionusemongoc_database_t*.v1::clientdatabase operations usemongoc_client_t*.T&for setters (method-chaining) where various functions used to returnvoid.v1::server_errormongocxx::v1::exceptionwith.code() == v1::source_errc::server..code()is always the server error code,.client_code()is the optional client error code, for consistency with the.raw()invariant).v1::event::*void*are labeled with a comment indicating the underlying type.v1::events::server_descriptionis the sole exception due tov1::events::topology_description::servers(), see "Removed" entry below.server_heartbeat_failed::message()return type:string->string_view.v1::read_concern: ignore unsupported values instead of throwing an exception.v1::read_preference: treat invalid read modes ask_primaryinstead of an error (same behavior asmongoc_read_prefs_get_mode()).v1::write_concern.tag(nullptr)ask_defaultinstead of an error..acknowledge_level()errors by converting unsupported values tok_unknownand document "notk_unknown" as a precondition for.to_document()(treat as if unset).v1::gridfs::uploaderflushes its buffer on destruction (it didn't already do this?!).T*parameters are madeT&instead for input/output streams tov1::gridfs::bucket.T*instead ofOptional<T*>for.key_vault_*()inv1::auto_encryptionandv1::client_encryption.v1::search_indexes::create_index()return type to be consistent withv1::indexes::create_one()..comment()with.comment_option()inv1::find_optionsfor consistency with other option classes.v1::collection's bulk write API.Int32->Int64for bulk write API result values and somev1::pipelinefields.id_mapforv1::bulk_write::resultand related API:Size->Int64for keys ofid_map(e.g.v1::bulk_write::result) for better spec accuracy.document::elementtotypes::viewto avoid unnecessary proxy type (element key is always"_id"; useless info) + consistency with other "result" API.v1::bulk_write::*operation single-argument constructors explicit.v1::client_session::options()->.opts()to avoid conflict with::options.with_transaction_cbparameter type fromT*(unconditional not-null) toT&.v_noabi::events::server_description::is_master()(deprecated by CXX-2262, CXX-2263 #841).v_noabi::events::topology_description::server_descriptions: move ownership of underlyingmongoc_server_description_tobjects tov1::events::server_descriptionand avoid the awkward intermediate class. The array itself (not its elements) is deallocated after thestd::vector<v1::events::server_description>is fully constructed.v_noabi::gridfs::chunks_and_bytes_offset: implementation detail not used by any public API.v_noabi::result::gridfs::uploadctor is made internal forv1::gridfs::upload_result.ordered()forv1::insert_one_options(specific tov1::insert_many_options).v_noabi::options::index::base_storage_optionsand related API.v_noabi::read_preference::hedge().v1::client..ssl()API.v_noabi::options::index::haystack*()API.v_noabi::options::index::version(). (Was this meant to be "textIndexVersion"?)v1::server_error(e.g.WriteConcernError,BulkWriteError, etc.).v1::server_error::raw()should continue to suffice as a workaround for obtaining error-specific fields manually.noexcept) via template parameter constraints (e.g. likebsoncxx::v1::document::value's deleter API).v1::events::*:.duration()and.round_trip_time(): changing the return type fromstd::int64_ttostd::chrono::microseconds.v1::events::server_heartbeat_failed: support returning or throwing amongocxx::v1::exceptionfor thebson_error_t?v1::events::server_descriptiondoes not conform to specification forServerDescription.v1::tlsunderspecification and lack of support for allmongoc_ssl_opt_tfields.v1::collection: missing access to raw server response (e.g..rename()), missing support for additional command options (e.g..rename()),missing session overloads (e.g.awkward range templates, use of bulk write operations instead of dedicated database commands (e.g..estimated_document_count()),.insert_one()using bulk write "insertMany" instead of theinsertdatabase command?).indexesvs.search_indexesAPI symmetry and specification compliance (exacerbated by themodelvs. command-specific options vs. common index options situation;v1::search_indexes::modelshould probably be split intoSearchIndexModelandSearchIndexOptions).collation && !is_acknowledgedcondition for.find_and_modify()and etc.).General Notes
\paramor\returnswhen behavior is plainly obvious from the declaration (see: Slack). Only include when at least one parameter or the return value requires documentation of special values and behaviors that is not apparent from the types alone. Prefer\par Preconditionsfor partial parameter documentation.Optional<T>return type for a "field" is "obviously" empty when "unset".0being equivalent to "unset" for anIntfield is not "obvious", thus it is documented.v1::client::start_sessiondocuments: "The client session object MUST be passed as the first argument to all operations that are intended to be executed in the context of a session." -> avoid needing countless repetition of\param sessionparagraphs. The overload complexity should be revisited and addressed at a later time... (CXX-1835)\copydocis not applicable.()is omitted given\ref func_namewhen more than one overload may apply.\{ ... \}Doxygen groups when able (helped by absence of\param).mongocxx::v1::exceptionfor all other runtime errors".ex.what(); encourage programmatic inspection viav1::source_errcandv1::type_errcinstead.errc(as in bsoncxx), of which there are currently only 9 in total (all other errors are deferred to mongoc):instance(multiple object detection: CXX-3320 migrate instance and logger to mongocxx::v1 #1469)server_api(translatefalsereturned by mongoc API for invalid versions into an exception)uri(server_selection_try_once()setter throws instead of returning a boolean)pool(wait queue timeout for.acquire())collection(narrowing conversion fromstd::chrono::durationInt64 into a UInt32 for mongoc API compatibility)indexes(index name"*"is not permitted for.drop_one(), no relevant mongoc API to defer to)v1::gridfs::bucket(GridFS file metadata validation + database commands)v1::gridfs::downloader(runtime GridFS file metadata validation and underlying stream state)v1::gridfs::uploader(runtime GridFS file metadata validation and underlying stream state)v1/database.hppno longer includesv1/collection.hppand all of its transitive dependencies. 🎉void fn(T)andT fn()do not requireTto be a complete type (even whenT = Optional<U>!).collectionmember functions)...bsoncxx::v1::document::view).iterator end() consteven wheniterator begin()(not-const).T*->T&when unconditionally not-null, or by changingOptional<T*>->T*(double-null).Review Focus Items
filter).client_encryption) or the parameter (class) type (e.g.write_concern).Questions for Review
ShouldApplied: now usingstd::unique_ptr<impl> _implbe madevoid* _implinstead (even when aclass implis used) to leave open the possibility of replacingimplwith a mongoc equivalent or vise versa as an ABI compatible change? Or is this "premature optimization" and usingstd::unique_ptr<T>is sufficient?void* _impl;for all mongocxx classes.Regardless of the question above, should classes which can currently be implemented entirely in terms of aApplied: annotated with underlying type when applicable.mongoc_*_ttype usevoid* _impl; // mongoc_*_t, e.g. to avoid double-indirection? (Initial changes propose that only the view-likev1::events::*classes do this.)Is providingRemoved extra overloads: simplechar const*+std::string+string_viewset of overloads premature optimization? Should they be simplified to just a singlestring_viewoverload with an internalstd::stringallocation?string_viewonly.Is removing the deprecatedReverted: removal is premature.v1::read_preference::hedge()premature? (CXX-3241)Is removing the deprecated readConcern + writeConcern + readPreference accessors inApplied: removal is acceptable.v1::clientpremature? (CXX-1939)Is removing the deprecated geoHaystack API forApplied: removal is acceptable.v1::indexespremature? (CXX-1978)ShouldApplied: may add later upon request.mongoc_cursor_clone()be implemented by mongocxx by making cursors copyable? Or shouldmongoc_cursor_clone()be deprecated given its confusing behavior (re-execute the underlying query)?ShouldNeither: spec expectsv1::change_stream::batch_size()supportstd::uint32_tfor consistency withmongoc_cursor_set_batch_size()? Or doesmongoc_cursor_set_batch_size()need to be updated to supportstd::int64_tinstead?Int32.