Skip to content

Commit c5393d1

Browse files
authored
Merge pull request #15367 from Automattic/vkarpov15/gh-15364
fix: clone POJOs and arrays when casting query filter to avoid mutating objects
2 parents ac0c086 + 14cbabd commit c5393d1

File tree

3 files changed

+130
-7
lines changed

3 files changed

+130
-7
lines changed

benchmarks/findOneWithCast.js

Lines changed: 69 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,69 @@
1+
'use strict';
2+
3+
const mongoose = require('../');
4+
5+
run().catch(err => {
6+
console.error(err);
7+
process.exit(-1);
8+
});
9+
10+
async function run() {
11+
await mongoose.connect('mongodb://127.0.0.1:27017/mongoose_benchmark');
12+
13+
const SchemaParticipant = new mongoose.Schema(
14+
{
15+
user: mongoose.Schema.Types.UUID,
16+
},
17+
{
18+
_id: false
19+
}
20+
);
21+
22+
const TestSchema = new mongoose.Schema(
23+
{
24+
participants1: { type: [SchemaParticipant] },
25+
participants2: { type: [SchemaParticipant] },
26+
participants3: { type: [SchemaParticipant] },
27+
participants4: { type: [SchemaParticipant] },
28+
participants5: { type: [SchemaParticipant] },
29+
date: { type: Number },
30+
},
31+
{
32+
collection: 'test_uuid_mutations',
33+
}
34+
);
35+
36+
const TestModel = mongoose.model('Test', TestSchema);
37+
38+
if (!process.env.MONGOOSE_BENCHMARK_SKIP_SETUP) {
39+
await TestModel.deleteMany({});
40+
}
41+
42+
const peer = {
43+
user: '1583b99d-8462-4343-8dfd-9105252e5662',
44+
};
45+
46+
const numIterations = 500;
47+
const queryStart = Date.now();
48+
for (let i = 0; i < numIterations; ++i) {
49+
for (let j = 0; j < 10; ++j) {
50+
await TestModel.findOne({
51+
$or: [
52+
{ participants1: { $elemMatch: peer } },
53+
{ participants2: { $elemMatch: peer } },
54+
{ participants3: { $elemMatch: peer } },
55+
{ participants4: { $elemMatch: peer } },
56+
{ participants5: { $elemMatch: peer } }
57+
]
58+
});
59+
}
60+
}
61+
const queryEnd = Date.now();
62+
63+
const results = {
64+
'Average findOne time ms': +((queryEnd - queryStart) / numIterations).toFixed(2)
65+
};
66+
67+
console.log(JSON.stringify(results, null, ' '));
68+
process.exit(0);
69+
}

lib/utils.js

Lines changed: 33 additions & 7 deletions
Original file line numberDiff line numberDiff line change
@@ -234,6 +234,38 @@ exports.omit = function omit(obj, keys) {
234234
return ret;
235235
};
236236

237+
/**
238+
* Simplified version of `clone()` that only clones POJOs and arrays. Skips documents, dates, objectids, etc.
239+
* @param {*} val
240+
* @returns
241+
*/
242+
243+
exports.clonePOJOsAndArrays = function clonePOJOsAndArrays(val) {
244+
if (val == null) {
245+
return val;
246+
}
247+
// Skip documents because we assume they'll be cloned later. See gh-15312 for how documents are handled with `merge()`.
248+
if (val.$__ != null) {
249+
return val;
250+
}
251+
if (isPOJO(val)) {
252+
val = { ...val };
253+
for (const key of Object.keys(val)) {
254+
val[key] = exports.clonePOJOsAndArrays(val[key]);
255+
}
256+
return val;
257+
}
258+
if (Array.isArray(val)) {
259+
val = [...val];
260+
for (let i = 0; i < val.length; ++i) {
261+
val[i] = exports.clonePOJOsAndArrays(val[i]);
262+
}
263+
return val;
264+
}
265+
266+
return val;
267+
};
268+
237269
/**
238270
* Merges `from` into `to` without overwriting existing properties.
239271
*
@@ -271,13 +303,7 @@ exports.merge = function merge(to, from, options, path) {
271303
continue;
272304
}
273305
if (to[key] == null) {
274-
if (isPOJO(from[key])) {
275-
to[key] = { ...from[key] };
276-
} else if (Array.isArray(from[key])) {
277-
to[key] = [...from[key]];
278-
} else {
279-
to[key] = from[key];
280-
}
306+
to[key] = exports.clonePOJOsAndArrays(from[key]);
281307
} else if (exports.isObject(from[key])) {
282308
if (!exports.isObject(to[key])) {
283309
to[key] = {};

test/model.query.casting.test.js

Lines changed: 28 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -826,6 +826,34 @@ describe('model query casting', function() {
826826
assert.equal(res.length, 2);
827827
assert.deepStrictEqual(res.map(doc => doc.arr[1].id), ['two', 'three']);
828828
});
829+
830+
it('should not mutate original object with UUID in query with $elemMatch (gh-15364)', async function() {
831+
const schemaParticipant = new Schema(
832+
{
833+
user: mongoose.Schema.Types.UUID
834+
},
835+
{ _id: false }
836+
);
837+
838+
const TestSchema = new Schema(
839+
{
840+
participants: { type: [schemaParticipant] },
841+
date: { type: Number }
842+
}
843+
);
844+
845+
const Test = db.model('Test', TestSchema);
846+
847+
const peer = {
848+
user: new mongoose.mongo.BSON.UUID('1583b99d-8462-4343-8dfd-9105252e5662')
849+
};
850+
851+
const originalUser = peer.user;
852+
853+
await Test.findOne({ participants: { $elemMatch: peer } });
854+
855+
assert.strictEqual(peer.user, originalUser);
856+
});
829857
});
830858

831859
function _geojsonPoint(coordinates) {

0 commit comments

Comments
 (0)