Add discriminator key to findOneAndUpdate

[JonathanZWhite]

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

What is the current behavior?
If I have the following discriminators:

const messageSchema = new Schema({ text: String });
const Message = mongoose.model('Message', messageSchema);

/* ============ */

const attachmentMessageSchema = new Schema({
  type: { type: String, default: 'ATTACHMENT' },
  attachment: String,
});

const AttachmentMessage = Message.discriminator(
  'AttachmentMessage',
  attachmentMessageSchema,
);

In order to update an instance of AttachmentMessage I would have to write AttachmentMessage.findOneAndUpdate. If I write Message.findOneAndupdate no changes will occur.

I think this is because the discriminator key is only added to find(), count(), and aggregate. As the docs state:

Discriminator models are special; they attach the discriminator key to queries. In other words, find(), count(), aggregate(), etc. are smart enough to account for discriminators.

However, there are cases where the server might not be aware of the discriminator type of the model I want to update. For example, the client might pass a generic message payload:

{
  text: 'some_text',
  attachment: <payload>
}

Ideally, it would be nice to be able to pass that payload to the following function:

updateMessage(id, data) {
  return Message.findByIdAndUpdate(id, data, {
    new: true,
  });
}

Could we automatically add the discriminator key to functions like findOneAndUpdate to address these cases? I saw a PR in Mongoose Schema extended that seems to model the desired behavior: https://github.com/briankircho/mongoose-schema-extend/pull/53/files

Please mention your node.js, mongoose and MongoDB version.
Node: 8.4.0
Mongo: 3.0.7
Mongoose: 4.4.6

[sobafuchs]

i think this is a reasonable feature request, thoughts @vkarpov ?

here's an example:

const mongoose = require('mongoose');
mongoose.Promise = global.Promise;

const Schema = mongoose.Schema;
const GITHUB_ISSUE = `gh-6087`;




/* ============ */



run().catch(error => console.error(error.stack));

async function run() {
  await mongoose.connect(`mongodb://localhost:27017/${GITHUB_ISSUE}`);
  const messageSchema = new Schema({ text: String });
  const Message = mongoose.model('Message', messageSchema);

  const attachmentMessageSchema = new Schema({
    type: { type: String, default: 'ATTACHMENT' },
    attachment: String,
  });

  const AttachmentMessage = Message.discriminator(
    'AttachmentMessage',
    attachmentMessageSchema,
  );

  const doc = await AttachmentMessage.create({ attachment: 'lol' });
  const update = await Message.findOneAndUpdate({ _id: doc._id, __t: 'AttachmentMessage' }, { attachment: 'test' }, { new: true });

  console.log(update);
}

In that last log, if this feature were implemented, it would have succsesfully updated attachment to be test

[JonathanZWhite]

@varunjayaraman Would it be feasible to remove the need to pass __t completely in findOneAndUpdate? Something like this:

const update = await Message.findOneAndUpdate({ _id: doc._id }, { attachment: 'test' }, { new: true });

From an API design standpoint, this feature above would simplify a generic update endpoint from this:

// [POST] /messages?type=ATTACHMENT

const MessageTypeModels = {
  'ATTACHMENT': AttachmentMessage,
  'LIST': ListMessage,
};

function updateMessage(id, type, data) {
  MessageTypeModels[type].findOneAndUpdate({ _id: id }, data);
}

To this:

// [POST] /messages

function updateMessage(id, data) {
  Message.findOneAndUpdate({ _id: id }, data);
}

[sobafuchs]

yeah i was posting that more as a proof of concept of how it would work under the hood :)

[lineus]

@JonathanZWhite, @rawalyogendra asked about this on the gitter.im channel for mongoose as well. I wrote a wrapper around the mongoose.model that works pretty well so far, at least in the interim while new features are developed in mongoose 5. here is a gist . I'm pretty new to javascript and mongoose, so I'd like to know if this is a really bad idea, just ok, or if it's useful.

[woh-dev]

Just want to flag that this would be super helpful for our app us as well. Would be awesome if this does get prioritized soon.

Thanks for all the great work on Mongoose!

[thernstig]

In the same regard, I noticed that this works:

Message.create({
  ...
  __t: 'AttachmentMessage'
})

It will then do validation against correct discriminator schema.

But this does not work:

Message.insertMany([{
  ...
  __t: 'AttachmentMessage'
}])

It will just drop all properties that are specific to the attachmentMessageSchema.

@vkarpov15 Does that belong to the same feature request as this issue, or should a new issue be created for that?

[vkarpov15]

@thernstig new issue. I'll open one now.

[nhitchins]

Since nested discriminator and array of discriminators are being cast correctly during an Update() it seems logical that Update() from the base model would also be cast automatically (if update contains a discriminatorKey value) and also considering that Create() does work this way.

The current approach of updating from the discriminator model has an issue as highlighted on #7843 when using findOneAndUpdate or findByIdAndUpdate. Since they "attach the discriminator key to queries" it will not match an existing doc with a different discriminatorKey value.

// allows you to set or change the discriminatorKey but not the 'attachment'
const update = await Message.findByIdAndUpdate(doc._id, { __t: 'AttachmentMessage', attachment: 'test' }, { new: true });

// fails when an existing doc exists but is missing the discriminatorKey (or trying to update it from ListMessage)
const update = await AttachmentMessage.findByIdAndUpdate(doc._id, { __t: 'AttachmentMessage', attachment: 'test' }, { new: true });

ie. you can only change the discriminatorKey value from the base model and therefore can not also include additional attributes defined only in the changed (destination) discriminator model.

[nhitchins]

I can understand the logic behind automatically attaching the discriminator key to certain queries for convenience but I think it needs to be optional.

findByIdAndUpdate is already constrained by a unique & indexed objectId so can not see any benefit of also adding the discriminator key to the query.

[vkarpov15]

@nhitchins the problem in this issue is that if you use AttachmentMessage.findOneAndUpdate(), it may also update messages that aren't of type AttachmentMessage, which is inconsistent with other query methods (find(), etc.) . It is not quite related to your comments from #7843

[ojboj]

I've been having the same issue as @nhitchins.

If the update changes the type of the document via a different, but valid, discriminatorKey, using the new discriminator model for the findByIdAndUpdate call fails to find the document as it doesn't have the right type for it.
If I just run it on the original, base model, then it doesn't update correctly using the right discriminator schema/validators.

  const baseSchema = new Schema({}, { discriminatorKey: 'type' });
  const baseModel = model('thing', baseSchema);

  const aSchema = new Schema(
    {
      aThing: { type: Number },
    },
    { _id: false, id: false },
  );
  const aModel = baseModel.discriminator('A', aSchema);

  const bSchema = new Schema(
    {
      bThing: { type: String },
    },
    { _id: false, id: false },
  );
  const bModel = baseModel.discriminator('B', bSchema);

  // Model is created as a type A
  const doc = await baseModel.create({ type: 'A', aThing: 1 });

  /// Want to update it to being a type B

  // Try through the sub model
  await bModel.findByIdAndUpdate(
    doc._id,
    { type: 'B', bThing: 'one' },
    { runValidators: true },
  );
  // fails to find the document, so fails to update
  console.log('doc after update on bModel', await baseModel.findById(doc._id).exec());
  // null

  // Try through the base model
  await baseModel.findByIdAndUpdate(
    doc._id,
    { type: 'B', bThing: 'one' },
    { runValidators: true },
  );
  // updates `type` to 'B' but does not assign `bThing` to 'one', or (ideally) remove `aThing` either
  console.log(
    'doc after update on baseModel',
    await baseModel.findById(doc._id).exec(),
  );
  // {
  //   "_id" : ObjectId("5ecd382747e868273d49a09b"),
  //   "type" : "B",
  //   "aThing" : NumberInt(1),
  //   "__v" : NumberInt(0)
  // }

edit: current work-around is to update the document twice.
First, update is to change the type:

baseModel.findByIdAndUpdate(id, { type: 'B' }, { runValidators: true })

Then, update on the new model with the original update:

bModel.findByIdAndUpdate(id, update, { runValidators: true })

Functional, but not exactly optimal

[vkarpov15]

For 5.10 we'll add an overwriteDiscriminatorKey option that lets you change the discriminator key in the update:

'use strict';
  
const mongoose = require('mongoose');

mongoose.set('debug', true);
mongoose.set('useFindAndModify', false);

const { Schema } = mongoose;

run().catch(err => console.log(err));

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

  await mongoose.connection.dropDatabase();

  const baseSchema = new Schema({}, { discriminatorKey: 'type' });
  const baseModel = mongoose.model('thing', baseSchema);

  const aSchema = new Schema(
    {
      aThing: { type: Number },
    },
    { _id: false, id: false },
  );
  const aModel = baseModel.discriminator('A', aSchema);

  const bSchema = new Schema(
    {
      bThing: { type: String },
    },
    { _id: false, id: false },
  );
  const bModel = baseModel.discriminator('B', bSchema);

  // Model is created as a type A
  const doc = await baseModel.create({ type: 'A', aThing: 1 });

  await bModel.findByIdAndUpdate(
    doc._id,
    { type: 'A', bThing: 'one', aThing: '2' },
    { runValidators: true, overwriteDiscriminatorKey: true },
  );
}

[himito]

Hi @vkarpov15 ,

Thank you for your work :).

I checked your example, however, the type has not been modified. It's still 'A'.

Best,
Jaime

[vkarpov15]

@himito you're right, the example was incorrect. Below is a corrected example:

'use strict';

const mongoose = require('mongoose');

run().catch(err => console.log(err));

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

  await mongoose.connection.dropDatabase();

  const baseSchema = new mongoose.Schema({}, { discriminatorKey: 'type' });
  const baseModel = mongoose.model('thing', baseSchema);

  const aSchema = new mongoose.Schema({ aThing: Number }, { _id: false, id: false });
  const aModel = baseModel.discriminator('A', aSchema);

  const bSchema = new mongoose.Schema({ bThing: String }, { _id: false, id: false });
  const bModel = baseModel.discriminator('B', bSchema);

  // Model is created as a type A
  const doc = await baseModel.create({ type: 'A', aThing: 1 });

  // Update the type to 'B'
  const updated = await aModel.findByIdAndUpdate(
    doc._id,
    { type: 'B', bThing: 'one', aThing: '2' },
    { runValidators: true, overwriteDiscriminatorKey: true, new: true },
  );
  console.log(updated.type); // 'B'
}

[himito]

Thank you @vkarpov15 !

[wassil]

@vkarpov15 the code example you provided doesn't work for custom discriminator model names.
I've opened a pull request with the fix. #12947