serious: deleteOne({id:null}), deleteOne({mid:"628c6582f24dcd25814c4f0d") and deleteOne() deletes random entry

[thiscantbeserious]

Prerequisites

• [X] I have written a descriptive issue title
• [X] I have searched existing issues to ensure the bug has not already been reported

Mongoose version

6.7.2

Node.js version

16.17.0

MongoDB server version

4.12.0

Typescript version (if applicable)

No response

Description

deleteOne with invalid parameters - be it an id that is accidently null or an misspelled id property leads to a query that actually will delete an entry instead of erroring out (the last?).

This can lead to deletion of entire production collections being wiped of the earth - I don't think thats anywhere near smart.

In case of memory leak, processing error, or anything else that wasn't catched - say for example params is an object that somebody or something changed.

Steps to Reproduce

let paramsFromExternalFunction = { renamedPropertyThatDoesntExist: false }
await LocationModel.deleteOne(paramsFromExternalFunction).exec(); //deletes randomly
await LocationModel.deleteOne({id:null}).exec(); //deletes randomly
await LocationModel.deleteOne().exec(); //arguable, also deletes randomly

Expected Behavior

A destructive method should only execute on code that absolutely matches the conditions and parameters.

If there is a non-existing parameter - be it in the model or database it should simply do nothing and/or throw an Error.

That is by sane defaults - not having to enable non-discarding of parameters that doesn't exist.

[Uzlopak]

Well. This is also affected by strictQuery filtering out invalid fields. So a virtual field like id will be potentially filtered out, resulting in a deleteOne({}) call,deleting a random document.

Anyway. I think this is something which is happening at runtime. In my opinion this has to be covered by unit tests of the implememtation.

If you have a technical solution to your issue, we can consider this to implement. Or you provide a PR with the solution.

But this is a normal behavior.

[sean-daley]

If related to strictQuery (which seems likely), additional info on this can be found in https://github.com/Automattic/mongoose/issues/10747 and https://github.com/Automattic/mongoose/issues/12389

Those are the delete specific ones. There are other issues which go into more detail around find as well.

https://github.com/Automattic/mongoose/issues/11861 also goes over the plan to try and change this default for Mongoose 7.

[vipul-sharma-deshaw]

Hi @thiscantbeserious, I suppose you can globally disable this behavior by setting mongoose.set('strictQuery', false); as documented here. However, I too find this weird and ideally instead of having to globally unset this, it should be like https://github.com/Automattic/mongoose/issues/11861.

[Uzlopak]

@DeviantCTO

Kind of harsh to claim that it I gave an unprofessional answer. I did not even blame the OP and gave just a short technical analysis and opinion of the behaviour. I assume that @thiscantbeserious works for you, and you are not satisfied with the first reaction.

strictQuery was introduced with mongoose v6 to ensure that the mongoose Schema gets strictly respected and avoiding storing data in fields, which are not specified in the Schema.

But anyhow you have been given a "proper" answer by @sean-daley, who wrote that you can disable strictQuery. Even @vipul-sharma-deshaw gave you the corresponding codeline to disable strictQuery. Also you have been informed, that we will set strictQuery to false in mongoose v7 as it would be backwards breaking.

So no need to wrap it in a function to check for fields. Just do mongoose.set('strictQuery', false);

I hope CTOs, Techleads and Architects all over the world can now sleep again calm. ;)

I sent a networking request on linkedin, if you want to talk.

[Uzlopak]

Duplicate of #11861

[sean-daley]

So while mongoose definitely has issues around the whole strictQuery thing, I think mongo itself will delete a random entry when passed in {id: null} or empty. The fact that deleteOne({mid:"xxxx"}) converts to deleteOne({}) if mid is not a valid field is a mongoose issue related to the strictQuery setting. But, at the very least, using the mongo shell, deleteOne has the following behavior:

> db.books.count()
2
> db.books.deleteOne({ unknownField: 'some_value' })
{ "acknowledged" : true, "deletedCount" : 0 }
> db.books.deleteOne({ unknownField: null })
{ "acknowledged" : true, "deletedCount" : 1 }
> db.books.deleteOne({})
{ "acknowledged" : true, "deletedCount" : 1 }

So, in the above, I have a collection with 2 records in it.

1. I do deleteOne({ unknownField: 'some_value' }) and nothing gets deleted ... seems fine
2. I do deleteOne({ unknownField: null }) and a "random" item gets deleted ... seems weird probably?
3. I do deleteOne({}) and a "random" item gets deleted ... may seem weird but it's documented behavior by MongoDB that an empty filter will delete the first document returned in the collection

I can only assume that either 2 is a bug in mongo or it's some expected behavior somehow. You'd have to ask those folks. Nothing pops out immediately in the docs on it.

I just wanted to write this as I don't think any strictQuery setting in mongoose will help all of the issues discussed here as even mongo will delete the first item in the collection when passed {} or {id: null}. I didn't do extensive testing here though so i may be missing something but I noticed that {id: null} would delete an item even when using mongoose 5 which should have the old strictQuery behavior I think. That got me looking at what the Mongo shell does on a deleteOne as well.

[Uzlopak]

Usually you would not fire and forget the mongo operation. You would call the operation and check the result of the operation. So if you would do a deleteOne with a valid query and get a deletedCount: 0 you could throw a not found error, if you want. This is also applicable for the mongo native driver. That is the reason why you get the result-document from mongo.

It is also not always desired to throw errors. E.g. in high performance systems. A Javascript native Error Object has also the stacktrace attached, which takes some cycles to be produced.

Also the value null can be serialized to bson. But mongo operations thread null differently. null means in find() all documents where the field is either null or not existing. So when we do a find() for documents with id: null, and no document exists with that field, than all documents are returned.

You actually would get an idea about this behaviour when using a findOne(). You should be confused when you your tests and always get the same document.

Also findOne also doesnt throw an Error, when the document does not exist.

[vkarpov15]

We're very sorry for any trouble this behavior change caused. We're switching this behavior back in Mongoose 7 with #11861

[thiscantbeserious]

@vkarpov15 thanks for the response, hopefully version 7.0.0 will happen soon ... one question: aren't important bugfixes merged down to older version or is the versioning schema of mongoose not structured like that (simply don't know it)?

@sean-daley

Doing a deleteOne({_id:null}) with the underlying mongodb-driver directly does not exhibit that behaviour (with the listed mongodb version above). However you are right in that there are circumstances where not throwing an error might be feasible, or even contraproductive.

@Uzlopak

It is also not possible to catch that behaviour by unit-tests as deleteOne only responds with the number of deleted entries -

0/1. So you deleted something or not.

Specifically saying - where is the unit-test within mongoose for that implementation change that catched this? (trick question)

Unit-Tests are written for the implementation they are targeting - so that goes with the changes that strictQuery/strict introduced.

They do not have access to the database, or just a mocked version of it. They are targed at catching implementation faults not related to the frameworks/libraries you're using but to make sure the results of your business implementation are somewhat sane.

They are not the answer to everything.

From how I'm seeing it this is the state since over 1 year now (if I look at the cross-mentioned issues above).

I don't have the time or desire to proof it because its no use to anyone, so I'm just leaving this as a side-note.

This is certainly not "normal behaviour" ... that's also why I took the time to report this here to begin with.

[Uzlopak]

@thiscantbeserious

Replace "normal behaviour" with "expected behaviour". Also you got the answer, that strictQuery will be set to false by default on mongoose 7. It is not a bug per se, if it is expected behaviour.

I quite dont catch your remarks. When I develop for a mongo database, I actually spawn a mongodb-memory-server and test against a real mongodb. This is also what we do with mongoose. We spawn a mongo server and check against a real database. For local development you can do a "npm run mongo" and spawn locally a mongo server. Then run the tests against it. So we actively avoid mocking in our unit tests. We can even specify a version of mongodb by doing e.g. "npm run mongo -- 5.0.0".

And this is also what we did at my former employer. We spawned mongo databases, run the unit tests against them and actually checked if the changes are really done on the database and we ensured we did not made faulty queries, like what happened in your case.

[thiscantbeserious]

@Uzlopak you can't really tell me that .deleteOne({id:null}) as to where null is considered expected === sane behaviour that it deletes a random/last entry.

The MongoDB-Driver itself also doesn't do that.

It litterally goes AGAINST what the very word STRICT defines in any textbook.

(STRICT = as in doesn't match the desired input parameters -> discard, do nothing or throw an error, don't allow it)

You are litterally the only one trying to argue that it's sane - as in expected - to do that.

So now by the very next change in 7.0.0 its set to "false" as default to get back the old behaviour.

So we're saying strictQuery === false is the new default but that is still not actually correct in terminology alone (what the underlying logic does put aside).

Why are you trying so hard to argue around the problem itself?

This is not about any kind of internal testing structure that I never mentioned or you actually don't know about (albeit giving tips, appreciated - but still does nothing to solve anything regards the issue itself)

And: Did you actually fire up your editor and tested the respective code-parts yourself or are you trusting your "tests"?

Because that's my experience lately with people not matter how big projects are: They just throw away the responsibilities and defend with word or experiences that don't match the very simple case given.

We can agree, to not aggree on the responsibility part as well how sanity looks like.

For me thats more or less a thing of personal attitude.

I wish you the very best and that you never get anything like that to happen to you or your business/customers.

Over and out. Edit: As in nothing constructive to add - also in regards to discussion whom is right or wrong (so I'm dropping out a here).

[Uzlopak]

Mistyping _id and id is not our fault. And even with native mongo doing a { id: null } would delete a random document. See above for the proof

[sean-daley]

The MongoDB-Driver itself also doesn't do that.

As I showed earlier, the mongo shell DOES do that. If you do db.deleteOne({id: null}) it will delete a "random" item in your collection. The javascript driver does the same:

const mongodb = require('mongodb')
// Replace the uri string with your MongoDB deployment's connection string.
const uri = "mongodb://127.0.0.1:27017";
const client = new mongodb.MongoClient(uri);
const run = async () => {
  try {
    const database = client.db("sample_mflix");
    const haiku = database.collection("haiku");
    // create a document to insert
    const doc = {
      title: "Record of a Shriveled Datum",
      content: "No bytes, no problem. Just insert a document, in MongoDB",
    }
    const result = await haiku.insertOne(doc);
    console.log(`Record count = ${await haiku.count()}`)
    await haiku.deleteOne({ id: null });
    console.log(`Record count = ${await haiku.count()}`)

  } finally {
    await client.close();
  }
}
run().catch(console.dir);

If you run that (at least against 4.4) you will get:

Record count = 1
Record count = 0

And there is nothing mongoose related in that program. It purely uses the mongodb driver.