Automattic / mongoose

MongoDB object modeling designed to work in an asynchronous environment.
https://mongoosejs.com
MIT License
26.93k stars 3.84k forks source link

StrictModeError: Path "$comment" is not in schema, strict mode is `true`, and upsert is `true` #14576

Closed spaiz closed 5 months ago

spaiz commented 5 months ago

Prerequisites

Mongoose version

5.9.7

Node.js version

12.x

MongoDB server version

3.5.5

Typescript version (if applicable)

No response

Description

Upgraded Mongoose to the latest 5.* version and tests failed. After diving in, I did find a breaking change which doesn't make sense to me.

This new behaviour was introduced in mongoose v5.9.7 and I think it was in this PR - https://github.com/Automattic/mongoose/issues/8698

I didn't expect the $commentto be interpreted as a field of the model.

From MongoDB documentation (https://www.mongodb.com/docs/manual/reference/operator/query/comment/)

You can use the $comment with any expression taking a query predicate, such as the query predicate in db.collection.updateOne() or in the $match stage of the aggregation pipeline. For an example, see Attach a Comment to an Aggregation Expression.

  1. What do you think?
  2. Is there any "quick fix" for that?
  3. Am I wrong in my expectations?

You can check that in mongoose v5.9.5 it will work and you will see in db logs the queries:

mongodb-ba2813e1-1850-3951-aa55-4836c44447a1-0-mongodb  | {"t":{"$date":"2024-05-07T14:29:10.190+00:00"},"s":"I",  "c":"WRITE",    "id":51803,   "ctx":"conn11824","msg":"Slow query","attr":{"type":"update","ns":"test.users","command":{"q":{"name":"Alex","$comment":"some comment"},"u":{"$set":{"nickname":"aa","name":"Alex"}},"multi":false,"upsert":true},"planSummary":"COLLSCAN","keysExamined":0,"docsExamined":0,"nMatched":0,"nModified":0,"nUpserted":1,"keysInserted":1,"numYields":0,"queryHash":"01AEE5EC","planCacheKey":"C04BB947","locks":{"ParallelBatchWriterMode":{"acquireCount":{"r":1}},"FeatureCompatibilityVersion":{"acquireCount":{"w":1}},"ReplicationStateTransition":{"acquireCount":{"w":1}},"Global":{"acquireCount":{"w":1}},"Database":{"acquireCount":{"w":1}},"Collection":{"acquireCount":{"w":1}},"Mutex":{"acquireCount":{"r":1}}},"flowControl":{"acquireCount":1,"timeAcquiringMicros":3},"storage":{},"remote":"192.168.65.1:37725","durationMillis":2}}
mongodb-ba2813e1-1850-3951-aa55-4836c44447a1-0-mongodb  | {"t":{"$date":"2024-05-07T14:29:10.190+00:00"},"s":"I",  "c":"COMMAND",  "id":51803,   "ctx":"conn11824","msg":"Slow query","attr":{"type":"command","ns":"test.$cmd","command":{"update":"users","updates":[{"q":{"name":"Alex","$comment":"some comment"},"u":{"$set":{"nickname":"aa","name":"Alex"}},"multi":false,"upsert":true}],"ordered":true,"lsid":{"id":{"$uuid":"38e95540-eb16-42c5-a13d-e269d336ed8d"}},"$clusterTime":{"clusterTime":{"$timestamp":{"t":1715092146,"i":1}},"signature":{"hash":{"$binary":{"base64":"AAAAAAAAAAAAAAAAAAAAAAAAAAA=","subType":"0"}},"keyId":0}},"$db":"test"},"numYields":0,"reslen":296,"locks":{"ParallelBatchWriterMode":{"acquireCount":{"r":1}},"FeatureCompatibilityVersion":{"acquireCount":{"r":1,"w":1}},"ReplicationStateTransition":{"acquireCount":{"w":2}},"Global":{"acquireCount":{"r":1,"w":1}},"Database":{"acquireCount":{"w":1}},"Collection":{"acquireCount":{"w":1}},"Mutex":{"acquireCount":{"r":1}}},"flowControl":{"acquireCount":1,"timeAcquiringMicros":3},"storage":{},"remote":"192.168.65.1:37725","protocol":"op_msg","durationMillis":2}}

Steps to Reproduce

nvm use 12
npm i mongoose@5.9.7
const mongoose = require('mongoose');
const { Schema } = mongoose;

mongoose.connect('mongodb://localhost:27017/test', { useNewUrlParser: true, });

const userSchema = new Schema({
    name: { type: String },
    nickname: { type: String },
});

const User = mongoose.model('User', userSchema);

run().catch(console.error);

async function run () {
    await User.bulkWrite([
        {
            updateOne: {
                filter: { name: 'Alex', $comment: "some comment" },
                update: { name: "Alex", nickname: "aa" },
                upsert: true
            }
        }
    ]);
}

As result, it will fail with

MongooseError [StrictModeError]: Path "$comment" is not in schema, strict mode is `true`, and upsert is `true`.

Expected Behavior

As per mongodb documentation, $comment should be passed to MongoDB and it will be logged.

P.S. Newer versions are broken as well. Tested: 8.3.4, 7.6.11, 6.12.8

spaiz commented 5 months ago

UPD Checked non bulk query... and looks like this behaviour on regular updateOne existed for a long time :/

    await User.updateOne({ name: 'Alex', $comment: "hui" }, { name: "Alex", nickname: "aa" }, { upsert: true });

throws

MongooseError [StrictModeError]: Path "$comment" is not in schema, strict mode is `true`, and upsert is `true`.

We are lucky we don't use updateOne a lot in our code, and we have tests which did catch this behaviour on bulkWrite.

Maybe it should be fixed for all the cases? I mean, to allow passing $comment?

P.S. Funny thing, .comment() allows pass value, but it is not propagated to MongoDB (a least for v5.*)

    await User.updateOne({ name: 'Alex', }, { name: "Alex", nickname: "ddddddddd" }, { upsert: true }).comment("my comment");
IslandRhythms commented 5 months ago

As of March 1st, we no longer support support 5.x https://mongoosejs.com/docs/version-support.html

IslandRhythms commented 5 months ago

Didn't see that last part about the newer versions having the same issue. Sorry about that.

spaiz commented 5 months ago

@IslandRhythms np. Tnx!

I wanted to try to upgrade to a newer version, but the issue exists on all the versions :/

Tried to bypass it by using { strict: false } on the bulkWrite method, and actually my tests got broken... took me 2 hours to understand we have a bug... and with { strict: false } the wrong data would go to db... so I'm not fun of doing it now :/

Anyway, if it will be fixed on newer version, I'll have more reasons to push upgrade in production.

vkarpov15 commented 5 months ago

Fix will be in v6.12.9. Mongoose 5.x is no longer maintained, please check out out guide to migrating from Mongoose 5 to Mongoose 6.