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

Add support for multiple outbound foreign keys constraints #77

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

pomier
Copy link

@pomier pomier commented Oct 24, 2014

This adds support for tables with multiples outbound foreign keys constraints. This is a modification of pull request #36 who allowed only one foreign key constraint.

The original problem in issue #34 was that lhm tries to create a table with foreign keys constraint having the same name than the original table, producing an error. #36 tried to fix it by modifying the last number at the end of the name. table_ibfk_1 would then become table_ibfk_2.
The problem was that the new constraints names were the same when the table had more than one outbound foreign keys, producing the same error than before. Another problem with that solution is that the names produced are not consistent when you undo or redo a migration. The original names are lost forever, so undo a migration won't give you the same structure than before. The new name depends on the original name, so redo a migration may not give the same structure either.

My solution appends a suffix "_lhmn" to the constraint name. If the suffix is already there, we remove it in the new name. If you apply two change_table, the constraint names will then be exactly as before. This allows migrations undo and redo to work as they should. Besides, the uniqueness of constraints names is no longer a problem, making lhm support as many outbound foreign key constraints as necessary.

@pomier pomier force-pushed the outbound-foreign-keys branch from 7372160 to 3455ed9 Compare November 25, 2014 15:38
@pomier
Copy link
Author

pomier commented Nov 25, 2014

Thanks to #82, the build has passed and this pull request is ready to be merged 😉

@masylum
Copy link

masylum commented Mar 4, 2015

What is needed to get this PR merged? Can we help on getting it merged?

@undergroundwebdesigns
Copy link

Bump, I'd also like to use this, is anything I can do to help get this merged?

@arthurnn
Copy link
Contributor

Yes, this is a problem, and we need to fix it.
I like the solution you proposed, however I am having a bit trouble follow the code, Can you squash all commits in one and rebase this from latest master.
After that I can give a last look and merge it in.

thanks

@pomier
Copy link
Author

pomier commented Mar 19, 2015

I've squashed the commits and rebased the branch.

@quentindemetz
Copy link

@arthurnn how does it look? We've been using @pomier's fork in production at PriceMatch for a while now and haven't run into any issues.

Thanks!

@trobrock
Copy link

👍

@arthurnn
Copy link
Contributor

arthurnn commented Apr 2, 2015

Now that we only support ActiveRecord, would be cool to use the FK method they provide https://github.com/rails/rails/blob/4-2-stable/activerecord/lib/active_record/connection_adapters/abstract_mysql_adapter.rb#L538-L548 , Only problem is that is only on rails 4.2+ . So maybe we should copy/paste that logic, if we are running on rails 4.2- . WDYT?

@quentindemetz
Copy link

I think we should focus on getting this out instead of continuously commenting / rebasing / refactoring once every six months. It's tested, and it's been working live for months now. It won't be too late to refactor it later to include the relevant rails 4.2 logic if that really stands out as a priority.

@quentindemetz
Copy link

@arthurnn @hannestyden I would be most thankful if you could merge this ? Thanks guys.

@ghempton
Copy link

👍 any remaining work here?

@flushentitypacket
Copy link

Just ran into this problem today. Thanks for putting in the work to resolve this!

@todd
Copy link

todd commented Oct 14, 2015

Any update on this? Would love to see this merged.

@treyx
Copy link

treyx commented Oct 20, 2015

👍 Also running into this problem. Thanks for looking into the fix

@buffym
Copy link

buffym commented Oct 22, 2015

Just got bit by this one too... 👍

@ghempton
Copy link

It's been almost a year since this PR was made. Can we get a maintainer to chime in on what it would take to get this merged in (if at all)?

@pomier pomier force-pushed the outbound-foreign-keys branch from d1f9004 to 491d563 Compare October 27, 2015 16:54
@pomier
Copy link
Author

pomier commented Oct 27, 2015

I've rebased this PR from latest master.

The tests are now randomly failing the same way it currently happens on master branch. Apart from that, everything is ready to be merged any time now.

@trobrock
Copy link

@arthurnn not sure if you are the right guy, but maybe you can point to the right person. What needs to be done to get this merged? It's the only thing preventing me from using this, and right now i have to resort to downtime migrations 😞

@quentindemetz
Copy link

@trobrock you can always use @pomier's fork directly in your gemfile:

gem 'lhm', git: 'https://github.com/pomier/lhm', branch: 'outbound-foreign-keys'

It's been rebased on this repo recently which means there are little drawbacks to using this fork IMO.

@flushentitypacket
Copy link

Kind of frustrating that project maintainers aren't moving at all on this PR. This is incredible work and would really improve the gem.

@myers
Copy link

myers commented May 23, 2016

I wish this PR was merged. It would solve the problem I have this morning.

@soulfly
Copy link

soulfly commented Sep 1, 2016

Guys, any updates with this merge?

@twelvelabs
Copy link

@arthurnn are there still changes required before this can be merged, or are you looking for a totally different direction? We'd like to avoid using a fork if at all possible.

@shiju-d
Copy link

shiju-d commented Dec 2, 2016

Hey Guys, Can we get this merged and avoid fork?

@tooluser
Copy link

tooluser commented May 1, 2017

@arthurnn We would love to get this in - and are willing to help with this if any changes are needed and help is desired. Can any maintainer let us know whether this is a good direction for the solution, and/or what we can do to help?

@sweetleon
Copy link

Any chance of getting this merged? It'd really help my project.

@tboyko
Copy link

tboyko commented Oct 21, 2019

door stuck

@dinesh-rdk
Copy link

It's 2020!!!

@SenikTony
Copy link

It's 2021!!! Knock-Knock...

@tboyko
Copy link

tboyko commented Jan 25, 2021

FWIW, the Shopify fork is more active these days.

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

Successfully merging this pull request may close these issues.