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 :)

@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