Fix event listener duplication

bin/cli.js

@@ -44,7 +44,10 @@ function initKnex(env, opts) {
   checkLocalModule(env);
   if (process.cwd() !== env.cwd) {
     process.chdir(env.cwd);
-    console.log('Working directory changed to', color.magenta(tildify(env.cwd)));
+    console.log(
+      'Working directory changed to',
+      color.magenta(tildify(env.cwd))
+    );
   }
 
   if (!opts.knexfile) {
@@ -102,7 +105,10 @@ function invoke(env) {
     .version(
       color.blue('Knex CLI version: ', color.green(cliPkg.version)) +
         '\n' +
-        color.blue('Local Knex version: ', color.green(env.modulePackage.version)) +
+        color.blue(
+          'Local Knex version: ',
+          color.green(env.modulePackage.version)
+        ) +
         '\n'
     )
     .option('--debug', 'Run with debugging.')

src/util/make-knex.js

@@ -4,7 +4,7 @@ import Migrator from '../migrate/Migrator';
 import Seeder from '../seed/Seeder';
 import FunctionHelper from '../functionhelper';
 import QueryInterface from '../query/methods';
-import { assign } from 'lodash';
+import { assign, merge } from 'lodash';
 import batchInsert from './batchInsert';
 import * as bluebird from 'bluebird';
 
@@ -13,6 +13,7 @@ export default function makeKnex(client) {
   function knex(tableName, options) {
     return createQueryBuilder(knex.context, tableName, options);
   }
+
   redefineProperties(knex, client);
   return knex;
 }
@@ -81,15 +82,16 @@ function initContext(knexFn) {
     withUserParams(params) {
       const knexClone = shallowCloneFunction(knexFn); // We need to include getters in our clone
       if (this.client) {
-        knexClone.client = Object.assign({}, this.client); // Clone client to avoid leaking listeners that are set on it
+        knexClone.client = Object.create(this.client.constructor.prototype); // Clone client to avoid leaking listeners that are set on it
+        merge(knexClone.client, this.client);
         knexClone.client.config = Object.assign({}, this.client.config); // Clone client config to make sure they can be modified independently
-        const parentPrototype = Object.getPrototypeOf(this.client);
-        if (parentPrototype) {
-          Object.setPrototypeOf(knexClone.client, parentPrototype);
-        }
       }
 
       redefineProperties(knexClone, knexClone.client);
+      _copyEventListeners('query', knexFn, knexClone);
+      _copyEventListeners('query-error', knexFn, knexClone);
+      _copyEventListeners('query-response', knexFn, knexClone);
+      _copyEventListeners('start', knexFn, knexClone);
       knexClone.userParams = params;
       return knexClone;
     },
@@ -99,6 +101,12 @@ function initContext(knexFn) {
     knexFn.context = knexContext;
   }
 }
+function _copyEventListeners(eventName, sourceKnex, targetKnex) {
+  const listeners = sourceKnex.listeners(eventName);
+  listeners.forEach((listener) => {
+    targetKnex.on(eventName, listener);
+  });
+}
 
 function redefineProperties(knex, client) {
   // Allow chaining methods from the root object, before
@@ -194,21 +202,36 @@ function redefineProperties(knex, client) {
     knex[key] = ee[key];
   }
 
+  if (knex._internalListeners) {
+    knex._internalListeners.forEach(({ eventName, listener }) => {
+      knex.client.removeListener(eventName, listener); // Remove duplicates for copies
+    });
+  }
+  knex._internalListeners = [];
+
   // Passthrough all "start" and "query" events to the knex object.
-  knex.client.on('start', function(obj) {
+  _addInternalListener(knex, 'start', (obj) => {
     knex.emit('start', obj);
   });
-  knex.client.on('query', function(obj) {
+  _addInternalListener(knex, 'query', (obj) => {
     knex.emit('query', obj);
   });
-  knex.client.on('query-error', function(err, obj) {
+  _addInternalListener(knex, 'query-error', (err, obj) => {
     knex.emit('query-error', err, obj);
   });
-  knex.client.on('query-response', function(response, obj, builder) {
+  _addInternalListener(knex, 'query-response', (response, obj, builder) => {
     knex.emit('query-response', response, obj, builder);
   });
 }
 
+function _addInternalListener(knex, eventName, listener) {
+  knex.client.on(eventName, listener);
+  knex._internalListeners.push({
+    eventName,
+    listener,
+  });
+}
+
 function createQueryBuilder(knexContext, tableName, options) {
   const qb = knexContext.queryBuilder();
   if (!tableName)

test/index.js

@@ -31,6 +31,8 @@ describe('Query Building Tests', function() {
   require('./unit/schema/oracledb');
   require('./unit/migrate/migration-list-resolver');
   require('./unit/seed/seeder');
+  // require('./unit/interface'); ToDo Uncomment after fixed
+  require('./unit/knex');
 });
 
 describe('Integration Tests', function() {

test/integration/builder/additional.js

@@ -986,7 +986,7 @@ module.exports = function(knex) {
         });
     });
 
-    it('Event: does not duplicate listeners on a copy with user params', function() {
+    it('Event: preserves listeners on a copy with user params', function() {
       let queryCount = 0;
 
       const onQueryResponse = function(response, obj, builder) {
@@ -1012,7 +1012,7 @@ module.exports = function(knex) {
         })
         .then(function() {
           expect(Object.keys(knex._events).length).to.equal(1);
-          expect(Object.keys(knexCopy._events).length).to.equal(0);
+          expect(Object.keys(knexCopy._events).length).to.equal(1);
           knex.removeListener('query-response', onQueryResponse);
           expect(Object.keys(knex._events).length).to.equal(0);
           expect(queryCount).to.equal(4);

test/integration/helpers/knex-builder.js

@@ -0,0 +1,15 @@
+const knex = require('../../../knex');
+const config = require('../../knexfile');
+
+/*
+Please do not remove this file even though it is not referenced anywhere.
+This helper is meant for local debugging of specific tests.
+ */
+
+function getSqliteKnex() {
+  return knex(config.sqlite3);
+}
+
+module.exports = {
+  getSqliteKnex,
+};

test/unit/knex.js

@@ -98,10 +98,68 @@ describe('knex', () => {
     const knexWithParams = knex.withUserParams({ userParam: '451' });
 
     expect(knexWithParams.migrate.knex.userParams).to.deep.equal({
+      isProcessingDisabled: true,
+      postProcessResponse: undefined,
       userParam: '451',
+      wrapIdentifier: undefined,
     });
   });
 
+  it('copying does not result in duplicate listeners', () => {
+    const knex = Knex({
+      client: 'sqlite',
+    });
+    const knexWithParams = knex.withUserParams();
+
+    expect(knex.client.listeners('start').length).to.equal(1);
+    expect(knex.client.listeners('query').length).to.equal(1);
+    expect(knex.client.listeners('query-error').length).to.equal(1);
+    expect(knex.client.listeners('query-response').length).to.equal(1);
+
+    expect(knexWithParams.client.listeners('start').length).to.equal(1);
+    expect(knexWithParams.client.listeners('query').length).to.equal(1);
+    expect(knexWithParams.client.listeners('query-error').length).to.equal(1);
+    expect(knexWithParams.client.listeners('query-response').length).to.equal(
+      1
+    );
+  });
+
+  it('listeners added to knex directly get copied correctly', () => {
+    const knex = Knex({
+      client: 'sqlite',
+    });
+    const onQueryResponse = function(response, obj, builder) {};
+    expect(knex.listeners('query-response').length).to.equal(0);
+    knex.on('query-response', onQueryResponse);
+
+    const knexWithParams = knex.withUserParams();
+
+    expect(knex.listeners('query-response').length).to.equal(1);
+    expect(knexWithParams.listeners('query-response').length).to.equal(1);
+  });
+
+  it('adding listener to copy does not affect base knex', () => {
+    const knex = Knex({
+      client: 'sqlite',
+    });
+
+    expect(knex.client.listeners('start').length).to.equal(1);
+    expect(knex.client.listeners('query').length).to.equal(1);
+    expect(knex.client.listeners('query-error').length).to.equal(1);
+    expect(knex.client.listeners('query-response').length).to.equal(1);
+
+    const knexWithParams = knex.withUserParams();
+    knexWithParams.client.on('query', (obj) => {
+      knexWithParams.emit('query', obj);
+    });
+
+    expect(knex.client.listeners('start').length).to.equal(1);
+    expect(knex.client.listeners('query').length).to.equal(1);
+    expect(knex.client.listeners('query-error').length).to.equal(1);
+    expect(knex.client.listeners('query-response').length).to.equal(1);
+    expect(knexWithParams.client.listeners('query').length).to.equal(2);
+  });
+
   it('sets correct postProcessResponse for builders instantiated from clone', () => {
     const knex = Knex({
       client: 'sqlite',
@@ -171,8 +229,10 @@ describe('knex', () => {
   });
 
   it('throws if client module has not been installed', () => {
-    expect(Knex({ client: 'oracle' })).to.throw(
-      /Knex: run\n$ npm install oracle/
+    expect(() => {
+      Knex({ client: 'oracledb', connection: {} });
+    }).to.throw(
+      "Knex: run\n$ npm install oracledb --save\nCannot find module 'oracledb'"
     );
   });