Skip to content

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

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.

Sign up for GitHub

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

Merged
merged 13 commits into from
Jan 11, 2023
Merged

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

Show file tree
Hide file tree
Changes from 10 commits
Commits
Show all changes
13 commits
Select commit Hold shift + click to select a range
ab9b240
feat(NODE-4648)!: remove collection insert, update, remove methods
nbbeeken Dec 21, 2022
a75b4bb
test: remove remove usage
nbbeeken Jan 3, 2023
5088836
assert usage doesn't increase
nbbeeken Jan 5, 2023
282dd49
fix: lint
nbbeeken Jan 5, 2023
d73ebba
fix: tests
nbbeeken Jan 6, 2023
7f9e3b3
remove keepGoing test
nbbeeken Jan 6, 2023
672f183
bump permitted legacy
nbbeeken Jan 6, 2023
4c869e1
rename and test clean up
nbbeeken Jan 9, 2023
1a87dae
docs: add migration table
nbbeeken Jan 9, 2023
9dd803c
Merge branch 'main' into NODE-4684-rm-coll-insert-update-remove
nbbeeken Jan 9, 2023
c360585
docs: update
nbbeeken Jan 9, 2023
76662d5
Merge branch 'main' into NODE-4684-rm-coll-insert-update-remove
nbbeeken Jan 10, 2023
29c36ce
Update etc/notes/CHANGES_5.0.0.md
nbbeeken Jan 10, 2023
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
28 changes: 28 additions & 0 deletions etc/notes/CHANGES_5.0.0.md
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -124,3 +124,31 @@ cursor.closed // true

Everywhere the driver sends a `hello` command (initial handshake and monitoring), it will now pass the command value as `1` instead of the
previous `true` for spec compliance.
### Removed `Collection.insert`, `Collection.update`, and `Collection.remove`

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' }])
Copy link
Contributor

@baileympearson baileympearson Jan 9, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Contributor Author

@nbbeeken nbbeeken Jan 9, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

done!

Copy link
Contributor

@baileympearson baileympearson Jan 9, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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"

Copy link
Contributor Author

@nbbeeken nbbeeken Jan 9, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Contributor Author

@nbbeeken nbbeeken Jan 9, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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?

```

| Removed API | API Migration |
|------------------------------------------------|----------------------------------------------------|
| `insert(document)` | `insertOne(document)` |
| `insert(arrayOfDocuments)` | `insertMany(arrayOfDocuments)` |
| `update(filter)` | `updateMany(filter)` |
| `remove(filter)` | `deleteMany(filter)` |

### Removed `keepGoing` option from `BulkWriteOptions`

The `keepGoing` option was a legacy name for setting `ordered` to `false` for bulk inserts.
It was only supported by the legacy `collection.insert()` method which is now removed as noted above.
71 changes: 0 additions & 71 deletions src/collection.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1630,77 +1630,6 @@ export class Collection<TSchema extends Document = Document> {
return this.s.db.s.logger;
}

/**
* Inserts a single document or a an array of documents into MongoDB. If documents passed in do not contain the **_id** field,
* one will be added to each of the documents missing it by the driver, mutating the document. This behavior
* can be overridden by setting the **forceServerObjectId** flag.
*
* @deprecated Use insertOne, insertMany or bulkWrite instead. Callbacks are deprecated and will be removed in the next major version. See [mongodb-legacy](https://github.com/mongodb-js/nodejs-mongodb-legacy) for migration assistance
* @param docs - The documents to insert
* @param options - Optional settings for the command
* @param callback - An optional callback, a Promise will be returned if none is provided
*/
insert(
docs: OptionalUnlessRequiredId<TSchema>[],
options: BulkWriteOptions,
callback: Callback<InsertManyResult<TSchema>>
): Promise<InsertManyResult<TSchema>> | void {
emitWarningOnce(
'collection.insert is deprecated. Use insertOne, insertMany or bulkWrite instead.'
);
if (typeof options === 'function') (callback = options), (options = {});
options = options || { ordered: false };
docs = !Array.isArray(docs) ? [docs] : docs;

return this.insertMany(docs, options, callback);
}

/**
* Updates documents.
*
* @deprecated use updateOne, updateMany or bulkWrite. Callbacks are deprecated and will be removed in the next major version. See [mongodb-legacy](https://github.com/mongodb-js/nodejs-mongodb-legacy) for migration assistance
* @param filter - The filter for the update operation.
* @param update - The update operations to be applied to the documents
* @param options - Optional settings for the command
* @param callback - An optional callback, a Promise will be returned if none is provided
*/
update(
filter: Filter<TSchema>,
update: UpdateFilter<TSchema>,
options: UpdateOptions,
callback: Callback<Document>
): Promise<UpdateResult> | void {
emitWarningOnce(
'collection.update is deprecated. Use updateOne, updateMany, or bulkWrite instead.'
);
if (typeof options === 'function') (callback = options), (options = {});
options = options ?? {};

return this.updateMany(filter, update, options, callback);
}

/**
* Remove documents.
*
* @deprecated use deleteOne, deleteMany or bulkWrite. Callbacks are deprecated and will be removed in the next major version. See [mongodb-legacy](https://github.com/mongodb-js/nodejs-mongodb-legacy) for migration assistance
* @param filter - The filter for the remove operation.
* @param options - Optional settings for the command
* @param callback - An optional callback, a Promise will be returned if none is provided
*/
remove(
filter: Filter<TSchema>,
options: DeleteOptions,
callback: Callback
): Promise<DeleteResult> | void {
emitWarningOnce(
'collection.remove is deprecated. Use deleteOne, deleteMany, or bulkWrite instead.'
);
if (typeof options === 'function') (callback = options), (options = {});
options = options ?? {};

return this.deleteMany(filter, options, callback);
}

/**
* An estimated count of matching documents in the db to a filter.
*
Expand Down
22 changes: 0 additions & 22 deletions test/integration/collection-management/collection.test.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -227,28 +227,6 @@ describe('Collection', function () {
);
});

it('should perform collection remove with no callback', function (done) {
const collection = db.collection('remove_with_no_callback_bug_test');
collection.insertOne({ a: 1 }, configuration.writeConcernMax(), err => {
expect(err).to.not.exist;
collection.insertOne({ b: 1 }, configuration.writeConcernMax(), err => {
expect(err).to.not.exist;
collection.insertOne({ c: 1 }, configuration.writeConcernMax(), err => {
expect(err).to.not.exist;
collection.remove({ a: 1 }, configuration.writeConcernMax(), err => {
expect(err).to.not.exist;
// Let's perform a count
collection.countDocuments((err, count) => {
expect(err).to.not.exist;
expect(count).to.equal(2);
done();
});
});
});
});
});
});

it('should correctly read back document with null', function (done) {
db.createCollection('shouldCorrectlyReadBackDocumentWithNull', {}, (err, collection) => {
// Insert a document with a date
Expand Down
65 changes: 0 additions & 65 deletions test/integration/crud/crud_api.test.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -655,71 +655,6 @@ describe('CRUD API', function () {
}
});

it('should correctly execute remove methods using crud api', {
Copy link
Contributor Author

@nbbeeken nbbeeken Jan 6, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

// Add a tag that our runner can trigger on
// in this case we are setting that node needs to be higher than 0.10.X to run
metadata: {
requires: { topology: ['single', 'replicaset', 'sharded', 'ssl', 'heap', 'wiredtiger'] }
},

test: function (done) {
client.connect(function (err, client) {
const db = client.db();

//
// Legacy update method
// -------------------------------------------------
const legacyRemove = function () {
deleteOne();
};

//
// Update one method
// -------------------------------------------------
const deleteOne = function () {
db.collection('t4_2').insertMany(
[{ a: 1 }, { a: 1 }],
{ writeConcern: { w: 1 } },
(err, r) => {
expect(err).to.not.exist;
expect(r).property('insertedCount').to.equal(2);

db.collection('t4_2').deleteOne({ a: 1 }, (err, r) => {
expect(err).to.not.exist;
expect(r).property('deletedCount').to.equal(1);

deleteMany();
});
}
);
};

//
// Update many method
// -------------------------------------------------
const deleteMany = function () {
db.collection('t4_3').insertMany(
[{ a: 1 }, { a: 1 }],
{ writeConcern: { w: 1 } },
(err, r) => {
expect(err).to.not.exist;
expect(r).property('insertedCount').to.equal(2);

db.collection('t4_3').deleteMany({ a: 1 }, (err, r) => {
expect(err).to.not.exist;
expect(r).property('deletedCount').to.equal(2);

client.close(done);
});
}
);
};

legacyRemove();
});
}
});

it('should correctly execute findAndModify methods using crud api', {
// Add a tag that our runner can trigger on
// in this case we are setting that node needs to be higher than 0.10.X to run
Expand Down
Loading