wrongly update timestamps on updateOne about mongoose HOT 18 OPEN

• 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?

from mongoose.

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

from mongoose.

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

from mongoose.

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.

from mongoose.

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}

from mongoose.

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

from mongoose.

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.

from mongoose.

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

from mongoose.

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.

from mongoose.

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();

from mongoose.

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.

from mongoose.

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

from mongoose.

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

from mongoose.

@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

from mongoose.

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.

from mongoose.

@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?

from mongoose.

@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

from mongoose.

I agree with every point made by @AbdelrahmanHafez.

from mongoose.