#60 ✓resolved
schof

Text dates are incorrectly converted into UTC

Reported by schof | March 2nd, 2009 @ 08:21 PM

My date searches are coming up wrong and it seems to be related to UTC.


>> search = Foo.new_search
=> #<FooSearch {:limit=>25}>
>> search.conditions.created_at_before = "03/02/2009"
=> "03/02/2009"
>> search.conditions
=> #<FooConditions {:created_at_less_than=>Mon Mar 02 00:00:00 UTC 2009}>

Yes, I think we want the condition in UTC so we can compare to timestamp but it appears to assume the string is already in UTC. Shouldn't we assume the local timezone instead?

Comments and changes to this ticket

  • schof

    schof March 3rd, 2009 @ 11:30 AM

    I believe I fixed this issue. There may be a more elegant fix. Also, I did not have time to create a test but existing tests run fine.

    http://github.com/schof/searchlo...

  • Ben Johnson

    Ben Johnson March 6th, 2009 @ 05:38 AM

    • State changed from “new” to “open”

    Searchlogic actually doesn't assume anything, I believe I delegate time zone conversions to ActiveRecord. Does ActiveRecord assume string dates and times are local? I feel like its best practice to follow the standards set by that, since that is what is expected of searchlogic.

  • schof

    schof March 6th, 2009 @ 12:34 PM

    You are correct that Searchlogic is using the ActiveRecord conversions. Specifically ActiveRecord::ConnectionAdapters::Column#string_to_date is the problem here.

    I totally agree with your philosophy of not messing with expected ActiveRecord behavior and I think in most cases it serves the project well. This is a case, however, where I feel the ActiveRecord behavior is at odds with what the user would expect when searching.

    Maybe we could have a global setting or something to use local time zone for string dates? There's no way that the average user would expect that the dates they are searching on are UTC. Nobody thinks in UTC but programmers. I could convert the dates on the server end but this takes away a lot of the beauty of using Searchlogic.

    Can we come up with a reasonable compromise here?

  • Ben Johnson

    Ben Johnson March 23rd, 2009 @ 03:07 AM

    I agree with you, but my apps are properly converting times. I have various time / date searching in my apps and it seems to be working. The same goes with saving objects.

    I have this in my evironment / config:

    config.time_zone = "Eastern Time (US & Canada)"
    ENV['TZ'] = 'UTC'
    
  • schof

    schof March 23rd, 2009 @ 11:32 AM

    I had the first line but not the second. The second line does seem to fix my console example shown above (in the original ticket description.)

    Why is the second line needed? Rails should always convert your times into UTC before saving. Does this variable have something to do with how the string dates are to be parsed? I'd like to understand a little more about what this setting does (Google yields surprisingly little insight so far.)

  • schof

    schof March 23rd, 2009 @ 12:08 PM

    Actually I just realized that I still had my custom gem installed when I tested your solution. It still does not work (even with the ENV['TZ'] thing.)

    Take a look at my example again

    
    >> search = Foo.new_search
    => #<FooSearch {:limit=>25}>
    >> search.conditions.created_at_before = "03/02/2009"
    => "03/02/2009"
    >> search.conditions
    => #<FooConditions {:created_at_less_than=>Mon Mar 02 00:00:00 UTC 2009}>
    

    I am passing a String in the created_at_before and not a Date. Are you getting results that are somehow different then what I get in my console? In your examples you usually show an actually Date object being used and those work fine. My problem is when the user enters a String in the search (a perfectly reasonable thing to do.)

  • schof

    schof March 23rd, 2009 @ 07:55 PM

    Ben, would you be willing to consider a patch that makes it a configuration option to create new string dates in local time zone? It wasn't clear to me from your last answer. We'd like to get off the forked gem and back on the mainline so we're willing to do the work.

    Please let me know if the problem is not entirely clear. I can provide further examples if necessary.

  • Ben Johnson

    Ben Johnson April 16th, 2009 @ 03:35 AM

    I get what you're say, I'd just like to reproduce the problem because searchlogic deals with none of this. All that is does is leverage the tools already provided by AR. It does nothing wih dates or times.

  • Ben Johnson

    Ben Johnson April 16th, 2009 @ 07:12 PM

    Yeah, write up a test case and I'll see what I can do. It shouldn't be a difficult problem to fix, I just don't want to make changes then other people start complaining about how it doesnt work for them.

  • schof

    schof April 16th, 2009 @ 07:21 PM

    Test case is probably the harder thing to do really since I generally use rspec. I have no problem contributing the fix as well. We'll make it so default behavior is to work as it does now so there are no problems. We just need the option to work in what I would argue is the more intuitive way. This won't jam anyone up.

  • schof

    schof April 17th, 2009 @ 11:35 AM

    OK I spent several hours with another developer looking into this. I created a test to show the problem but surprisingly it worked. Finally I figured out that if you set @@ActiveRecord::Base.default_timezone = :utc@@ the test fails. Why is this important? Because this is what automatically happens when you set @@config.time_zone@@ in your @@environment.rb@@

    See the results at http://pastie.textmate.org/449890 which show both my new test and your existing test failing. We could still go the configuration route but this clearly is a bug since your own test fails under these conditions.

  • schof

    schof April 17th, 2009 @ 11:36 AM

    bahh ignore the @@ crap, trying to format nicely for you

  • dale

    dale April 17th, 2009 @ 12:50 PM

    Sean's fix works nicely for me, but i noticed the tests have problems with it. Apparently the Time.zone.utc_offset is unavailable in the testsuite.

    At the moment without this fix you need to convert the dates from your form input fields in your controller. This kinda defies the concept of having all your search logic in your view.

    Would be nice if we could get this resolved.

    Keep up the great work, really liking your gems and the excellent documentation that comes with them.

  • schof

    schof April 17th, 2009 @ 03:07 PM

    This is a test and partial fix for Issue #60. Actually, it completely fixes #60 but it still breaks another test.

    http://github.com/schof/searchlo...

    The test that is still broken breaks with or without my fix, as long as you use my extra lines in the test_helper which I would argue are essential (since they replicate standard AR behavior when config.timezone is used in Rails.)

    So we need to fix the broken test anyways. Maybe you can take this the rest of the way Ben? Sorry to keep harping on this but we are using this in a large open source project and everybody keeps having problems with the vendored gem we are resorting to now (that's a rails problem with vendor/gems not searchlogic.)

  • Ben Johnson

    Ben Johnson April 17th, 2009 @ 03:13 PM

    No problem, when I get done with work today I'll take care of it. To be honest, I'd like to rewrite searchlogic to be a little simpler and integrate less into ActiveRecord. I almost want it to be completely separate. Ex: you can only do the search logic magic off of a searchlogic object. Anyways, just thinking out loud.

  • Ben Johnson

    Ben Johnson April 18th, 2009 @ 01:37 AM

    • State changed from “open” to “resolved”

    This is all set. Let me know if you have any other problems.

  • schof

    schof April 18th, 2009 @ 08:25 AM

    Thanks a lot. Do you mind pushing another release? The biggest problem our users are having is that we are putting our own copy of the gem in vendor/gems and Rails seems to have issues with that on Linux based systems (don't ask me why.)

    By the way, I love searchlogic and I've been getting a lot of compliments on it from Rails devs that have discovered it through the Spree project.

  • schof

    schof April 20th, 2009 @ 07:01 AM

    Thanks for the new release!

Please Sign in or create a free account to add a new ticket.

With your very own profile, you can contribute to projects, track your activity, watch tickets, receive and update tickets through your email and much more.

New-ticket Create new ticket

Create your profile

Help contribute to this project by taking a few moments to create your personal profile. Create your profile ยป

Provides common named scopes and object based searching.

People watching this ticket

Referenced by

Pages