Very, very, very odd Collection.Update bug!


#1

Okay I’ve got a bug that just makes no sense at all.

I have a collection hook defined in /server/hooks/products.js:

///////////////////////////////////////////////////////////////////
//                         HOOKS                                 //
///////////////////////////////////////////////////////////////////

Products.before.remove(function (userId, doc) {
  // Ensure all print list entries that reference this product are removed
  PrintLists.update(
    { 'items.product': doc._id },
    { $pull: { items: { product: doc._id } } },
    { multi: true }
  );

  // Ensures that any recipes that reference this product are removed
  Recipes.remove({ productRef: doc._id });
}, { fetchPrevious: false });

which I’ve checked, it fires, and calls PrintLists.update to remove any items that reference the product that is about to be removed… BUT it doesn’t get removed.

So I thought I’d just try running the command in the meteor shell, same thing, returns a 1 but doesn’t remove the document.

I thought I’d try bypassing the Collection2 validations, and the hooks, so I tried a PrintLists.direct.update({...}, {...}, { validate: false }) and still nope, the sub-document in the items array remains.

So I thought lets see if the mongo shell will remove it:

db.printLists.update({ 'items.product': 'CoMn8WLxCyN9F3Fm5' }, { $pull: { items: { product: 'CoMn8WLxCyN9F3Fm5' } } }, { multi: true })
WriteResult({ "nMatched" : 1, "nUpserted" : 0, "nModified" : 1 })

Sure enough the item was then removed, all fine. So why the hell is this not working in Meteor!?


Weird problem -- $set doesn't run, no error?
How to update an array of objects (Noob question)
#2

Have you tried to $set a manually modified version of items?

I thought you can do $pull: {'items.product':doc._id} or is that just wishful thinking…

What’s your schema for PrintList look like?


#3

Nope, I haven’t tried manually modifying the array and saving it back… Might give that a go in the meteor shell now. But in short, the mongo shell accepted it and did the operation, so why the hell wouldn’t meteor!?

My PrintList schema looks like this:

Schemas.PrintListItem = new SimpleSchema({
  _id: {
    type: String,
    label: 'List Item Id',
    optional: false,
    autoValue: function () {
      if (!this.isSet) {
        return Random.id();
      }
    }
  },
  product: {
    type: String,
    label: 'Product',
    optional: false
  },
  template: {
    type: String,
    label: 'Template',
    optional: false
  },
  quantity: {
    type: Number,
    label: 'Quantity of labels',
    defaultValue: 1
  },
  overrides: {
    type: [Object],
    label: 'Overrides',
    optional: false,
    minCount: 0,
    defaultValue: []
  },
  'overrides.$.binding': {
    type: String,
    label: 'Override Binding',
    optional: false
  },
  'overrides.$.value': {
    type: String,
    label: 'Override Value',
    optional: false
  }
});

Schemas.PrintList = new SimpleSchema({
  // Force value to be current date (on server) upon insert
  // and prevent updates thereafter.
  createdAt: {
    type: Date,
      autoValue: function() {
        if (this.isInsert) {
          return new Date;
        } else if (this.isUpsert) {
          return {$setOnInsert: new Date};
        } else {
          this.unset();
        }
      }
  },
  // Force value to be current date (on server) upon update
  // and don't allow it to be set upon insert.
  updatedAt: {
    type: Date,
    autoValue: function() {
      if (this.isUpdate) {
        return new Date();
      }
    },
    denyInsert: true,
    optional: true
  },
  site: {
    type: String,
    label: 'Site',
    optional: false,
    index: true,
    autoValue: function () {
      // If this is an update from server, trusted code
      // allow it with whatever value is already set.
      if (this.isSet && this.isFromTrustedCode) {
        return;
      }

      // Otherwise set the site reference to that which
      // the user making the request belongs too
      var user = Meteor.users.findOne(this.userId);
      if (this.isInsert) {
        return user.profile.site;
      } else if (this.isUpsert) {
        return { $setOnInsert: user.profile.site };
      } else {
        // Disallow changes to the site field
        // on anything other than an insert
        this.unset();
      }
    }
  },
  name: {
    type: String,
    label: 'Name',
    optional: false
  },
  label: {
    type: String,
    label: 'Label',
    optional: false,
    custom: function () {
      // Validate that the label referenced actually exists
      var label = Labels.findOne(this.value, { fields: { _id: 1 } });
      if (!label) {
        return 'invalid-label';
      }
      return true;
    }
  },
  defaultTemplate: {
    type: String,
    label: 'Default Template',
    optional: true
  },
  items: {
    type: [Schemas.PrintListItem],
    label: 'Print List Items',
    optional: false
  }
});

#4

If mongo shell does it then it has to be smart schema related unless it’s a bug… I’ll bet on the former.

Are there other items in the list? I’m not sure how optional:false treats [] or how a $pull treats pulling the last element?


#5

Also, the schema defaults to optional:false, you only need to specify the optional fields.


#6

But if it were schema related, setting { validate: false } should have fixed it, but it didn’t.


#7

Agreed. (Padding to 20 characters)


#8

It’s worth noting that a “manual” deletion of the item from the array using this Meteor method, works:

deletePrintItems: function (printListId, items) {
    check(printListId, String);
    check(items, [String]);

    // Get the print list
    var printList = PrintLists.findOne(printListId);
    if (!printList) {
      throw new Meteor.Error('print-list-not-found',  "The specified print list does not exist");
    }

    // Remove any items which are in the removal array
    printList.items = _.reject(printList.items, function (item) {
      return _.contains(items, item._id);
    });

    // Overwrite the old items array with the new one
    PrintLists.update({ _id: printList._id }, { $set: { items: printList.items } });
  },

#9

BINGO! You were right.

I just commented out the line:
PrintLists.attachSchema(Schemas.PrintList);
and sure enough, it suddenly starts working again.

Clearly a schema issue. Maybe @aldeed could chime in as to where I’ve gone wrong!?


#10

I’m right? This does not happen very often and calls for a celebration :tropical_drink:

What I’d like to know is, does changing items: {optional:false -> true} make any difference. Removing the schema… isn’t a solution though (oh my code doesn’t validate, let’s remove ALL checks :stuck_out_tongue:)

I think your manual workaround is better atm.


#11

Unfortunately @mordrax, nope, that doesn’t work… I’m going to try writing the schema in a slightly different way, see where that gets me.

EDIT: It didn’t get me far. Still royally stuck.


#12

@aldeed I really need your help here, I’m at a complete loss… In the meantime I’ve been forced to do this:

  // Ensure all print list entries that reference this product are removed

  // TODO: REMOVE THIS HACK!!!!!!!
  var printLists = PrintLists.find({ 'items.product': doc._id }).fetch();

  _.each(printLists, function (printList) {
    // Remove any items which relate to this deleted product
    printList.items = _.reject(printList.items, function (item) {
      return item.product === doc._id;
    });

    // Overwrite the old items array with the new one
    PrintLists.update({ _id: printList._id }, { $set: { items: printList.items } });
  });
  // TODO: REMOVE THIS HACK!!!!!!!


  //PrintLists.update(
  //  { 'items.product': doc._id },
  //  { $pull: { items: { product: doc._id } } },
  //  { multi: true }
  //);

#13

I think that is fine, if your items array is only a couple hundred, it shouldn’t affect performance.

Have you considered doing a flat reference instead of nesting? It works out better for when you’re doing your security/validation as they are typically collection level.


#14

And there is no hook on PrintLists which could affect that?
Now is time to experiment with this and reproduce on meteorpad in simple example scenario.


#15

Try adding each of the options to prevent some of the cleaning that is done: https://github.com/aldeed/meteor-collection2#inserting-or-updating-without-cleaning

It’s not failing validation so most likely your modifier is being cleaned improperly. If you can create a full reproduction repo and identify which cleaning operation is causing it, you can submit an issue on GH and it will be easier to fix.


#16

I have the same problem. I tried to remove the options and here it is - {getAutoValues: false} solved the problem of removing subdocuments

Unfortonately I am not used to git but you can find a reproduction at: http://meteorpad.com/pad/jjy5gZdCKERzXRzuu/Copy%20of%20Pull-Subcollection-Array