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 max_batch when using knexSnakeCaseWrappers (Objection) #2644

Closed
heisian opened this Issue Jun 5, 2018 · 32 comments

Comments

Projects
None yet
4 participants
@heisian
Copy link
Contributor

heisian commented Jun 5, 2018

Bug

When running knex migrations configured with Objection's knexSnakeCaseWrappers, the promise handler for the _latestBatchNumber query always returns a default value of 0:

// knex/src/migrate/index.js, lines 329-332

_latestBatchNumber(trx = this.knex) {
    return trx.from(getTableName(this.config.tableName, this.config.schemaName))
      .max('batch as max_batch').then(obj => obj[0].max_batch || 0);
  }

This is because max_batch does not exist in obj. The actual structure of obj, due to knexSnakeCaseWrappers is:

obj = {
  maxBatch: 2
}

Defaulting to 0 results in a batch number of 1 always being inserted into the knex_migrations table when using knexSnakeCaseWrappers.

In effect, this makes all of your migrations only use a single batch, which is potentially catastrophic when performing a rollback.

Environment

Knex version: 0.14.6
Database + version: Postgres 10.1
OS: macOS/Linux

Select applicable template from below.
If issue is about oracledb support tag @ atiertant. For MSSql tag @ smorey2 .
Rest of dialects doesn't need tags.

@heisian

This comment has been minimized.

Copy link
Contributor

heisian commented Jun 5, 2018

@koskimas not sure if this was more of an Objection or Knex issue, but just wanted to make you aware of this. thanks

@koskimas

This comment has been minimized.

Copy link
Collaborator

koskimas commented Jun 5, 2018

@heisian This is a knex issue. postProcessResponse doesn't receive information whether the query is an "internal" one or not. objection's posProcessResponse always maps the response and there is nothing we could do there. Knex should probably not use wrapIdentifier and postProcessResponse in these kind of internal queries.

@heisian

This comment has been minimized.

Copy link
Contributor

heisian commented Jun 5, 2018

OK, sounds like a slightly deep-rooted issue.. will think this over - am using my own patched branch for the moment.

Would it be prudent to warn about using knexSnakeCaseWrappers in Objection, since the use of such will cause only a single batch to be created and thereby roll back the entire db if a rollback is performed?

@koskimas

This comment has been minimized.

Copy link
Collaborator

koskimas commented Jun 5, 2018

We should probably add a warning about using knexSnakeCaseWrappers in migrations. You can safely use it in the actual code.

@kibertoad

This comment has been minimized.

Copy link
Collaborator

kibertoad commented Nov 16, 2018

@heisian Any chance you could create a failing test for reproduction of the issue? I want to try implementing overriding config within single query scope and would appreciate easy repro for the issue to play with.

@kibertoad

This comment has been minimized.

Copy link
Collaborator

kibertoad commented Nov 17, 2018

@heisian Actually, no need, I've succesfully reproduced everything that I needed and finalizing solution now :)

@kibertoad

This comment has been minimized.

Copy link
Collaborator

kibertoad commented Nov 17, 2018

@heisian Please review! #2914

@heisian

This comment has been minimized.

Copy link
Contributor

heisian commented Nov 18, 2018

This is great! I still have much more to learn about the Knex internals. @ekeric13 and I will check it out on Monday

@kibertoad

This comment has been minimized.

Copy link
Collaborator

kibertoad commented Nov 18, 2018

@heisian ahahaha, yeah, Knex internals are fun :D

@heisian

This comment has been minimized.

Copy link
Contributor

heisian commented Nov 19, 2018

clutch, thanks again for fixing this @kibertoad

@kibertoad

This comment has been minimized.

Copy link
Collaborator

kibertoad commented Nov 19, 2018

@heisian Anything else before you can return upstream :-P?

@heisian

This comment has been minimized.

Copy link
Contributor

heisian commented Nov 19, 2018

I could use some Grey Poupon....

@kibertoad

This comment has been minimized.

Copy link
Collaborator

kibertoad commented Nov 19, 2018

:D

@kibertoad

This comment has been minimized.

Copy link
Collaborator

kibertoad commented Nov 21, 2018

@heisian 0.16.0-next4 is available now! Please let me know how testing it goes :)

@heisian

This comment has been minimized.

Copy link
Contributor

heisian commented Nov 21, 2018

woooooo!

@heisian

This comment has been minimized.

Copy link
Contributor

heisian commented Nov 21, 2018

@kibertoad It seems that specifying a knexfile doesn't work:
knex/bin/cli.js:296 - console.log(argv.knexfile) // outputs path correctly
knex/bin/cli.js:58 - console.log(env.pathToKnexFile) // undefined

@kibertoad

This comment has been minimized.

Copy link
Collaborator

kibertoad commented Nov 21, 2018

ouch. thanks for bug report. let me do a hotfix and update our testsuite

@kibertoad

This comment has been minimized.

Copy link
Collaborator

kibertoad commented Nov 21, 2018

@heisian Please review! #2923

@kibertoad

This comment has been minimized.

Copy link
Collaborator

kibertoad commented Nov 23, 2018

@heisian Can we do the take 2? 0.16.0-next5 with fix is already out.

@heisian

This comment has been minimized.

Copy link
Contributor

heisian commented Nov 26, 2018

@kibertoad sorry, we were on short week here last week, will be sure to test it out again today!

@heisian

This comment has been minimized.

Copy link
Contributor

heisian commented Nov 26, 2018

@kibertoad It seems that the --cwd flag no longer applies to the --knexfile flag.

With 0.16.0-next5 I need to specify my --knexfile path relative to node_modules/knex/lib/index.js, regardless of the value of --cwd

@heisian

This comment has been minimized.

Copy link
Contributor

heisian commented Nov 26, 2018

Ah okay, I've found a big issue - knexSnakeCaseMappers (and more specifically, postProcessResponse and wrapIdentifier) no longer seem to get applied to migrations. here is my knexfile, which is the same as when using knex 0.15.x:

module.exports = Object.assign({
      client: 'pg',
      connection: {
        host: PSQL_HOST,
        port: PSQL_PORT,
        database: PSQL_DATABASE,
        user: PSQL_USERNAME,
        password: PSQL_PASSWORD,
      },
      pool: {
        min: 1,
        max: poolMax,
        afterCreate(conn, cb) {
          const queryString = 'SET timezone="UTC";'
          conn.query(queryString, (err) => {
            cb(err, conn)
          })
        },
      },
      migrations: {
        directory: `${__dirname}/../migrations`,
        stub: `${__dirname}/stubMigrations.js`,
      },
      seeds: {
        directory: process.env.PSQL_SEED_DIR || `${__dirname}/../seeds`,
        stub: `${__dirname}/stubSeeds.js`,
      },
      debug: ['debug'].includes(process.env.DEBUG),
    },
    knexSnakeCaseMappers()
})

When running migrations with 0.16.0-next5:

exports.up = (knex) =>
  knex.schema.withSchema('auth').createTable('emails', (t) => {
    t.increments()
    t.integer('userId').notNullable().index()
    t.foreign('userId').references('id').inTable('auth.users')
    t.string('email').notNullable()
    t.boolean('primary')
    t.boolean('verified').defaultTo(false)
    t.timestamp('createdAt').notNullable()
    t.timestamp('updatedAt').notNullable()
    t.timestamp('deletedAt')
  })

exports.down = (knex) => knex.schema.withSchema('auth').dropTable('emails')
SELECT column_name FROM information_schema.columns WHERE table_name = 'emails';
/*
Returns:
id
userId
email
primary
verified
createdAt
updatedAt
deletedAt
*/
@elhigu

This comment has been minimized.

Copy link
Collaborator

elhigu commented Nov 27, 2018

Ouch... that is really bad, and I just released 0.16.0... we need to fix that and release 0.16.1 asap.

@elhigu

This comment has been minimized.

Copy link
Collaborator

elhigu commented Nov 27, 2018

Unpublished 0.16.0 and added mention about it to changelog

@kibertoad

This comment has been minimized.

Copy link
Collaborator

kibertoad commented Dec 4, 2018

@heisian 0.16.1-next1 should have addressed CWD and query execution processing regressions, could you take a look? (and sorry for needing so many iterations to get it exactly right)

@heisian

This comment has been minimized.

Copy link
Contributor

heisian commented Dec 4, 2018

no worries, thank you all for the efforts. I will test it out today. thanks for the update

@kibertoad

This comment has been minimized.

Copy link
Collaborator

kibertoad commented Dec 4, 2018

@heisian There is known issue that resolving knexfile without passing path to it explicitly doesn't work. If this is your use-case, please wait for next2 with fix, otherwise (hopefully) it's good enough!

@heisian

This comment has been minimized.

Copy link
Contributor

heisian commented Dec 4, 2018

okay, we always explicitly specify the knexfile, so will give it a shot, ty for the heads up

@heisian

This comment has been minimized.

Copy link
Contributor

heisian commented Dec 5, 2018

All of our test suites pass w/ 0.16.1-next2 (master)! Can you create an official tag so we can use it in our CI?

@kibertoad

This comment has been minimized.

Copy link
Collaborator

kibertoad commented Dec 5, 2018

@heisian It's already on npm as 0.16.1-next2 - but a final 0.16.1 is also coming in a couple of days if there are no serious regressions reported.

@heisian

This comment has been minimized.

Copy link
Contributor

heisian commented Dec 5, 2018

oh got it ok will start using it in CI, and will report any discrepancies. I've manually verified that batches are handled correctly. thank you!

@kibertoad

This comment has been minimized.

Copy link
Collaborator

kibertoad commented Dec 5, 2018

Woohoo! Glad to hear that!

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