Use UTC when encoding datetime values into cursor.

[xijo]

Hello,

Rails stores datetime values in the DB as UTC as a default. When using a custom order_by on a datetime field, e.g. updated_at however, the order_field_value is encoded in the local time zone, which leads to faulty pagination.

This PR fixes this issue by always treating the value as UTC if it responds to utc. To provide a meaningful test case I've change the spec setup slightly, so that it operates in a different time zone, but still treats datetimes correctly.

To some users, if they were deviating from rails defaults and stored datetimes in local time, this might be a breaking change. However, I'd assume that majority of rails applications adheres to the default.

[aaronsama]

Hello @xijo and thanks for opening a PR. I did a bit of research and it looks like only attributes of type TIMESTAMP are stored using UTC in MySQL > 5 (reference: https://stackoverflow.com/a/602038). That would be the case of a created_at column for example, which is usually added as follows in a migration (reference: https://edgeguides.rubyonrails.org/active_record_migrations.html)

Other types such as datetime and time are "timezone aware" and are converted to the local timezone when pulled from the DB (reference: https://api.rubyonrails.org/classes/ActiveRecord/Timestamp.html)

It looks like this can be customized according to the docs I linked above, possibly making the conversion to UTC problematic. I guess this is what you mean when you say it would be a "breaking change" for some users. What about skipping the conversion to UTC if we detect that the application is set to config.active_record.time_zone_aware_attributes = false or config.active_record.default_timezone = :local? I guess our main use case for this is when a timestamp is used to generate the cursor (btw, there's no guarantee that the timestamp would be unique)

[xijo]

Hi @aaronsama,

according to the docs that you've linked:

By default, these values are stored in the database as UTC and converted back to the current Time.zone when pulled from the database.

Going with the principle of least surprise, having the gem work out of the box for the current rails default seems like the right thing to do, right?

As for respecting the configuration, that is a solid plan, I'll add that to the PR. 👍

Just to clarify my current use case a bit. We're storing a field content_updated_at with nanosecond precision in order to bubble up new/updated content in our feed. With that in mind, the cursor for our use case could look very differently (it would not need to include the id at all).

[xijo]

@aaronsama PR is updated accordingly, added a couple more test cases as well.

[aaronsama]

Thanks again for your PR. I added some comments to improve the tests a bit. In general I would try to avoid having the timezone related configuration modified for the whole suite and move it to the tests where it's needed. Please have a look if it makes sense to you.

[aaronsama]

What about adding two test cases, one for timestamps and one for a datetime attribute? They are slightly different and would better reflect a real world scenario

Suggested change

	t.datetime :created_at
	t.timestamps
	t.datetime :some_other_datetime_field

[aaronsama]

I would rather mock the response values or change the setup only in the tests where we need it rather than changing the configuration for the whole test suite. Something like the code below would be enough probably.

  # This can be different for different contexts
  before do
    ActiveRecord::Base.time_zone_aware_attributes = true
    Time.zone = 'Berlin'
  end

  after do
    # what you did in the file above
  end

It looks like stubbing Time.zone can be problematic but I found this: https://stackoverflow.com/a/26741932

[aaronsama]

Since you are altering the context in which these examples are run I propose moving them to their own context. See the comment below.

Something like

context 'when timezone aware attributes are disabled' do
  before do
    ActiveRecord::Base.time_zone_aware_attributes = false
  end

  # your examples here
end

context 'when using :local as default timezone' do
  before do
    ActiveRecord::Base.default_timezone = :local
  end

  # your examples here
end

[aaronsama]

I would try to make this more generig

Suggested change

	context 'when ordering by created_at' do
	context 'when ordering by a timestamp attribute' do

[aaronsama]

This is addressed in #142 now. Closing this PR.