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

timestamps: updatedAt after findByIdAndUpdate #4768

Closed
qqilihq opened this issue Dec 3, 2016 · 11 comments
Closed

timestamps: updatedAt after findByIdAndUpdate #4768

qqilihq opened this issue Dec 3, 2016 · 11 comments
Labels
enhancement This issue is a user-facing general improvement that doesn't fix a bug or add a new feature
Milestone

Comments

@qqilihq
Copy link

qqilihq commented Dec 3, 2016

I wanted to replace the 3rd party mongoose-timestamp plugin through native mongoose functionality and specified the timestamps schema option.

In contrast to the mentioned plugin however, mongoose does not update when invoking findByIdAndUpdate. Is this by design? I've often stumbled over the differences between document and query middleware, so I'm quite used to this now, but I feel, that timestamps should logically update whenever a document is updated.

To make sure that I'm not doing anything wrong, here's a minimal example:

var mongoose = require('mongoose');

mongoose.connect('mongodb://localhost:27017/test');

var MyModel = mongoose.model('mySchema', new mongoose.Schema({
  name: String
}, {
  timestamps: true
}));

new MyModel({name: 'boob'}).save(function(err, doc) {
  console.log(doc.updatedAt);
  setTimeout(function() {
    MyModel.findByIdAndUpdate(doc._id, {name: 'bob'}, function(err, doc) {
      console.log(doc.updatedAt); // should by +5 seconds, shouldn't it?
    });
  }, 5000);
});

Would appreciate any feedback. If this behavior is really intentional, it should be stated in the docs, imho.

@vkarpov15
Copy link
Collaborator

Set the new option to true and it'll work as expected. The timestamp gets set, you're just not using findByIdAndUpdate() properly.

@qqilihq
Copy link
Author

qqilihq commented Dec 4, 2016

Thanks for pointing this out. However this was not my issue, I just noticed that my example was too minimal in that regard. Apologies.

The "problem" seems to be, that mongoose does not update the updatedAt value when using findByIdAndUpdate in case it is already given in the data. The example which resembles my real-world scenario should more look like this:

var mongoose = require('mongoose');

mongoose.connect('mongodb://localhost:27017/test');

var MyModel = mongoose.model('mySchema', new mongoose.Schema({
  name: String
}, {
  timestamps: true
}));

new MyModel({name: 'boob'}).save(function(err, doc) {
  console.log(doc.updatedAt);
  setTimeout(function() {
    doc = doc.toObject();
    doc.name = 'bob';
    MyModel.findByIdAndUpdate(doc._id, doc, {new: true}, function(err, doc) {
      console.log(doc.updatedAt); // should by +5 seconds, shouldn't it?
    });
  }, 5000);
});

In contrast, saving the document via its save function seems to update the timestamp correctly, even if it is present in the document:

new MyModel({name: 'boob'}).save(function(err, doc) {
  console.log(doc.updatedAt);
  setTimeout(function() {
    doc.name = 'bob';
    doc.save(function() {
      MyModel.findById(doc._id, function(err, doc) {
        console.log(doc.updatedAt); // here the timestamp is +5 seconds, as expected
      });
    });
  }, 5000);
});

Is this the expected behavior?

@vkarpov15
Copy link
Collaborator

Yeah this is expected, when you use update() or findOneAndUpdate() mongoose only sets timestamps if you don't set them yourself. Reasons are 2-fold: 1) prevent surprises, 2) it's easier to recommend doing delete doc.updatedAt before updating than to add an extra option to overwrite timestamps

@qqilihq
Copy link
Author

qqilihq commented Dec 7, 2016

Alright, thank you for clarifying!

@viktornord
Copy link

@vkarpov15 is there a way to tell mongoose always overwrite timestamps?

@vkarpov15
Copy link
Collaborator

@viktornord unfortunately not. We'll consider adding an option for this for a future release.

@vkarpov15 vkarpov15 reopened this Nov 30, 2018
@vkarpov15 vkarpov15 added this to the 5.x Unprioritized milestone Nov 30, 2018
@vkarpov15 vkarpov15 modified the milestones: 5.x Unprioritized, 5.7 Jul 21, 2019
@vkarpov15 vkarpov15 added the enhancement This issue is a user-facing general improvement that doesn't fix a bug or add a new feature label Jul 21, 2019
@huangxuewu
Copy link

if findByIdAndUpdate doesn't trigger updateAt field update , i don't see any point to use the build in timestamp.

@vkarpov15
Copy link
Collaborator

@huangxuewu it does trigger updatedAt update

@huangxuewu
Copy link

@vkarpov15 somehow it is not updating in my code. I have to add
Announcement.pre("findOneAndUpdate", function (next) { this.updateAt = new Date(); next(); })
to make it work.

const Announcement = new Schema({
title: String,
brief: String,
content: String,
creator: String,
target: String,
targets: [],
noticed: [],
enable: Boolean
}, {
timestamps: true
});

function updateAnnouncement(data, done) { Model.announcement.findByIdAndUpdate(data._id, data, { upsert: true }).then(() => done()); }

is anything wrong with my code?

@vkarpov15
Copy link
Collaborator

What does data look like in your code? It might be that there's a updatedAt property in there already.

@vkarpov15
Copy link
Collaborator

Looking into this more closely, this is actually a bug rather than a feature we need to opt in to. That's because in OP's example, updatedAt does get bumped if you use $set:

var mongoose = require('mongoose');

mongoose.connect('mongodb://localhost:27017/test');

var MyModel = mongoose.model('mySchema', new mongoose.Schema({
  name: String
}, {
  timestamps: true
}));

mongoose.set('debug', true);

new MyModel({name: 'boob'}).save(function(err, doc) {
  console.log(doc.updatedAt);
  setTimeout(function() {
    doc = doc.toObject();
    doc.name = 'bob';
    // Add `$set`, now `updatedAt` gets bumped
    MyModel.findByIdAndUpdate(doc._id, { $set: doc }, {new: true}, function(err, doc) {
      console.log(doc.updatedAt); // should by +5 seconds, shouldn't it?
    });
  }, 1000);
});

The code is there that sets $set.updatedAt, the issue is that updatedAt without $set overwrites it. We'll fix this for 5.7 👍

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement This issue is a user-facing general improvement that doesn't fix a bug or add a new feature
Projects
None yet
Development

No branches or pull requests

5 participants