quoted_datetime raises an error when given an ActiveSupport::TimeWithZone instance

[dball]

It raises an ArgumentError: wrong number of arguments (1 for 0), apparently when calling the iso8601 method with the 3 argument:

 /Users/dball/.rvm/gems/ree-1.8.7-2012.02@decisiv/gems/activerecord-sqlserver-adapter-3.2.2/lib/active_record/connection_adapters/sqlserver/quoting.rb:70:in `iso8601' /Users/dball/.rvm/gems/ree-1.8.7-2012.02@decisiv/gems/activerecord-sqlserver-adapter-3.2.2/lib/active_record/connection_adapters/sqlserver/quoting.rb:70:in `quoted_datetime'

Here:

 68         def quoted_datetime(value)
 69           if value.acts_like?(:time)
 70             debugger if $debug
 71             value.is_a?(Date) ? quoted_value_acts_like_time_filter(value).to_time.xmlschema.to(18) : quoted_value_acts_like_time_filter(value).iso8601(3).to(22)
 72           else
 73             quoted_date(value)
 74           end
 75         end

[metaskills]

Donald, can you drop me some more info? Do the adapter/activerecord tests fail for you? When I released the latest gem, all tests were green and I am finding it hard to think that this situation would not be exercised.

[dball]

I found this exception being thrown in the unit tests for a rails app I'm upgrading from 2 to 3. I have not yet run the adapter tests, but will make a point to try to do so and see if I can expose this with a specific failing test case.

The workaround is unfortunately trivial. I just call to_s on the ActiveSupport::TimeWithZone object before passing it to the attribute setter method. (While I'm glad to have a workaround, it lessens the chances I'll have time to follow up on this issue sooner than later :(.)

[metaskills]

Please do, the likelihood is that this has nothing to do with the adapter. But I would really like to know either way and will keep this open till I hear back from you.

[metaskills]

In active_support/core_ext/date/conversions.rb

  def iso8601
    strftime('%F')
  end if RUBY_VERSION < '1.9'

Some example IRB code.

# Ruby 1.9.3
require 'active_support/all'
# => true
Time.now.iso8601(3)
# => "2012-04-19T16:03:19.866-04:00"
DateTime.now.iso8601(3)
# => "2012-04-19T16:03:32.308-04:00"

# Ruby 1.8.7/REE
require 'active_support/all'
# => true
Time.now.iso8601(3)
# => "2012-04-19T16:07:39.526-04:00"
DateTime.now.iso8601(3)
# ArgumentError: wrong number of arguments (1 for 0)
#   from (irb):4:in `iso8601'
#   from (irb):4

[metaskills]

I've long run the tests in both 1.8.7/REE and 1.9. here lately, the adapter was only tested in 1.9.3. But that code above is way old. So it has passed before in 1.8.7/REE. Please do the leg work and let me know where things are breaking down and when.

[dball]

The iso8601 method was added to ActiveSupport::DateTime in this patch:

https://github.com/rails/rails/commit/944d314244676932eb1aa285d23f7d91f0678e68

Looks like it was introduced circa 3.1.0beta.

The adapter tests don't actually exercise this case, of course. quoted_datetime is exercised indirectly, but never with an ActiveSupport::DateTime instance. This runs clean in ruby 1.8.7 with rails 3.2.3:

diff --git a/lib/active_record/connection_adapters/sqlserver/quoting.rb b/lib/active_record/connection_adapters/sqlserve
index 1aad866..5d86d5e 100644
--- a/lib/active_record/connection_adapters/sqlserver/quoting.rb
+++ b/lib/active_record/connection_adapters/sqlserver/quoting.rb
@@ -67,7 +67,15 @@ module ActiveRecord

         def quoted_datetime(value)
           if value.acts_like?(:time)
-            value.is_a?(Date) ? quoted_value_acts_like_time_filter(value).to_time.xmlschema.to(18) : quoted_value_acts_
+            if value.is_a?(Date)
+              quoted_value_acts_like_time_filter(value).to_time.xmlschema.to(18)
+            elsif value.class.name == 'ActiveSupport::DateTime'
+              raise "hell"
+            else
+              quoted_value_acts_like_time_filter(value).iso8601(3).to(22)
+            end
           else
             quoted_date(value)
           end

A correct solution would be to patch ActiveSupport's iso8601 method to conform to the method signature of the ruby-1.9 iso8601 method. I will pursue that, but in the interim, would you accept a patch that either eliminated the fractional seconds argument, or one that examined the arity of the iso8601 method and called it with the arguments it expects?

Assuming you're amenable, it's unclear where the failing tests case ought to live. Quoting doesn't seem to have a specific unit test, and I haven't found anywhere else in the test cases where you assert quoting behavior for values, only for column and table names. I'd be happy to take a stab at writing such a unit test for the Quoting module if you'd like?

[dball]

The tests even continue to pass when I correct the name of the DateTime constant from "ActiveSupport:DateTime" to "DateTime" :)

[metaskills]

Proper arity patch would be nice. Look around for a test place, there should be a quote section somewhere.

[gburch]

I am seeing this behavior as well with 3.2.3. A temporary work around is to use Time rather than DateTime if possible.

[dball]

I wrote out a full spec for the Quoting module but was unable to reproduce the error therein. Curious, I fired up the app test suite which gave me the error in the first place and added a debugger at the error spot, then did a comparable exploration in the working test suite. The results are here:

https://gist.github.com/2434109

The upshot is that the ActiveSupport::TimeWithZone instance's @utc instance variable contains a DateTime in the bad case, and a Time in the good case. Hold cats, that's bizarre, and awfully subtle.

I'm having trouble tracking down how the instance variable gets its bad DateTime value. I'm actually not yet even able to track where the ActiveSupport::TimeWithZone instance comes from. I've put a break point on the class's constructor, but nothing fires it in the requisite time frame.

I'm continuing to explore, I just figured I'd post an update with this finding.

[metaskills]

TL;DR - Don't use datetimeoffset or time data types.

Good followup, here are a few of my thoughts. Re: your finding the bug when it is a DateTime wrapped with AS::TWZ reflects my finding above. No matter what the class is wrapped in, the core issue is that Ruby 1.8 implementations of DateTime#iso8601 has no arguments to control the #usec of the output. Since Rails 3.2 is the last version to support 1.8 then I can see how we too would not concern ourselves with this past our 3.2 series too. However, I do want to fix this so 3-2-stable is in a solid position.

So, here are some thoughts on why we never see this in the test suite. Here is the calling case from quoting.rb.

when Date, Time
  if column && column.sql_type == 'datetime'
    "'#{quoted_datetime(value)}'"
  elsif column && (column.sql_type == 'datetimeoffset' || column.sql_type == 'time')
    "'#{quoted_full_iso8601(value)}'"
  else

So, we can see this only happens for the special datetimeoffset and time data types. Now go look at this documentation in TinyTDS. https://github.com/rails-sqlserver/tiny_tds/blob/master/test/schema_test.rb#L305

See the issue. FreeTDS does not support these data types yet. That means they will come back as strings. What happens in this situation with the adapter and TinyTDS is what happens under RubyODBC, any non-native type that comes back from the database (since RubyODBC does not do this like TinyTDS in some cases) ActiveRecord will take over and cast things.

If you look closely at both projects, both of them have no tests for the unsupported data types of FreeTDS. I felt no need to test the returned strings under TinyTDS and I have never tested the adapter with them either. Basically, even if we did fix this, using these datatypes is a bad idea. Mainly because the strings that come back in no way represent the true underlying data from each type. Like there is no TZ info in the datetimeoffset type.

[dball]

The bug actually manifests itself entirely within the quoted_datetime method. I think you aptly demonstrate the potential for yet another flavor of date/time bug within FreeTDS when using the datetimeoffset or time colums, but that's beyond the scope of this bug.

I have a failing test case and a fix which I'll attach in a pull request momentarily.

[metaskills]

Fixed in #189