feat(NODE-4684)!: remove collection insert, update, remove methods #3500

nbbeeken · 2022-12-21T22:37:33Z

Description

What is changing?

Removes the legacy crud methods.

What is the motivation for this change?

Modernize code base, remove duplicate code.

Double check the following

TODO:

Migration guide
Feedback on shimming old methods

Ran npm run check:lint script
Self-review completed using the steps outlined here
PR title follows the correct format: type(NODE-xxxx)[!]: description
- Example: feat(NODE-1234)!: rewriting everything in coffeescript
Changes are covered by tests
New TODOs have a related JIRA ticket

nbbeeken · 2023-01-03T20:55:31Z

I collected the usage counts of the legacy methods in this patch.

4.0-replica_set reports: { insert: 326, update: 19, remove: 9 } so I can probably look for the removes and maybe the updates, but the inserts are many 😅

nbbeeken · 2023-01-05T22:24:34Z

From slack: Looking into search and replace solutions, there wasn't an approach that would make a significant enough impact in the number of insert usages. We'll defer the work to clean this up to a later time

nbbeeken · 2023-01-06T19:47:29Z

test/integration/crud/crud_api.test.ts

@@ -655,71 +655,6 @@ describe('CRUD API', function () {
    }
  });

-  it('should correctly execute remove methods using crud api', {


Wasn't using .remove even and is covered by CRUD spec tests

nbbeeken · 2023-01-06T19:48:33Z

test/integration/crud/find.test.js

 const { expect } = require('chai');
 const sinon = require('sinon');
 const { setTimeout } = require('timers');
 const { Code, ObjectId, Long, Binary, ReturnDocument } = require('../../../src');

 describe('Find', function () {
-  before(function () {
-    return setupDatabase(this.configuration);
+  let client;


introducing mocha hooks cus there was a close leak, classic callback didn't reach the end stuff

Why did these leaks only show up now? We didn't change any codepaths so I would expect no change in test behavior.

This is one of those drive by cleanups that I thought we've been trying to do to the tests. There isn't a new persistent leak, it's just when some of the tests in this file were failing with my first pass at doing this work they crash, but in EVG you don't learn about the crash until 20mins later cus of the client that never gets closed. The tests are fixed now so they could continue to create and close the client but we know the hooks provide better DX.

nbbeeken · 2023-01-06T19:48:42Z

test/integration/crud/find.test.js

-              function (err, r) {
-                // Fetch the id
-                var id = r.insertedIds[0];
+      var db = client.db(configuration.db);


nbbeeken · 2023-01-06T19:48:48Z

test/integration/crud/find.test.js

-
-          transactions: transactions
-        };
+      var db = client.db(configuration.db);


leak, etc. I'll stop commenting each one

nbbeeken · 2023-01-06T19:50:41Z

test/integration/node-specific/auto_connect.test.ts

@@ -483,15 +483,6 @@ describe('When executing an operation for the first time', () => {
      });
    });

-    describe(`#insert()`, () => {


Don't need to check auto connect on APIs that don't exist

nbbeeken · 2023-01-06T19:50:49Z

test/integration/node-specific/operation_examples.test.ts

-   * example-class Collection
-   * example-method insert
-   */
-  it('Should correctly execute insert with keepGoing option on mongod >= 1.9.1 With Promises', {


keepGoing removed

nbbeeken · 2023-01-06T19:51:59Z

test/integration/objectid.test.js

-          number_of_tests_done++;
-        });
-      });
+  it('generates new ObjectId for documents without _id property', async function () {


It failed at first b/c of the insert usage, but then I also cleaned it up cus the setInterval logic was too interesting

test/tools/runner/hooks/legacy.ts

test/integration/collection-management/collection.test.ts

test/integration/crud/remove.test.js

baileympearson · 2023-01-09T17:25:10Z

test/integration/crud/find.test.js

 const { expect } = require('chai');
 const sinon = require('sinon');
 const { setTimeout } = require('timers');
 const { Code, ObjectId, Long, Binary, ReturnDocument } = require('../../../src');

 describe('Find', function () {
-  before(function () {
-    return setupDatabase(this.configuration);
+  let client;


Why did these leaks only show up now? We didn't change any codepaths so I would expect no change in test behavior.

baileympearson · 2023-01-09T17:25:31Z

etc/notes/CHANGES_5.0.0.md

+These legacy methods have been removed. `update` and `remove` invocations are identical to `updateMany` and `deleteMany` respectively.
+The `insert` method is equivalent to `insertMany` but the first argument **MUST** be an array.
+
+```ts
+// Single document insert:
+await collection.insert({ name: 'spot' });
+// Migration:
+await collection.insertMany([{ name: 'spot' }]);
+
+// Multi-document insert:
+await collection.insert([{ name: 'fido' }, { name: 'luna' }])
+// Migration:
+await collection.insertMany([{ name: 'fido' }, { name: 'luna' }])


just a thought - a table that has columns for legacy method / existing crud replacement might be an easier format for users to understand.

Ah, I was thinking something along the lines of

replacing 129-130 with something like "Three legacy crud helpers on the collection class have been removed:"

removing the typescript code example

putting code examples of before / after in the table, so users clearly see a mapping between the "old code" and "new code"

I had code samples when I first wrote the table but it isn't easy to read b/c of how wide it gets, it doesn't get the same scroll effect normal code blocks do. Let me fix up the text part and maybe move the code sample below the table

Pushed the change, also the bracket are bit hard to notice, "arrayOfDocuments" is obviously just a variable name that implies something. Maybe I should use type annotations instead?

W-A-James

Apart from the question I had about replacing instances of colllection.insert in tests that are already being modified, lgtm.

test/integration/crud/find.test.js

etc/notes/CHANGES_5.0.0.md

Co-authored-by: Bailey Pearson <[email protected]>

nbbeeken force-pushed the NODE-4684-rm-coll-insert-update-remove branch 2 times, most recently from d132852 to 672ddc1 Compare January 3, 2023 20:19

nbbeeken marked this pull request as ready for review January 3, 2023 20:21

nbbeeken force-pushed the NODE-4684-rm-coll-insert-update-remove branch from ab21337 to 340565a Compare January 5, 2023 22:23

nbbeeken changed the title ~~feat(NODE-4648)!: remove collection insert, update, remove methods~~ feat(NODE-4684)!: remove collection insert, update, remove methods Jan 5, 2023

baileympearson self-requested a review January 6, 2023 13:41

baileympearson added the Primary Review In Review with primary reviewer, not yet ready for team's eyes label Jan 6, 2023

nbbeeken added 5 commits January 6, 2023 13:52

feat(NODE-4648)!: remove collection insert, update, remove methods

ab9b240

test: remove remove usage

a75b4bb

assert usage doesn't increase

5088836

fix: lint

282dd49

fix: tests

d73ebba

nbbeeken force-pushed the NODE-4684-rm-coll-insert-update-remove branch from ab2f100 to d73ebba Compare January 6, 2023 18:52

nbbeeken added 2 commits January 6, 2023 13:52

remove keepGoing test

7f9e3b3

bump permitted legacy

672f183

nbbeeken commented Jan 6, 2023

View reviewed changes

baileympearson requested changes Jan 9, 2023

View reviewed changes

nbbeeken added 3 commits January 9, 2023 14:51

rename and test clean up

4c869e1

docs: add migration table

1a87dae

Merge branch 'main' into NODE-4684-rm-coll-insert-update-remove

9dd803c

nbbeeken requested a review from baileympearson January 9, 2023 19:59

W-A-James reviewed Jan 9, 2023

View reviewed changes

test/integration/crud/find.test.js Show resolved Hide resolved

docs: update

c360585

baileympearson reviewed Jan 10, 2023

View reviewed changes

etc/notes/CHANGES_5.0.0.md Outdated Show resolved Hide resolved

nbbeeken and others added 2 commits January 10, 2023 16:56

Merge branch 'main' into NODE-4684-rm-coll-insert-update-remove

76662d5

Update etc/notes/CHANGES_5.0.0.md

29c36ce

Co-authored-by: Bailey Pearson <[email protected]>

baileympearson approved these changes Jan 11, 2023

View reviewed changes

baileympearson merged commit 14427d1 into main Jan 11, 2023

baileympearson deleted the NODE-4684-rm-coll-insert-update-remove branch January 11, 2023 14:58

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat(NODE-4684)!: remove collection insert, update, remove methods #3500

feat(NODE-4684)!: remove collection insert, update, remove methods #3500

nbbeeken commented Dec 21, 2022 •

edited

Loading

nbbeeken commented Jan 3, 2023

nbbeeken commented Jan 5, 2023

nbbeeken Jan 6, 2023

nbbeeken Jan 6, 2023

baileympearson Jan 9, 2023

nbbeeken Jan 9, 2023

nbbeeken Jan 6, 2023

nbbeeken Jan 6, 2023

nbbeeken Jan 6, 2023

nbbeeken Jan 6, 2023

nbbeeken Jan 6, 2023

baileympearson Jan 9, 2023

baileympearson Jan 9, 2023

nbbeeken Jan 9, 2023

baileympearson Jan 9, 2023

nbbeeken Jan 9, 2023

nbbeeken Jan 9, 2023

W-A-James left a comment

feat(NODE-4684)!: remove collection insert, update, remove methods #3500

feat(NODE-4684)!: remove collection insert, update, remove methods #3500

Conversation

nbbeeken commented Dec 21, 2022 • edited Loading

Description

What is changing?

What is the motivation for this change?

Double check the following

nbbeeken commented Jan 3, 2023

nbbeeken commented Jan 5, 2023

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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

W-A-James left a comment

Choose a reason for hiding this comment

nbbeeken commented Dec 21, 2022 •

edited

Loading