search

Meteor-Community-Packages / meteor-collection-hooks

Meteor Collection Hooks
https://atmospherejs.com/matb33/collection-hooks
MIT License
658 stars 90 forks source link

Options on insert #233

Open petr24 opened 6 years ago

petr24 commented 6 years ago

I was using this package at version 0.8.1 with Meteor 1.2, I also changed the package locally to have the ability to have an options parameter for insert and remove hooks.

All was good, then I upgraded Meteor to 1.6 along with this package to version 0.9.0-rc.4, and wanted to have the same options as before on insert and remove hooks.

Now I am getting errors whenever I try Collection.insert(object, {exampleOption: "something"}); And the error is always callback is not a function

I went back to an older version I had used before to 0.8.1 and the same error happens. The other issue I am having is options are not passed to the package at all. I have my code below and console results.

At this point I am wondering if Meteor changed something that is causing the root of the problem, any help would be appreciated, thanks!

insert.js of collection hooks.

CollectionHooks.defineAdvice("insert",
  function (userId, _super, instance, aspects, getTransform, args, suppressAspects) {
      var self     = this;
      var ctx      = {context: self, _super: _super, args: args};
      var callback = _.last(args);
      var async    = _.isFunction(callback);
      var abort, ret;
      var options  = {};

      // args[0] : doc
      // args[1] : options (optional)
      // args[1] : callback

      console.log("ARGS[0] ", args[0]);
      console.log("ARGS[1] ", args[1]);
      console.log("ARGS[2] ", args[2]);

      // before
      if (!suppressAspects) {
          try {
              _.each(aspects.before, function (o) {
                  var r = o.aspect.call(_.extend({transform: getTransform(args[0])}, ctx), userId, args[0], options);
                  if (r === false) {
                      abort = true;
                  }
              });

              if (abort) {
                  return false;
              }
          } catch (e) {
              if (async) {
                  return callback.call(self, e);
              }
              throw e;
          }
      }

      function after(id, err) {
          var doc = args[0];
          if (id) {
              doc     = EJSON.clone(args[0]);
              doc._id = id;
          }
          if (!suppressAspects) {
              var lctx = _.extend({transform: getTransform(doc), _id: id, err: err}, ctx);
              _.each(aspects.after, function (o) {
                  o.aspect.call(lctx, userId, doc, options);
              });
          }
          return id;
      }

      if (async) {
          args[args.length - 1] = function (err, obj) {
              after(obj && obj[0] && obj[0]._id || obj, err);
              return callback.apply(this, arguments);
          };
          return _super.apply(self, args);
      } else {
          ret = _super.apply(self, args);
          return after(ret && ret[0] && ret[0]._id || ret);
      }
  });

Console result

I20171220-10:56:24.903(-8)? ARGS[0]  { name: 'test 6',
I20171220-10:56:24.904(-8)?   owner: 'T55yofSExwq6Tdurd',
I20171220-10:56:24.904(-8)?   extras: { extraId: '78c5caf8-e7b5-49ce-bf4a-64de7b883db7' },
I20171220-10:56:24.904(-8)?   _id: '3bnhcdxnJTTTFeuDf' }
I20171220-10:56:24.904(-8)? ARGS[1]  (error, result) => {
I20171220-10:56:24.904(-8)?     callback(error, !error && convertResult(result));
I20171220-10:56:24.905(-8)?   }
I20171220-10:56:24.905(-8)? ARGS[2]  undefined

I20171220-10:56:24.986(-8)? Exception in callback of async function: TypeError: callback is not a function
I20171220-10:56:24.986(-8)?     at packages/mongo/collection.js:618:5
I20171220-10:56:24.986(-8)?     at args.(anonymous function) (packages/old_collection-hooks.js:386:31)
I20171220-10:56:24.986(-8)?     at runWithEnvironment (packages/meteor.js:1240:24)
I20171220-10:56:24.986(-8)?     at packages/meteor.js:1253:14
I20171220-10:56:24.986(-8)?     at packages/mongo/mongo_driver.js:319:7
I20171220-10:56:24.986(-8)?     at runWithEnvironment (packages/meteor.js:1240:24)

Also to note, I renamed the package to "old:collection-hooks" just because I have it locally and it was the older version, that's why is coming up as "packages/old_collection-hooks.js"

zimme commented 6 years ago

This package doesn't work or doesn't work properly with Meteor 1.6+ https://github.com/meteor/meteor/issues/9383 I have a branch meteor-1.6.1. where I've tried to work around this but I didn't have time to try more then those 2 commits and that didn't work.

petr24 commented 6 years ago

Damn, this is unfortunate. Is there a Meteor version you recommend to use in the meantime with this package? It's essential to a our codebase so getting rid of it isn't an option at the moment.

zimme commented 6 years ago

Version previous to Meteor 1.6.1 "should" work.

petr24 commented 6 years ago

I am currently running 1.6.0.1

zimme commented 6 years ago

I'll need a repoduction repo to be able to help you with this. I need to see the problem and be able to follow the code down to Meteor's mongo/mini mongo collection level.

I'm spread quite thin currently so I can't say when I'll have time to look at this.

petr24 commented 6 years ago

Gotcha, I totally understand. I'll post back once I get a repo up, and I'll do some more digging as well.

petr24 commented 6 years ago

@zimme

Here is the link to the test repo. It's very simple, you have two buttons, one inserts into DB without options the other with options, and when you insert with options, you will see a error in the console that "callback is not a function".

I haven't been able to deduce the issue yet, but maybe I will find something this weekend with the free time.

https://github.com/petr24/collection-hooks-test

petr24 commented 6 years ago

Just a thought, wondering why we are getting the error in the first place with the package.

Example if I extend the mongo collection, I can pass options to the hook, but don't need to pass those options to the actual insert, example below . And all works. 

class collectionWithHooksTwo extends Mongo.Collection {
    insert(doc, options, callback) {
        console.log("INSERT  DOC ", doc);
        console.log("INSERT OPTIONS ", options);
        console.log("INSERT CALLBACK ", callback);
        return super.insert(doc, callback);
    }

    update(selector, modifier, options, callback) {
        return super.update.apply(this, arguments);
    }

    remove(selector, options, callback) {
        return super.remove.apply(this, arguments);
    }
}

Is there a reason why in "insert" hooks isn't getting my options parameter in the first place? I can pass it to the hook, but I would remove it before the actual insert. Just wondering why I am not seeing it in the first place.

If you look at the console, the options never get to the function itself. Any ideas what could be causing that? 

I20171220-10:56:24.903(-8)? ARGS[0]  { name: 'test 6',
I20171220-10:56:24.904(-8)?   owner: 'T55yofSExwq6Tdurd',
I20171220-10:56:24.904(-8)?   extras: { extraId: '78c5caf8-e7b5-49ce-bf4a-64de7b883db7' },
I20171220-10:56:24.904(-8)?   _id: '3bnhcdxnJTTTFeuDf' }
I20171220-10:56:24.904(-8)? ARGS[1]  (error, result) => {
I20171220-10:56:24.904(-8)?     callback(error, !error && convertResult(result));
I20171220-10:56:24.905(-8)?   }
I20171220-10:56:24.905(-8)? ARGS[2]  undefined

I20171220-10:56:24.986(-8)? Exception in callback of async function: TypeError: callback is not a function
I20171220-10:56:24.986(-8)?     at packages/mongo/collection.js:618:5
I20171220-10:56:24.986(-8)?     at args.(anonymous function) (packages/old_collection-hooks.js:386:31)
I20171220-10:56:24.986(-8)?     at runWithEnvironment (packages/meteor.js:1240:24)
I20171220-10:56:24.986(-8)?     at packages/meteor.js:1253:14
I20171220-10:56:24.986(-8)?     at packages/mongo/mongo_driver.js:319:7
I20171220-10:56:24.986(-8)?     at runWithEnvironment (packages/meteor.js:1240:24)
zimme commented 6 years ago

I looked at the code once more of your "own" version and the problem is that you don't strip out the "optional" options object from args before you the code in the insert advice is calling return _super.apply(self, args); or ret = _super.apply(self, args); at the end.

petr24 commented 6 years ago

I attached the insert.js file, on how I used to do it before 1.6, and in that one I did remove the options.

What confused me now is the options are never passed to the function in the first place, so there is nothing to strip out. 

CollectionHooks.defineAdvice("insert",
  function (userId, _super, instance, aspects, getTransform, args, suppressAspects) {
      var self     = this;
      var ctx      = {context: self, _super: _super, args: args};
      var callback = _.last(args);
      var async    = _.isFunction(callback);
      var abort, ret;
      var options  = {};

      // args[0] : doc
      // args[1] : options (optional)
      // args[2] : callback

      if (_.isFunction(args[1])) {
          callback = args[1];
          args[1]  = {};
      }

      if (args[1]) {
          options = args[1];
          args.splice(1, 1);
      }

      // before
      if (!suppressAspects) {
          try {
              _.each(aspects.before, function (o) {
                  var r = o.aspect.call(_.extend({transform: getTransform(args[0])}, ctx), userId, args[0], options);
                  if (r === false) {
                      abort = true;
                  }
              });

              if (abort) {
                  return false;
              }
          } catch (e) {
              if (async) {
                  return callback.call(self, e);
              }
              throw e;
          }
      }

      function after(id, err) {
          var doc = args[0];
          if (id) {
              doc     = EJSON.clone(args[0]);
              doc._id = id;
          }
          if (!suppressAspects) {
              var lctx = _.extend({transform: getTransform(doc), _id: id, err: err}, ctx);
              _.each(aspects.after, function (o) {
                  o.aspect.call(lctx, userId, doc, options);
              });
          }
          return id;
      }

      if (async) {
          args[args.length - 1] = function (err, obj) {
              after(obj && obj[0] && obj[0]._id || obj, err);
              return callback.apply(this, arguments);
          };
          return _super.apply(self, args);
      } else {
          ret = _super.apply(self, args);
          return after(ret && ret[0] && ret[0]._id || ret);
      }
  });
petr24 commented 6 years ago

@zimme

So I setup another repo running 1.2.1, and did a side by side test. Also I put a bunch of logs in the main function to follow the execution path. Not being to familiar with the code, this helped me understand it more. And the arguments that this main function gets never even gets the options to pass on.

Below is the main functions with all the logs shown. 

CollectionHooks.extendCollectionInstance = function extendCollectionInstance (self, constructor) {

  // console.log("COLLECTION HOOKS ARGS 1 ", arguments);
  console.log("LOG 1 ", arguments);
  // Offer a public API to allow the user to define aspects
  // Example: collection.before.insert(func);
  _.each(['before', 'after'], function (pointcut) {
    console.log("LOG 2");
    _.each(advices, function (advice, method) {
      console.log("LOG 3");
      if (advice === 'upsert' && pointcut === 'after') return

        console.log("LOG 4");
      Meteor._ensure(self, pointcut, method)
      Meteor._ensure(self, '_hookAspects', method)

      console.log("LOG 5");
      self._hookAspects[method][pointcut] = []
      console.log("LOG 5");
      self[pointcut][method] = function (aspect, options) {
        console.log("LOG 6");
        var len = self._hookAspects[method][pointcut].push({
          aspect: aspect,
          options: CollectionHooks.initOptions(options, pointcut, method)
        })
        console.log("LOG 7");

        return {
          replace: function (aspect, options) {
            console.log("LOG 8");
            self._hookAspects[method][pointcut].splice(len - 1, 1, {
              aspect: aspect,
              options: CollectionHooks.initOptions(options, pointcut, method)
            })
          },
          remove: function () {
            console.log("LOG 9");
            self._hookAspects[method][pointcut].splice(len - 1, 1)
          }
        }
      }
    })
  })

  console.log("LOG 10");

  // console.log("COLLECTION HOOKS ARGS 2 ", arguments);
  // Offer a publicly accessible object to allow the user to define
  // collection-wide hook options.
  // Example: collection.hookOptions.after.update = {fetchPrevious: false};
  self.hookOptions = EJSON.clone(CollectionHooks.defaults)

  console.log("LOG 11");
  // Wrap mutator methods, letting the defined advice do the work
  _.each(advices, function (advice, method) {

    console.log("LOG 12");
// console.log("COLLECTION HOOKS ARGS 3 ", arguments);

    var collection = Meteor.isClient || method === 'upsert' ? self : self._collection

    console.log("LOG 13");
    // Store a reference to the original mutator method
    var _super = collection[method]

    console.log("LOG 14");
    Meteor._ensure(self, 'direct', method)
    self.direct[method] = function () {
      console.log("LOG 15");
      var args = arguments
      return CollectionHooks.directOp(function () {
        console.log("LOG 16");
        return constructor.prototype[method].apply(self, args)
      })
    }

    collection[method] = function (PAR1, PAR2, PAR3, PAR4, PAR5) {

      console.log("PAR 1 ", PAR1);
      console.log("PAR 2 ", PAR2);
      console.log("PAR 3 ", PAR3);
      console.log("PAR 4 ", PAR4);
      console.log("PAR 5 ", PAR5);

      console.log("LOG 17 ", arguments);
      if (CollectionHooks.directEnv.get() === true) {
        console.log("LOG 18");
        return _super.apply(collection, arguments)
      }

      console.log("LOG 19");
      // NOTE: should we decide to force `update` with `{upsert:true}` to use
      // the `upsert` hooks, this is what will accomplish it. It's important to
      // realize that Meteor won't distinguish between an `update` and an
      // `insert` though, so we'll end up with `after.update` getting called
      // even on an `insert`. That's why we've chosen to disable this for now.
      // if (method === "update" && _.isObject(arguments[2]) && arguments[2].upsert) {
      //   method = "upsert";
      //   advice = CollectionHooks.getAdvice(method);
      // }
      // console.log("COLLECTION HOOKS ARGS 4 ", arguments);

      return advice.call(this,
        CollectionHooks.getUserId(),
        _super,
        self,
        method === 'upsert' ? {
          insert: self._hookAspects.insert || {},
          update: self._hookAspects.update || {},
          upsert: self._hookAspects.upsert || {}
        } : self._hookAspects[method] || {},
        function (doc) {
          console.log("LOG 20");
          return (
            _.isFunction(self._transform)
            ? function (d) { return self._transform(d || doc) }
            : function (d) { return d || doc }
          )
        },
        _.toArray(arguments),
        false
      )
    }
  })
}

1.6 Look at the parameters that are called "PAR 1, PAR 2... etc" In 1.6 you get the object, and a callback function right away. 

I20171222-17:03:57.526(-8)? INSERT OPTIONS METHOD CALELD 
I20171222-17:03:57.526(-8)? PAR 1  { test: 'Custom options', _id: 'dmeGcwgu8GBFdYucp' }
I20171222-17:03:57.527(-8)? PAR 2  (error, result) => {
I20171222-17:03:57.527(-8)?     callback(error, !error && convertResult(result));
I20171222-17:03:57.527(-8)?   }
I20171222-17:03:57.527(-8)? PAR 3  undefined
I20171222-17:03:57.527(-8)? PAR 4  undefined
I20171222-17:03:57.527(-8)? PAR 5  undefined
I20171222-17:03:57.527(-8)? LOG 17  { '0': { test: 'Custom options', _id: 'dmeGcwgu8GBFdYucp' },
I20171222-17:03:57.528(-8)?   '1': [Function] }
I20171222-17:03:57.528(-8)? LOG 19
I20171222-17:03:57.528(-8)? INSERT ARGS ARE  [ { test: 'Custom options', _id: 'dmeGcwgu8GBFdYucp' },
I20171222-17:03:57.528(-8)?   [Function] ]
I20171222-17:03:57.528(-8)? INSERT ARGS 0  { test: 'Custom options', _id: 'dmeGcwgu8GBFdYucp' }
I20171222-17:03:57.528(-8)? INSERT ARGS 1  (error, result) => {
I20171222-17:03:57.528(-8)?     callback(error, !error && convertResult(result));
I20171222-17:03:57.528(-8)?   }
I20171222-17:03:57.528(-8)? INSERT ARGS 2  undefined
I20171222-17:03:57.529(-8)? OPTIONS ARE  {}
I20171222-17:03:57.529(-8)? LOG 20
I20171222-17:03:57.529(-8)? BEFORE INSERT    { test: 'Custom options', _id: 'dmeGcwgu8GBFdYucp' }
I20171222-17:03:57.529(-8)? BEFORE INSERT OPTIONS    undefined
I20171222-17:03:57.629(-8)? Exception while invoking method 'insertOptions' TypeError: callback is not a function
I20171222-17:03:57.629(-8)?     at testYo.insert (packages/mongo/collection.js:495:7)
I20171222-17:03:57.629(-8)?     at testYo.insert (lib/test_collection.js:3:16)
I20171222-17:03:57.629(-8)?     at DDPCommon.MethodInvocation.insertOptions (lib/methods.js:9:25)
I20171222-17:03:57.629(-8)?     at maybeAuditArgumentChecks (packages/ddp-server/livedata_server.js:1768:12)
I20171222-17:03:57.630(-8)?     at DDP._CurrentMethodInvocation.withValue (packages/ddp-server/livedata_server.js:719:19)
I20171222-17:03:57.630(-8)?     at Meteor.EnvironmentVariable.EVp.withValue (packages/meteor.js:1186:15)
I20171222-17:03:57.630(-8)?     at DDPServer._CurrentWriteFence.withValue (packages/ddp-server/livedata_server.js:717:46)
I20171222-17:03:57.630(-8)?     at Meteor.EnvironmentVariable.EVp.withValue (packages/meteor.js:1186:15)
I20171222-17:03:57.630(-8)?     at Promise (packages/ddp-server/livedata_server.js:715:46)
I20171222-17:03:57.630(-8)?     at new Promise (<anonymous>)

1.2.1 and hooks package 0.8.1 (newest version has dependencies not compatible with 1.2.1) Same thing, look at the "PAR 1..." logs, and the second one is the custom options. 

I20171222-17:02:57.484(-8)? INSERT OPTIONS METHOD CALELD 
I20171222-17:02:57.484(-8)? PAR 1  { test: 'Custom options', _id: 'YGitdnfixJHWgrJP3' }
I20171222-17:02:57.484(-8)? PAR 2  { options: { test: 'hey' } }
I20171222-17:02:57.485(-8)? PAR 3  undefined
I20171222-17:02:57.485(-8)? PAR 4  undefined
I20171222-17:02:57.485(-8)? PAR 5  undefined
I20171222-17:02:57.485(-8)? LOG 17  { '0': { test: 'Custom options', _id: 'YGitdnfixJHWgrJP3' },
I20171222-17:02:57.486(-8)?   '1': { options: { test: 'hey' } },
I20171222-17:02:57.486(-8)?   '2': undefined }
I20171222-17:02:57.486(-8)? LOG 19
I20171222-17:02:57.486(-8)? ARGS[0]  { test: 'Custom options', _id: 'YGitdnfixJHWgrJP3' }
I20171222-17:02:57.486(-8)? ARGS[1]  { options: { test: 'hey' } }
I20171222-17:02:57.486(-8)? ARGS[2]  undefined
I20171222-17:02:57.486(-8)? LOG 20
I20171222-17:02:57.486(-8)? BEFORE INSERT    { test: 'Custom options', _id: 'YGitdnfixJHWgrJP3' }
I20171222-17:02:57.486(-8)? BEFORE INSERT OPTIONS    { options: { test: 'hey' } }
I20171222-17:02:57.487(-8)? LOG 20

Also link to second repo in case you want it. https://github.com/petr24/collection-hooks-test-1.2.1

Not sure where I can go next to debug why the second parameter in 1.6 is a function.