-
Notifications
You must be signed in to change notification settings - Fork 548
v1: pipeline (CXX-3237, CXX-3238) #1513
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
kevinAlbs
left a comment
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.
| scoped_bson{R"({"x": 1, "y": 2})"}, | ||
| })); | ||
|
|
||
| CAPTURE(input); |
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.
On a failed assert, this capture prints the address:
input := 0x16f6ffef8
Suggest adding a StringMaker implementation for scoped_bson:
template <>
struct Catch::StringMaker<mongocxx::scoped_bson> {
static std::string convert(mongocxx::scoped_bson const& b) {
return Catch::StringMaker<bsoncxx::v1::document::view>::convert(b.view());
}
};Resulting in:
input := {"x": 1}
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.
To be resolved by #1514.
| /// Convert to the @ref mongocxx::v1 equivalent. | ||
| /// | ||
| /// @par Postconditions: | ||
| /// - `other` is in an assign-or-destroy-only state. |
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.
| /// - `other` is in an assign-or-destroy-only state. | |
| /// - `this` is in an assign-or-destroy-only state. |
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.
Good catch. To be resolved by #1515.
|
|
||
| void append(bsoncxx::v1::document::view doc) { | ||
| _doc += scoped_bson{BCON_NEW(this->idx(), BCON_DOCUMENT(scoped_bson_view{doc}.bson()))}; | ||
| ++_count; |
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.
Aside: I expect the unchecked ++_count is OK. If a caller were to append INT32_MAX stages, _doc would sooner overflow an result in an exception when appending to _doc.
Resolves CXX-3237 and CXX-3238 for the
v1::pipelinecomponent.The
internal::doc()API is used allow direct access to the pipelinescoped_bsondocument byv_noabi::(collection|database)::_aggregate()when callingmongoc_database_aggregate()with abson_t const* pipelineargument. We can consider redefining the_aggregate()function so it builds the pipeline document usingscoped_bsonrather than deferring it to mongoc (which would avoid the need for this internal API), e.g. as currently done inv_noabi::collection::_watch()andclient::_watch()via the basic builder API. However, this PR (initially) proposes making minimal changes to the behavior of unrelated components during the v_noabi refactor of a given component to facilitate easier scoping and review. The current arrangement also helps avoid the need for multiple intermediate temporary (view) objects (pipeline.view_array()->scoped_bson_view()->.bson()).