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

Replace time by UTC #57

Merged
merged 1 commit into from Aug 25, 2018

Conversation

Projects
None yet
3 participants
@dks17
Copy link
Collaborator

dks17 commented Aug 24, 2018

Replaced Time.now by Time.now.utc.
I think this is helpfully for better synchronization of applications located in several time zones.

Mongoid also uses UTC

def set_created_at
  if !timeless? && !created_at
    time = Time.now.utc
    self.updated_at = time if is_a?(Updated) && !updated_at_changed?
    self.created_at = time
  end
  clear_timeless_option
end

@dks17 dks17 referenced this pull request Aug 24, 2018

Closed

Release v. Next #56

@dks17 dks17 force-pushed the dks17:utc_time branch from b30f92e to b5729ba Aug 24, 2018

@afeld
Copy link
Collaborator

afeld left a comment

My only concern is that this could cause bugs for users that have outstanding locks, as the time zones won't necessarily match. Not sure that should be a blocker though.

@@ -15,14 +15,14 @@ module ClassMethods
#
# @return [Mongoid::Criteria]
def locked
where locked_until_field.gt => Time.now
where locked_until_field.gt => Time.now.utc

This comment has been minimized.

@afeld

afeld Aug 24, 2018

Collaborator

Can we get a 'now' method extracted so all of these references are deduplicated?

This comment has been minimized.

@dks17

dks17 Aug 25, 2018

Collaborator

Do you suggest put Time.now.utc into now method and use it like this?

where locked_until_field.gt => Mongoid::Locker.now

This comment has been minimized.

@afeld

afeld Aug 25, 2018

Collaborator

No worries, I will send a separate pull request.

@dks17

This comment has been minimized.

Copy link
Collaborator

dks17 commented Aug 25, 2018

I suppose that Ruby compares time object correctly.

irb(main):001:0> time = Time.now
=> 2018-08-25 10:23:35 +0300
irb(main):002:0> t = Time.at(time)
=> 2018-08-25 10:23:35 +0300
irb(main):003:0> utc = Time.at(time).utc
=> 2018-08-25 07:23:35 UTC
irb(main):004:0> 
irb(main):005:0> t.to_f
=> 1535181815.72359
irb(main):006:0> utc.to_f
=> 1535181815.72359
irb(main):007:0> t <=> utc
=> 0
irb(main):008:0> t.eql? utc
=> true
irb(main):009:0> t.equal? utc
=> false
@dblock

This comment has been minimized.

Copy link
Collaborator

dblock commented Aug 25, 2018

While I support this change because it clarifies the intent of storing times in UTC (in seconds since epoch), it doesn't actually do anything differently in the database or the application, stores times in ... utc and compares them correctly regardless in which timezone your application is. I wasn't sure, so I experimented a bit, https://code.dblock.org/2018/08/25/storing-timestamps-in-mongodb-with-mongoid.html. AFAIK extracing time into a now method and merging this is as fine as closing the issue. Your call @dks17.

@afeld

afeld approved these changes Aug 25, 2018

@afeld afeld merged commit 673a871 into mongoid:master Aug 25, 2018

1 check passed

continuous-integration/travis-ci/pr The Travis CI build passed
Details
@afeld

This comment has been minimized.

Copy link
Collaborator

afeld commented Aug 25, 2018

Thanks for the thorough testing folks!

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