postgres timestamptz should return a ActiveSupport::TimeWithZone via OID::Timezone #9431

Merged
merged 1 commit into from Mar 2, 2013

Conversation

Projects
None yet
4 participants
Contributor

troyk commented Feb 26, 2013

Comments were correct, timestamptz should return a date.

Member

steveklabnik commented Feb 26, 2013

/cc @pixeltrix

We'd also need a CHANGELOG entry and a test, I think.

Owner

pixeltrix commented Feb 26, 2013

👍 to what @steveklabnik said

Contributor

troyk commented Feb 27, 2013

/cc @steveklabnik, @pixeltrix

Sorry for overlooking the changelog, it would be nice to get this into the next beta release before more people get hit with it. If your using plain ruby for migrations/schema it's a non-issue as rails uses timestamp and not timestamptz, however most pg shops I know prefer the timestamptz as postgres stores the time internally in UTC allowing the local to be set by the client.

The code comments from timestamptz returning a string stated as much that it probably wasn't right and it's the first thing that blew up when I tried rails4 beta today.

Member

steveklabnik commented Feb 27, 2013

This doesn't merge cleanly, so a rebase needs done, and they'll need to be squashed into one commit as well.

Owner

pixeltrix commented Feb 27, 2013

@troyk still needs a test as well

Contributor

troyk commented Feb 28, 2013

@steveklabnik

followed these steps per the contributing rails guide:

$ git fetch upstream
$ git checkout my_pull_request
$ git rebase upstream/master
$ git rebase -i

< Choose 'squash' for all of your commits except the first one. >
< Edit the commit message to make sense, and describe all your changes. >

$ git push origin my_pull_request -f

but the pull request looks borked now, was I not supposed to pick other commits in the rebase -i ?

rebase -i is only necessary for squashing, and it should show only your commits if it's rebased with current rails master.

Contributor

troyk commented Mar 1, 2013

@steveklabnik @pixeltrix

ok, this should be good to go, sorry for the noise. It's now in a single commit with changelog and I found the reason the test was passing with it returning a string and added an assertion to also verify the attribute is being type cast.

@pixeltrix pixeltrix commented on an outdated diff Mar 1, 2013

...ecord/test/cases/adapters/postgresql/datatype_test.rb
@@ -573,6 +573,7 @@ def test_timestamp_with_zone_values_with_rails_time_zone_support
@first_timestamp_with_zone = PostgresqlTimestampWithZone.find(1)
assert_equal Time.utc(2010,1,1, 11,0,0), @first_timestamp_with_zone.time
+ assert_instance_of ActiveSupport::TimeWithZone, @first_timestamp_with_zone
@pixeltrix

pixeltrix Mar 1, 2013

Owner

Isn't @first_timestamp_with_zone a Active Model? Should this line be:

assert_instance_of ActiveSupport::TimeWithZone, @first_timestamp_with_zone.time
Owner

pixeltrix commented Mar 1, 2013

@troyk why was the test passing passing?

Contributor

troyk commented Mar 1, 2013

@pixeltrix

Sorry about missing the .time attribute, I end up copy/pasting the changes on a clean rebase because I can't figure out how to rebase without bringing in others commits, mostly because I can't get a clean CHANGELOG merge.

The test as it sits passes because:

Time.utc(2010,1,1, 11,0,0) == "2010-01-01 11:00:00+00"
=> true

Notice the right side is a string, not a Time object. So in our app we use SQL in our migrations to take advantage of pg's rich datatypes and constraints and every date/time attribute on our models is currently returning a String when we moved to 4beta and of course this made it through because the test assert_equal Time.utc(2010,1,1, 11,0,0), @first_timestamp_with_zone.time will pass with a string value.

Thus the added assert_instance_of will complete the test to ensure equality and type.

I should probably switch testing the instance for ActiveSupport::TimeWithZone to Time begin the assert_equal has the value covered...

@troyk troyk Fix PostgreSQL TIMESTAMP WITH TIME ZONE to return ActiveSupport::Time
In an AR model a timestamptz attribute would return a ruby string and AR
tests did not check for any type casting.  Previous tests would pass
only because an assert_equal was being used on a Time.utc object, which
will parse the right side of the eq to a valid Time instance for
comparision.

switch to test instance of Time instead of ActiveSupport::TimeWithZone
2cc0944

@pixeltrix pixeltrix added a commit that referenced this pull request Mar 2, 2013

@pixeltrix pixeltrix Merge pull request #9431 from troyk/patch-2
Fix PostgreSQL TIMESTAMP WITH TIME ZONE to return ActiveSupport::Time
c09f934

@pixeltrix pixeltrix merged commit 2cc0944 into rails:master Mar 2, 2013

Owner

pixeltrix commented Mar 2, 2013

Merged c09f934 - thanks @troyk ❤️

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