Very, very, very odd Collection.Update bug!

help
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!?

1 Like
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!?

1 Like
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.

2 Likes
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

1 Like