Pull #6986 causes regression in version 3.2.9 #8195

Closed
sferik opened this Issue Nov 13, 2012 · 10 comments

Projects

None yet

5 participants

@sferik
sferik commented Nov 13, 2012

Pull #6986 causes a backward-incompatible change in behavior in Rails version 3.2.9.

I have two issues with this pull request:

  1. The code was introduced to fix a MySQL-specific behavior but it affects people who aren't using a mysql adapter.
  2. It causes a regression in behavior from the previous patch version.

Here is an example test that passes in version 3.2.8 but fails in 3.2.9:

class Parent < ActiveRecord::Base
  has_one :child
end

class Child < ActiveRecord::Base
  belongs_to :parent, :touch => true
end

setup do
  @parent = Parent.create
  Child.create(:parent => @parent)
end

def test_cache_key_changes_when_child_touched
  key = @parent.cache_key
  @parent.child.touch
  @parent.reload
  assert_not_equal key, @parent.cache_key # F
end

In version 3.2.8, this test passed because it only required a fraction of a second to pass between when the parent was created an touched.

The only way I can make this test pass in version 3.2.9 (without monkey patching) is by adding sleep 1 before @object.child.touch.

@rafaelfranca
Member

This is also an issue in master and should be fixed before the 4.0.0 and backported to 3-2-stable.

@nikitug nikitug added a commit to nikitug/rails that referenced this issue Nov 13, 2012
@nikitug nikitug Add a regression test on #8195 ff2973a
@nikitug
nikitug commented Nov 13, 2012

@rafaelfranca I've failed to reproduce this particular cache_key issue in master. Also this problem seems not to be related to #6986. It was fixed in #6376 and can be safely backported.

I've submitted a PR with a regression test #8201. Ping me back if I should backport this PRs (#8201, #6376) too 🍺

@rafaelfranca rafaelfranca added a commit to rafaelfranca/omg-rails that referenced this issue Nov 13, 2012
@nikitug nikitug Add a regression test on #8195 6be2f5e
@rafaelfranca
Member

@nikitug you are right. Thank you to take a look on it.

@sferik could you test your application against this branch? https://github.com/rafaelfranca/rails/tree/3-2-stable-fix

@sferik
sferik commented Nov 13, 2012

@rafaelfranca I'm still getting those errors when I run my tests against your branch.

@jacobstr

I added the test as specified by @sferik . It add some models that are likely quite redundant.

@jacobstr jacobstr added a commit to ECE322RailsTest/rails that referenced this issue Nov 28, 2012
@jacobstr jacobstr Added regression test for #8195. bfa9861
@rafaelfranca
Member

@sferik I just merged the @jacobstr tests and reverted the commit added in #6986 and the tests are still failing. So the cause is other. I'm trying to track down and solve it.

Sorry for the delay.

@rafaelfranca
Member

@sferik just testes in 3-2-stable, got the @jacobstr commit and the tests was failing. Cherry picked #6376 and the tests are passing.

See rafaelfranca@rails:3-2-stable...rafaelfranca:3-2-stable-cache-key

Are you sure your tests are not passing in this branch. I still can't reproduce your issue. Could you give me more details?

@sferik
sferik commented Dec 1, 2012

Not sure why it wasn't working earlier but my tests do seem to be passing now. Thanks for fixing this.

@parndt
parndt commented Dec 6, 2012

Is this issue resolved?

@rafaelfranca
Member

@parndt need some work from my part, but yes, it is. I'll close when push my commits.

@rafaelfranca rafaelfranca was assigned Dec 6, 2012
@rafaelfranca rafaelfranca added a commit to rafaelfranca/omg-rails that referenced this issue Dec 10, 2012
@jacobstr jacobstr Added regression test for #8195. cc99580
@rafaelfranca rafaelfranca added a commit that referenced this issue Dec 10, 2012
@rafaelfranca rafaelfranca Make sure the tests pass in the case closer to described in #8195
Conflicts:
	activerecord/test/models/bulb.rb
	activerecord/test/schema/schema.rb
3142bf5
@rafaelfranca rafaelfranca added a commit that closed this issue Dec 10, 2012
@rafaelfranca rafaelfranca Allow users to choose the timestamp format in the cache key
This can be done using the class attribute cache_timestamp_format

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