Skip to content
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.

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

Filter is mutated in place by Query modifiers #14567

Closed
2 tasks done
sderrow opened this issue May 5, 2024 · 4 comments · Fixed by #14580
Closed
2 tasks done

Filter is mutated in place by Query modifiers #14567

sderrow opened this issue May 5, 2024 · 4 comments · Fixed by #14580
Labels
confirmed-bug We've confirmed this is a bug in Mongoose and will fix it.
Milestone

Comments

@sderrow
Copy link
Contributor

sderrow commented May 5, 2024

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.

@vkarpov15 vkarpov15 added this to the 8.3.5 milestone May 6, 2024
@vkarpov15 vkarpov15 added the has repro script There is a repro script, the Mongoose devs need to confirm that it reproduces the issue label May 6, 2024
@IslandRhythms IslandRhythms added confirmed-bug We've confirmed this is a bug in Mongoose and will fix it. and removed has repro script There is a repro script, the Mongoose devs need to confirm that it reproduces the issue labels May 7, 2024
@IslandRhythms
Copy link
Collaborator

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
Copy link
Collaborator

@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
Copy link
Collaborator

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

@sderrow
Copy link
Contributor Author

sderrow commented May 9, 2024

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

vkarpov15 added a commit that referenced this issue May 11, 2024
fix(query): shallow clone $or, $and if merging onto empty query filter
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
confirmed-bug We've confirmed this is a bug in Mongoose and will fix it.
Projects
None yet
Development

Successfully merging a pull request may close this issue.

3 participants