Fix/method binding on knex proxy (#3717)

* knex methods are proxies for context methods ...

... as opposed to injecting the methods directly onto the knex
function.  (Which was then causing `this` to point to the wrong
object when evaluating the context methods)

* Moved CONTEXT_METHODS constant to a higher scope

* mv knex.context -> this.context

* Extracted KNEX_PROPERTY_DEFINITIONS to module scope ...

... which was possible since all of the properties reference
`this` instead of `knex` now

* shallowCloneFunction no longer accesses _context ...

... instead, it uses the normal context property

* transaction method delegates to _transaction ...

... This way, we can be sure that the lower-level details are
consistent across implementations.  Individual implementations
just need to handle the quirks around setting up the `config`
and `outerTx`

* CONTEXT_METHODS shared. Fixed override of withUserParams ...

Restructured the code so that CONTEXT_METHODS populates the
KNEX_PROPERTY_DEFINITIONS with the proxy methods.

In doing so, it also exposed the fact that the withUserParams(..)
method was being overridden on the Transactor instead of its
context.  So, that bug was fixed as well.

* Added a TODO to remove client.makeKnex(..) in future PR

* Added a warning about QueryBuilder.extend(..) and side-effects

lib/transaction.js

@@ -247,7 +247,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'
     );
@@ -256,21 +256,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');
         });
       });