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

Rails 3.2.5 Regression: incorrect _changed? for datetimes #6591

Closed
earnold opened this Issue Jun 1, 2012 · 21 comments

Comments

Projects
None yet
@earnold
Contributor

earnold commented Jun 1, 2012

I just upgraded from 3.2.2 to 3.2.5, and now I have failing specs. I've identified the problem as this.

When you have this model:

  create_table "posts", :force => true do |t|
    t.string   "title"
    t.datetime "published_at"
  end

previously you could do:

1.9.3p125 :013 >p = Post.last
1.9.3p125 :014 > p.published_at = p.published_at
1.9.3p125 :015 > p.changed?
=> false 

but now:

1.9.3p125 :013 >p = Post.last
1.9.3p125 :014 > p.published_at = p.published_at
1.9.3p125 :015 > p.changed?
=> true 
1.9.3p125 :006 > p.changes
 => {"published_at"=>[Thu, 12 Apr 2012 19:32:33 UTC +00:00, Thu, 12 Apr 2012 19:32:33 UTC +00:00]} 

I'm going to see if I can put together a failing test around this.

@carlosantoniodasilva

This comment has been minimized.

Show comment
Hide comment
@carlosantoniodasilva

carlosantoniodasilva Jun 1, 2012

Member

Just a side question: have you upgraded straight from 3.2.2 to 3.2.5? If so, can you try 3.2.3 as well to check the issue? Thanks.

Member

carlosantoniodasilva commented Jun 1, 2012

Just a side question: have you upgraded straight from 3.2.2 to 3.2.5? If so, can you try 3.2.3 as well to check the issue? Thanks.

@earnold

This comment has been minimized.

Show comment
Hide comment
@earnold

earnold Jun 1, 2012

Contributor

I just confirmed that this works correctly with 3.2.3, so it is a new problem

Contributor

earnold commented Jun 1, 2012

I just confirmed that this works correctly with 3.2.3, so it is a new problem

@carlosantoniodasilva

This comment has been minimized.

Show comment
Hide comment
@carlosantoniodasilva

carlosantoniodasilva Jun 1, 2012

Member

Great, we'll investigate, thanks.

Member

carlosantoniodasilva commented Jun 1, 2012

Great, we'll investigate, thanks.

@earnold

This comment has been minimized.

Show comment
Hide comment
@earnold

earnold Jun 1, 2012

Contributor

Here is a failing spec: earnold@5f27132

Contributor

earnold commented Jun 1, 2012

Here is a failing spec: earnold@5f27132

@kennyj

This comment has been minimized.

Show comment
Hide comment
@kennyj

kennyj Jun 3, 2012

Contributor

Please see #3965 and ddb5d2f .
I think they are related to this issue.

Contributor

kennyj commented Jun 3, 2012

Please see #3965 and ddb5d2f .
I think they are related to this issue.

@kennyj

This comment has been minimized.

Show comment
Hide comment
@kennyj

kennyj Jun 3, 2012

Contributor

All tests are passed if we add the above commit.
But I don't realize deeply. Tomorrow, I'll read #3965 more :-)

Contributor

kennyj commented Jun 3, 2012

All tests are passed if we add the above commit.
But I don't realize deeply. Tomorrow, I'll read #3965 more :-)

@kennyj

This comment has been minimized.

Show comment
Hide comment
@kennyj

kennyj Jun 4, 2012

Contributor

cc/ @lest

Contributor

kennyj commented Jun 4, 2012

cc/ @lest

@lest

This comment has been minimized.

Show comment
Hide comment
@lest

lest Jun 4, 2012

Contributor

Datetime attributes was forced to be changed in #5196 because of incorrect dirty tracking described in #3965. Then it was backported to the 3-2-stable branch in #5618.

@kennyj Your fix seems good to me.

Contributor

lest commented Jun 4, 2012

Datetime attributes was forced to be changed in #5196 because of incorrect dirty tracking described in #3965. Then it was backported to the 3-2-stable branch in #5618.

@kennyj Your fix seems good to me.

@kennyj

This comment has been minimized.

Show comment
Hide comment
@kennyj

kennyj Jun 4, 2012

Contributor

I read #3965. and I also do not think the my fix was bad.
I'll submit PR to master and 3-2-stable.

Thanks @lest for reviewing ! :-)

Contributor

kennyj commented Jun 4, 2012

I read #3965. and I also do not think the my fix was bad.
I'll submit PR to master and 3-2-stable.

Thanks @lest for reviewing ! :-)

@kennyj

This comment has been minimized.

Show comment
Hide comment
@kennyj

kennyj Jun 4, 2012

Contributor

I submit PR.
See #6619 .

Contributor

kennyj commented Jun 4, 2012

I submit PR.
See #6619 .

@kennyj kennyj closed this in 635032f Jun 4, 2012

@carlosantoniodasilva

This comment has been minimized.

Show comment
Hide comment
@carlosantoniodasilva

carlosantoniodasilva Jun 4, 2012

Member

Hey guys, thank you for fixing this :)

Member

carlosantoniodasilva commented Jun 4, 2012

Hey guys, thank you for fixing this :)

brunoadacosta pushed a commit to brunoadacosta/rails that referenced this issue Jun 4, 2012

Merge pull request #6619 from kennyj/fix_6591
Fix #6591 Rails 3.2.5 Regression: incorrect _changed? for datetimes

kennyj added a commit to kennyj/rails that referenced this issue Jun 4, 2012

arunagw pushed a commit to arunagw/rails that referenced this issue Jun 4, 2012

Merge pull request #6622 from kennyj/fix_6591-32
[3-2-stable back ported] Fix #6591 Rails 3.2.5 Regression: incorrect _changed? for datetimes
@pacoguzman

This comment has been minimized.

Show comment
Hide comment
@pacoguzman

pacoguzman Jul 5, 2012

Contributor

Hi guys, I've found a similar issue in current rails 3-2-stable. Adding the following test to activerecord/test/cases/dirty_test.rb

  def test_setting_time_attributes_with_time_zone_field_to_same_time_should_not_be_marked_as_a_change
    in_time_zone 'Paris' do
      target = Class.new(ActiveRecord::Base)
      target.table_name = 'pirates'

      created_on = Time.now

      pirate = target.create(:created_on => created_on)
      pirate.reload # Here mysql truncate the usec value to 0

      pirate.created_on = created_on
      assert !pirate.created_on_changed?
    end
  end

I getting the following:

ARCONN=mysql2 ruby -Itest test/cases/dirty_test.rb -n test_setting_time_attributes_with_time_zone_field_to_same_time_should_not_be_marked_as_a_changeUsing mysql2 with Identity Map off
Run options: -n test_setting_time_attributes_with_time_zone_field_to_same_time_should_not_be_marked_as_a_change

# Running tests:

F

Finished tests in 0.087599s, 11.4157 tests/s, 11.4157 assertions/s.

  1) Failure:
test_setting_time_attributes_with_time_zone_field_to_same_time_should_not_be_marked_as_a_change(DirtyTest) [test/cases/dirty_test.rb:104]:
Failed assertion, no message given.

1 tests, 1 assertions, 1 failures, 0 errors, 0 skips
ARCONN=sqlite3 ruby -Itest test/cases/dirty_test.rb -n test_setting_time_attributes_with_time_zone_field_to_same_time_should_not_be_marked_as_a_change
Using sqlite3 with Identity Map off
Run options: -n test_setting_time_attributes_with_time_zone_field_to_same_time_should_not_be_marked_as_a_change

# Running tests:

.

Finished tests in 0.058396s, 17.1245 tests/s, 17.1245 assertions/s.

1 tests, 1 assertions, 0 failures, 0 errors, 0 skips

I have a clue in why this is happening but I don't know where is the right place to fix it or maybe this is not consider a bug.

Time.now.usec
 => 339426

Time.zone.local(2012, 02, 20, 9).usec
 => 0 

Thanks in advance

Contributor

pacoguzman commented Jul 5, 2012

Hi guys, I've found a similar issue in current rails 3-2-stable. Adding the following test to activerecord/test/cases/dirty_test.rb

  def test_setting_time_attributes_with_time_zone_field_to_same_time_should_not_be_marked_as_a_change
    in_time_zone 'Paris' do
      target = Class.new(ActiveRecord::Base)
      target.table_name = 'pirates'

      created_on = Time.now

      pirate = target.create(:created_on => created_on)
      pirate.reload # Here mysql truncate the usec value to 0

      pirate.created_on = created_on
      assert !pirate.created_on_changed?
    end
  end

I getting the following:

ARCONN=mysql2 ruby -Itest test/cases/dirty_test.rb -n test_setting_time_attributes_with_time_zone_field_to_same_time_should_not_be_marked_as_a_changeUsing mysql2 with Identity Map off
Run options: -n test_setting_time_attributes_with_time_zone_field_to_same_time_should_not_be_marked_as_a_change

# Running tests:

F

Finished tests in 0.087599s, 11.4157 tests/s, 11.4157 assertions/s.

  1) Failure:
test_setting_time_attributes_with_time_zone_field_to_same_time_should_not_be_marked_as_a_change(DirtyTest) [test/cases/dirty_test.rb:104]:
Failed assertion, no message given.

1 tests, 1 assertions, 1 failures, 0 errors, 0 skips
ARCONN=sqlite3 ruby -Itest test/cases/dirty_test.rb -n test_setting_time_attributes_with_time_zone_field_to_same_time_should_not_be_marked_as_a_change
Using sqlite3 with Identity Map off
Run options: -n test_setting_time_attributes_with_time_zone_field_to_same_time_should_not_be_marked_as_a_change

# Running tests:

.

Finished tests in 0.058396s, 17.1245 tests/s, 17.1245 assertions/s.

1 tests, 1 assertions, 0 failures, 0 errors, 0 skips

I have a clue in why this is happening but I don't know where is the right place to fix it or maybe this is not consider a bug.

Time.now.usec
 => 339426

Time.zone.local(2012, 02, 20, 9).usec
 => 0 

Thanks in advance

@rafaelfranca

This comment has been minimized.

Show comment
Hide comment
@rafaelfranca

rafaelfranca Jul 5, 2012

Member

@pacoguzman could you open an issue or a pull request with the failing test case?

@pixeltrix @kennyj could you guys take a look on this one?

Member

rafaelfranca commented Jul 5, 2012

@pacoguzman could you open an issue or a pull request with the failing test case?

@pixeltrix @kennyj could you guys take a look on this one?

@pacoguzman

This comment has been minimized.

Show comment
Hide comment
@pacoguzman

pacoguzman Jul 5, 2012

Contributor

@rafaelfranca of course, it's here #6975

Contributor

pacoguzman commented Jul 5, 2012

@rafaelfranca of course, it's here #6975

@kennyj

This comment has been minimized.

Show comment
Hide comment
@kennyj

kennyj Jul 5, 2012

Contributor

@rafaelfranca @pacoguzman I'm taking a look on #6975.

Contributor

kennyj commented Jul 5, 2012

@rafaelfranca @pacoguzman I'm taking a look on #6975.

@tinderfields

This comment has been minimized.

Show comment
Hide comment
@tinderfields

tinderfields Nov 3, 2014

I'm getting this issue in 4.1.6 as well

tinderfields commented Nov 3, 2014

I'm getting this issue in 4.1.6 as well

@luxflux

This comment has been minimized.

Show comment
Hide comment
@luxflux

luxflux Jul 10, 2015

Me too, Rails 4.1.11.

luxflux commented Jul 10, 2015

Me too, Rails 4.1.11.

@mintuhouse

This comment has been minimized.

Show comment
Hide comment
@mintuhouse

mintuhouse Dec 30, 2015

I am able to reproduce it on 4.1.13
using postgres and passing the time from browser as string in format "2015-12-30 4:58 PM"

mintuhouse commented Dec 30, 2015

I am able to reproduce it on 4.1.13
using postgres and passing the time from browser as string in format "2015-12-30 4:58 PM"

@benbartling

This comment has been minimized.

Show comment
Hide comment
@benbartling

benbartling Jan 19, 2016

I'm also having this issue. Rails 4.2.3

benbartling commented Jan 19, 2016

I'm also having this issue. Rails 4.2.3

@maclover7

This comment has been minimized.

Show comment
Hide comment
@maclover7

maclover7 Jan 19, 2016

Member

Is this an issue on master?

Member

maclover7 commented Jan 19, 2016

Is this an issue on master?

@maclover7 maclover7 reopened this Jan 19, 2016

@rafaelfranca

This comment has been minimized.

Show comment
Hide comment
@rafaelfranca

rafaelfranca Jan 20, 2016

Member

Until we have reproduction steps and confirmation that the issue is really present in recent versions we should keep it closed.

Member

rafaelfranca commented Jan 20, 2016

Until we have reproduction steps and confirmation that the issue is really present in recent versions we should keep it closed.

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