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

Added support for custom foreign key names #1311

Closed
wants to merge 1 commit into
base: master
from

Conversation

Projects
None yet
4 participants
@natorojr

natorojr commented Mar 25, 2016

Greetings,

I'm working on a project which requires the ability to set foreign key constraint names that follow a specific naming convention. I noticed that Knex allows its users to set custom index names via table.index() and table.unique() methods, but not via table.foreign() or column.references().inTable().

This PR adds the ability to set a custom FK constraint name either as a second parameter to table.foreign() or with a chainable column.references().inTable().withKeyName() method.

I only have access to a couple of databases (SQLite and PostgreSQL) so I could only test those. Unless I'm overlooking something, the implementations seems to be completely backwards compatible, so it will not hurt existing code in the wild.

Note, I also updated the documentation accordingly.

My project is time sensitive so I'm hoping I could get this reviewed and merged in sooner than later with a new npm package (v0.10.1?), but I understand you may have other requirements to meet before merging. I'll be happy to accommodate any feedback into this PR.

Thanks,
Nestor

@@ -66,7 +66,7 @@ TableCompiler.prototype.alter = function () {
TableCompiler.prototype.foreign = function (foreignData) {
if (foreignData.inTable && foreignData.references) {
var keyName = this._indexCommand('foreign', this.tableNameRaw, foreignData.column);
var keyName = foreignData.keyName || this._indexCommand('foreign', this.tableNameRaw, foreignData.column);

This comment has been minimized.

@natorojr

natorojr Mar 25, 2016

Should I wrap foreignData.keyName with this.formatter.wrap()?
If you think this feature is likely to be accepted, I'll be happy to update the PR.

This comment has been minimized.

@wubzz

wubzz Mar 25, 2016

Collaborator

Yes, .wrap should be called on keyName as well.

@wubzz

This comment has been minimized.

Collaborator

wubzz commented Mar 25, 2016

@natorojr This looks good, and thanks for doing this. Interesting that .dropForeign had support for keyName but not .foreign. I just have a few questions if you don't mind:

  • I'm not sure build/knex.js should be in pull request
  • Could you add to documentation under foreign that keyName is accepted too?
  • Can we get a test for this ?
  • Is there any usecase for withKeyName() if already calling .foreign with a specified keyName? I'm not sure why this was a needed addition, but I could be missing something.
@natorojr

This comment has been minimized.

natorojr commented Mar 26, 2016

@wubzz,

Thanks for the feedback.

I'm not sure build/knex.js should be in pull request

I removed build/knex.js from the PR

Is there any usecase for withKeyName() if already calling .foreign with a specified keyName? I'm not sure why this was a needed addition, but I could be missing something.

withKeyName() is needed when you create a foreign key on a column using the .references() chainable method.

In other words, it allows supporting both of the following:

table.foreign('target_id', 'my_foreign_key_name').references('id').inTable('target');
table.integer('target_id').references('id').inTable('target').withKeyName('my_foreign_key_name');

Could you add to documentation under foreign that keyName is accepted too?

I added a note about keyName in the documentation under table.foreign().

However, the current documentation suggests that table.foreign() can take multiple columns. I am very confused by this statement because, as far as I know, a foreign key constraint can only be added on a column-by-column basis. Seems like this change was made in the following commit d18b815

Do you have any idea why the documentation for table.foreign() was changed to suggest it can accept multiple columns? If that is in fact supported, then adding keyName to table.foreign() may not be a compatible change.

Can we get a test for this ?

I'll gladly write some unit tests, but don't want to waste time if this is an incompatible change with supprt for multiple columns (see note above).

@wubzz

This comment has been minimized.

Collaborator

wubzz commented Mar 26, 2016

@natorojr Yes, it does accept multiple columns. I'm not too sure about most dialects, but here's a page with Postgres references and examples.

The output of using multiple columns is the following:

let qBuilder = knex.schema.createTable('foo', function(table) {
    table.foreign(['bar', 'bar2']).references('id').inTable('bar');
});
console.log(qBuilder.toString());
//create table "foo" ();
//alter table "foo" add constraint foo_bar_bar2_foreign foreign key ("bar", "bar2") references "bar" ("id")

Support for multiple columns is only when specifying the first argument as an array, it does not work when supplying several column arguments.

Multiple columns or not I believe that's a moot argument in regards to keyName as that is simply the name of the constraint being created. In this case it would replace foo_bar_bar2_foreign (generated from columns) with the specified keyName. According to your change here this is now possible, and I think it should be documented for that reason.

Edit: Which I now see you added. 👍

@natorojr

This comment has been minimized.

natorojr commented Mar 26, 2016

Wow, I learned something new today. Thanks for the Postgres documentation reference.

From the Postgres documentation:

FOREIGN KEY (b, c) REFERENCES other_table (c1, c2)
Of course, the number and type of the constrained columns need to match the number and type of the referenced columns.

Does that imply your example above should be:

table.foreign(['bar', 'bar2']).references(['id', 'id2']).inTable('bar')

?

Either way, to your point, it seems like my changes should be compatible.

I'll work on some unit tests in the next few days and update the PR. I only have access to Postgres and SQLite on my machine at this time, so I'll start there. I'll try to add MySQL after that. Apologies in advance for delays.

@vellotis

This comment has been minimized.

Contributor

vellotis commented May 28, 2016

( Ping! ) I would love to have this feature too. @natorojr Do you still plan to finisih your PR?

Related: #415

@natorojr

This comment has been minimized.

natorojr commented May 29, 2016

@vellotis Sorry for the delayed response. I had planned to finish this PR but, as is the case for many contributors, I got tied up with other work efforts/commitments.

You're welcome to rebase and add the tests yourself. Otherwise, I'll try to circle back to this when the dust settles.

vellotis added a commit to vellotis/knex that referenced this pull request Oct 6, 2016

@elhigu elhigu closed this Feb 16, 2017

elhigu added a commit that referenced this pull request Feb 16, 2017

Support custom foreign key names (continued work of #1311) (#1726)
* Added support for custom foreign key name

* Update unit test for #1311

* Fix TableCompiler::foreign to respect previous behavior + typo

* Add dialect specific unit tests

* Fixed comment

Actually trying to trigger travis to run tests with latest travis configuration... (maybe it just doesn't run tests against merged branch..)
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment