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

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

Undefined max_batch when using knexSnakeCaseWrappers (Objection) #2644

heisian opened this issue Jun 5, 2018 · 32 comments

Comments

@heisian
Copy link
Contributor

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

@heisian 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
Copy link
Collaborator

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

@heisian 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
Copy link
Collaborator

@koskimas 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
Copy link
Collaborator

@kibertoad 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
Copy link
Collaborator

@kibertoad kibertoad commented Nov 17, 2018

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

@kibertoad
Copy link
Collaborator

@kibertoad kibertoad commented Nov 17, 2018

@heisian Please review! #2914

@heisian
Copy link
Contributor Author

@heisian 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
Copy link
Collaborator

@kibertoad kibertoad commented Nov 18, 2018

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

@heisian
Copy link
Contributor Author

@heisian heisian commented Nov 19, 2018

clutch, thanks again for fixing this @kibertoad

@kibertoad
Copy link
Collaborator

@kibertoad kibertoad commented Nov 19, 2018

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

@heisian
Copy link
Contributor Author

@heisian heisian commented Nov 19, 2018

I could use some Grey Poupon....

@kibertoad
Copy link
Collaborator

@kibertoad kibertoad commented Nov 19, 2018

:D

@kibertoad
Copy link
Collaborator

@kibertoad kibertoad commented Nov 21, 2018

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

@heisian
Copy link
Contributor Author

@heisian heisian commented Nov 21, 2018

woooooo!

@heisian
Copy link
Contributor Author

@heisian 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
Copy link
Collaborator

@kibertoad kibertoad commented Nov 21, 2018

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

@kibertoad
Copy link
Collaborator

@kibertoad kibertoad commented Nov 21, 2018

@heisian Please review! #2923

@kibertoad
Copy link
Collaborator

@kibertoad kibertoad commented Nov 23, 2018

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

@heisian
Copy link
Contributor Author

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

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

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

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

@elhigu elhigu commented Nov 27, 2018

Unpublished 0.16.0 and added mention about it to changelog

@kibertoad
Copy link
Collaborator

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

@heisian heisian commented Dec 4, 2018

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

@kibertoad
Copy link
Collaborator

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

@heisian heisian commented Dec 4, 2018

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

@heisian
Copy link
Contributor Author

@heisian 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
Copy link
Collaborator

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

@heisian 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
Copy link
Collaborator

@kibertoad 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
Labels
None yet
Projects
None yet
Linked pull requests

Successfully merging a pull request may close this issue.

4 participants