Filter is mutated in place by Query modifiers

[sderrow]

Prerequisites

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

Mongoose version

8.3.3

Node.js version

18.8.0

MongoDB server version

7.0

Typescript version (if applicable)

4.5.2

Description

Mongoose doesn't clone the source when using merge, so filter query inputs that have objects for values at the top level are modified in place by query modifiers like .and, .or, .where, etc., and those changes persist because of JS's call-by-sharing.

This is such core behavior that maybe we're doing it on purpose, but I find it unintuitive that Mongoose is taking my filter input and mutating it so it might end up as something else when I go to use the same input.

Is there a reason we don't just do source = clone(source) or something at the top of Query.prototype.merge to protect against errant mutations? Is it a performance thing?

Fundamentally, this is the specific problem line. When a key in from doesn't exist in to, we just copy over the entire value from from into to. When that value is an object itself, it's just an address in memory, so it's literally the same object that gets copied. Later on, when the query is mutating its own conditions (rightfully so), this means that the initial externally-provided query also gets changed.

Steps to Reproduce

export const queryMergeDemo = async (): Promise<$TSFixMe> => {
  /********************************* SETUP *********************************/
  type Person = {
    _id: mongoose.Types.ObjectId;

    name: string;
    age: number;
    active: boolean;

    createdAt: number;
    updatedAt: number;
  };

  const schema = new mongoose.Schema<Person>({
    name: { type: String, required: true },
    age: { type: Number, required: true },
    active: { type: Boolean, required: true, default: true },

    createdAt: { type: Number },
    updatedAt: { type: Number },
  });

  const M = mongoose.model<Person>("Person", schema);

  await M.create([
    { name: "Alice", age: 30 },
    { name: "Bob", age: 27 },
    { name: "Charlie", age: 25 },
    { name: "David", age: 8 },
    { name: "Eve", age: 20, active: false },
  ]);

  /********************************* EXAMPLE #1 *********************************/
  const activeQuery: FilterQuery<Person> = { $and: [{ active: true }] };

  const activeAdults = await M.countDocuments(activeQuery)
    .and([{ age: { $gte: 18 } }])
    .exec();
  // EXPECTED: activeAdults === 3
  // ACTUAL: activeAdults === 3

  const allActive = await M.countDocuments(activeQuery).exec();
  // EXPECTED: allActive === 4
  // ACTUAL: allActive === 3

  /********************************* EXAMPLE #2 *********************************/
  const adultQuery: FilterQuery<Person> = { age: { $gte: 18 } };

  const youngAdults = await M.countDocuments(adultQuery).where("age").lte(25).exec();
  // EXPECTED: youngAdults === 2
  // ACTUAL: youngAdults === 2

  const allAdults = await M.countDocuments(adultQuery).exec();
  // EXPECTED: allAdults === 4
  // ACTUAL: allAdults === 2

  /********************************* CLEANUP *********************************/
  await M.db.dropCollection("people");

  return { activeAdults, allActive, youngAdults, allAdults };
};

Expected Behavior

See comments in the code block above for two different examples with expectation vs actual.

[IslandRhythms]

const mongoose = require('mongoose');
const assert = require('assert');

const schema = new mongoose.Schema({
  name: { type: String, required: true },
  age: { type: Number, required: true },
  active: { type: Boolean, required: true, default: true },

  createdAt: { type: Number },
  updatedAt: { type: Number },
});

const M = mongoose.model("Person", schema);


async function run() {
  await mongoose.connect('mongodb://localhost:27017');
  await mongoose.connection.dropDatabase();
  await M.create([
    { name: "Alice", age: 30 },
    { name: "Bob", age: 27 },
    { name: "Charlie", age: 25 },
    { name: "David", age: 8 },
    { name: "Eve", age: 20, active: false },
  ]);
  
  /********************************* EXAMPLE #1 *********************************/
  const activeQuery = { $and: [{ active: true }] };
  
  const activeAdults = await M.countDocuments(activeQuery)
    .and([{ age: { $gte: 18 } }])
  // EXPECTED: activeAdults === 3
  // ACTUAL: activeAdults === 3
  assert.equal(activeAdults, 3)
  const allActive = await M.countDocuments(activeQuery).exec();
  // EXPECTED: allActive === 4
  // ACTUAL: allActive === 3
  assert.equal(allActive, 4)
  /********************************* EXAMPLE #2 *********************************/
  const adultQuery = { age: { $gte: 18 } };
  
  const youngAdults = await M.countDocuments(adultQuery).where("age").lte(25);
  // EXPECTED: youngAdults === 2
  // ACTUAL: youngAdults === 2
  assert.equal(youngAdults, 2)
  
  const allAdults = await M.countDocuments(adultQuery);
  // EXPECTED: allAdults === 4
  // ACTUAL: allAdults === 2
  assert.equal(allAdults, 4)
}

run();

[vkarpov15]

@sderrow I opened up a PR with a partial fix #14580. Re: clone(source), we'd prefer not to do that for performance reasons, but we may have to because #14580 doesn't fix your example 2. Will look into it some more.

[vkarpov15]

@sderrow WDYT of #14580 now? Handles both cases you mentioned.

[sderrow]

@vkarpov15 Nice! Thanks for tackling quickly. I left a couple comments on the PR.