Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Implement support for returning started transaction without using transaction() methods callback function #3099

Merged
merged 17 commits into from May 27, 2019
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
1 change: 1 addition & 0 deletions .eslintrc.js
Expand Up @@ -23,5 +23,6 @@ module.exports = {
env: {
node: true,
mocha: true,
es6: true,
},
};
2 changes: 1 addition & 1 deletion src/dialects/sqlite3/index.js
Expand Up @@ -164,7 +164,7 @@ assign(Client_SQLite3.prototype, {

formatter() {
return new SQLite3_Formatter(this, ...arguments);
}
},
});

export default Client_SQLite3;
24 changes: 22 additions & 2 deletions src/transaction.js
Expand Up @@ -18,6 +18,16 @@ export default class Transaction extends EventEmitter {

const txid = (this.txid = uniqueId('trx'));

// If there is no container provided, assume user wants to get instance of transaction and use it directly
if (!container) {
this.initPromise = new Promise((resolve, reject) => {
this.initRejectFn = reject;
container = (transactor) => {
resolve(transactor);
};
});
}

this.client = client;
this.logger = client.logger;
this.outerTx = outerTx;
Expand Down Expand Up @@ -67,14 +77,24 @@ export default class Transaction extends EventEmitter {
}
return null;
})
.catch((e) => this._rejecter(e));
.catch((e) => {
if (this.initRejectFn) {
this.initRejectFn();
}
return this._rejecter(e);
});

return new Promise((resolver, rejecter) => {
this._resolver = resolver;
this._rejecter = rejecter;
});
}
);
).catch((err) => {
if (this.initRejectFn) {
this.initRejectFn(err);
}
throw err;
});

this._completed = false;

Expand Down
26 changes: 23 additions & 3 deletions src/util/make-knex.js
Expand Up @@ -34,12 +34,31 @@ function initContext(knexFn) {
return batchInsert(this, table, batch, chunkSize);
},

// Runs a new transaction, taking a container and returning a promise
// for when the transaction is resolved.
// Creates a new transaction.
// If container is provided, returns a promise for when the transaction is resolved.
// If container is not provided, returns a promise with a transaction that is resolved
// when transaction is ready to be used.
transaction(container, config) {
const trx = this.client.transaction(container, config);
trx.userParams = this.userParams;
return trx;

if (container) {
elhigu marked this conversation as resolved.
Show resolved Hide resolved
return trx;
}
// If no container was passed, assume user wants to get a transaction and use it directly
else {
return trx.initPromise;
}
},

transactionProvider(config) {
let trx;
return () => {
if (!trx) {
trx = this.transaction(config);
}
return trx;
};
},

// Typically never needed, initializes the pool for a knex client.
Expand Down Expand Up @@ -131,6 +150,7 @@ function redefineProperties(knex, client) {
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;
Expand Down
20 changes: 20 additions & 0 deletions test/integration/builder/transaction.js
Expand Up @@ -35,6 +35,26 @@ module.exports = function(knex) {
});
});

it('supports direct retrieval of a transaction without a callback', () => {
const trxPromise = knex.transaction();
const query =
knex.client.driverName === 'oracledb'
? '1 as "result" from DUAL'
: '1 as result';

let transaction;
return trxPromise
.then((trx) => {
transaction = trx;
expect(trx.client.transacting).to.equal(true);
return knex.transacting(trx).select(knex.raw(query));
})
.then((rows) => {
expect(rows[0].result).to.equal(1);
return transaction.commit();
});
});

it('should throw when null transaction is sent to transacting', function() {
return knex
.transaction(function(t) {
Expand Down
44 changes: 27 additions & 17 deletions test/knexfile.js
Expand Up @@ -19,6 +19,16 @@ var pool = {
},
};

var poolSqlite = {
min: 0,
max: 1,
acquireTimeoutMillis: 5000,
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If possible we could try to have subsecond timeouts in tests to prevent test runs to take too long just waiting around.

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Tried reducing it, but I think I was getting some flaky tests that way. Decreased it, let's see how it goes.

afterCreate: function(connection, callback) {
assert.ok(typeof connection.__knexUid !== 'undefined');
callback(null, connection);
},
};

var mysqlPool = _.extend({}, pool, {
afterCreate: function(connection, callback) {
Promise.promisify(connection.query, { context: connection })(
Expand Down Expand Up @@ -50,8 +60,8 @@ var testConfigs = {
charset: 'utf8',
},
pool: mysqlPool,
migrations: migrations,
seeds: seeds,
migrations,
seeds,
},

mysql2: {
Expand All @@ -65,8 +75,8 @@ var testConfigs = {
charset: 'utf8',
},
pool: mysqlPool,
migrations: migrations,
seeds: seeds,
migrations,
seeds,
},

oracledb: {
Expand All @@ -78,8 +88,8 @@ var testConfigs = {
// https://github.com/oracle/node-oracledb/issues/525
stmtCacheSize: 0,
},
pool: pool,
migrations: migrations,
pool,
migrations,
},

postgres: {
Expand All @@ -92,9 +102,9 @@ var testConfigs = {
user: 'testuser',
password: 'knextest',
},
pool: pool,
migrations: migrations,
seeds: seeds,
pool,
migrations,
seeds,
},

redshift: {
Expand All @@ -107,19 +117,19 @@ var testConfigs = {
port: '5439',
host: process.env.REDSHIFT_HOST || '127.0.0.1',
},
pool: pool,
migrations: migrations,
seeds: seeds,
pool,
migrations,
seeds,
},

sqlite3: {
client: 'sqlite3',
connection: testConfig.sqlite3 || {
filename: __dirname + '/test.sqlite3',
},
pool,
migrations: migrations,
seeds: seeds,
pool: poolSqlite,
migrations,
seeds,
},

mssql: {
Expand All @@ -132,8 +142,8 @@ var testConfigs = {
database: 'knex_test',
},
pool: pool,
migrations: migrations,
seeds: seeds,
migrations,
seeds,
},
};

Expand Down
34 changes: 34 additions & 0 deletions test/unit/knex.js
Expand Up @@ -310,6 +310,40 @@ describe('knex', () => {
});
});

it('propagates error correctly when all connections are in use', function() {
this.timeout(30000);
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why this needs 30secs timeout? 🤔

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It is unlikely to use any more than just a couple of seconds, it is going to crash and be handled way earlier than that.

const knex = Knex(sqliteConfig);
return knex
.transaction()
.then(() => {
return knex.transaction();
})
.then(() => {
throw new Error('Should not reach here');
})
.catch((err) => {
expect(err.message).to.include('Timeout acquiring a connection');
});
});

it('supports direct retrieval of a transaction from provider', () => {
const knex = Knex(sqliteConfig);
elhigu marked this conversation as resolved.
Show resolved Hide resolved
const trxProvider = knex.transactionProvider();
const trxPromise = trxProvider();

let transaction;
return trxPromise
.then((trx) => {
transaction = trx;
expect(trx.client.transacting).to.equal(true);
return knex.transacting(trx).select(knex.raw('1 as result'));
})
.then((rows) => {
expect(rows[0].result).to.equal(1);
return transaction.commit();
});
});

it('creating transaction copy with user params should throw an error', () => {
if (!sqliteConfig) {
return;
Expand Down
2 changes: 1 addition & 1 deletion test/unit/query/builder.js
Expand Up @@ -1427,7 +1427,7 @@ describe('QueryBuilder', function() {
sql:
'select * from `users` where (`a`, `b`) in ( values (?, ?), (?, ?), (?, ?))',
bindings: [1, 2, 3, 4, 5, 6],
}
},
}
);
});
Expand Down
4 changes: 2 additions & 2 deletions test/unit/schema/oracledb.js
Expand Up @@ -672,7 +672,7 @@ describe('OracleDb SchemaBuilder', function() {
tableSql = client
.schemaBuilder()
.table('users', function() {
this.dateTime('foo', {useTz: true});
this.dateTime('foo', { useTz: true });
})
.toSQL();
equal(1, tableSql.length);
Expand All @@ -694,7 +694,7 @@ describe('OracleDb SchemaBuilder', function() {
tableSql = client
.schemaBuilder()
.table('users', function() {
this.dateTime('foo', {useTz: false});
this.dateTime('foo', { useTz: false });
})
.toSQL();
equal(1, tableSql.length);
Expand Down
11 changes: 10 additions & 1 deletion types/index.d.ts
Expand Up @@ -266,8 +266,17 @@ interface Knex<TRecord extends {} = any, TResult = unknown[]>
__knex__: string;

raw: Knex.RawBuilder<TRecord, TResult>;

transactionProvider(
config?: any
): () => Promise<Knex.Transaction>;
transaction(
transactionScope?: undefined | null,
config?: any
): Promise<Knex.Transaction>;
transaction<T>(
transactionScope: (trx: Knex.Transaction) => Promise<T> | Bluebird<T> | void
transactionScope: (trx: Knex.Transaction) => Promise<T> | Bluebird<T> | void,
config?: any
): Bluebird<T>;
initialize(config?: Knex.Config): void;
destroy(callback: Function): void;
Expand Down