feat(NODE-3191): backport versioned api #2850

emadum · 2021-06-16T19:59:37Z

Includes backports for the following 4.0 PRs:

feat: versioned api #2736 - versioned api (NODE-2950)
fix(NODE-3074): update estimated document count for v1 api #2764 - update estimated document count for v1 api (NODE-3074)
fix(NODE-3183,NODE-3249): bring versioned API impl up to date #2814 - versioned api updates
- NODE-3183 - Use "hello" command when API Version is declared
- NODE-3249 - Require passing Versioned API options to getMore and transaction-continuing commands

nbbeeken

Despite the size, a majority of this is accepting hello and legacy hello in the tests, most of what I callout here is minor requests.

lib/collection.js

test/functional/unified-spec-runner/entities.ts

test/functional/buffering_proxy.test.js

test/functional/collations.test.js

test/functional/cursor.test.js

test/unit/core/single/sessions.test.js

test/unit/sessions/collection.test.js

package.json

test/functional/unified-spec-runner/entities.ts

nbbeeken

LGTM

emadum · 2021-06-22T12:25:11Z

.evergreen/config.yml

@@ -1,6 +1,6 @@
 stepback: true
 command_type: system
-exec_timeout_secs: 1200


Note: this change is unrelated to the versioned API, we've just been experiencing some premature timeouts on our longer running tasks; this gives them another 5 minutes to complete.

…on (#2845)

dariakp

@emadum @nbbeeken The PR looks fine to me, minus the couple of questions I had around the test changes

dariakp · 2021-06-22T18:58:02Z

test/functional/view.test.js

@@ -58,6 +58,7 @@ describe('Views', function() {
          ) {
            expect(r).to.exist;
            expect(err).to.not.exist;
+            delete commandResult.apiVersion;


why do we want to delete this key instead of checking for it? just for the sake of the deep equal comparison? it could be misleading in terms of the expected behavior

I wasn't sure about the best approach to take here. Below we currently use a deep equal check on the commandResult, which may or may not contain apiVersion depending on how the test is run, e.g. the presence of the MONGODB_API_VERSION environment variable.

We could use containsSubset instead of doing a deep equal check, but that allows the presence of extraneous keys we don't actually want to be there. Deleting the apiVersion key (which is otherwise irrelevant to this test) seemed like a reasonable compromise, but I'm open to better ideas! Or perhaps just adding a comment would suffice.

so there's a couple of different approaches possible here:

if it is possible to know what the expected apiVersion is based on the test context, then we can optionally assert on it by pulling the expected object out into its own const expectedResult = { create, viewOn, pipeline } and then assigning the apiVersion with the expected value if we expect there to be one;

if it is possible to know whether or not the apiVersion is set at all, but not what it's set to, we could use the keys assertion (expect(obj).to.have.all.keys([...])) to check that all expected keys are present and then assert on the 3 that we know one at a time;

if it is not possible to know whether the apiVersion is even set, then just a comment to that end would be fine

Went with option 1 👍

test/functional/crud_spec.test.js

dariakp · 2021-06-22T19:01:52Z

test/functional/cursor.test.js

@@ -4117,59 +4119,6 @@ describe('Cursor', function() {
    }
  });

-  it('Correctly decorate the collection cursor count command with skip, limit, hint, readConcern', {


why was this test deleted?

Currently taking another look at this test actually, deleting it may have been a mistake.

Yeah it was a bug that was causing this test to fail, fixed now.

…propagate (#2852)

dariakp

Thanks for all the test updates!

…d-api-backport

emadum added the wip label Jun 16, 2021

emadum changed the base branch from 3.7 to 3.6 June 16, 2021 20:02

emadum changed the base branch from 3.6 to 3.7 June 16, 2021 20:02

feat(NODE-3191): backport versioned api

2862914

emadum force-pushed the NODE-3191/3.7/versioned-api-backport branch from 0d7f8ac to 2862914 Compare June 17, 2021 16:20

emadum requested a review from nbbeeken June 17, 2021 20:27

emadum marked this pull request as ready for review June 17, 2021 20:27

emadum removed the wip label Jun 17, 2021

nbbeeken requested changes Jun 17, 2021

View reviewed changes

emadum force-pushed the NODE-3191/3.7/versioned-api-backport branch from f2a987d to 8fd0886 Compare June 21, 2021 15:04

nbbeeken requested changes Jun 21, 2021

View reviewed changes

test/functional/unified-spec-runner/entities.ts Outdated Show resolved Hide resolved

nbbeeken approved these changes Jun 21, 2021

View reviewed changes

nbbeeken requested review from dariakp and durran June 21, 2021 21:29

nbbeeken added the Team Review Needs review from team label Jun 21, 2021

emadum commented Jun 22, 2021

View reviewed changes

code review

7e5b14a

emadum force-pushed the NODE-3191/3.7/versioned-api-backport branch from 9cade0d to 7e5b14a Compare June 22, 2021 14:41

test(NODE-2856): ensure defaultTransactionOptions get used from sessi…

68b4665

…on (#2845)

durran approved these changes Jun 22, 2021

View reviewed changes

dariakp reviewed Jun 22, 2021

View reviewed changes

dariakp and others added 4 commits June 24, 2021 08:51

refactor(NODE-3356): Update command monitoring logging (#2853)

b98f206

fix(NODE-2035): Exceptions thrown from awaited cursor forEach do not …

a917dfa

…propagate (#2852)

restore deleted test, fix count bug

3446724

review feedback

caab8f1

emadum requested a review from dariakp June 24, 2021 23:53

dariakp approved these changes Jun 25, 2021

View reviewed changes

Merge remote-tracking branch 'origin/3.6' into NODE-3191/3.7/versione…

346b961

…d-api-backport

emadum merged commit 93a47fd into 3.7 Jun 25, 2021

emadum deleted the NODE-3191/3.7/versioned-api-backport branch June 25, 2021 16:07

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat(NODE-3191): backport versioned api #2850

feat(NODE-3191): backport versioned api #2850

emadum commented Jun 16, 2021 •

edited

nbbeeken left a comment

nbbeeken left a comment

emadum Jun 22, 2021 •

edited

dariakp left a comment

dariakp Jun 22, 2021

emadum Jun 23, 2021 •

edited

dariakp Jun 23, 2021

emadum Jun 24, 2021

dariakp Jun 22, 2021

emadum Jun 23, 2021

emadum Jun 24, 2021

dariakp left a comment

feat(NODE-3191): backport versioned api #2850

feat(NODE-3191): backport versioned api #2850

Conversation

emadum commented Jun 16, 2021 • edited

nbbeeken left a comment

Choose a reason for hiding this comment

nbbeeken left a comment

Choose a reason for hiding this comment

emadum Jun 22, 2021 • edited

Choose a reason for hiding this comment

dariakp left a comment

Choose a reason for hiding this comment

dariakp Jun 22, 2021

Choose a reason for hiding this comment

emadum Jun 23, 2021 • edited

Choose a reason for hiding this comment

dariakp Jun 23, 2021

Choose a reason for hiding this comment

emadum Jun 24, 2021

Choose a reason for hiding this comment

dariakp Jun 22, 2021

Choose a reason for hiding this comment

emadum Jun 23, 2021

Choose a reason for hiding this comment

emadum Jun 24, 2021

Choose a reason for hiding this comment

dariakp left a comment

Choose a reason for hiding this comment

emadum commented Jun 16, 2021 •

edited

emadum Jun 22, 2021 •

edited

emadum Jun 23, 2021 •

edited