From e5b44d3967fb1708020faf496cee3530227213cd Mon Sep 17 00:00:00 2001 From: Philipp Thun Date: Wed, 8 Apr 2026 13:53:15 +0200 Subject: [PATCH] Add default ORDER BY id to Sequel model queries Adds a Sequel extension that automatically appends ORDER BY id to model queries, ensuring deterministic results for consistent API responses and reliable test behavior in parallel runs. Skips adding the default order when: - Query already has an explicit ORDER BY clause - Query has GROUP BY (aggregated results don't have individual ids) - Query is schema introspection (LIMIT 0) - Query has UNION/INTERSECT/EXCEPT (ORDER BY before UNION is a syntax error) - Query has DISTINCT ON (requires matching ORDER BY) - Query is a subquery (outer query handles ordering) - Model doesn't have id as primary key - id is not in the select list For JOIN queries (including many_to_many associations), it uses a qualified column (table.id) to avoid ambiguity. --- .../runtime/buildpack_lifecycle_data_model.rb | 3 +- .../runtime/cnb_lifecycle_data_model.rb | 3 +- lib/cloud_controller/db.rb | 2 + lib/sequel/extensions/default_order_by_id.rb | 118 ++++++++++++++++++ .../extensions/default_order_by_id_spec.rb | 76 +++++++++++ 5 files changed, 198 insertions(+), 4 deletions(-) create mode 100644 lib/sequel/extensions/default_order_by_id.rb create mode 100644 spec/unit/lib/sequel/extensions/default_order_by_id_spec.rb diff --git a/app/models/runtime/buildpack_lifecycle_data_model.rb b/app/models/runtime/buildpack_lifecycle_data_model.rb index f966c7fd262..aae0e098c9f 100644 --- a/app/models/runtime/buildpack_lifecycle_data_model.rb +++ b/app/models/runtime/buildpack_lifecycle_data_model.rb @@ -28,8 +28,7 @@ class BuildpackLifecycleDataModel < Sequel::Model(:buildpack_lifecycle_data) one_to_many :buildpack_lifecycle_buildpacks, class: '::VCAP::CloudController::BuildpackLifecycleBuildpackModel', key: :buildpack_lifecycle_data_guid, - primary_key: :guid, - order: :id + primary_key: :guid plugin :nested_attributes nested_attributes :buildpack_lifecycle_buildpacks, destroy: true add_association_dependencies buildpack_lifecycle_buildpacks: :destroy diff --git a/app/models/runtime/cnb_lifecycle_data_model.rb b/app/models/runtime/cnb_lifecycle_data_model.rb index 63bd6a32142..850fbb02ec3 100644 --- a/app/models/runtime/cnb_lifecycle_data_model.rb +++ b/app/models/runtime/cnb_lifecycle_data_model.rb @@ -27,8 +27,7 @@ class CNBLifecycleDataModel < Sequel::Model(:cnb_lifecycle_data) one_to_many :buildpack_lifecycle_buildpacks, class: '::VCAP::CloudController::BuildpackLifecycleBuildpackModel', key: :cnb_lifecycle_data_guid, - primary_key: :guid, - order: :id + primary_key: :guid plugin :nested_attributes nested_attributes :buildpack_lifecycle_buildpacks, destroy: true add_association_dependencies buildpack_lifecycle_buildpacks: :destroy diff --git a/lib/cloud_controller/db.rb b/lib/cloud_controller/db.rb index 78648843d50..32b2f47f8ba 100644 --- a/lib/cloud_controller/db.rb +++ b/lib/cloud_controller/db.rb @@ -5,6 +5,7 @@ require 'cloud_controller/execution_context' require 'sequel/extensions/query_length_logging' require 'sequel/extensions/request_query_metrics' +require 'sequel/extensions/default_order_by_id' module VCAP::CloudController class DB @@ -45,6 +46,7 @@ def self.connect(opts, logger) add_connection_expiration_extension(db, opts) add_connection_validator_extension(db, opts) db.extension(:requires_unique_column_names_in_subquery) + db.extension(:default_order_by_id) add_connection_metrics_extension(db) db end diff --git a/lib/sequel/extensions/default_order_by_id.rb b/lib/sequel/extensions/default_order_by_id.rb new file mode 100644 index 00000000000..8c6cdb746d2 --- /dev/null +++ b/lib/sequel/extensions/default_order_by_id.rb @@ -0,0 +1,118 @@ +# frozen_string_literal: true + +# This Sequel extension adds a default ORDER BY id to model queries. +# +# It skips adding the default order when: +# - Query already has an explicit ORDER BY clause +# - Query has GROUP BY (aggregated results don't have individual ids) +# - Query is schema introspection (LIMIT 0) +# - Query has UNION/INTERSECT/EXCEPT (ORDER BY before UNION is a syntax error) +# - Query has DISTINCT ON (requires matching ORDER BY) +# - Query is a subquery (outer query handles ordering) +# - Model doesn't have id as primary key +# - id is not in the select list +# +# For JOIN queries (including many_to_many associations), it uses a qualified +# column (table.id) to avoid ambiguity. +# +# This ensures deterministic query results, which is important for: +# - Consistent API responses +# - Reliable test behavior (especially in parallel test runs) +# +# Usage: +# DB.extension(:default_order_by_id) +# +module Sequel + module DefaultOrderById + module DatasetMethods + def select_sql + id_column = find_id_column + if id_column + order(id_column).select_sql + else + super + end + end + + private + + def find_id_column + return nil if should_skip_default_order? + + find_id_in_select_list + end + + def should_skip_default_order? + opts[:order] || # Already has explicit order + opts[:group] || # Aggregated results don't have individual ids + opts[:limit] == 0 || # Schema introspection, not a real query + opts[:compounds] || # ORDER BY before UNION is a syntax error + distinct_on? || # DISTINCT ON requires matching ORDER BY + subquery? || # Outer query handles ordering + !model_has_id_primary_key? # No id column to order by + end + + def distinct_on? + opts[:distinct].is_a?(Array) && opts[:distinct].any? + end + + def subquery? + opts[:from].is_a?(Array) && opts[:from].any? { |f| f.is_a?(Sequel::SQL::AliasedExpression) } + end + + def model_has_id_primary_key? + return false unless respond_to?(:model) && model + + model.primary_key == :id + rescue StandardError + false + end + + def find_id_in_select_list + select_cols = opts[:select] + + # SELECT * includes all columns including id + if select_cols.nil? || select_cols.empty? + return qualified_id_column if opts[:join] + + return :id + end + + # Find id column in select list + select_cols.each do |col| + # ColumnAll (e.g., SELECT table.*) includes all columns including id + if col.is_a?(Sequel::SQL::ColumnAll) && col.table == model.table_name + return qualified_id_column if opts[:join] + + return :id + end + + # Check for :id, :table__id, or aliased id + id_col = extract_id_column(col) + return id_col if id_col + end + + nil + end + + def qualified_id_column + Sequel.qualify(model.table_name, :id) + end + + def extract_id_column(col) + case col + when Symbol + col if col == :id || col.to_s.end_with?('__id') + when Sequel::SQL::Identifier + col if col.value == :id + when Sequel::SQL::QualifiedIdentifier + col if col.column == :id + when Sequel::SQL::AliasedExpression + col.alias if col.alias == :id # Use alias symbol (not the full expression with AS) + end + end + end + end + + Dataset.register_extension(:default_order_by_id, DefaultOrderById::DatasetMethods) +end diff --git a/spec/unit/lib/sequel/extensions/default_order_by_id_spec.rb b/spec/unit/lib/sequel/extensions/default_order_by_id_spec.rb new file mode 100644 index 00000000000..606320ccc6f --- /dev/null +++ b/spec/unit/lib/sequel/extensions/default_order_by_id_spec.rb @@ -0,0 +1,76 @@ +# frozen_string_literal: true + +require 'spec_helper' +require 'sequel/extensions/default_order_by_id' + +RSpec.describe 'Sequel::DefaultOrderById' do + # Use an existing model for testing + let(:model_class) { VCAP::CloudController::Organization } + + describe 'adding default order' do + it 'adds ORDER BY id to model queries' do + expect(model_class.dataset.sql).to match(/ORDER BY .id.$/) + end + + it 'preserves explicit order' do + expect(model_class.dataset.order(:name).sql).to match(/ORDER BY .name.$/) + end + end + + describe 'skipping default order' do + it 'skips for queries with GROUP BY' do + expect(model_class.dataset.group(:status).sql).not_to match(/ORDER BY/) + end + + it 'skips for UNION queries' do + ds1 = model_class.dataset.where(name: 'a') + ds2 = model_class.dataset.where(name: 'b') + sql = ds1.union(ds2, all: true, from_self: false).sql + # ORDER BY before UNION is a syntax error; subsequent datasets are parenthesized so it's harmless there + expect(sql).to start_with('SELECT * FROM "organizations" WHERE ("name" = \'a\') UNION') + end + + it 'skips for DISTINCT ON queries' do + sql = model_class.dataset.distinct(:guid).sql + # DISTINCT ON requires ORDER BY to match the distinct columns + expect(sql).not_to match(/ORDER BY/) + end + + it 'skips for subqueries (from_self)' do + sql = model_class.dataset.where(name: 'a').from_self.sql + # Outer query should not have ORDER BY - subquery handles it + expect(sql).to end_with('AS "t1"') + end + + it 'skips for queries where id is not in select list' do + expect(model_class.dataset.select(:guid, :name).sql).not_to match(/ORDER BY/) + end + end + + describe 'handling JOIN queries' do + it 'uses qualified column to avoid ambiguity' do + sql = model_class.dataset.join(:spaces, organization_id: :id).sql + expect(sql).to match(/ORDER BY .organizations.\..id.$/) + end + + it 'handles ColumnAll in select list (many_to_many pattern)' do + # many_to_many associations use SELECT table.* which creates a ColumnAll + sql = model_class.dataset.select(Sequel::SQL::ColumnAll.new(:organizations)).join(:spaces, organization_id: :id).sql + expect(sql).to match(/ORDER BY .organizations.\..id.$/) + end + end + + describe 'handling qualified id columns' do + it 'uses qualified column when present in select' do + sql = model_class.dataset.select(:organizations__id, :organizations__name).sql + expect(sql).to match(/ORDER BY .organizations.\..id./) + end + end + + describe 'handling aliased id columns' do + it 'orders by alias when id is aliased' do + sql = model_class.dataset.select(Sequel.as(:organizations__id, :id), :name).sql + expect(sql).to match(/ORDER BY .id.$/) + end + end +end