Automattic / mongoose

MongoDB object modeling designed to work in an asynchronous environment.
https://mongoosejs.com
MIT License
26.9k stars 3.83k forks source link

"createdAt" timestamp for subdocuments gets changed on "findOneAndUpdate" #8954

Closed JohnRandom closed 4 years ago

JohnRandom commented 4 years ago

Do you want to request a feature or report a bug? Report a bug / ask a question

What is the current behavior? When updating a document using findOneAndUpdate the timestamps for the root document are being updated properly, i.e. createdAt retains is original value and updatedAt gets a new timestamp. However, any embedded document has its createdAt timestamp updated along with its updatedAt timestamp and both set to the same value. I'm using the { new: true } option to return the updated version of the document from the query, in case that makes any difference. According to the docs, it shouldn't, though.

I have a model with a subdocument schema like so:

const TodoSchema = mongoose.Schema({
  name: String
}, {
  timestamps: true,
});

const TodoListSchema = mongoose.Schema({
  user: {
    type: mongoose.Schema.Types.ObjectId,
    ref: 'User',
    required: true,
  },
  entries: [TodoSchema]
}, {
  timestamps: true,
});

The controller code handling a PUT request looks like this:

async function(req, res, next) {
    try {
      const { id } = req.params;
      const data = mergeDeepRight(req.body, {
        user: mongoose.Types.ObjectId(req.user._id),
      });
      const filters = {
        _id: id,
        user: mongoose.Types.ObjectId(req.user._id),
      };
      const opts = { new: true };

      const todoList = await TodoList.findOneAndUpdate(filters, data, opts).exec();

      if (!todoList) {
        return res.status(404).json({
          error: `TodoList with id: "${id}" does not exist.`
        });
      }

      res.json(todoList);
    } catch (err) {
      next(err);
    }
  }

As you can see, nothing fancy going on here. The code is mainly making sure that a user cannot overwrite the user property during a PUT request.

If the current behavior is a bug, please provide the steps to reproduce. First of all, my question is whether or not this is intended behavior given the my code snippets. If not, I hope the code above is sufficient to reproduce the issue. I can probably set up a repo, in case you need more information.

What is the expected behavior? I would not expect the createdAt timestamps to change on embedded documents when using findOneAndUpdate.

What are the versions of Node.js, Mongoose and MongoDB you are using? Note that "latest" is not a version. I'm running node 12 and mongoose 5.9.12

JohnRandom commented 4 years ago

After some more testing, it seems that the issue can be extended to the functionality of this.isNew during document update. I added a manually maintained timestamp for document creation:

const TodoSchema = mongoose.Schema({
  name: String,
  dateCreated: Date,
}, {
  timestamps: true,
});

And added a custom pre-save hook that looks like this:

TodoSchema.pre('save', function() {
  if (this.isNew) {
    this.dateCreated = Date.now();
  }
});

This timestamp also gets updated on each findOneAndUpdate of the parent document. Changing the hook to:

TodoSchema.pre('save', function() {
  if (!this.dateCreated) {
    this.dateCreated = Date.now();
  }
});

Obviously fixes the issue, since the timestamp can only be assigned once. Maybe it's a naive approach, but that should be the expected behavior, right?

AbdelrahmanHafez commented 4 years ago

I am assuming that your req.body includes the full entries array, and you replace the entries in MongoDB with the entries in req.body.

This actually creates new entries even if they have the same names of the older ones. If you want to avoid that, you should instead $push and $pull specific entries instead of passing the whole array every time. Perhaps create an endpoint that adds (pushes) a single entry at a time, and one to remove (pull) a single entry at a time.

The script below demonstrates this:

'use strict';
const mongoose = require('mongoose');
const { Schema } = mongoose;
const assert = require('assert');

run().catch(console.error);

async function run() {
  await mongoose.connect('mongodb://localhost:27017/test', {
    useNewUrlParser: true,
    useUnifiedTopology: true
  });

  await mongoose.connection.dropDatabase();

  const todoSchema = new Schema({
    name: String
  },
  { timestamps: true });

  const todoListSchema = new Schema({ entries: [todoSchema] }, { timestamps: true });

  const TodoList = mongoose.model('TodoList', todoListSchema);

  const createdList = await TodoList.create({
    entries: [{ name: 'Morning workout' }]
  });

  const updatedList = await TodoList.findOneAndUpdate(
    { _id: createdList._id },
    { $push: { entries: { name: 'Write for 30 minutes' } } },
    { new: true }
  );

  assert.deepEqual(createdList.entries[0].createdAt, updatedList.entries[0].createdAt);

  console.log('All assertions passed.');
}
JohnRandom commented 4 years ago

Ok, so from your comment I gather that the behavior I described is actually intended, is that correct? It just seems a bit counterintuitive that documents that already have an ID when written again are considered to be new documents. I guess it's one of those gotchas. I assume MongoDB is responsible for this behavior and not mongoose?

AbdelrahmanHafez commented 4 years ago

Well, if we are sending an array with the subdocuments, we are replacing the existing array with new subdocuments. I agree, this is one of those gotchas. I actually faced a similar situation a couple of years ago with subdocuments. That's intended behavior.

I'll close this issue, please reopen if you need any further assistance with this issue.

tareqdayya commented 3 years ago

Sorry to revive an older issue, but i'm having the exact same issue.

Please correct me if I misunderstood you, but it seems the original problem is having "createdAt" update when it shouldn't, and the solution fixes that but creates another problem where "updatedAt" doesn't update when it should. Not to mention that it creates a new element in the array (I couldn't get the "$push" and "$pull" in one operation thing to work. It seems using both together results in a noop).

vkarpov15 commented 3 years ago

@tareqdayya please open a new issue and follow the issue template. See Mongoose timestamp docs and Mongoose timestamps.