Recover precision when serializing Time, TimeWithZone and DateTime.

[tgxworld]

Resolves #39648
Related #35713

[tgxworld]

@pixeltrix I saw that you commented about this in #39648 (comment). Would you mind reviewing this PR? Thank you!

[pixeltrix]

So this could be replace with the following:

argument.change(nsec: argument.sec_fraction.floor(ActiveJob::Serializers.time_precision) * 1000000000)

Which got me thinking that may be we should add floor method to Time 🤔

class Time
  def floor(precision = 0)
    change(nsec: sec_fraction.floor(precision) * 1000000000)
  end
end

In which case that code would now be

argument.floor(ActiveJob::Serializers.time_precision)

Another alternative would be to add :sec_fraction to the list of keys to change, e.g:

argument.change(sec_fraction: argument.sec_fraction.floor(ActiveJob::Serializers.time_precision))

this would be fairly easy since we calculate the second fraction internal from :nsec or :usec. In fact we could make both changes to simplify the floor method to just this:

class Time
  def floor(precision = 0)
    change(sec_fraction: sec_fraction.floor(precision))
  end
end

Can you do a separate PR to add these and we can have a discussion about them - seems like they'd be useful additions. If they're added then we can update this PR to use them.

[rafaelfranca]

I don't think we should add an option for this. We should just always serialize with the max precision that makes sense.

[pixeltrix]

I don't think we should add an option for this. We should just always serialize with the max precision that makes sense.

That would be a breaking change though, wouldn't it?

[tgxworld]

@rafaelfranca Dropping the precision seems to be an intentional change from #35713. The concern was specifically around the size of the job payloads.

We can fix this by adding precision (now = Time.now; Time.iso8601(now.iso8601(20)) == now), but that adds unnecessary size to job payloads.

[rafaelfranca]

I'm not sure if I consider this a breaking change given we are just adding information, not removing it. Yes, if you were doing something with the time objects and you expected it to have lost precision, that will break you app, but this is the same as any bug fix change we do. If you expect framework to do X, but X is a bug and we fix, your code will break.

About the option, do you think any user would chose to lose precision when serializing an object if given them the option? If they wouldn't chose that why are we allowing them the chose?

We dropped precision in the assertion, and that is an ok thing to do, if we don't care about the precision in the assertion. But if the lack of precision is changing the behavior in production code, so it is not ok to drop the precision. Adding 10 characters more to in the payload to keep precision seems to be a nice trade-off.

[rafaelfranca]

If we are still concerned about backward compatibility I'm not opposed to add an option like keep_precision_on_time = true (pending better name) as a new framework default that will be removed in 6.2. I only think giving control over the precision of the time objects on this doesn't add much other than just to avoid the possible breaking change.

[pixeltrix]

If we are still concerned about backward compatibility I'm not opposed to add an option like keep_precision_on_time = true (pending better name) as a new framework default that will be removed in 6.2. I only think giving control over the precision of the time objects on this doesn't add much other than just to avoid the possible breaking change.

What if we forget about the option, store the full precision and just add patches to Active Support to ensure that round, floor and ceil exist for both Time and DateTime in the Ruby versions we support? That way it's up to the developer when and how to lose the precision, e.g:

MyJob.perform_later(completed_at.floor)

As you say it's unlikely that most apps are expecting to be losing precision so won't notice the difference.

[tgxworld]

Adding 10 characters more to in the payload to keep precision seems to be a nice trade-off.

Actually if I think about it more now, I think it shouldn't be a default precision of 10. Instead, we should maintain the precision of the Time or DateTime object that has been passed as arguments to the job. For example,

[70] pry(main)> now = Time.now
=> 2020-07-11 21:58:24.384288808 +0800
[71] pry(main)> now.floor(5)
=> 2020-07-11 21:58:24.38428 +0800
[72] pry(main)> now.floor(5).iso8601(5)
=> "2020-07-11T21:58:24.38428+08:00"

If we get an object with a precision of 5, we should serialized it with a precision of 5.

@pixeltrix I guess we're adding the methods for Ruby versions smaller than 2.7? In Ruby 2.7, Time#floor, Time#ceil and Time#round has been added to Time. https://bugs.ruby-lang.org/issues/15653 and https://bugs.ruby-lang.org/issues/15772

[pixeltrix]

If we get an object with a precision of 5, we should serialized it with a precision of 5.

How would you know an object has a precision of 5? You'd have to do something like this:

>> Time.now.getutc.iso8601(9).gsub(/0*Z\z/, 'Z')
=> "2020-07-11T15:11:45.433135Z"

I guess we're adding the methods for Ruby versions smaller than 2.7? In Ruby 2.7, Time#floor, Time#ceil and Time#round has been added to Time.

Yes, it's something we've done before for other methods.

[tgxworld]

How would you know an object has a precision of 5?

We can count the number of decimal places of the float obtained by converting Time#subsec to float. There are some edge cases we need to consider but it does seem possible.

[165] pry(main)> Time.now.floor(6).subsec.to_f
=> 0.339143

[tgxworld]

How would you know an object has a precision of 5?

@pixeltrix @rafaelfranca So I couldn't find a decent way to determine the precision of a Time object so I'm dropping the idea above for now. Instead I've updated the PR to always serialize up to the nanoseconds.

What if we forget about the option, store the full precision and just add patches to Active Support to ensure that round, floor and ceil exist for both Time and DateTime in the Ruby versions we support?

@pixeltrix I'm having trouble with implementing Time#ceil in Ruby because of the behavior described in https://bugs.ruby-lang.org/issues/16470#note-17.

Time.utc(2007, 11, 1, 15, 25, 0, 123456.789) creates a Time object which has
0.123456789000000004307366907596588134765625 as a fractional second
because the exact value of float 123456.789 is
123456.789000000004307366907596588134765625.

If our only concern here is for users to be able to lose precision, I guess Time#floor and Time#round will be sufficient?

[pixeltrix]

@pixeltrix I'm having trouble with implementing Time#ceil in Ruby because of the behavior described in https://bugs.ruby-lang.org/issues/16470#note-17.

Can you be a bit more specific about the problems you're facing? If you specify the second fraction as a rational and not a float don't the issues with representation go away?

[tgxworld]

Can you be a bit more specific about the problems you're facing?

I'm stuck on figuring a way in replicating the following Time#ceil behavior in Ruby land.

irb(main):003:0> time = Time.utc(2016, 4, 30, 23, 59, "59.123456789".to_r)
irb(main):004:0> time
=> 2016-04-30 23:59:59.123456789 UTC
irb(main):005:0> time.ceil(9).subsec
=> (12345679/100000000)
irb(main):006:0> time.subsec.ceil(9)
=> (123456789/1000000000)

Note how the fractional second is different. I think this is due to the behavior described in the Ruby bug report where

Time.utc(2007, 11, 1, 15, 25, 0, 123456.789) creates a Time object which has
0.123456789000000004307366907596588134765625 as a fractional second
because the exact value of float 123456.789 is
123456.789000000004307366907596588134765625.

If you specify the second fraction as a rational and not a float don't the issues with representation go away?

We don't have control over what kind of Time object is going to call ceil though so the user might have constructed a Time object by passing the seconds argument as a Float.

[rails-bot]

This pull request has been automatically marked as stale because it has not had recent activity. It will be closed if no further activity occurs.
Thank you for your contributions.

[tgxworld]

This is still relevant.

[tgxworld]

Thank you for reviewing @rafaelfranca @pixeltrix