CXX-2138 Implement versioned API #792

benjirewis · 2021-05-10T16:11:17Z

Implements versioned API. First commit adds and updates spec tests files for versioned API. Remaining commits update unified test runner, create new server_api class, and allow setting of server_api_options through client. Beyond creating the class and allowing configuration through client, few changes were required apart from wrapping the C driver implementation.

Further CXX changes will be required to allow versioned API testing in Evergreen. This work will be done as part of CXX-2254.

benjirewis

The server_api class and its configuration through client is based off the auto_encryption class.

benjirewis · 2021-05-10T16:14:44Z

src/mongocxx/options/server_api.hpp

+    ///
+    /// Enum representing the possible values for server API version.
+    ///
+    enum class version { version_1 };


Used the name version, as server_api_version would mean this is called options::server_api::server_api_version which seems redundant.

Calling it version is fine, but the value needs to be k_version_1 to follow the constant naming used elsewhere in the driver.

You could also potentially not make it an enum class? Is it harmful if the constants exist at class scope? It might even be clearer: options::server_api_version::version_1 still duplicates version, but options::server_api::k_version_1 reads pretty well.

Switched to k_version_1, but I kept it as an enum. It would probably be good for users to be able to switch on a version and have their IDE catch any un-handled server API versions.

src/mongocxx/test/spec/util.hh

codecov-commenter · 2021-05-10T16:46:17Z

Codecov Report

Merging #792 (f465d5f) into master (908f0e6) will decrease coverage by 4.40%.
The diff coverage is 86.31%.

@@            Coverage Diff             @@
##           master     #792      +/-   ##
==========================================
- Coverage   90.76%   86.36%   -4.41%     
==========================================
  Files         377      380       +3     
  Lines       21757    21860     +103     
==========================================
- Hits        19748    18879     -869     
- Misses       2009     2981     +972

Impacted Files	Coverage Δ
src/mongocxx/options/client.hpp	`100.00% <ø> (ø)`
src/mongocxx/pool.cpp	`56.86% <14.28%> (-11.32%)`	⬇️
src/mongocxx/options/private/server_api.hh	`80.00% <80.00%> (ø)`
src/mongocxx/client.cpp	`85.71% <85.71%> (ø)`
src/mongocxx/options/server_api.cpp	`86.95% <86.95%> (ø)`
src/mongocxx/events/server_description.cpp	`100.00% <100.00%> (ø)`
src/mongocxx/options/client.cpp	`83.33% <100.00%> (+4.38%)`	⬆️
src/mongocxx/options/server_api.hpp	`100.00% <100.00%> (ø)`
src/mongocxx/private/index_view.hh	`95.14% <100.00%> (ø)`
src/mongocxx/private/libmongoc_symbols.hh	`100.00% <100.00%> (ø)`
... and 42 more

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 908f0e6...f465d5f. Read the comment docs.

data/versioned-api/crud-api-version-1-strict.json

src/mongocxx/client.cpp

src/mongocxx/options/client.hpp

src/mongocxx/options/server_api.hpp

acmorrow · 2021-05-11T16:17:59Z

src/mongocxx/options/server_api.cpp

+// See the License for the specific language governing permissions and
+// limitations under the License.
+
+#include <mongocxx/config/private/prelude.hh>


The docs are wrong, but prelude.hh should come after all the driver headers. I will file a ticket to fix the docs and hopefully get it bulk corrected.

Ah ok, thanks for filing the docs ticket. I will say that my version of clang (11.1.0) disagrees, and will try to place it on top. Done.

We'll use CXX-2258 to change the .clang-format file and re-format the whole codebase to make sure preludes come last. For now, server_api.cpp and server_api.hpp will just follow the rest of the codebase.

OK, ignore my comment above about ordering then.

src/mongocxx/options/server_api.cpp

acmorrow · 2021-05-11T16:19:50Z

src/mongocxx/options/server_api.hpp

+
+#pragma once
+
+#include <mongocxx/config/prelude.hpp>


Move to after driver headers

The .clang-format has a macro enforcing prelude.hpp appears before other headers:

mongo-cxx-driver/.clang-format

Line 12 in 908f0e6

- Regex: 'prelude\.(hpp|hh)' # preludes

I vote we revert this so the lint task passes for now, and fix them all, with the .clang-format file, as part of CXX-2258.

src/mongocxx/test/CMakeLists.txt

kevinAlbs

Looking good!

Can you investigate the test failure for: estimatedDocumentCount appends declared API version

kevinAlbs · 2021-05-14T17:33:11Z

src/mongocxx/options/server_api.cpp

+    if (!version.compare("1")) {
+        return server_api::version::k_version_1;
+    }
+    throw std::logic_error{"invalid server API version"};


Nit: append the version to the error.

kevinAlbs · 2021-05-14T17:38:04Z

src/mongocxx/options/server_api.cpp

+
+    auto mongoc_server_api_opts = libmongoc::server_api_new(mongoc_api_version);
+    if (!mongoc_server_api_opts) {
+        throw std::logic_error{"could not get object from libmongoc"};


The C++ driver hides details about libmongoc/libbson from users from the public API. We should probably avoid mentioning it in error messages:

Suggested change

throw std::logic_error{"could not get object from libmongoc"};

throw std::logic_error{"could not create server API"};

Good to know. Done.

src/mongocxx/client.cpp

kevinAlbs · 2021-05-14T20:35:32Z

src/mongocxx/options/server_api.hpp

+    ///   The enum value to convert to a string.
+    ///
+    /// @return
+    ///   The string value of the given enum value.


Minor: Note that these methods can throw with @throws mongocxx::logic_error on an invalid argument

Added here and on version_from_string.

kevinAlbs · 2021-05-14T20:38:05Z

src/mongocxx/options/server_api.hpp

+    ///
+    /// The specified API version will be sent to the server. This will cause
+    /// the server to behave in a manner compatible with that API version.
+    /// It also causes the driver to behave in a manner compatible with the


driver to behave in a manner compatible with the driver's behavior as of the release when the driver

Reads a little strange to me. It looks like this is close to the Background, perhaps it could use that directly:

The driver will behave in a manner compatible with a server configured with that API version, regardless of the server's actual release version.

Hmm good point; used that documentation instead.

kevinAlbs · 2021-05-14T20:40:30Z

src/mongocxx/test/spec/unified_tests/runner.cpp

        FAIL("must specify a version when using serverApi");
    }

+    REQUIRE(sav.type() == type::k_string);
+    auto version =
+        options::server_api::version_from_string(string::to_string(sav.get_string().value));


Minor: Now that version_from_string takes a string_view I think this can be simplified:

Suggested change

options::server_api::version_from_string(string::to_string(sav.get_string().value));

options::server_api::version_from_string(sav.get_string().value);

kevinAlbs · 2021-05-14T20:51:26Z

src/mongocxx/options/server_api.hpp

+
+#pragma once
+
+#include <mongocxx/config/prelude.hpp>


The .clang-format has a macro enforcing prelude.hpp appears before other headers:

mongo-cxx-driver/.clang-format

Line 12 in 908f0e6

- Regex: 'prelude\.(hpp|hh)' # preludes

I vote we revert this so the lint task passes for now, and fix them all, with the .clang-format file, as part of CXX-2258.

acmorrow

Small things only. I took a more or less cursory run through the tests, because I don't think I'm a particularly useful reviewer for that part. But, overall, this looks good with a few small things I called out. I don't particularly think any of them need another round of review.

LGTM

src/mongocxx/options/private/server_api.hh

acmorrow · 2021-05-20T15:25:00Z

src/mongocxx/options/private/server_api.hh

+using unique_server_api =
+    std::unique_ptr<mongoc_server_api_t, decltype(libmongoc::server_api_destroy)>;
+
+static unique_server_api make_server_api(const server_api& opts) {


I think you need to include-what-you-use for mongocxx::server_api.

acmorrow · 2021-05-20T15:26:28Z

src/mongocxx/options/server_api.cpp

+// See the License for the specific language governing permissions and
+// limitations under the License.
+
+#include <mongocxx/config/private/prelude.hh>


OK, ignore my comment above about ordering then.

acmorrow · 2021-05-20T15:28:47Z

src/mongocxx/options/server_api.hpp

+    ///
+    /// Enum representing the possible values for server API version.
+    ///
+    enum class version { k_version_1 };


I suppose, if you wanted to, that you could replace k_version_1 here with k_1, since you have kept it as an enum class, and version is already in the name. Totally up to you. At three characters, k_1 is getting awfully short and potentially into the "what if it conflicts with some crazy system macro" territory.

Yeah server_api::version::k_version_1 reads a little clearer to me even if redundant. Unless @kevinAlbs has a strong opinion, I'll probably keep it as k_version_1.

acmorrow · 2021-05-20T15:30:32Z

src/mongocxx/options/server_api.hpp

+    /// @param version
+    ///   The server api version to send to the server.
+    ///
+    server_api(version version);


Yes, but you can still move it up a good deal. Please do before commit, but no need to post an update for that.

acmorrow · 2021-05-20T15:32:02Z

src/mongocxx/options/server_api.hpp

+    /// @return
+    ///   The version enum value specifying the declared server api version.
+    ///
+    version api_version() const;


Fair. Let's call it get_version then. No need to post an update for that.

benjirewis

The ubuntu1804-power8 tasks are failing because their server version is outdated; looking into that.

Some more tasks will start to fail until getMore + transaction-continuing commands append API version (CDRIVER-3974).

src/mongocxx/options/private/server_api.hh

benjirewis · 2021-05-20T16:21:16Z

src/mongocxx/options/server_api.hpp

+    ///
+    /// Enum representing the possible values for server API version.
+    ///
+    enum class version { k_version_1 };


Yeah server_api::version::k_version_1 reads a little clearer to me even if redundant. Unless @kevinAlbs has a strong opinion, I'll probably keep it as k_version_1.

benjirewis · 2021-05-20T16:21:33Z

src/mongocxx/options/server_api.hpp

+    /// @param version
+    ///   The server api version to send to the server.
+    ///
+    server_api(version version);


Moved to directly after enum declaration.

benjirewis · 2021-05-20T16:21:42Z

src/mongocxx/options/server_api.hpp

+    /// @return
+    ///   The version enum value specifying the declared server api version.
+    ///
+    version api_version() const;


Renamed to get_version.

benjirewis · 2021-05-20T20:35:27Z

src/mongocxx/private/libmongoc_symbols.hh

@@ -277,7 +277,7 @@ MONGOCXX_LIBMONGOC_SYMBOL(server_api_get_strict)
 MONGOCXX_LIBMONGOC_SYMBOL(server_api_get_version)
 MONGOCXX_LIBMONGOC_SYMBOL(server_description_host)
 MONGOCXX_LIBMONGOC_SYMBOL(server_description_id)
-MONGOCXX_LIBMONGOC_SYMBOL(server_description_ismaster)
+MONGOCXX_LIBMONGOC_SYMBOL(server_description_hello_response)


server_description_ismaster is now deprecated (CDRIVER-3947) and will cause build failures, so I've updated usages to server_description_hello_response.

acmorrow · 2021-05-20T20:59:36Z

LGTM still

kevinAlbs · 2021-05-21T20:58:49Z

src/mongocxx/options/private/server_api.hh

+
+    auto mongoc_server_api_opts = libmongoc::server_api_new(mongoc_api_version);
+    if (!mongoc_server_api_opts) {
+        throw std::logic_error{"could not get object create server API"};


Minor: This reads a little strange.

Suggested change

throw std::logic_error{"could not get object create server API"};

throw std::logic_error{"could create server API"};

src/mongocxx/options/private/server_api.hh

src/mongocxx/pool.cpp

kevinAlbs · 2021-05-21T21:07:15Z

data/versioned-api/crud-api-version-1.json

+      ]
+    },
+    {
+      "description": "find command with declared API version appends to the command, but getMore does not",


This may be out of date and need to be resynced to get tests passing.

Resynced all tests one more time!

kevinAlbs

The latest changes LGTM.

The mongohouse failure looks related to the server's handling of batch size and getMore and unrelated to these changes.

benjirewis commented May 10, 2021

View reviewed changes

benjirewis requested review from kevinAlbs and acmorrow May 10, 2021 16:36

benjirewis marked this pull request as ready for review May 10, 2021 16:41

acmorrow suggested changes May 11, 2021

View reviewed changes

benjirewis requested a review from acmorrow May 11, 2021 17:02

benjirewis added 11 commits May 13, 2021 17:23

Add new spec test files

0bb454c

Uncomment versioned API TEST_CASE

627ca8b

Create server_api Class

064578a

Allow client configuration with server_api

9d09976

Initial test runner changes

028ca44

Actually uncomment versioned API TEST_CASE

de38bab

Create enum for server API version

ea20b3f

Allow errorCodeName in unified test runner

e0ef76c

Andrew comments

18a9d33

Use stdx::string_view

12e354f

Revert to api_version

d9cc46d

kevinAlbs reviewed May 14, 2021

View reviewed changes

benjirewis added 2 commits May 17, 2021 11:45

Kevin nits

340603f

Use RAII for server API and allow pool configuration

3d92e34

acmorrow approved these changes May 20, 2021

View reviewed changes

Andrew final comments

335e636

benjirewis commented May 20, 2021

View reviewed changes

benjirewis requested a review from kevinAlbs May 20, 2021 16:24

Use hello_response instead of ismaster

37eabd8

benjirewis commented May 20, 2021

View reviewed changes

kevinAlbs reviewed May 21, 2021

View reviewed changes

Correct comment and resync tests

1e6e4a4

benjirewis requested a review from kevinAlbs May 24, 2021 15:14

benjirewis added 2 commits May 24, 2021 11:50

Remove serverless test parameter for now

f465d5f

batchSize workaround for getMore in CM spec tests

b19c06d

kevinAlbs approved these changes May 26, 2021

View reviewed changes

benjirewis merged commit c585e14 into mongodb:master May 26, 2021

benjirewis deleted the versionedAPI.2138 branch May 26, 2021 20:04

	throw std::logic_error{"could not get object from libmongoc"};
	throw std::logic_error{"could not create server API"};

	options::server_api::version_from_string(string::to_string(sav.get_string().value));
	options::server_api::version_from_string(sav.get_string().value);

	throw std::logic_error{"could not get object create server API"};
	throw std::logic_error{"could create server API"};


		#pragma once

		#include <mongocxx/config/prelude.hpp>


		#pragma once

		#include <mongocxx/config/prelude.hpp>

CXX-2138 Implement versioned API #792

CXX-2138 Implement versioned API #792

Conversation

benjirewis commented May 10, 2021 • edited

benjirewis left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

codecov-commenter commented May 10, 2021 • edited

Codecov Report

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

kevinAlbs left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

acmorrow left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

benjirewis left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

acmorrow commented May 20, 2021

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

kevinAlbs left a comment

Choose a reason for hiding this comment

benjirewis commented May 10, 2021 •

edited

codecov-commenter commented May 10, 2021 •

edited