Skip to content

Commit

Permalink
id setter virtual
Browse files Browse the repository at this point in the history
  • Loading branch information
@IslandRhythms
IslandRhythms committed Jun 16, 2023
1 parent 1db7cf4 commit 32a84b7
Show file tree
Hide file tree
Showing 2 changed files with 24 additions and 1 deletion.
13 changes: 12 additions & 1 deletion lib/helpers/schema/idGetter.js
View file
Original file line number Diff line number Diff line change
Expand Up @@ -12,8 +12,8 @@ module.exports = function addIdGetter(schema) {
if (!autoIdGetter) {
return schema;
}

schema.virtual('id').get(idGetter);
schema.virtual('id').set(idSetter);

return schema;
};
Expand All @@ -30,3 +30,14 @@ function idGetter() {

return null;
}

/**
*
* @param {String} v the id to set
* @api private
*/

function idSetter(v) {
this._id = v;
return;
}
12 changes: 12 additions & 0 deletions test/document.test.js
View file
Original file line number Diff line number Diff line change
Expand Up @@ -12212,6 +12212,18 @@ describe('document', function() {
const fromDb = await Test.findById(x._id).lean();
assert.equal(fromDb.c.x.y, 1);
});
it('can change the value of the id property on documents gh-10096', async function() {
const testSchema = new Schema({
name: String
});
const Test = db.model('Test', testSchema);
const doc = new Test({ name: 'Test Testerson ' });
const oldVal = doc.id;
doc.id = '648b8aa6a97549b03835c0b3';
await doc.save();
assert.notEqual(oldVal, doc.id);
assert.equal(doc.id, '648b8aa6a97549b03835c0b3');
});
});

describe('Check if instance function that is supplied in schema option is availabe', function() {
Expand Down

8 comments on commit 32a84b7

@sagrawal31
Copy link

@sagrawal31 sagrawal31 commented on 32a84b7 Jul 30, 2024 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is an undocumented change in the release notes of v7.4.0, at least I couldn't understand from the listed points.

Because of this, our code (which used to duplicate) an existing document (in TypeScript) started to fail-

await new this.campaignModel({
    ...existingCampaign.toJSON(),  // This was dumping "id" apart from "_id" which was not getting saved before v7.4.0
    _id: undefined,
}).save();

@sagrawal31
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Seems it got reverted later in v8 #13784

@vkarpov15
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@sagrawal31 that is good point, we added this change to 7.4 changelog here: 87fb382

@sagrawal31
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thank you, @vkarpov15. I appreciate this. Also, does it make sense to highlight this point in bold or mark it as a breaking change because this broke the application in many places? This was the case with me, and it sure must be with any mid- to large-size apps using Mongoose. Just a thought!

@vkarpov15
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We also added this change to our migration guide for mongoose 7 to highlight it as a breaking change, would that be sufficient in your case? Or did you primarily look at the change log when searching for breaking changes?

@sagrawal31
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

In my case, I was migrating from 7.3.1 to 7.6, so I would not have consulted the migration guide from Mongoose v6 to v7, so I prefer to read the change logs of all the releases under the same major release and yes look for breaking changes, if any.

@vkarpov15
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Makes sense, we added a note here: 0deb234

@sagrawal31
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks a lot, @vkarpov15. This will help fellow developers.

Please sign in to comment.