When ordering by 'created_at', add another order by 'id'

This ensures a chronological order of records with the same 'created_at'
timestamp.

Author: philippthun

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]
 

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

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
 

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