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

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

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

Comments

@agdevbridge
Copy link
Contributor

@agdevbridge 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
Copy link
Member

@wubzz 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
Copy link
Member

@elhigu 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
Copy link
Member

@elhigu 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
Copy link
Contributor

@villelahdenvuo villelahdenvuo commented May 19, 2016

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

@villelahdenvuo
Copy link
Contributor

@villelahdenvuo villelahdenvuo commented May 19, 2016

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

@elhigu
Copy link
Member

@elhigu 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
Copy link
Contributor

@villelahdenvuo 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
Copy link
Member

@wubzz 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
…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
Projects
None yet
Linked pull requests

Successfully merging a pull request may close this issue.

None yet
4 participants