Fix/method binding on knex proxy #3717

briandamaged · 2020-03-10T01:04:22Z

The make-knex.js module was taking a few shortcuts when creating the knex proxy. As a result, most (all?) of the methods within the context object were bound to knex instead of context. This would then cause reference errors when one context method attempted to call another context method.

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

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

briandamaged · 2020-03-10T01:07:20Z

lib/transaction.js

@@ -264,7 +264,7 @@ function makeTransactor(trx, connection, trxClient) {
  transactor.isTransaction = true;
  transactor.userParams = trx.userParams || {};

-  transactor.transaction = function(container, options) {
+  transactor.context.transaction = function(container, options) {


It looks like this line was only working because of the way the proxy shortcuts had been setup. In other words: one bug was canceling another bug.

So, now that the proxies are being setup correctly, we need to make sure we override the method in the correct place. (on the context object rather than the knex function)

briandamaged · 2020-03-10T01:09:37Z

lib/util/make-knex.js

+    },
+    configurable: true,
+  },
+};


I replaced all of the references to knex with references to this instead. This means that the property definitions can now be identical for every knex instance. Therefore, I moved the property definitions into the top-level module so that they could be reused. (Rather than creating a new instance of the definitions for each knex instance)

briandamaged · 2020-03-10T01:17:01Z

lib/util/make-knex.js

+    knex[m] = function(...args) {
+      return this.context[m](...args);
+    };
+  }


This was really the key change. Previously, these methods were not actually being proxied. Instead, they would be injected directly into the knex function. For example:

knex.transaction = context.transaction; knex.raw = context.raw

The problem: this meant that the methods would be invoked through knex now instead of context. Thus, this would point to knex instead of context. As a result, it was impossible to define methods in context such as:

const context = { transaction(container, _options) { // Do some implementation-specific stuff const outerTx = createOuterTxForTopLevelTransactions(); // This line would fail because `this` is pointing to `knex` instead of `context`! return this._transaction(container, options, outerTx); }, _transaction(container, options, outerTx) { // Do stuff that is common across all implementations... } }

briandamaged · 2020-03-10T01:18:14Z

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


Previously, the stacktrace said Function here because the underlying method was not actually being proxied. Instead, it was being invoked through knex, which was a Function.

... instead, it uses the normal context property

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

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.

briandamaged · 2020-03-10T15:16:54Z

lib/util/make-knex.js

    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);
        });
      }


I refactored the transaction(..) method just to demonstrate that the this pointer was referencing the correct object now. So, this allows this.transaction(..) to call this._transaction(..) without encountering any reference errors.

briandamaged · 2020-03-10T17:18:05Z

lib/util/make-knex.js

+  //       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.)


I decided to omit this part from the PR until we have a chance to discuss it.

On one hand, we could probably work around this by using an actual Proxy(..). On the other hand, it seems strange that we're allowing knex data / structures to be directly manipulated in this way. Perhaps we can come up w/ some way to preserve the original QueryBuilder, yet also allow end-users to create/use custom QueryBuilders?

briandamaged · 2020-03-10T18:59:01Z

lib/transaction.js

-        trxClient.transaction(resolve, options, trx).catch(_reject);
-      });
-    }
+    return this._transaction(container, options, trx);


this correctly refers to the transaction.context now, which means that this method call works now. Hooray!

briandamaged · 2020-03-10T19:08:43Z

In the near-term, this PR just fixes the this pointer within the context methods.

Longer-term, this also allows us to treat context like a regular Object or Class. Most importantly, this means that we can subclass it rather than patching its methods on the fly. For example:

// Default implementation
class Context {
  constructor() {
    // ...
  }

  transaction(container, config) {
    // ...
  }
}

// Implementation used specifically by Transactions
class TransactionContext extends Context {
  constructor({outerTx}) {
    super();
    this.outerTx = outerTx;
  }

  transaction(container, options) {
    return this._transaction(container, options, this.outerTx);
  }
}

Of course, there are pros and cons w/ this approach. But, the possibility will at least be open to us now.

kibertoad · 2020-03-11T11:13:32Z

@briandamaged The reason why I'm concerned about using actual Proxy here is performance. See this: https://thecodebarbarian.com/thoughts-on-es6-proxies-performance

briandamaged · 2020-03-11T12:33:01Z

@kibertoad : Agreed about the Proxy performance. That's the main reason I decided to omit that part of the refactoring.

briandamaged · 2020-03-11T16:16:22Z

@maximelkin : do you also want a chance to look at this PR before it gets merged? Or, should we go ahead and merge it?

maximelkin · 2020-03-11T19:04:32Z

Go ahead, if this covered by tests

briandamaged · 2020-03-11T20:01:54Z

@maximelkin : Thx for reviewing it!

briandamaged · 2020-03-11T20:03:12Z

Oh -- and thx @kibertoad as well!

briandamaged added 4 commits March 9, 2020 17:51

knex methods are proxies for context methods ...

121871d

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

f9e9299

mv knex.context -> this.context

753ad0a

Extracted KNEX_PROPERTY_DEFINITIONS to module scope ...

a59bf99

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

briandamaged commented Mar 10, 2020

View reviewed changes

briandamaged added 3 commits March 9, 2020 21:42

shallowCloneFunction no longer accesses _context ...

6557423

... instead, it uses the normal context property

transaction method delegates to _transaction ...

d8bc7f3

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

briandamaged commented Mar 10, 2020

View reviewed changes

briandamaged added 2 commits March 10, 2020 12:02

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

c4731bd

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

e37574c

briandamaged changed the title ~~WIP: Fix/method binding on knex proxy~~ Fix/method binding on knex proxy Mar 10, 2020

briandamaged requested review from kibertoad and maximelkin March 10, 2020 17:12

briandamaged commented Mar 10, 2020

View reviewed changes

kibertoad approved these changes Mar 11, 2020

View reviewed changes

maximelkin approved these changes Mar 11, 2020

View reviewed changes

briandamaged merged commit 37d9c30 into knex:master Mar 11, 2020

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Fix/method binding on knex proxy #3717

Fix/method binding on knex proxy #3717

briandamaged commented Mar 10, 2020

briandamaged Mar 10, 2020

briandamaged Mar 10, 2020

briandamaged Mar 10, 2020

briandamaged Mar 10, 2020

briandamaged Mar 10, 2020

briandamaged Mar 10, 2020

briandamaged Mar 10, 2020

briandamaged commented Mar 10, 2020

kibertoad commented Mar 11, 2020

briandamaged commented Mar 11, 2020

briandamaged commented Mar 11, 2020

maximelkin commented Mar 11, 2020

briandamaged commented Mar 11, 2020

briandamaged commented Mar 11, 2020

Fix/method binding on knex proxy #3717

Fix/method binding on knex proxy #3717

Conversation

briandamaged commented Mar 10, 2020

briandamaged Mar 10, 2020

Choose a reason for hiding this comment

briandamaged Mar 10, 2020

Choose a reason for hiding this comment

briandamaged Mar 10, 2020

Choose a reason for hiding this comment

briandamaged Mar 10, 2020

Choose a reason for hiding this comment

briandamaged Mar 10, 2020

Choose a reason for hiding this comment

briandamaged Mar 10, 2020

Choose a reason for hiding this comment

briandamaged Mar 10, 2020

Choose a reason for hiding this comment

briandamaged commented Mar 10, 2020

kibertoad commented Mar 11, 2020

briandamaged commented Mar 11, 2020

briandamaged commented Mar 11, 2020

maximelkin commented Mar 11, 2020

briandamaged commented Mar 11, 2020

briandamaged commented Mar 11, 2020