Use the index name explicitly provided in a migration when reverting. #8868

Closed
wants to merge 1 commit into
from

Conversation

Projects
None yet
6 participants
Contributor

tehgeekmeister commented Jan 10, 2013

schema_statements uses the column name by default to construct the index name, and then raises an exception if it doesn't exist, even if the name option is specified, which causes #8858. this commit makes index_name_for_remove fall back to constructing the index name to remove based on the name option.

Can you provide a test case please? Thanks.

Contributor

tehgeekmeister commented Jan 10, 2013

On it.

Contributor

tehgeekmeister commented Jan 10, 2013

Testing has found an edge case, so I'm working on that too.

Contributor

tehgeekmeister commented Jan 10, 2013

Added a test case, rebased, squashed.

BTW.

The original bug report (bug #8858) reported by me needed to manually include the name of the index because the auto-generated name was to large (> 64 characters).

I suspect this limitation is imposed by sqlite3 and other databases will have larger limits (correct me if I am wrong).
Since I am writing an engine, and the engine tests (and migrations) are ran on sqlite3, but should work on mysql, postgresql & sqlite3, I need to create a name that is acceptable for every database, which is the lowest common denominator.

Would it be possible to adapt the auto-name-generator of the index name to include the limitations (length, included characters..) of the current database before creating the scheme.rb
This would mean that if the engine is used on another database platform (in another application) it would generate the "best" possible name for that platform.

One disadvantage I see is that the autogenerated name should be added explicitly to scheme.rb because the naming rules can change over time and need to be able to rollback an old migration.

Just my 2 cents.

Contributor

tehgeekmeister commented Jan 11, 2013

Seems interesting and sensible to me, but I've seen a trend that the more experienced contributors/rails core people ask that feature requests go onto a certain mailing list, rather than here. Unfortunately, I can't remember which one they mention.

I bet @steveklabnik knows what you should do with such an idea, though! Also, I might be willing and able to help out with implementing such a thing, but not for a little while now.

Contributor

tehgeekmeister commented Jan 13, 2013

Hey @carlosantoniodasilva, it's been a few days since I've heard from you. Is the test case I've added sufficient? It fails without the changes, and passes with them. I've rebased this onto master again to make it easy to merge, once it's accepted, but I'm also happy to rework any bits of it if need be.

Contributor

AxisOfEval commented Mar 7, 2013

Damn! This is EXTREMELY annoying. Every time I have to roll back migrations, things bork and I have to do the rake db::drop && rake db:create dance. Why is this not in the core yet? The assumptions and code seem pretty safe to me...

Contributor

tehgeekmeister commented Mar 7, 2013

This is run by volunteers. I can't merge it, and the people who can and
know more about it have been, quite understandably, busy with security and
release stuff. I'm hoping it gets in soon, too! It's my first significant
patch, so I'd be pretty stoked for it to make it's way in.

On Thursday, March 7, 2013, Amol Hatwar wrote:

Damn! This is EXTREMELY annoying. Every time I have to roll migrations,
things bork and I have to do the rake db::drop && rake db:create dance.
Why is this not in the core yet? The assumptions and code seem pretty safe
to me...


Reply to this email directly or view it on GitHubhttps://github.com/rails/rails/pull/8868#issuecomment-14584926
.

Contributor

tehgeekmeister commented Mar 7, 2013

Alternately, though, you should be able to merge it into your local install
relatively smoothly, I'd guess. Or you could pay someone to do it for you,
if you don't want to do it yourself and can't wait.

On Thursday, March 7, 2013, Ezekiel Smithburg wrote:

This is run by volunteers. I can't merge it, and the people who can and
know more about it have been, quite understandably, busy with security and
release stuff. I'm hoping it gets in soon, too! It's my first significant
patch, so I'd be pretty stoked for it to make it's way in.

On Thursday, March 7, 2013, Amol Hatwar wrote:

Damn! This is EXTREMELY annoying. Every time I have to roll migrations,
things bork and I have to do the rake db::drop && rake db:create dance.
Why is this not in the core yet? The assumptions and code seem pretty safe
to me...


Reply to this email directly or view it on GitHubhttps://github.com/rails/rails/pull/8868#issuecomment-14584926
.

Owner

rafaelfranca commented Mar 7, 2013

We need a CHANGELOG entry

@ghost ghost assigned rafaelfranca Mar 7, 2013

Contributor

tehgeekmeister commented Mar 7, 2013

Will add one once I'm at a computer!

On Thursday, March 7, 2013, Rafael Mendonça França wrote:

We need a CHANGELOG entry


Reply to this email directly or view it on GitHubhttps://github.com/rails/rails/pull/8868#issuecomment-14588888
.

Owner

rafaelfranca commented Mar 7, 2013

Great! Ping me when done

If an index can't be found by column, use the index name.
schema_statements uses the column name by default to construct the index name, and then raises an exception if it doesn't exist, even if the name option is specified, which causes #8858.  this commit makes index_name_for_remove fall back to constructing the index name to remove based on the name option.
Contributor

tehgeekmeister commented Mar 8, 2013

@rafaelfranca I think it's ready.

yahonda added a commit to yahonda/oracle-enhanced that referenced this pull request Mar 10, 2013

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