-
Notifications
You must be signed in to change notification settings - Fork 546
CXX-2697 Add search index spec tests and management helpers #986
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
src/mongocxx/search_index_model.hpp
Outdated
@@ -32,7 +32,7 @@ class MONGOCXX_API search_index_model { | |||
/// | |||
/// Move assigns a search_index_model. | |||
/// | |||
search_index_model& operator=(search_index_model&&) noexcept; |
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.
Marking a function as noexcept
requires that all members of the class must also have noexcept
on the corresponding function. While the _definition
field is a bsoncxx::document::value
which has its move assign operator as noexcept
, the bsoncxx::stdx::optional<std::string> _name
field does not have the same tag on its move assign operator. Instead of making changes to stdx::optional
which could affect the entire codebase, it's easier to just remove the noexcept
from the move assign operator.
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.
This file is for future testing that cannot be done at the moment.
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.
This file contains the code that was previously in options::aggregate::append
. In order to reduce code duplication, the best practice would be to use append
to extract the options from the options::aggregate
variables passed through to list
. However, because append
is private, search_index_view
would need to become a friend
of options::aggregate
. Because the code in append
only accesses public variables and functions in options::aggregate
and it seemed unneccessary to give search_index_view
access to all of options::aggregate
, we opted to move the function to a private file.
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.
Nice work. LGTM with one comment.
Co-authored-by: Kevin Albertson <[email protected]>
…rch_index_view::impl to be copied/moved
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.
Small feedback remaining; otherwise, LGTM. 👍
src/mongocxx/search_index_view.cpp
Outdated
|
||
const search_index_view::impl& search_index_view::_get_impl() const { | ||
if (!_impl) { | ||
throw logic_error{error_code::k_invalid_client_object}; |
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.
Needs similar fix as search_index_model
w.r.t. error_code
enumerator.
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 with minor comments addressed.
#include <mongocxx/private/client_session.hh> | ||
#include <mongocxx/private/libbson.hh> | ||
#include <mongocxx/private/libmongoc.hh> |
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.
#include <mongocxx/private/client_session.hh> | |
#include <mongocxx/private/libbson.hh> | |
#include <mongocxx/private/libmongoc.hh> | |
#include <mongocxx/search_index_model.hpp> |
Include search_index_model.hpp
directly. Remove unnecessary headers.
|
||
const search_index_model::impl& search_index_model::_get_impl() const { | ||
if (!_impl) { | ||
throw logic_error{error_code::k_invalid_search_index_model}; |
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.
throw logic_error{error_code::k_invalid_search_index_model}; | |
throw mongocxx::logic_error{error_code::k_invalid_search_index_model}; |
search_index_model.cpp
appears to rely on headers included throughclient_session.hh
. Update search_index_model.cpp
to include headers needed for throwing error:
#include <mongocxx/exception/error_code.hpp>
#include <mongocxx/exception/logic_error.hpp>
Summary
This PR will add the search index spec tests and respective functionality needed to pass the spec tests.
Background
Due to the spec test additions made in mongodb/specifications#1423, it was necessary to sync the spec test changes made and then ensure that the driver can pass those tests. To accomplish this, we chose to add an index view API and referenced the pseudocode function definitions found in the specifications PR.
Design
The
search_index_view
structure is very similar to the existingindex_view
structure. All of the public API functions call their private counterpart which constructs a BSON command and sends it to the server. Bothsearch_index_view
andsearch_index_model
are designed with the PIMPL pattern.Testing
At the moment, there are only spec tests for search indexes from this commit. There is no capability for e2e testing yet but when there is a separate PR will be made with additions to
test/search_index_view.cpp
. Edit: e2e testing was implemented in this ticket: #1002What's New
search_index_view
from a collectionsearch_index_model.hpp
andsearch_index_model.cpp
search_index_view.hpp
andsearch_index_view.cpp
private/search_index_view.hh
private/search_index_model.hh
operations.cpp
private/append_aggregate_options
: a helper function foroptions::aggregate::append
andsearch_index_view::impl::list