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

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

@igor-savin-ht
Copy link
Contributor

@igor-savin-ht 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
Copy link
Member

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

@igor-savin-ht 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
Copy link
Member

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

@elhigu elhigu commented Feb 21, 2018

Related discussion #2026

@larsthorup
Copy link

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

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

@esabiun 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
Copy link

@dofrisec 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
Copy link

@nunorafaelrocha nunorafaelrocha commented Mar 22, 2018

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

@elhigu
Copy link
Member

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

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

@igor-savin-ht 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
Copy link
Member

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

@igor-savin-ht 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
Copy link
Collaborator

@kibertoad kibertoad commented Apr 4, 2018

@elhigu PR is up!

@kibertoad
Copy link
Collaborator

@kibertoad kibertoad commented Apr 6, 2018

@elhigu Any timeline for a release with this fix?

@elhigu
Copy link
Member

@elhigu elhigu commented Apr 6, 2018

Asap, this weekend I suppose.

@kibertoad
Copy link
Collaborator

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

Successfully merging a pull request may close this issue.

None yet
7 participants