Vincit / objection.js

An SQL-friendly ORM for Node.js
https://vincit.github.io/objection.js
MIT License
7.25k stars 636 forks source link

Is it safe to use knexSnakeCaseMappers that way? #1144

Closed theprobugmaker closed 5 years ago

theprobugmaker commented 5 years ago

I saw that it's not safe to use knexSnakeCaseMappers and somehow I didn't understand when it's safe or it's not safe.

That is how my knexfile looks like:

require('dotenv').config()

module.exports = {
  client: process.env.DB_CLIENT,
  connection: {
    host: process.env.DB_HOST,
    port: process.env.DB_PORT,
    user: process.env.DB_USER,
    password: process.env.DB_PASSWORD,
    database: process.env.DB_DATABASE
  },
  migrations: {
    directory: './src/migrations',
    stub: './src/migrations/migration.stub'
  },
  debug: true
}

and in my database.js:

import Knex from 'knex'
import { Model, knexSnakeCaseMappers } from 'objection'

import config from '../knexfile'

export default () => {
  const knex = Knex({ ...config, ...knexSnakeCaseMappers() })
  Model.knex(knex)
}

Is it safe using that way or i'm going to have problem when using migrations?

heisian commented 5 years ago

In my experience, using knexSnakeCaseMappers is unsafe because of the way knex handles inserts into the knex_migrations table, which keeps track of previously-run migrations.

Using knexSnakeCaseMappers will cause all migrations to have the same batch number (0). Usually, migrations run at different times will have different batch numbers. That way, if you need to roll back, you only roll back the most recently-run batch.

However, with knexSnakeCaseMappers and all migrations having the same batch number, if you ever roll back, you will roll back all migrations.

heisian commented 5 years ago

That being said, I use knexSnakeCaseMappers in both migrations and Objection. I have to use a forked version of knex, however, in order to get my batch numbers to work correctly.

kibertoad commented 5 years ago

@heisian Why does that happen? Is that some kind of a bug? Maybe this is something that should be addressed upstream?

heisian commented 5 years ago

I'm glad you asked: https://github.com/tgriesser/knex/issues/2644 https://github.com/tgriesser/knex/pull/2645

heisian commented 5 years ago

Potentially, applying a very simple (but somewhat "hacky") fix, would be to just rename max_batch to max. That way, a snake <=> camel conversion run on the key name has no effect.

The only thing that might? break is if someone used an UPPERCASE converter, for which support was added not too long ago.

theprobugmaker commented 5 years ago

@heisian Thank you for your response, for now I'm only developing the application and not too worry about that right now, but I hope in the future this is somehow fixed in knex.js.

theprobugmaker commented 5 years ago

@heisian

I use knexSnakeCaseMappers in both migrations and Objection

What you mean by that?

heisian commented 5 years ago

my knex config:

    {
      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()

This config is used when running knex migrate:*.

For my objection models:

const { Model, snakeCaseMappers } = require('objection');

class BaseModel extends Model {
  static get columnNameMappers() {
    return snakeCaseMappers();
  }
}

So both the knex migrations and the Objection ORM (which uses knex internally) use knexSnakeCaseMappers.

theprobugmaker commented 5 years ago

I'm doing that in a different way, I'm not using knexSnakeCaseMappers for migrations, mostly cause when I write the migrations I do use naming conventions like verified_at instead of verifiedAt. I'm only concern about the conversion in the Objection level, as I have a GraphQL server I need to have the verified_at as verifiedAt.

And for that reason I'm doing in my database.js:

import Knex from 'knex'
import { Model, knexSnakeCaseMappers } from 'objection'

import config from '../knexfile'

export default () => {
  const knex = Knex({ ...config, ...knexSnakeCaseMappers() })
  Model.knex(knex)
}

That is why I asked the question in the first place, if I would still have problems with migrations by using knexSnakeCaseMappers only in Objection and not in the migrations.

heisian commented 5 years ago

So you should be fine, then, to use knexSnakeCaseMappers (in Objection only) without any issues. The only place (AFAIK) where issues arise is if you were to use them in your migrations as well.

theprobugmaker commented 5 years ago

@heisian I got what you mean now, but anyway, I hope this is fixed so people can use without problems.