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

Include tableName in MySQL fk contraint names #6008

Merged
merged 1 commit into from Jun 2, 2016

Conversation

hagabaka
Copy link
Contributor

@hagabaka hagabaka commented Jun 1, 2016

Pull Request check-list

  • Does npm run test or npm run test-DIALECT pass with this change (including linting)?
  • Does your issue contain a link to existing issue (Closes #[issue]) or a description of the issue you are solving?
  • Have you added new tests to prevent regressions?
  • Is a documentation update included (if this change modifies existing APIs, or introduces new ones)?
  • Have you added an entry under Future in the changelog?

Description of change

When creating a foreign key constraint in MySQL, use the template
{referring_table}_{attribute}_foreign_idx, rather than just
{attribute}_foreign_idx, to avoid duplicate constraint names.

See GitHub #5826

When creating a foreign key constraint in MySQL, use the template
{referring_table}_{attribute}_foreign_idx, rather than just
{attribute}_foreign_idx, to avoid duplicate constraint names.

Related tests are updated.

See GitHub sequelize#5826
@mickhansen
Copy link
Contributor

LGTM, /cc @janmeier

@danielepolencic
Copy link

any chance this could be released?

@mickhansen
Copy link
Contributor

@danielepolencic This PR targeted master so it's slated for the next 4.0 pre release.

@eliias
Copy link

eliias commented Sep 9, 2016

@mickhansen any chance of backporting this to 3.x? As it is impossible to set the name of the constraint, only raw SQL is left for the rescue.

@felixfbecker
Copy link
Contributor

Isnt this a beaking change?

@mickhansen
Copy link
Contributor

@felixfbecker I don't believe so since we don't really depend on constraint names anywhere.
@eliias It's very unlikely that the core team will get around to that, but a PR is certainly welcome.

@felixfbecker
Copy link
Contributor

@mickhansen we don't, but a user might (think raw queries in migration files, etc). It's like changing the default casing of table names.

@mickhansen
Copy link
Contributor

@felixfbecker That is true

@felixfbecker
Copy link
Contributor

@eliias it will land in v4

@diosney
Copy link
Contributor

diosney commented Sep 23, 2016

It is really sad that we have to wait until v4 for this.

In the meantime, as a workaround, I'm setting the column to be something like ModeNameColumnName.

If you find a nicer workaround please share it!

nathanpjones pushed a commit to nathanpjones/sequelize that referenced this pull request Mar 25, 2017
Fix for MySQL fk constraint names
Cherry-picked from 85477dd
See also issue sequelize#5826
nathanpjones pushed a commit to nathanpjones/sequelize that referenced this pull request Mar 25, 2017
Fix for MySQL fk constraint names to incorporate referring table name
and avoid possible name collisions.
Cherry-picked from 85477dd
See also issue sequelize#5826
nathanpjones pushed a commit to nathanpjones/sequelize that referenced this pull request Mar 25, 2017
Fix for MySQL fk constraint names to incorporate referring table name
and avoid possible name collisions.

Cherry-picked from 85477dd
See also issue sequelize#5826
janmeier pushed a commit that referenced this pull request Mar 27, 2017
Fix for MySQL fk constraint names to incorporate referring table name
and avoid possible name collisions.

Cherry-picked from 85477dd
See also issue #5826
@blackrabbit99
Copy link

Thank you for breaking changes!

holm pushed a commit to holm/sequelize that referenced this pull request Nov 13, 2017
Fix for MySQL fk constraint names to incorporate referring table name
and avoid possible name collisions.

Cherry-picked from 85477dd
See also issue sequelize#5826
@jramos-br
Copy link

This problem also affects mssql dialect. Any plans to change it there?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

9 participants