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

Use UTC when encoding datetime values into cursor. #92

Closed
Closed
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
7 changes: 5 additions & 2 deletions CHANGELOG.md
Original file line number Diff line number Diff line change
Expand Up @@ -8,6 +8,9 @@ The format is based on [Keep a Changelog](https://keepachangelog.com/en/1.0.0/),

These are the latest changes on the project's `master` branch that have not yet been released.

### Fixed
- Use UTC when encoding datetime values into cursor, therefore playing nicely with rails' default behaviour of storing UTC in the database.

<!---
If you submit a pull request for this gem, please add a summary of your changes here.
This will ensure that they're also mentioned in the next release description.
Expand All @@ -34,7 +37,7 @@ These are the latest changes on the project's `master` branch that have not yet
### Changed
- **Breaking change:** The way records are retrieved from a given cursor has been changed to no longer use `CONCAT` but instead simply use a compound `WHERE` clause in case of a custom order and having both the custom field as well as the `id` field in the `ORDER BY` query. This is a breaking change since it now changes the internal order of how records with the same value of the `order_by` field are returned.
- Remove `ORDER BY` clause from `COUNT` queries

### Fixed
- Only trigger one SQL query to load the records from the database and use it to determine if there was a previous / is a next page
- Memoize the `Paginator#page` method which is invoked multiple times to prevent it from mapping over the `records` again and again and rebuilding all cursors
Expand All @@ -56,7 +59,7 @@ These are the latest changes on the project's `master` branch that have not yet
### Fixed
- Pagination for relations in which a custom `SELECT` does not contain cursor-relevant fields like `:id` or the field specified via `order_by`

## [0.1.1] - 2021-01-21
## [0.1.1] - 2021-01-21

### Added
- Add support for handling `nil` for `order` and `order_by` values as if they were not passed
Expand Down
13 changes: 12 additions & 1 deletion lib/rails_cursor_pagination/cursor.rb
Original file line number Diff line number Diff line change
Expand Up @@ -68,7 +68,7 @@ def decode(encoded_string:, order_field: :id)
def initialize(id:, order_field: :id, order_field_value: nil)
@id = id
@order_field = order_field
@order_field_value = order_field_value
@order_field_value = normalized_order_field_value(order_field_value)

return if !custom_order_field? || !order_field_value.nil?

Expand Down Expand Up @@ -104,5 +104,16 @@ def encode
def custom_order_field?
@order_field != :id
end

# Returns the value given, but transforms datetime values to UTC if
# applicable
#
# @return [Object]
def normalized_order_field_value(value)
return value unless ActiveRecord::Base.time_zone_aware_attributes
return value unless ActiveRecord::Base.default_timezone == :utc

value.respond_to?(:utc) ? value.utc : value
end
end
end
39 changes: 39 additions & 0 deletions spec/rails_cursor_pagination/cursor_spec.rb
Original file line number Diff line number Diff line change
Expand Up @@ -51,6 +51,45 @@
expect(decoded.order_field_value).to eq record.author
end
end

context 'when ordering by created_at' do
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

before { record.update!(created_at: '2022-10-04 16:58:43 +0200') }

after do
ActiveRecord::Base.time_zone_aware_attributes = true
ActiveRecord::Base.default_timezone == :utc
end

subject(:encoded) do
described_class.from_record(record: record,
order_field: :created_at).encode
end

let(:decoded_time) do
decoded = described_class.decode(encoded_string: encoded,
order_field: :created_at)
Time.parse(decoded.order_field_value)
end

it 'can be decoded back to the originally encoded value' do
expect(decoded_time).to eq record.created_at
expect(decoded_time).to be_utc
end

it 'respects the time_zone_aware_attributes setting' do
ActiveRecord::Base.time_zone_aware_attributes = false

expect(decoded_time).to eq record.created_at
expect(decoded_time).not_to be_utc
end

it 'respects the default_timezone setting' do
ActiveRecord::Base.default_timezone = :local

expect(decoded_time).to eq record.created_at
expect(decoded_time).not_to be_utc
end
Comment on lines +79 to +91
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

end
end

describe '.from_record' do
Expand Down
2 changes: 1 addition & 1 deletion spec/rails_cursor_pagination/paginator_spec.rb
Original file line number Diff line number Diff line change
Expand Up @@ -229,7 +229,7 @@
)
end
end
let(:expected_attributes) { %i[id author content] }
let(:expected_attributes) { %i[id author content created_at] }

it 'has the correct format' do
is_expected.to be_a Hash
Expand Down
5 changes: 5 additions & 0 deletions spec/spec_helper.rb
Original file line number Diff line number Diff line change
Expand Up @@ -22,6 +22,10 @@ class Post < ActiveRecord::Base; end
ActiveRecord::Base.logger = Logger.new(ENV['VERBOSE'] ? $stdout : nil)
ActiveRecord::Migration.verbose = ENV.fetch('VERBOSE', nil)

# Configure time_zone like it would be in rails applications
ActiveRecord::Base.time_zone_aware_attributes = true
Time.zone = 'Berlin'
Comment on lines +25 to +27
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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


ActiveRecord::Base.establish_connection(
adapter: ENV.fetch('DB_ADAPTER', 'mysql2'),
database: 'rails_cursor_pagination_testing',
Expand All @@ -35,6 +39,7 @@ class Post < ActiveRecord::Base; end
ActiveRecord::Migration.create_table :posts do |t|
t.string :author
t.string :content
t.datetime :created_at
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

end

config.before(:each) { Post.delete_all }
Expand Down