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

undefined fields in insert/update causes exception when using transaction #1423

Closed
agdevbridge opened this Issue May 18, 2016 · 8 comments

Comments

Projects
None yet
4 participants
@agdevbridge
Contributor

agdevbridge commented May 18, 2016

Inserts/updates in transaction fails if there are undefined fields. This started happening with 0.11.2.
Exception:

TypeError: Cannot read property 'toString' of undefined
      at Object.prepareValue (app\node_modules\knex\lib\dialects\postgres\utils.js:48:13)
      at app\node_modules\knex\lib\dialects\postgres\index.js:61:20
      at arrayMap (app\node_modules\knex\node_modules\lodash\lodash.js:595:23)
      at map (app\node_modules\knex\node_modules\lodash\lodash.js:8778:14)
      at Client.prepBindings (app\node_modules\knex\lib\dialects\postgres\index.js:60:28)
      at QueryCompiler_PG.toSQL (app\node_modules\knex\lib\query\compiler.js:47:37)

      at QueryBuilder.toSQL (app\node_modules\knex\lib\query\builder.js:40:44)
      at app\node_modules\knex\lib\runner.js:34:32
      at tryCatcher (app\node_modules\knex\node_modules\bluebird\js\release\util.js:16:23)
      at app\node_modules\knex\node_modules\bluebird\js\release\using.js:185:26
      at bound (domain.js:287:14)
      at runBound (domain.js:300:12)
      at tryCatcher (app\node_modules\knex\node_modules\bluebird\js\release\util.js:16:23)
      at Promise._settlePromiseFromHandler (app\node_modules\knex\node_modules\bluebird\js\release\promise.js:502:31)
      at Promise._settlePromise (app\node_modules\knex\node_modules\bluebird\js\release\promise.js:559:18)
      at Promise._settlePromise0 (app\node_modules\knex\node_modules\bluebird\js\release\promise.js:604:10)
      at Promise._settlePromises (app\node_modules\knex\node_modules\bluebird\js\release\promise.js:683:18)
      at Promise._fulfill (app\node_modules\knex\node_modules\bluebird\js\release\promise.js:628:18)
      at PromiseArray._resolve (app\node_modules\knex\node_modules\bluebird\js\release\promise_array.js:125:19)
      at PromiseArray._promiseFulfilled (app\node_modules\knex\node_modules\bluebird\js\release\promise_array.js:143:14)
      at Promise._settlePromise (app\node_modules\knex\node_modules\bluebird\js\release\promise.js:564:26)
      at Promise._settlePromise0 (app\node_modules\knex\node_modules\bluebird\js\release\promise.js:604:10)

Code:

knex.transaction(function (tr) {
    return knex('users').transacting(tr)
        .insert({email: 'a@example.com'})
        .insert({email: 'b@example.com', age: undefined});
}).then(function () {
    console.log('OK');
}).catch(function (err) {
    console.log('Fail', err);
});

Output:

Fail [TypeError: Cannot read property 'toString' of undefined]
@wubzz

This comment has been minimized.

Collaborator

wubzz commented May 18, 2016

@agdevbridge Is this explicitly a transaction issue, or does the same thing happen outside transactions?

Seems strange this would break between 0.10 -> 0.11 as there's tests for it. But perhaps no tests while in transaction..

@elhigu

This comment has been minimized.

Collaborator

elhigu commented May 18, 2016

Thing you are doing is actually not multi insert, but just single insert equal to:

knex.transaction(function (tr) { 
  return tr('users').insert({email: 'b@example.com', age: undefined}); 
});

Second consecutive .insert in QueryBuilder overrides the first one. Anyways I'll write test case with transaction and try to reproduce in master.

Without transaction it seems to work though:

knex('users')
  .insert({email: 'a@example.com'})
  .insert({email: 'b@example.com', age: undefined})
  .toSQL();

{ method: 'insert',
  options: {},
  timeout: false,
  bindings: [ 'b@example.com' ],
  sql: 'insert into "users" ("age", "email") values (DEFAULT, ?)',
  returning: undefined }
@elhigu

This comment has been minimized.

Collaborator

elhigu commented May 19, 2016

I've been able to reproduce the problem with this:

    it('#1423 should replace undefined keys in single insert with DEFAULT also in transacting query', function() {
      if (knex.client.dialect === 'sqlite3') {
        return true;
      }
      return knex.transaction(function(trx) {
        return trx('accounts')
          .insert({
            last_name: 'First Item',
            email:'findme@example.com',
            logins: undefined,
            about: 'Lorem ipsum Dolore labore incididunt enim.',
            created_at: new Date(),
            updated_at: new Date()
          })
          .then(function (results) {
            return knex('accounts').where('email', 'findme@example.com');
          })
          .then(function (results) {
            expect(results[0].logins).to.equal(1);
            // cleanup to prevent needs for too much changes to other tests
            return knex('accounts').delete().where('id', results[0].id);
          });
      });
    });

Still tracing the problem... It is really strange that the same query without transaction works just fine...

@villelahdenvuo

This comment has been minimized.

Contributor

villelahdenvuo commented May 19, 2016

Also got this after updating to 0.11.3 Thankfully our integration tests caught it.

@villelahdenvuo

This comment has been minimized.

Contributor

villelahdenvuo commented May 19, 2016

This seems to be the culprit: 9114bdd#commitcomment-17538902

@elhigu

This comment has been minimized.

Collaborator

elhigu commented May 19, 2016

@tuhoojabotti Yep, partly. I'm pretty sure there are still something wrong in insert compile phase... I'm going to look into it today after work.

@villelahdenvuo

This comment has been minimized.

Contributor

villelahdenvuo commented May 19, 2016

Good to know, I had to update knex to support nested join on clauses, but I have to wait for a fix for this before I can deploy.

@wubzz

This comment has been minimized.

Collaborator

wubzz commented May 19, 2016

@elhigu I would say this was broken in 0.10.0 as well. See this example:

NoTransaction insert into "test" ("test", "test2") values ('123', DEFAULT)
Transaction insert into "test" ("test", "test2") values ('123', NULL)

The issue from what I can tell is that valueForUndefined is never inherited on the transaction client here, which ironically results in valueForUndefined being just that -- undefined.

@rhys-vdw rhys-vdw closed this in 69e33c7 May 20, 2016

rhys-vdw added a commit that referenced this issue May 20, 2016

Merge pull request #1429 from elhigu/pass-valueForUndefined-from-clie…
…nt-to-transacting-client

Pass valueForUndefined also to transacting client. Fixes #1423
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment