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

Options on insert #233

Open
petr24 opened this issue Dec 20, 2017 · 11 comments
Open

Options on insert #233

petr24 opened this issue Dec 20, 2017 · 11 comments

Comments

@petr24
Copy link

petr24 commented Dec 20, 2017

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
Copy link
Member

zimme commented Dec 20, 2017

This package doesn't work or doesn't work properly with Meteor 1.6+
meteor/meteor#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
Copy link
Author

petr24 commented Dec 20, 2017

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
Copy link
Member

zimme commented Dec 20, 2017

Version previous to Meteor 1.6.1 "should" work.

@petr24
Copy link
Author

petr24 commented Dec 20, 2017

I am currently running 1.6.0.1

@zimme
Copy link
Member

zimme commented Dec 20, 2017

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
Copy link
Author

petr24 commented Dec 20, 2017

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

@petr24
Copy link
Author

petr24 commented Dec 22, 2017

@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
Copy link
Author

petr24 commented Dec 22, 2017

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
Copy link
Member

zimme commented Dec 22, 2017

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
Copy link
Author

petr24 commented Dec 22, 2017

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
Copy link
Author

petr24 commented Dec 23, 2017

@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.

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