wrongly update timestamps on updateOne

[abarriel]

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.0.4

Node.js version

16

MongoDB server version

6.0.6

Typescript version (if applicable)

No response

Description

Hi,

When updating a doc for a schema that includes the { timestamp: true } option, I've noticed that even when no updated fields are found, the updatedAt field still gets updated.

In my opinion, this shouldn't be the case since the doc are not updated.
Same things when updating the doc when the fields have the same values as the original ones.

Disabling timestamps is not an option in this particular update, since most of the time the update does in fact really update the doc with new fields.
Thank you

Steps to Reproduce

const mongoose = require("mongoose");

const eventSchema = new mongoose.Schema(
  { url: String, city: String },
  { timestamps: true }
);

const Event = mongoose.model("Event", eventSchema);

async function run() {
  await mongoose.connect("mongodb://localhost:27017");

  await Event.create({ url: "google.com" });

  const res = await Event.updateOne({ url: "google.com" }, { $set: { url: "google.com" } });
  console.log(res); // should not show  { modifiedCount: 1 }, neither update updateAt field,  { timestamp: false } is not an solution
  
  const res = await Event.updateOne({ url: "google.com" }, { $set: { url: "yahoo.com" } });
  console.log(res); // this is OK to update updatedAt field since the docs is in fact updated
}

run();

Expected Behavior

No response

[donoftime2018]

https://mongoosejs.com/docs/api/model.html#Model.updateOne()

Just add option {timestamps: false} after {$set: {}} update query like

const res = await Event.updateOne({ url: "google.com" }, { $set: {} }, {timestamps: false}); //outputs {acknowledged: false}

[abarriel]

@donoftime2018 Appreciate your input. However, as mentioned, I'm reluctant to disable the timestamps option entirely.

timestamps shouldn't be updated when no field are update at all, this is the point of the issue.

I will edit the example, to show all the use cases.

[donoftime2018]

If you read the docs, this keeps the timestamps for the schema, but skips the timestamp for the update query.

[donoftime2018]

What I tried doing was making a pre schema for updateOne and checking if city and url were null, undefined or empty string , or check that they equal the current url/city. If so, then keep the update.updatedAt = this.updatedAt.

eventSchema.pre('updateOne', async function(){
  const update = this.getUpdate()
  console.log(update.url)
  console.log(update.city)
  if (((update.url === undefined || update.url === null || update.url === "") ||
      (update.city === undefined || update.city === null || update.city === "")) || (
        (update.url === this.url) || (update.city === this.city)
      ))
  {
    update.updatedAt = this.updatedAt
  }

  console.log(update.updatedAt)
})


const res = await Event.updateOne({ url: "google.com" }, { $set: {} });


  const res = await Event.updateOne({ url: "google.com" }, { $set: { url: "google.com" } });

  console.log(res); //modifiedCount: 0

  const res2 = await Event.updateOne({ url: "google.com" }, { $set: { url: "yahoo.com" } });

  console.log(res2); //modifiedCount: 1

  const res3 = await Event.updateOne({ url: "google.com" }, { $set: { }});

  console.log(res3); //modifiedCount: 0

[abarriel]

I am sorry @donoftime2018 but if you really think this a solution. This is one of the most ugliest work around I have ever seen.

I am not looking for this kind of solution nor a solution at all.
I reported a bug. and would like to talk with core dev to understand why we update the updatedat field when no field are updated.

[abarriel]

@vkarpov15 I don't think, this need to be fix anymore, since the repo script has an error since there is no way to know the original doc before update, we don't want to do 1 find and 1 update.
Then in comments, I show a case where we do have a bug since an empty object $set, should not trigger an update at all. This can be fix, but not the case in repo script ( where $set contains values ), I would close the issue since it's not clear.

[vkarpov15]

In the Event.updateOne({ url: "google.com" }, { $set: { url: "google.com" } }); case I think we can also avoid updating updatedAt, because we know that url will not change. I'll keep this issue open because it has all the info we need.

[IslandRhythms]

const mongoose = require("mongoose");

const eventSchema = new mongoose.Schema(
  { url: String, city: String },
  { timestamps: true }
);

const Event = mongoose.model("Event", eventSchema);

async function run() {
  await mongoose.connect("mongodb://localhost:27017");
  await mongoose.connection.dropDatabase();

  await Event.create({ url: "google.com" });

  const res = await Event.updateOne({ url: "google.com" }, { $set: { url: "google.com" } });
  console.log(res); // should not show  { modifiedCount: 1 }, neither update updateAt field,  { timestamp: false } is not an solution
  
  const res2 = await Event.updateOne({ url: "google.com" }, { $set: { url: "yahoo.com" } });
  console.log(res2); // this is OK to update updatedAt field since the docs is in fact updated
}

run();

[vkarpov15]

I took a look and it looks like we should punt this to Mongoose 9, because right now Mongoose always increments updatedAt when you call updateOne(), even if no other properties are updated. We should consider changing that behavior in Mongoose 9, along with not updating timestamps in the await Event.updateOne({ url: "google.com" }, { $set: { url: "yahoo.com" } }) case.

[guda-art]

I think this is normal behavior because it is your initiative to update the document

[donoftime2018]

@vkarpov15 I'd like to help with adding this feature to mongoose 9.

[vkarpov15]

@guda-art you have a point, but for the sake of argument, shouldn't updateOne({}, {}) (updateOne with empty update) not update updatedAt? Given that no other properties were updated?

@donoftime2018 this feature is currently more of a discussion, we're not committing to making updateOne avoid updating timestamps if it can infer no properties will be changed just yet. Right now, the most helpful contribution would be to provide your honest opinion on whether updateOne({}, {}) and updateOne({ name: 'foo' }, { name: 'foo' }) should update updatedAt and reasoning to back up your opinion

[Imanghvs]

In my opinion it makes sense to update the timestamp, as we have called an update on the document, no matter if it's the same data it had before; And I think most of the developers are used to see this as updated. So I don't suggest changing it as many developers are using this feature with this mindset.

[vkarpov15]

@abarriel @hasezoey @AbdelrahmanHafez @IslandRhythms what do you think?

@Imanghvs thanks for your thoughts. Why do you think most developers are used to this behavior - is it because Mongoose has had this behavior for so long, or is it because other libs do something similar?

[AbdelrahmanHafez]

• I agree with the notion that when a document is updated with information that's the same as the current information, we should not update the timestamp, I can understand why we would be doing that in the case of Document#save(...) since we already know the exact document and the exact existing data.
• For the scenario of Model.updateOne({ _id: user._id }, {});, I think we could, and should save the DB call and make it a no-op (as long as upsert is not true) since the update object could be programmatically constructed, and sometimes the update object will be empty.
• However, for the scenario of User.updateOne({ url: "google.com" }, { url: "google.com" });, while it's technically possible for this particular scenario to infer that it's a no-op, it'll open the door for more complex scenarios where it's not possible to infer that change, such as User.updateOne({ url: { $in: ["google.com", "youtube.com"] } }, { url: "google.com }); In this scenario the url value could be the same and could be different, and we'd be updating the timestamp anyway. Which is why I think we should just be consistent and update the timestamp as long as we're sending an update command. If we're not sending an update command at all (empty $set), then it makes sense to not update the timestamps at all.

What do you think everyone?

[Imanghvs]

@Imanghvs thanks for your thoughts. Why do you think most developers are used to this behavior - is it because Mongoose has had this behavior for so long, or is it because other libs do something similar?

It's because Mongoose has had this behavior for a long time.

P.S. I totally agree with @AbdelrahmanHafez

[abarriel]

I agree with every point made by @AbdelrahmanHafez.

[hasezoey]

i also agree with @AbdelrahmanHafez, as in that as long as a command is send we should update the updatedAt. i also agree with maybe saving the empty update call (provided that options dont change it).
though maybe such a change should either be done behind a option or in a major update.

[sderrow]

To some extent, the "right" answer here depends on your use case for performing updates. Imagine a situation where you're keeping a document synced up with an external system's data. You would want updatedAt to change even if no properties in the underlying document actually changed, since updatedAt reflects how recently the document has been synced to the external system (i.e., how stale the data is). If you run a daily sync cron job, but the data never changes, updatedAt tells you the last time the job ran successfully.

I see both sides, but ultimately it feels cleaner to maintain existing behavior because updatedAt is easier to reason about when it simply means "executed a Mongo update command". It's a separate discussion about optimizing the Mongoose wrapper around update commands to detect when the update query itself is empty and aborting the update in that event.

Ultimately seems like this issue is more about a theoretical "changedAt" property, which explicitly is about changes.