From a55e0910d93a8e14e556211f40e6f13ff93530da Mon Sep 17 00:00:00 2001 From: Philipp Thun Date: Tue, 10 Feb 2026 18:08:45 +0100 Subject: [PATCH] When ordering by 'created_at', add another order by 'id' This ensures a chronological order of records with the same 'created_at' timestamp. --- .../paging/sequel_paginator.rb | 27 ++++++-- spec/request_spec_shared_examples.rb | 4 +- spec/support/bootstrap/fake_model_tables.rb | 2 +- .../paging/sequel_paginator_spec.rb | 64 +++++++++++++------ 4 files changed, 67 insertions(+), 30 deletions(-) diff --git a/lib/cloud_controller/paging/sequel_paginator.rb b/lib/cloud_controller/paging/sequel_paginator.rb index c104b0de08b..7cfc2797fcd 100644 --- a/lib/cloud_controller/paging/sequel_paginator.rb +++ b/lib/cloud_controller/paging/sequel_paginator.rb @@ -6,21 +6,28 @@ def get_page(dataset, pagination_options) page = pagination_options.page per_page = pagination_options.per_page order_direction = pagination_options.order_direction - order_by = pagination_options.order_by + order_by = pagination_options.order_by.to_sym table_name = dataset.model.table_name - has_guid_column = dataset.model.columns.include?(:guid) - order_type = Sequel.send(order_direction, Sequel.qualify(table_name, order_by)) - dataset = dataset.order(order_type) + # 1. Order the dataset by the specified column and direction. + dataset = dataset.order(Sequel.send(order_direction, Sequel.qualify(table_name, order_by))) + # 2. When ordering by 'created_at', add another order by 'id' to ensure a chronological order of records with the same 'created_at' timestamp. + dataset = dataset.order_append(Sequel.send(order_direction, Sequel.qualify(table_name, :id))) if order_by == :created_at && has_column?(dataset, :id) + + # 3. Add a secondary order in case 'secondary_default_order_by' is specified and no custom order has been provided. secondary_order_by = pagination_options.secondary_order_by dataset = dataset.order_append(Sequel.send(order_direction, Sequel.qualify(table_name, secondary_order_by))) if secondary_order_by - dataset = dataset.order_append(Sequel.send(order_direction, Sequel.qualify(table_name, :guid))) if order_by != 'id' && has_guid_column + + # 4. If the dataset is not already ordered by a unique column, add another order by 'guid' to ensure a deterministic ordering of records. + if !order_includes_unique_column?(dataset.opts[:order]) && has_column?(dataset, :guid) + dataset = dataset.order_append(Sequel.send(order_direction, Sequel.qualify(table_name, :guid))) + end distinct_opt = dataset.opts[:distinct] if !distinct_opt.nil? && !distinct_opt.empty? # DISTINCT ON order_opt = dataset.opts[:order] - dataset = if order_opt.any? { |o| %i[id guid].include?(o.expression.column.to_sym) } + dataset = if order_includes_unique_column?(order_opt) # If ORDER BY columns are unique, use them for the DISTINCT ON clause. dataset.distinct(*order_opt.map(&:expression)) else @@ -46,6 +53,14 @@ def can_paginate_with_window_function?(dataset) private + def has_column?(dataset, column_name) + dataset.model.columns.include?(column_name) + end + + def order_includes_unique_column?(order_opt) + order_opt.any? { |o| %i[id guid].include?(o.expression.column.to_sym) } + end + def paginate_with_window_function(dataset, per_page, page, table_name) dataset = dataset.from_self if dataset.opts[:distinct] diff --git a/spec/request_spec_shared_examples.rb b/spec/request_spec_shared_examples.rb index 95f817491d5..77e7682bdbb 100644 --- a/spec/request_spec_shared_examples.rb +++ b/spec/request_spec_shared_examples.rb @@ -363,8 +363,8 @@ def errors_without_test_mode_info(parsed_response) context 'order_by created_at' do let!(:resource_1) { resource_klass.make(guid: '1', created_at: '2020-05-26T18:47:03Z', **additional_resource_params) } - let!(:resource_2) { resource_klass.make(guid: '2', created_at: '2020-05-26T18:47:02Z', **additional_resource_params) } - let!(:resource_3) { resource_klass.make(guid: '3', created_at: '2020-05-26T18:47:01Z', **additional_resource_params) } + let!(:resource_2) { resource_klass.make(guid: '3', created_at: '2020-05-26T18:47:02Z', **additional_resource_params) } + let!(:resource_3) { resource_klass.make(guid: '2', created_at: '2020-05-26T18:47:02Z', **additional_resource_params) } let!(:resource_4) { resource_klass.make(guid: '4', created_at: '2020-05-26T18:47:04Z', **additional_resource_params) } it 'sorts ascending' do diff --git a/spec/support/bootstrap/fake_model_tables.rb b/spec/support/bootstrap/fake_model_tables.rb index e51c25d5845..900a583a5ab 100644 --- a/spec/support/bootstrap/fake_model_tables.rb +++ b/spec/support/bootstrap/fake_model_tables.rb @@ -278,7 +278,7 @@ def drop_tables_for_query_spec def tables_for_sequel_paginator_spec db.create_table :table_without_guid do primary_key :id - DateTime :created_at + String :name end end diff --git a/spec/unit/lib/cloud_controller/paging/sequel_paginator_spec.rb b/spec/unit/lib/cloud_controller/paging/sequel_paginator_spec.rb index c1ef2d196cd..af8980b9be4 100644 --- a/spec/unit/lib/cloud_controller/paging/sequel_paginator_spec.rb +++ b/spec/unit/lib/cloud_controller/paging/sequel_paginator_spec.rb @@ -187,28 +187,37 @@ class TableWithoutGuid < Sequel::Model(:table_without_guid); end expect(paginated_result.records[3].guid).to eq(app_model2.guid) end - it 'orders by GUID as a secondary field when available' do - options = { page: page, per_page: 2, order_by: 'created_at', order_direction: 'asc' } - app_model1.update(guid: '1', created_at: '2019-12-25T13:00:00Z') - app_model2.update(guid: '2', created_at: '2019-12-25T13:00:00Z') + context 'deterministic ordering' do + before do + app_model1.update(guid: '2', created_at: '2019-12-25T13:00:00Z', updated_at: '2019-12-25T13:00:00Z') + app_model2.update(guid: '1', created_at: '2019-12-25T13:00:00Z', updated_at: '2019-12-25T13:00:00Z') + end - pagination_options = PaginationOptions.new(options) - paginated_result = paginator.get_page(dataset, pagination_options) - expect(paginated_result.records.first.guid).to eq(app_model1.guid) - expect(paginated_result.records.second.guid).to eq(app_model2.guid) - end + context "when ordered by 'created_at'" do + it "orders by 'id' as a secondary field" do + options = { page: page, per_page: 2, order_by: 'created_at', order_direction: 'asc' } - it 'does not order by GUID when the table has no GUID' do - options = { page: page, per_page: 2, order_by: 'created_at', order_direction: 'asc' } + pagination_options = PaginationOptions.new(options) + expect do + paginated_result = paginator.get_page(dataset, pagination_options) + expect(paginated_result.records.first.guid).to eq(app_model1.guid) + expect(paginated_result.records.second.guid).to eq(app_model2.guid) + end.to have_queried_db_times(/ORDER BY .\w*.\..created_at. ASC, .\w*.\..id. ASC LIMIT/i, 1) + end + end - pagination_options = PaginationOptions.new(options) - ds = TableWithoutGuid.dataset - expect do - paginator.get_page(ds, pagination_options) - end.to have_queried_db_times(/ORDER BY .\w*.\..created_at. ASC LIMIT/i, 1) - expect do - paginator.get_page(ds, pagination_options) - end.to have_queried_db_times(/ORDER BY .\w*.\..created_at. ASC, .\w*.\..guid. ASC LIMIT/i, 0) + context "when ordered by a non-unique column other than 'created_at'" do + it "orders by 'guid' as a secondary field" do + options = { page: page, per_page: 2, order_by: 'updated_at', order_direction: 'asc' } + + pagination_options = PaginationOptions.new(options) + expect do + paginated_result = paginator.get_page(dataset, pagination_options) + expect(paginated_result.records.first.guid).to eq(app_model2.guid) + expect(paginated_result.records.second.guid).to eq(app_model1.guid) + end.to have_queried_db_times(/ORDER BY .\w*.\..updated_at. ASC, .\w*.\..guid. ASC LIMIT/i, 1) + end + end end it 'does not order by GUID when the primary order is by ID' do @@ -223,6 +232,19 @@ class TableWithoutGuid < Sequel::Model(:table_without_guid); end end.to have_queried_db_times(/ORDER BY .\w*.\..id. ASC, .\w*.\..guid. ASC LIMIT/i, 0) end + it 'does not order by GUID when the table has no GUID' do + options = { page: page, per_page: 2, order_by: 'name', order_direction: 'asc' } + + pagination_options = PaginationOptions.new(options) + ds = TableWithoutGuid.dataset + expect do + paginator.get_page(ds, pagination_options) + end.to have_queried_db_times(/ORDER BY .\w*.\..name. ASC LIMIT/i, 1) + expect do + paginator.get_page(ds, pagination_options) + end.to have_queried_db_times(/ORDER BY .\w*.\..name. ASC, .\w*.\..guid. ASC LIMIT/i, 0) + end + context 'when a DISTINCT ON clause is used' do # MySQL uses GROUP BY instead let(:distinct_dataset) { dataset.distinct(:id) } @@ -237,12 +259,12 @@ class TableWithoutGuid < Sequel::Model(:table_without_guid); end end context 'when ordered by other column' do - let(:pagination_options) { PaginationOptions.new({ order_by: 'created_at' }) } + let(:pagination_options) { PaginationOptions.new({ order_by: 'name' }) } it 'uses other column and GUID for DISTINCT ON clause' do expect do paginator.get_page(distinct_dataset, pagination_options) - end.to have_queried_db_times(/(select distinct on \(.*created_at.*,.*guid.*\) .* from)|(group by)/i, paginator.can_paginate_with_window_function?(dataset) ? 1 : 2) + end.to have_queried_db_times(/(select distinct on \(.*name.*,.*guid.*\) .* from)|(group by)/i, paginator.can_paginate_with_window_function?(dataset) ? 1 : 2) end context 'when table has no GUID column' do