Knex tries to drop not null during alter table migration

[yhaskell]

Environment

Knex version: 0.95.3
Database + version: PostgreSQL 12.5
OS: MacOS 11

Bug

I am trying to write a migration that changes a type of a column to the other type:

// migration #1

export const up = ({ schema }) => schema.createTable("test", t => {
  t.string("foo").notNullable().primary();
  t.string("bar").notNullable();
});

// migration #2

export const up = ({ schema }) => schema.alterTable("test", t => {
  t.text("foo").alter();
});

// migration #2 (alternative)

export const up = ({ schema }) => schema.alterTable("test", t => {
  t.text("foo").notNullable().primary().alter();
});

Migration #1 passes correctly and is not relevant in any way except for providing example structure.

Migration #2 fails (for both primary and alternative version) with the following message:

migration file "migration-2" failed
migration failed with error: alter table "test" alter column "foo" drop not null - column "foo" is in a primary key

I added a log for queries in the compiler.js file and it outputs the following:

[
  {
    sql: 'alter table "test" alter column "foo" drop default',
    bindings: []
  },
  {
    sql: 'alter table "test" alter column "foo" drop not null',
    bindings: []
  },
  {
    sql: 'alter table "test" alter column "foo" type text using ("foo"::text)',
    bindings: []
  }
]

where as I expect the one and only query:

[
  {
    sql: 'alter table "test" alter column "foo" type text using ("foo"::text)',
    bindings: []
  }
]

[elhigu]

That is how it was designed to work http://knexjs.org/#Schema-alter
. If you want to keep .notNullable() constraint after .alter() it should be given in the alter statement:

t.text("foo").notNullable().alter(); 

Though looks like it doesn't work for changing primary key column type (it actually only works for some really basic cases). I suppose that it could recognize when primary column in PG is modified and not try to drop null constraint in that case.

That method actually works only for really limited simple use cases.

[yhaskell]

I also noticed that if you provide the constraint, it first deletes it and then applies it again.

Tuat is an easier way to do it I guess, but it doesn't work in cases like that.

I think it might be doable to check if there is still a not null able constraint and in this case not generate drop not null at all.

Or make it explicit (which would be a breaking change)

[tkalliom]

Since set not null is idempotent in PostgreSQL, how about skipping drop not null if notNullable() is present? That would make a primary key compliant implementation possible without introspecting the schema. Although having to always add notNullable() after primary() when altering would be clumsy, it would still be better than changes to primary keys being impossible.

[elhigu]

I also noticed that if you provide the constraint, it first deletes it and then applies it again.

It comes from the design that knex actually doesn't know anything about the schema so it doesn't know if the constraint was there in the first place.

Having schema system completely out from knex core repo would be a lot better for the project to allow more relaxed feature development on that side. I wont like to have really sophisticated schema functionality on core repository, because It will easily require 10x more code to implement well than the knex core query builder + execution abstracting parts.

[yhaskell]

I think simplifying it would be better actually:
adding .dropNonNullable() explicitly should drop the non null constraint, and not adding it should not touch it at all.

That would make the schema alter completely deterministic, and also not require any schema reading.

[kibertoad]

@yhaskell PR for this functionality, as long as it is not a breaking change, would be most welcome!

[elhigu]

I think simplifying it would be better actually: adding .dropNonNullable() explicitly should drop the non null constraint, and not adding it should not touch it at all.

That would be a breaking change and would induce need for all other type of additional .dropXXX() methods, which can already be used separately without .alter() syntax. For example with mysql no dropping is done, because it allows alterin column type and constraints exactly the way this feature does.

In which case drop + recreate is a problem?

Giving option to .alter({ dropNotNullable: false, }) which defaults to true could be fine though. There could be similar options for other droppable things if always drop causes a problem.

[yhaskell]

@elhigu drop + recreate fails on all alters of a primary key column

[elhigu]

@yhaskell Ah right. Yeah then giving options to .alter({ dropNotNullable: false, }) would fit the best in the original design of the feature and it is also backwards compatible.

[kibertoad]

Released in 1.0.2