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

Inner required validator fails on $pull #6972

Closed
sebastian-nowak opened this issue Sep 4, 2018 · 5 comments
Closed

Inner required validator fails on $pull #6972

sebastian-nowak opened this issue Sep 4, 2018 · 5 comments

Comments

@sebastian-nowak
Copy link

Required validator for a subdocument field fails on $pull even though the field is defined.

#!/usr/bin/env node
(async function () {
  const mongoose = require('mongoose');
  mongoose.connect('mongodb://localhost:27017/test', {useNewUrlParser: true});

  const schema = new mongoose.Schema({
    someArray: {
      type: [{
        innerField: {
          type: mongoose.Schema.Types.ObjectId,
          required: [true, 'Should not fail']
        }
      }]
    }
  });

  const Model = mongoose.model('pr', schema);
  await Model.findOneAndUpdate({}, {
    $pull: {someArray: {innerField: '507f191e810c19729de860ea'}}
  }, {
    runValidators: true
  });
})();

As a side note, I don't think required validators should be called here at all, because the full subdocument isn't present and it's valid to omit fields in $pull. It's a query.

Tested on mongoose 5.2.13

@vkarpov15 vkarpov15 added this to the 5.x Unprioritized milestone Sep 8, 2018
@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 Sep 8, 2018
@vkarpov15
Copy link
Collaborator

Thanks for reporting, will fix

@sebastian-nowak
Copy link
Author

Thanks @vkarpov15. Are you sure you wanted to tag it as an 'enhancement'? It's a pretty obvious bug, it's causing the $pull operation to fail in lots of scenarios when combined with global runValidators option. It's common to have a nested required validator, so it's definitely affecting lots of larger projects.

@vkarpov15 vkarpov15 modified the milestones: 5.x Unprioritized, 5.2.17 Sep 14, 2018
@vkarpov15 vkarpov15 removed the enhancement This issue is a user-facing general improvement that doesn't fix a bug or add a new feature label Sep 14, 2018
@vkarpov15
Copy link
Collaborator

Good point, we will treat this as a bug

@vkarpov15 vkarpov15 modified the milestones: 5.2.17, 5.2.18 Sep 21, 2018
@sebastian-nowak
Copy link
Author

sebastian-nowak commented Sep 23, 2018

I've just updated to 5.2.17 and all of my tests are now passing. The repro code from the first post seems to be working correctly too. I don't know how, but it looks like it has been accidentally fixed :)

I'm not closing the issue myself in case you'd want to add a regression test.

Thanks!

@vkarpov15 vkarpov15 removed this from the 5.2.18 milestone Sep 24, 2018
@vkarpov15
Copy link
Collaborator

Glad it works now 👍

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

2 participants