Issue with use of DateTime in Recoverable concern

[emeryamiller]

I'm getting a NoMethodError - undefined method `getlocal' when calling the /oauth/token path.

I'm using Ruby 2.1.5 and Rails 4.2 with Doorkeeper 2.2.1.

It looks like the issue stems from ActiveRecord calling getlocal on the object it's passed when updating the database. The issue is that the Recoverable concern passes a DateTime object, which doesn't have a getlocal method. Only Time has that.

As an expedient fix for my code, I've updated the line:

def revoke(clock = DateTime)
        update_attribute :revoked_at, clock.now
end

to

def revoke(clock = Time)
        update_attribute :revoked_at, clock.now
end

This fixes the issue, but this looks more like an ActiveRecord bug, since I don't see why it wouldn't be able to handle DateTime objects.

Here is part of the stacktrace:

Started POST "/oauth/token" for ::1 at 2015-05-28 10:34:26 -0400
...
NoMethodError - undefined method getlocal' for Thu, 28 May 2015 10:34:26 -0400:DateTime: activerecord (4.2.0) lib/active_record/type/date_time.rb:14:intype_cast_for_database'
activerecord (4.2.0) lib/active_record/attribute.rb:46:in value_for_database' activerecord (4.2.0) lib/active_record/attribute_methods/dirty.rb:164:instore_original_raw_attribute'
activerecord (4.2.0) lib/active_record/attribute_methods/dirty.rb:93:in write_attribute' activerecord (4.2.0) lib/active_record/attribute_methods.rb:50:in__temp__275667f6b65646f51647'
activerecord (4.2.0) lib/active_record/persistence.rb:238:in update_attribute' doorkeeper (2.2.1) lib/doorkeeper/models/concerns/revocable.rb:5:inrevoke'

And the relevant code from active record:

activerecord (4.2.0) lib/active_record/type/date_time.rb, line 14

    9
   10         def type_cast_for_database(value)
   11           zone_conversion_method = ActiveRecord::Base.default_timezone == :utc ? :getutc : :getlocal
   12
   13           if value.acts_like?(:time)
   14             value.send(zone_conversion_method)
   15           else
   16             super
   17           end
   18         end
   19

[tute]

Thanks for your report, @emeryamiller!

@sgrif Is this an AR bug? In that case we can move it to Rails.

The following patch keeps CI green indeed, I'm not against such a PR into doorkeeper:

diff --git a/lib/doorkeeper/models/concerns/revocable.rb b/lib/doorkeeper/models/concerns/revocable.rb
index 07b8ce8..96b70e1 100644
--- a/lib/doorkeeper/models/concerns/revocable.rb
+++ b/lib/doorkeeper/models/concerns/revocable.rb
@@ -1,12 +1,12 @@
 module Doorkeeper
   module Models
     module Revocable
-      def revoke(clock = DateTime)
+      def revoke(clock = Time)
         update_attribute :revoked_at, clock.now
       end

       def revoked?
-        !!(revoked_at && revoked_at <= DateTime.now)
+        !!(revoked_at && revoked_at <= Time.now)
       end
     end
   end

[tute]

It's surprising to me nobody saw this before, though.

[emeryamiller]

Ya, I was surprised too. Had to double check it before reporting it to make sure I wasn't doing something silly.

[sgrif]

No, this doesn't look like a Rails bug. Generally we don't advise using DateTime with Active Record, as it never works as well as Time does.

[sgrif]

https://twitter.com/sgrif/status/604823869061406723

[tute]

Here is a gist explaining why it should be Time: https://gist.github.com/pixeltrix/e2298822dd89d854444b. Can you send a PR, @emeryamiller?
Thanks for your help, Sean!

[tute]

Closing in favor of good PR. Thanks!