Instantiating model with model instance causes confusion

[maximilianschmitt]

Do you want to request a feature or report a bug?

Not sure if this is a bug, but it's behaviour we tripped over and spent about 4-5 hours debugging.

What is the current behavior?

Running the following piece of code, there will be a promise rejection "VersionError":

const co = require("co").default;
const mongoose = require("mongoose");
mongoose.Promise = Promise;

const SampleDoc = mongoose.model(
  "SampleDoc",
  new mongoose.Schema({
    subdocs: [
      {
        a: Number
      }
    ]
  })
);

co(function*() {
  mongoose.connect("mongodb://localhost/mongoose_test");

  const doc1 = yield SampleDoc.create({ subdocs: [] });

  // Instantate doc2 with model instance:
  const doc2 = new SampleDoc(doc1); // <---- this is the problematic line

  doc2.subdocs.push({ a: 2 });
  yield doc2.save();

  doc2.subdocs[0].a = 3;
  yield doc2.save();

  mongoose.disconnect();
});

The issue is that we're passing a model instance to the model constructor.

What is the expected behavior?

Can mongoose throw an error or warn in case you pass a model instance into a model constructor?? There is already a little safeguard for passing a schema as the second argument to the model constructor:

function Model(doc, fields, skipId) {
  if (fields instanceof Schema) {
    throw new TypeError('2nd argument to `Model` must be a POJO or string, ' +
      '**not** a schema. Make sure you\'re calling `mongoose.model()`, not ' +
      '`mongoose.Model()`.');
  }
  Document.call(this, doc, fields, skipId, true);
}

Please mention your node.js, mongoose and MongoDB version.

• node@6.11.0
• mongoose@4.12.2
• MongoDB: v3.4.9

[vkarpov15]

That warning is to prevent people from mixing up mongoose.model() and mongoose.Model(). I don't see what the issue is with your code, what's wrong with copying a document?

[sobafuchs]

@vkarpov15 i think @maximilianschmitt is just saying that they would like a warning from mongoose if you try to pass a mongoose doc to the Model constructor. Additionally, it errors if you do new SampleDoc(doc1.toObject()) because it will preserve the _id, I wonder if we could auto strip that value and assign the object a new _id?

[maximilianschmitt]

Yeah, the problem is that I intuitively expect mongoose to create a copy (or reference) of the model instance if I pass it into the model constructor again. Instead, I'm not exactly sure what happens but there are issues with version keys (promise rejecting with VersionError).

By the way, mongodb.ObjectId will simply return the original instance if you pass an instantiated objectId into the ObjectId constructor:

var oid1 = new mongodb.ObjectId()
var oid2 = new mongodb.ObjectId(oid1)

console.log(oid1 === oid2) // true

[despairblue]

This may seem contrived, but for clarification that is how we came upon this:

In our codebase we usually call toObject or use lean when we use mongoose for retrieving data and do all $sets ourself. That is to keep our business logic as decoupled from mongoose as possible. Now we're having a more complicated Model and decided that we want to use the property-marking abilities (only use $set for changed properties) of mongoose for only that one Model. To still have the decoupling we created a shallow class that wraps the Document and provides methods like logMessage (we could have used model-methods but that's beside the point). This was the constructor:

  constructor(document) {
    this.document = new Document(document)
  }

First this was only used to create new documents that weren't in the database before. For that it works fine.

Later we added routes that changed documents already in the database. The function we use to query the database was one that actually returns a mongoose document and does not use lean (we're not very consistent about this 😢).
That worked mostly fine, so we did not notice our error, but when we edited a couple of arrays we received VersionErrors. When looking at the mongo log and looking at the queries we found that now mongoose did an update that $set the whole document regardless of what properties where changed it just contains a huge $set and no $inc. Regardless it incremented the version key in the instantiated document and included that in the next update (again having a huge $set) leading to the VersionErrors which actually weren't any. There was no race condition.
That's when it dawned to us that something was fundamentally wrong. Since I wrote that wrapper and could remember that I wondered what would happen if an instance is passed to new (and I assumed it would work like ObjectId) I changed the constructor to:

  constructor(document) {
    if (document instanceof Document) {
      this.document = document;
    } else {
      this.document = new Document(document);
    }
  }

Which works now. (it still has an edge case where it fails when being passed a lean document with an _id, which needs to be handled with hydrate, but that's beside the point).

---

So what we're asking for is just an Error in case someone is as oblivious as us.

Or having it behave like ObjectId, but that may also confuse people who may think this creates a copy of the document in the db.

[vkarpov15]

Thanks for the detailed clarification @despairblue . I think changing it so that new Model(doc) returns just doc would be backwards breaking, but the fact that the given script throws a VersionError seems like a bug. In fact, your script succeeds if you turn versioning off, so there's something to fix there. Will investigate ASAP 👍