Skip to content
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

Account for time zones in DateTime serializations #102

Merged
merged 2 commits into from Dec 16, 2022

Conversation

nshki
Copy link
Contributor

@nshki nshki commented Oct 16, 2022

Issue

Fixes #101.

Overview

This PR fixes an issue with DateTime serializations where time zones could break string equality checks if DateTimes with differing time zones get serialized. As mentioned in the issue above, I was able to produce an error using the following in a Rails console:

Here's some code that I ran in a Rails console to replicate:

datetime_list = Kredis.list("testing", typed: :datetime)
datetime_list.append([1.minute.ago, 1.hour.ago, 1.day.ago])
datetime_list.remove(datetime_list.elements.first)
datetime_list.elements #=> Still has 3 elements

This works if I don't specify typed:

datetime_list = Kredis.list("testing")
datetime_list.append([1.minute.ago, 1.hour.ago, 1.day.ago])
datetime_list.remove(datetime_list.elements.first)
datetime_list.elements #=> Only has 2 elements, so it worked!

After digging into the source code a bit, I realized that Kredis::Type::DateTime was always serializing to a string, which could produce different results for the same DateTime depending on the set time zone.

My proposed solution here is to convert DateTime instances to UTC before serialization so that strings get compared in the context of UTC. I thought this should be acceptable since it will still be able to read existing values from Redis without problem and perform operations on them.

The first commit produces a red test. The second commit gets it green.

This proves the existence of a time zone-related bug where serializing
with time zone information breaks equality checks.
@nshki nshki changed the title Account for time zones in datetime serializations Account for time zones in DateTime serializations Oct 16, 2022
@dhh dhh merged commit ff09f2b into rails:main Dec 16, 2022
lewispb added a commit to lewispb/kredis that referenced this pull request Jun 19, 2023
* main: (21 commits)
  Bump version for 1.4.0
  Update nokogiri for compatibility
  Revert "Improved version of UniqueList: OrderedSet (rails#76)" (rails#111)
  Add `last` to lists (rails#97)
  Improved version of UniqueList: OrderedSet (rails#76)
  Return Time objects instead of deprecated DateTime (rails#106)
  Fix possible deserialization of untrusted data
  Typecast return of Set#take (rails#105)
  Declare Active Model dependency (rails#107)
  Address LogSubscriber deprecation (rails#98)
  Account for time zones in DateTime serializations (rails#102)
  Add sample to set (rails#100)
  Bump version for 1.3.0
  Allow Redis 5.x
  Add ltrim to lists
  Coalesce "current pipeline or redis" into the redis method itself
  Pefer a thread_mattr_accessor over a thread local variable
  Delete list of keys in batch (rails#90)
  Use a thread-local variable for pipeline
  Revert "Use block parameter to pipeline in Redis#multi (rails#68)"
  ...
lewispb added a commit to basecamp/kredis that referenced this pull request Jul 8, 2023
…tialize

* origin/main: (22 commits)
  Add kredis_ordered_set for OrderedSet usage in models
  Add a development console
  Bump version for 1.5.0
  Fix ordered set prepend bug (rails#115)
  Unique list with sorted set (rails#114)
  Eliminating Ruby Warnings (rails#112)
  CI against Redis 7, Ruby 3.1, and Ruby 3.2 (rails#113)
  Bump version for 1.4.0
  Update nokogiri for compatibility
  Revert "Improved version of UniqueList: OrderedSet (rails#76)" (rails#111)
  Add `last` to lists (rails#97)
  Improved version of UniqueList: OrderedSet (rails#76)
  Return Time objects instead of deprecated DateTime (rails#106)
  Fix possible deserialization of untrusted data
  Typecast return of Set#take (rails#105)
  Declare Active Model dependency (rails#107)
  Address LogSubscriber deprecation (rails#98)
  Account for time zones in DateTime serializations (rails#102)
  Add sample to set (rails#100)
  Bump version for 1.3.0
  ...
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Development

Successfully merging this pull request may close these issues.

Can't remove DateTimes from a typed list
2 participants