Fix/method binding on knex proxy

lib/transaction.js

@@ -255,7 +255,7 @@ finallyMixin(Transaction.prototype);
 function makeTransactor(trx, connection, trxClient) {
   const transactor = makeKnex(trxClient);
 
-  transactor.withUserParams = () => {
+  transactor.context.withUserParams = () => {
     throw new Error(
       'Cannot set user params on a transaction - it can only inherit params from main knex instance'
     );
@@ -264,21 +264,16 @@ function makeTransactor(trx, connection, trxClient) {
   transactor.isTransaction = true;
   transactor.userParams = trx.userParams || {};
 
-  transactor.transaction = function(container, options) {
+  transactor.context.transaction = function(container, options) {
     if (!options) {
       options = { doNotRejectOnRollback: true };
     } else if (isUndefined(options.doNotRejectOnRollback)) {
       options.doNotRejectOnRollback = true;
     }
 
-    if (container) {
-      return trxClient.transaction(container, options, trx);
-    } else {
-      return new Promise((resolve, _reject) => {
-        trxClient.transaction(resolve, options, trx).catch(_reject);
-      });
-    }
+    return this._transaction(container, options, trx);
   };
+
   transactor.savepoint = function(container, options) {
     return transactor.transaction(container, options);
   };

lib/util/make-knex.js

@@ -7,6 +7,92 @@ const QueryInterface = require('../query/methods');
 const { merge, isUndefined } = require('lodash');
 const batchInsert = require('./batchInsert');
 
+// Javascript does not officially support "callable objects".  Instead,
+// you must create a regular Function and inject properties/methods
+// into it.  In other words: you can't leverage Prototype Inheritance
+// to share the property/method definitions.
+//
+// To work around this, we're creating an Object Property Definition.
+// This allow us to quickly inject everything into the `knex` function
+// via the `Object.defineProperties(..)` function.  More importantly,
+// it allows the same definitions to be shared across `knex` instances.
+const KNEX_PROPERTY_DEFINITIONS = {
+  client: {
+    get() {
+      return this.context.client;
+    },
+    set(client) {
+      this.context.client = client;
+    },
+    configurable: true,
+  },
+
+  userParams: {
+    get() {
+      return this.context.userParams;
+    },
+    set(userParams) {
+      this.context.userParams = userParams;
+    },
+    configurable: true,
+  },
+
+  schema: {
+    get() {
+      return this.client.schemaBuilder();
+    },
+    configurable: true,
+  },
+
+  migrate: {
+    get() {
+      return new Migrator(this);
+    },
+    configurable: true,
+  },
+
+  seed: {
+    get() {
+      return new Seeder(this);
+    },
+    configurable: true,
+  },
+
+  fn: {
+    get() {
+      return new FunctionHelper(this.client);
+    },
+    configurable: true,
+  },
+};
+
+// `knex` instances serve as proxies around `context` objects.  So, calling
+// any of these methods on the `knex` instance will forward the call to
+// the `knex.context` object. This ensures that `this` will correctly refer
+// to `context` within each of these methods.
+const CONTEXT_METHODS = [
+  'raw',
+  'batchInsert',
+  'transaction',
+  'transactionProvider',
+  'initialize',
+  'destroy',
+  'ref',
+  'withUserParams',
+  'queryBuilder',
+  'disableProcessing',
+  'enableProcessing',
+];
+
+for (const m of CONTEXT_METHODS) {
+  KNEX_PROPERTY_DEFINITIONS[m] = {
+    value: function(...args) {
+      return this.context[m](...args);
+    },
+    configurable: true,
+  };
+}
+
 function makeKnex(client) {
   // The object we're potentially using to kick off an initial chain.
   function knex(tableName, options) {
@@ -38,20 +124,25 @@ function initContext(knexFn) {
     // when transaction is ready to be used.
     transaction(container, _config) {
       const config = Object.assign({}, _config);
-      config.userParams = this.userParams || {}
-      if(isUndefined(config.doNotRejectOnRollback)) {
+      config.userParams = this.userParams || {};
+      if (isUndefined(config.doNotRejectOnRollback)) {
         // Backwards-compatibility: default value changes depending upon
         // whether or not a `container` was provided.
         config.doNotRejectOnRollback = !container;
       }
 
+      return this._transaction(container, config);
+    },
 
-      if(container) {
-        const trx = this.client.transaction(container, config);
+    // Internal method that actually establishes the Transaction.  It makes no assumptions
+    // about the `config` or `outerTx`, and expects the caller to handle these details.
+    _transaction(container, config, outerTx = null) {
+      if (container) {
+        const trx = this.client.transaction(container, config, outerTx);
         return trx;
       } else {
         return new Promise((resolve, reject) => {
-          const trx = this.client.transaction(resolve, config);
+          const trx = this.client.transaction(resolve, config, outerTx);
           trx.catch(reject);
         });
       }
@@ -138,89 +229,46 @@ function _copyEventListeners(eventName, sourceKnex, targetKnex) {
 function redefineProperties(knex, client) {
   // Allow chaining methods from the root object, before
   // any other information is specified.
+  //
+  // TODO: `QueryBuilder.extend(..)` allows new QueryBuilder
+  //       methods to be introduced via external components.
+  //       As a side-effect, it also pushes the new method names
+  //       into the `QueryInterface` array.
+  //
+  //       The Problem: due to the way the code is currently
+  //       structured, these new methods cannot be retroactively
+  //       injected into existing `knex` instances!  As a result,
+  //       some `knex` instances will support the methods, and
+  //       others will not.
+  //
+  //       We should revisit this once we figure out the desired
+  //       behavior / usage.  For instance: do we really want to
+  //       allow external components to directly manipulate `knex`
+  //       data structures?  Or, should we come up w/ a different
+  //       approach that avoids side-effects / mutation?
+  //
+  //      (FYI: I noticed this issue because I attempted to integrate
+  //       this logic directly into the `KNEX_PROPERTY_DEFINITIONS`
+  //       construction.  However, `KNEX_PROPERTY_DEFINITIONS` is
+  //       constructed before any `knex` instances are created.
+  //       As a result, the method extensions were missing from all
+  //       `knex` instances.)
   QueryInterface.forEach(function(method) {
     knex[method] = function() {
-      const builder = knex.queryBuilder();
+      const builder = this.queryBuilder();
       return builder[method].apply(builder, arguments);
     };
   });
 
-  Object.defineProperties(knex, {
-    context: {
-      get() {
-        return knex._context;
-      },
-      set(context) {
-        knex._context = context;
-
-        // Redefine public API for knex instance that would be proxying methods from correct context
-        knex.raw = context.raw;
-        knex.batchInsert = context.batchInsert;
-        knex.transaction = context.transaction;
-        knex.transactionProvider = context.transactionProvider;
-        knex.initialize = context.initialize;
-        knex.destroy = context.destroy;
-        knex.ref = context.ref;
-        knex.withUserParams = context.withUserParams;
-        knex.queryBuilder = context.queryBuilder;
-        knex.disableProcessing = context.disableProcessing;
-        knex.enableProcessing = context.enableProcessing;
-      },
-      configurable: true,
-    },
-
-    client: {
-      get() {
-        return knex.context.client;
-      },
-      set(client) {
-        knex.context.client = client;
-      },
-      configurable: true,
-    },
-
-    userParams: {
-      get() {
-        return knex.context.userParams;
-      },
-      set(userParams) {
-        knex.context.userParams = userParams;
-      },
-      configurable: true,
-    },
-
-    schema: {
-      get() {
-        return knex.client.schemaBuilder();
-      },
-      configurable: true,
-    },
-
-    migrate: {
-      get() {
-        return new Migrator(knex);
-      },
-      configurable: true,
-    },
-
-    seed: {
-      get() {
-        return new Seeder(knex);
-      },
-      configurable: true,
-    },
-
-    fn: {
-      get() {
-        return new FunctionHelper(knex.client);
-      },
-      configurable: true,
-    },
-  });
+  Object.defineProperties(knex, KNEX_PROPERTY_DEFINITIONS);
 
   initContext(knex);
   knex.client = client;
+
+  // TODO: It looks like this field is never actually used.
+  //       It should probably be removed in a future PR.
   knex.client.makeKnex = makeKnex;
+
   knex.userParams = {};
 
   // Hook up the "knex" object as an EventEmitter.
@@ -283,7 +331,7 @@ function shallowCloneFunction(originalFunction) {
 
   const clonedFunction = knexFnWrapper.bind(fnContext);
   Object.assign(clonedFunction, originalFunction);
-  clonedFunction._context = knexContext;
+  clonedFunction.context = knexContext;
   return clonedFunction;
 }
 

test/integration/builder/additional.js

@@ -1089,7 +1089,7 @@ module.exports = function(knex) {
       });
       it('should capture stack trace on raw query', () => {
         return knex.raw('select * from some_nonexisten_table').catch((err) => {
-          expect(err.stack.split('\n')[2]).to.match(/at Function\.raw \(/); // the index 2 might need adjustment if the code is refactored
+          expect(err.stack.split('\n')[2]).to.match(/at Object\.raw \(/); // the index 2 might need adjustment if the code is refactored
           expect(typeof err.originalStack).to.equal('string');
         });
       });