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

Knex migration attempts to create already existing changelog and fails entire migration #2499

Closed
igor-savin-ht opened this Issue Feb 20, 2018 · 18 comments

Comments

Projects
None yet
7 participants
@igor-savin-ht
Contributor

igor-savin-ht commented Feb 20, 2018

Environment

Knex 0.14.4
PostgreSQL 9.6.6

Bug

Since the upgrade to 0.14.4 we've started getting this error in CI:

 Knex:warning - migration failed with error: create table "crm"."crm_migrations" ("id" serial primary key, "name" varchar(255), "batch" integer, "migration_time" timestamptz) - relation "crm_migrations" already exists
02:18:00 Error migrating:  { error: relation "crm_migrations" already exists

It looks like 7ff766f actually wasn't as innocent as expected.
Might be related to the fact that we are using custom migration changelog table via

tableName: ${SCHEMA}.crm_migrations

parameter.

@elhigu

This comment has been minimized.

Collaborator

elhigu commented Feb 20, 2018

I think that syntax is not supported, but there has been requests to support something like that. Basically the problem is probably that .hasTable('schema.tableName') doesn't work (IIRC there was long discussion why it does not work).

Probably it is necessary to change configuration to also support schemaName in addition to tableName parameter.

Need to dig up old discussions and check out the code to be able to decide exactly how to support that.

@igor-savin-ht

This comment has been minimized.

Contributor

igor-savin-ht commented Feb 20, 2018

@elhigu AFAIR, there is also plan to get rid of createTableIfNotExists in favour of hasTable. If former supported schemas and latter will not, that's going to be a rather painful breaking change.

@elhigu

This comment has been minimized.

Collaborator

elhigu commented Feb 21, 2018

Former did support schema only by accident. It has never been an official tested / documented feature. Schema reference should have been given currently with .withSchema() builder method.

Reason for not supporting schema.table.column references has been ambiguity of the syntax, when at some places part1.part2 means schema.table and in others table.column. It always has depended of the context where identifier has been used.

Anyways I'am interested in looking through all the knex APIs and see if it would be feasible to allow, test and document schema.table.column syntax officially.

@elhigu

This comment has been minimized.

Collaborator

elhigu commented Feb 21, 2018

Related discussion #2026

@larsthorup

This comment has been minimized.

larsthorup commented Feb 25, 2018

We had this issue as well, and have worked around it by locking to knex@0.14.2. Is there another workaround that allows us to keep the migrations table inside a schema with latest knex?

@elhigu

This comment has been minimized.

Collaborator

elhigu commented Feb 26, 2018

I would say that fastest fix for this would be to add schemaName to configuration and supporting schema.table syntax is bigger issue to resolve.

@esabiun

This comment has been minimized.

esabiun commented Feb 28, 2018

Kind-of related issue/request #1138

I fully support comment from there

It would be great if we can have something like knex.migrate.withSchema('schema-name').latest()

@dofrisec

This comment has been minimized.

dofrisec commented Mar 21, 2018

We've had the same issue, locked to knex@0.14.3 to work around it.
Something like 'knex.migrate.withSchema(...)...' as suggested above, or perhaps a 'withSchema' property on the migrations config object would be a good fix.

@nunorafaelrocha

This comment has been minimized.

nunorafaelrocha commented Mar 22, 2018

Any news on this? I guess that it's pretty bad to have the current version broken... 😿

@elhigu

This comment has been minimized.

Collaborator

elhigu commented Mar 22, 2018

@nunorafaelrocha broken is a bit strong term for missing feature. But yeah it wouldn't hurt if someone implements this by adding that schemaName attribute to configuration.

@nunorafaelrocha

This comment has been minimized.

nunorafaelrocha commented Mar 23, 2018

Hi @elhigu. Sorry, I meant no harm. But adding this feature as a patch and then force developers to change the use of the lib doesn't quite follows semver (it actually sounds like a BC). In a way, doing it on a patch have broken the use of it for everyone that allows patch updates.

@igor-savin-ht

This comment has been minimized.

Contributor

igor-savin-ht commented Mar 23, 2018

Exactly, after this update came out, our CI builds started failing without us changing anything, and that is pretty much a definition of semver breaking change.
I'll look into providing a PR for this next week.

@elhigu

This comment has been minimized.

Collaborator

elhigu commented Mar 23, 2018

@nunorafaelrocha no harm done, just mentioned about dangers of using implementation deatails as features.

This breaking/non-breaking/semver discussion has actually been discussed here at least couple of times before :) Knex is following semver and Knex is still < 1.0 so semver says that every change can be breaking change (of course it is debateable if knex version 1.0 should already have been released).

That aside line between non-breaking change and non breaking ones is pretty shady, since every small change that can be tested also changes functionality in some way and may break someone's code (even making simple bugfixes / adding new attribute somewhere can break someone's tests). So mostly I've been drawing that line where changing functionality that is not documented/tested public API is non-breaking change and changing documented functionality is breaking.

That being said of course if I would have known that people is using this implementation detail here, I would have marked it as breaking one even that it was undocumented functionality.

@igor-savin-ht nice I'm looking forward to check that PR 👍

@igor-savin-ht

This comment has been minimized.

Contributor

igor-savin-ht commented Apr 3, 2018

Just a heads-up: I've finally started working on it, hopefully will be able to produce something working soon.

@kibertoad

This comment has been minimized.

Collaborator

kibertoad commented Apr 4, 2018

@elhigu PR is up!

@kibertoad

This comment has been minimized.

Collaborator

kibertoad commented Apr 6, 2018

@elhigu Any timeline for a release with this fix?

@elhigu

This comment has been minimized.

Collaborator

elhigu commented Apr 6, 2018

Asap, this weekend I suppose.

@kibertoad

This comment has been minimized.

Collaborator

kibertoad commented Apr 6, 2018

Awesome, thank you!

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