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

Fix GH #4674. Reset column information and sequence name when setting table_name. #5267

Merged
merged 2 commits into from Mar 4, 2012

Conversation

kennyj
Copy link
Contributor

@kennyj kennyj commented Mar 4, 2012

In now implementation, when calling table_name=, we reset @quoted_table_name, @arel_table, but we don't reset column information and sequence name.

Please see #4674 .

BTW, the original issue was happen on 3-2-stable.

@josevalim
Copy link
Contributor

/cc @jonleighton

@jonleighton
Copy link
Member

@kennyj I am concerned about wiping out @sequence_name. What if the user does:

class Foo < AR::Base
  self.sequence_name = "omg"
  self.table_name    = "lol" # sequence name is now wiped
end

I guess we probably need to get track of any explicitly set sequence name, separately from the cached sequence name value, can you look at that?

Thanks

@kennyj
Copy link
Contributor Author

kennyj commented Mar 4, 2012

@jonleighton OK. I'll look at that.

@kennyj
Copy link
Contributor Author

kennyj commented Mar 4, 2012

@jonleighton Rebased and updated !

Please review it. If necessary, I'll squash commits.

jonleighton added a commit that referenced this pull request Mar 4, 2012
Fix GH #4674. Reset column information and sequence name when setting table_name.
@jonleighton jonleighton merged commit 2fdb521 into rails:master Mar 4, 2012
@ebeigarts
Copy link
Contributor

Can somebody explain why there is a unless before_seq.blank? && after_seq.blank? in test assertion?

I think something is wrong here, sequence_name now returns false, since reset_sequence_name now returns always false and it is assigned to @sequence_name.

model_schema.rb#L168:

@sequence_name ||= reset_sequence_name

@kennyj
Copy link
Contributor Author

kennyj commented Apr 13, 2012

@ebeigarts Thanks ! I was wrong, and I'll fix it.

BTW: unless before_seq.blank? && after_seq.blank? means that sequence is supported ;-)

@kennyj
Copy link
Contributor Author

kennyj commented Apr 13, 2012

I sent a PR #5832. Thanks :-)

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

4 participants