Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

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

Projects
None yet
4 participants
Contributor

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.

Contributor

josevalim commented Mar 4, 2012

Member

jonleighton commented Mar 4, 2012

@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

Contributor

kennyj commented Mar 4, 2012

@jonleighton OK. I'll look at that.

Contributor

kennyj commented Mar 4, 2012

@jonleighton Rebased and updated !

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

@jonleighton jonleighton added a commit that referenced this pull request Mar 4, 2012

@jonleighton jonleighton Merge pull request #5267 from kennyj/fix_4674
Fix GH #4674. Reset column information and sequence name when setting table_name.
2fdb521

@jonleighton jonleighton merged commit 2fdb521 into rails:master Mar 4, 2012

Contributor

ebeigarts commented Apr 12, 2012

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
Contributor

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 ;-)

Contributor

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