Added support for custom foreign key names

[natorojr]

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

[natorojr]

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.

[wubzz]

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

[wubzz]

@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]

@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]

@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]

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]

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

Related: #415

[natorojr]

@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.