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

batchInsert with existing transaction #1354

Closed
alexwhitman opened this Issue Apr 18, 2016 · 6 comments

Comments

Projects
None yet
3 participants
@alexwhitman
Contributor

alexwhitman commented Apr 18, 2016

batchInsert automatically starts/commits/rollsback a transaction but it would be useful if it could be used with an existing transaction which then isn't automatically completed.

@rhys-vdw

This comment has been minimized.

Collaborator

rhys-vdw commented May 7, 2016

@alexwhitman Does this not work?

knex.transaction(trx => {
  return trx.batchInsert(/* ... */);
});
@alexwhitman

This comment has been minimized.

Contributor

alexwhitman commented May 7, 2016

I'm not at work to test it now but in theory it should. However, from looking at the code wouldn't it start a new transaction within the existing? If so that would seem redundant.

@wubzz

This comment has been minimized.

Collaborator

wubzz commented May 14, 2016

@alexwhitman This has been added to knex 0.11.1, and the documentation has been updated as to how this works.

@wubzz wubzz closed this May 14, 2016

@alexwhitman

This comment has been minimized.

Contributor

alexwhitman commented May 16, 2016

@wubzz Thanks, just tested and it works fine.

Just a thought though, the example that @rhys-vdw gave was to call batchInsert on the transaction itself but that still starts a nested transaction. Instead it would have to be called as

trx.batchInsert(data).transacting(trx);

which looks slightly redundant. Would it be possible to modify so that both

knex.batchInsert(data).transacting(trx);

trx.batchInsert(data);

act in the same way?

@alexwhitman

This comment has been minimized.

Contributor

alexwhitman commented May 16, 2016

The following patch appears to work but I'm not sure if it's the correct way to implement it:

diff --git a/src/util/batchInsert.js b/src/util/batchInsert.js
index 4aaaa86..5594c7f 100644
--- a/src/util/batchInsert.js
+++ b/src/util/batchInsert.js
@@ -19,7 +19,10 @@ export default class BatchInsert {
     this._returning       = void 0;
     this._transaction     = null;
     this._autoTransaction = true;
-    
+
+    if (client.transacting) {
+      this.transacting(client);
+    }
   }

   /**
@wubzz

This comment has been minimized.

Collaborator

wubzz commented May 16, 2016

@alexwhitman I can see the convenience in that, and I think your diff makes perfect sense. Feel free to make a PR on that!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment