Bugs in upsert hooks (and fixed code formatting)

.eslintrc.json

@@ -6,7 +6,6 @@
     "standard"
   ],
   "parserOptions": {
-    "ecmaVersion": 6,
     "sourceType": "module",
     "ecmaFeatures": {
       "modules": true

collection-hooks.js

@@ -1,10 +1,7 @@
-import { Meteor } from 'meteor/meteor';
-import { Mongo } from 'meteor/mongo';
-import { EJSON } from 'meteor/ejson';
-import { LocalCollection } from 'meteor/minimongo';
-
-/* global Package Meteor Mongo LocalCollection CollectionHooks _ EJSON */
-/* eslint-disable no-proto, no-native-reassign, no-global-assign */
+import { Meteor } from 'meteor/meteor'
+import { Mongo } from 'meteor/mongo'
+import { EJSON } from 'meteor/ejson'
+import { LocalCollection } from 'meteor/minimongo'
 
 // Relevant AOP terminology:
 // Aspect: User code that runs before/after (hook)
@@ -19,44 +16,15 @@ export const CollectionHooks = {
     all: { insert: {}, update: {}, remove: {}, find: {}, findOne: {}, all: {} }
   },
   directEnv: new Meteor.EnvironmentVariable(),
-  directOp(func) {
+  directOp (func) {
     return this.directEnv.withValue(true, func)
   },
-  hookedOp(func) {
+  hookedOp (func) {
     return this.directEnv.withValue(false, func)
   }
 }
 
-CollectionHooks.getUserId = function getUserId() {
-  let userId
-
-  if (Meteor.isClient) {
-    Tracker.nonreactive(function () {
-      userId = Meteor.userId && Meteor.userId()
-    })
-  }
-
-  if (Meteor.isServer) {
-    try {
-      // Will throw an error unless within method call.
-      // Attempt to recover gracefully by catching:
-      userId = Meteor.userId && Meteor.userId()
-    } catch (e) { }
-
-    if (userId == null) {
-      // Get the userId if we are in a publish function.
-      userId = publishUserId.get()
-    }
-  }
-
-  if (userId == null) {
-    userId = CollectionHooks.defaultUserId
-  }
-
-  return userId
-}
-
-CollectionHooks.extendCollectionInstance = function extendCollectionInstance(self, constructor) {
+CollectionHooks.extendCollectionInstance = function extendCollectionInstance (self, constructor) {
   // Offer a public API to allow the user to define aspects
   // Example: collection.before.insert(func);
   ['before', 'after'].forEach(function (pointcut) {
@@ -74,13 +42,13 @@ CollectionHooks.extendCollectionInstance = function extendCollectionInstance(sel
         })
 
         return {
-          replace(aspect, options) {
+          replace (aspect, options) {
             self._hookAspects[method][pointcut].splice(len - 1, 1, {
-              aspect: aspect,
+              aspect,
               options: CollectionHooks.initOptions(options, pointcut, method)
             })
           },
-          remove() {
+          remove () {
             self._hookAspects[method][pointcut].splice(len - 1, 1)
           }
         }
@@ -134,8 +102,8 @@ CollectionHooks.extendCollectionInstance = function extendCollectionInstance(sel
         function (doc) {
           return (
             typeof self._transform === 'function'
-            ? function (d) { return self._transform(d || doc) }
-            : function (d) { return d || doc }
+              ? function (d) { return self._transform(d || doc) }
+              : function (d) { return d || doc }
           )
         },
         args,
@@ -149,16 +117,16 @@ CollectionHooks.defineAdvice = (method, advice) => {
   advices[method] = advice
 }
 
-CollectionHooks.getAdvice = method => advices[method];
+CollectionHooks.getAdvice = method => advices[method]
 
 CollectionHooks.initOptions = (options, pointcut, method) =>
-  CollectionHooks.extendOptions(CollectionHooks.defaults, options, pointcut, method);
+  CollectionHooks.extendOptions(CollectionHooks.defaults, options, pointcut, method)
 
 CollectionHooks.extendOptions = (source, options, pointcut, method) =>
-  ({...options, ...source.all.all, ...source[pointcut].all, ...source.all[method], ...source[pointcut][method]});
+  ({ ...options, ...source.all.all, ...source[pointcut].all, ...source.all[method], ...source[pointcut][method] })
 
 CollectionHooks.getDocs = function getDocs (collection, selector, options) {
-  const findOptions = {transform: null, reactive: false} // added reactive: false
+  const findOptions = { transform: null, reactive: false } // added reactive: false
 
   /*
   // No "fetch" support at this time.
@@ -181,7 +149,7 @@ CollectionHooks.getDocs = function getDocs (collection, selector, options) {
       findOptions.limit = 1
     }
     const { multi, upsert, ...rest } = options
-    Object.assign(findOptions, rest);
+    Object.assign(findOptions, rest)
   }
 
   // Unlike validators, we iterate over multiple docs, so use
@@ -204,7 +172,7 @@ CollectionHooks.normalizeSelector = function (selector) {
 // ~/.meteor/packages/mongo-livedata/collection.js
 // It's contained in these utility functions to make updates easier for us in
 // case this code changes.
-CollectionHooks.getFields = function getFields(mutator) {
+CollectionHooks.getFields = function getFields (mutator) {
   // compute modified fields
   const fields = []
   // ====ADDED START=======================
@@ -259,12 +227,12 @@ CollectionHooks.reassignPrototype = function reassignPrototype (instance, constr
   // Note: Assigning a prototype dynamically has performance implications
   if (hasSetPrototypeOf) {
     Object.setPrototypeOf(instance, constr.prototype)
-  } else if (instance.__proto__) {
-    instance.__proto__ = constr.prototype
+  } else if (instance.__proto__) { // eslint-disable-line no-proto
+    instance.__proto__ = constr.prototype // eslint-disable-line no-proto
   }
 }
 
-CollectionHooks.wrapCollection = function wrapCollection(ns, as) {
+CollectionHooks.wrapCollection = function wrapCollection (ns, as) {
   if (!as._CollectionConstructor) as._CollectionConstructor = as.Collection
   if (!as._CollectionPrototype) as._CollectionPrototype = new as.Collection(null)
 
@@ -282,7 +250,7 @@ CollectionHooks.wrapCollection = function wrapCollection(ns, as) {
   ns.Collection.prototype = proto
   ns.Collection.prototype.constructor = ns.Collection
 
-  for (let prop of Object.keys(constructor)) {
+  for (const prop of Object.keys(constructor)) {
     ns.Collection[prop] = constructor[prop]
   }
 
@@ -299,9 +267,3 @@ if (typeof Mongo !== 'undefined') {
 } else {
   CollectionHooks.wrapCollection(Meteor, Meteor)
 }
-
-// Make the above available for packages with hooks that want to determine
-// whether they are running inside a publish function or not.
-CollectionHooks.isWithinPublish = function isWithinPublish() {
-  return publishUserId.get() !== undefined
-}

findone.js

@@ -1,9 +1,6 @@
 import { CollectionHooks } from './collection-hooks'
 
 CollectionHooks.defineAdvice('findOne', function (userId, _super, instance, aspects, getTransform, args, suppressAspects) {
-  // args[0] : selector
-  // args[1] : options
-
   const ctx = { context: this, _super, args }
   const selector = CollectionHooks.normalizeSelector(instance._getFindSelector(args))
   const options = instance._getFindOptions(args)

insert.js

@@ -1,9 +1,10 @@
-import { EJSON } from 'meteor/ejson';
-import { CollectionHooks } from './collection-hooks';
+import { EJSON } from 'meteor/ejson'
+import { Mongo } from 'meteor/mongo'
+import { CollectionHooks } from './collection-hooks'
 
 CollectionHooks.defineAdvice('insert', function (userId, _super, instance, aspects, getTransform, args, suppressAspects) {
-  const ctx = {context: this, _super, args}
-  let [doc, callback] = args;
+  const ctx = { context: this, _super, args }
+  let [doc, callback] = args
   const async = typeof callback === 'function'
   let abort
   let ret
@@ -12,7 +13,7 @@ CollectionHooks.defineAdvice('insert', function (userId, _super, instance, aspec
   if (!suppressAspects) {
     try {
       aspects.before.forEach((o) => {
-        const r = o.aspect.call({transform: getTransform(doc), ...ctx}, userId, doc)
+        const r = o.aspect.call({ transform: getTransform(doc), ...ctx }, userId, doc)
         if (r === false) abort = true
       })
 
@@ -39,7 +40,7 @@ CollectionHooks.defineAdvice('insert', function (userId, _super, instance, aspec
       doc._id = id
     }
     if (!suppressAspects) {
-      const lctx = {transform: getTransform(doc), _id: id, err, ...ctx}
+      const lctx = { transform: getTransform(doc), _id: id, err, ...ctx }
       aspects.after.forEach((o) => {
         o.aspect.call(lctx, userId, doc)
       })

remove.js

@@ -1,15 +1,15 @@
-import { EJSON } from 'meteor/ejson';
-import { CollectionHooks } from './collection-hooks';
+import { EJSON } from 'meteor/ejson'
+import { CollectionHooks } from './collection-hooks'
 
-const isEmpty = a => !Array.isArray(a) || !a.length;
+const isEmpty = a => !Array.isArray(a) || !a.length
 
 CollectionHooks.defineAdvice('remove', function (userId, _super, instance, aspects, getTransform, args, suppressAspects) {
-  const ctx = {context: this, _super, args};
-  const [ selector, callback ] = args;
+  const ctx = { context: this, _super, args }
+  const [selector, callback] = args
   const async = typeof callback === 'function'
   let docs
   let abort
-  let prev = []
+  const prev = []
 
   if (!suppressAspects) {
     try {
@@ -25,7 +25,7 @@ CollectionHooks.defineAdvice('remove', function (userId, _super, instance, aspec
       // before
       aspects.before.forEach((o) => {
         docs.forEach((doc) => {
-          const r = o.aspect.call({transform: getTransform(doc), ...ctx}, userId, doc)
+          const r = o.aspect.call({ transform: getTransform(doc), ...ctx }, userId, doc)
           if (r === false) abort = true
         })
       })
@@ -41,7 +41,7 @@ CollectionHooks.defineAdvice('remove', function (userId, _super, instance, aspec
     if (!suppressAspects) {
       aspects.after.forEach((o) => {
         prev.forEach((doc) => {
-          o.aspect.call({transform: getTransform(doc), err, ...ctx}, userId, doc)
+          o.aspect.call({ transform: getTransform(doc), err, ...ctx }, userId, doc)
         })
       })
     }

server.js

@@ -36,7 +36,7 @@ Meteor.publish = function (name, handler, options) {
 
 // Make the above available for packages with hooks that want to determine
 // whether they are running inside a publish function or not.
-CollectionHooks.isWithinPublish = () => publishUserId.get() !== void 0
+CollectionHooks.isWithinPublish = () => publishUserId.get() !== undefined
 
 export {
   CollectionHooks

tests/find_findone_userid.js

@@ -117,14 +117,14 @@ if (Meteor.isServer) {
 }
 
 if (Meteor.isClient) {
-  function cleanup () {
+  const cleanup = () => {
     beforeFindUserId = null
     afterFindUserId = null
     beforeFindOneUserId = null
     afterFindOneUserId = null
   }
 
-  function withLogin (testFunc) {
+  const withLogin = (testFunc) => {
     return function (...args) {
       const wrapper = (cb) => {
         InsecureLogin.ready(() => {

tests/find_users.js

@@ -1,6 +1,6 @@
-import { Meteor } from 'meteor/meteor';
-import { Tinytest } from 'meteor/tinytest';
-import { InsecureLogin } from './insecure_login';
+import { Meteor } from 'meteor/meteor'
+import { Tinytest } from 'meteor/tinytest'
+import { InsecureLogin } from './insecure_login'
 
 Tinytest.addAsync('users - find hooks should be capable of being used on special Meteor.users collection', function (test, next) {
   const aspect1 = Meteor.users.before.find(function (userId, selector, options) {
@@ -16,10 +16,10 @@ Tinytest.addAsync('users - find hooks should be capable of being used on special
   })
 
   InsecureLogin.ready(function () {
-    const selector = {test: 1}
+    const selector = { test: 1 }
     Meteor.users.find(selector)
-    test.equal(selector.hasOwnProperty('a'), true)
-    test.equal(selector.hasOwnProperty('b'), true)
+    test.equal(Object.prototype.hasOwnProperty.call(selector, 'a'), true)
+    test.equal(Object.prototype.hasOwnProperty.call(selector, 'b'), true)
     aspect1.remove()
     aspect2.remove()
 
@@ -34,7 +34,7 @@ Tinytest.addAsync('users - find hooks should be capable of being used on wrapped
     return Object.assign(this, doc)
   }
 
-  Meteor.users.__transform = doc => new TestUser(doc);
+  Meteor.users.__transform = doc => new TestUser(doc)
 
   const MeteorUsersFind = Meteor.users.find
 
@@ -57,8 +57,8 @@ Tinytest.addAsync('users - find hooks should be capable of being used on wrapped
   InsecureLogin.ready(function () {
     const selector = { test: 1 }
     Meteor.users.find(selector)
-    test.equal(selector.hasOwnProperty('a'), true)
-    test.equal(selector.hasOwnProperty('b'), true)
+    test.equal(Object.prototype.hasOwnProperty.call(selector, 'a'), true)
+    test.equal(Object.prototype.hasOwnProperty.call(selector, 'b'), true)
     aspect1.remove()
     aspect2.remove()
 

tests/server/fetch.js

@@ -1,5 +1,6 @@
-import { Mongo } from 'meteor/mongo';
-import { Tinytest } from 'meteor/tinytest';
+import { Mongo } from 'meteor/mongo'
+import { Tinytest } from 'meteor/tinytest'
+import { InsecureLogin } from '../insecure_login'
 
 Tinytest.addAsync('general - local collection documents should only have fetched fields', function (test, next) {
   const collection = new Mongo.Collection(null)
@@ -12,7 +13,7 @@ Tinytest.addAsync('general - local collection documents should only have fetched
     const fields = ['fetch_value1', 'fetch_value2']
 
     collection.after.update(function (userId, doc, fieldNames, modifier) {
-      let { _id, ...docKeys } = Object.keys(doc);
+      const { _id, ...docKeys } = Object.keys(doc)
       test.equal(same(docKeys, fields), true)
       next()
     }, {

tests/transform.js

@@ -1,18 +1,18 @@
-import { Mongo } from 'meteor/mongo';
-import { Tinytest } from 'meteor/tinytest';
-import { InsecureLogin } from './insecure_login';
+import { Mongo } from 'meteor/mongo'
+import { Tinytest } from 'meteor/tinytest'
+import { InsecureLogin } from './insecure_login'
 
-const isFunction = (fn) => typeof fn === 'function';
+const isFunction = (fn) => typeof fn === 'function'
 
 Tinytest.addAsync('general - hook callbacks should have this.transform function that works', function (test, next) {
   const collection = new Mongo.Collection(null, {
     transform: doc => ({ ...doc, isTransformed: true })
   })
 
   collection.allow({
-    insert() { return true },
-    update() { return true },
-    remove() { return true }
+    insert () { return true },
+    update () { return true },
+    remove () { return true }
   })
 
   const counts = {
@@ -41,11 +41,11 @@ Tinytest.addAsync('general - hook callbacks should have this.transform function
     // to pass? Probably not. Think more on this -- it could be that we simply
     // shouldn't be running a .transform() in a before.insert -- how will we
     // know the _id? And that's what transform is complaining about.
-    collection.insert({_id: '1', start_value: true}, function (err, id) {
+    collection.insert({ _id: '1', start_value: true }, function (err, id) {
       if (err) throw err
-      collection.update({_id: id}, {$set: {update_value: true}}, function (err) {
+      collection.update({ _id: id }, { $set: { update_value: true } }, function (err) {
         if (err) throw err
-        collection.remove({_id: id}, function (nil) {
+        collection.remove({ _id: id }, function (nil) {
           test.equal(counts.before.insert, 1, 'before insert should have 1 count')
           test.equal(counts.before.update, 1, 'before update should have 1 count')
           test.equal(counts.before.remove, 1, 'before remove should have 1 count')

tests/update_both.js

@@ -81,7 +81,7 @@ if (Meteor.isClient) {
 
       collection2.after.update(function (userId, doc, fieldNames, modifier) {
         test.equal(doc.update_value, true)
-        test.equal(this.previous.hasOwnProperty('update_value'), false)
+        test.equal(Object.prototype.hasOwnProperty.call(this.previous, 'update_value'), false)
         n()
       })