From 75b719880ee415607d1277609c00885f1cdb2944 Mon Sep 17 00:00:00 2001 From: Philipp Thun Date: Thu, 16 Apr 2026 14:44:24 +0200 Subject: [PATCH] Add default ORDER BY id to Sequel model queries Adds a Sequel extension that hooks into fetch methods (all, each, first) to add ORDER BY id just before execution, ensuring deterministic query results for consistent API responses and reliable test behavior. Skips ordering when incompatible (GROUP BY, compounds, DISTINCT ON, from_self) or unnecessary (explicit ORDER BY, no id column). --- .../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 | 134 ++++++++++++++++++ .../extensions/default_order_by_id_spec.rb | 118 +++++++++++++++ 5 files changed, 256 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..e5ad2566273 --- /dev/null +++ b/lib/sequel/extensions/default_order_by_id.rb @@ -0,0 +1,134 @@ +# frozen_string_literal: true + +# Sequel extension that adds default ORDER BY id to model queries. +# +# Hooks into fetch methods (all, each, first) to add ORDER BY id just before +# execution. This ensures ordering is only added to the final query, not to +# subqueries or compound query parts. +# +# Skips default ordering when: +# - Query already has explicit ORDER BY +# - Query is incompatible (GROUP BY, compounds, DISTINCT ON, from_self) +# - Query is schema introspection (LIMIT 0) +# - Model doesn't have id as primary key +# - id is not in the select list +# +# For JOIN queries with SELECT *, uses qualified column (table.id) to avoid +# ambiguity. +# +# Ensures deterministic query results for consistent API responses and +# reliable test behavior. +# +# Usage: +# DB.extension(:default_order_by_id) +# +module Sequel + module DefaultOrderById + module DatasetMethods + def all(*, &) + id_col = id_column_for_order + return super unless id_col + + order(id_col).all(*, &) + end + + def each(*, &) + id_col = id_column_for_order + return super unless id_col + + order(id_col).each(*, &) + end + + def first(*, &) + id_col = id_column_for_order + return super unless id_col + + order(id_col).first(*, &) + end + + private + + def id_column_for_order + return if already_ordered? || incompatible_with_order? || not_a_data_query? || !model_has_id_primary_key? + + find_id_column + end + + def already_ordered? + opts[:order] + end + + def incompatible_with_order? + opts[:group] || # Aggregated results don't have individual ids + opts[:compounds] || # Compound queries (e.g. UNION) have own ordering + distinct_on? || # DISTINCT ON requires matching ORDER BY + from_self? # Outer query handles ordering + end + + def distinct_on? + opts[:distinct].is_a?(Array) && opts[:distinct].any? + end + + def from_self? + opts[:from].is_a?(Array) && opts[:from].any? { |f| f.is_a?(Sequel::SQL::AliasedExpression) } + end + + def not_a_data_query? + opts[:limit] == 0 # Schema introspection query + end + + def model_has_id_primary_key? + return false unless respond_to?(:model) && model + + model.primary_key == :id + end + + def find_id_column + select_cols = opts[:select] + + if select_cols.nil? || select_cols.empty? + # SELECT * includes id + if opts[:join] + # Qualify to avoid ambiguity with joined tables + return Sequel.qualify(model.table_name, :id) + end + + return :id + end + + select_cols.each do |col| + # SELECT table.* includes id + return :id if col.is_a?(Sequel::SQL::ColumnAll) && col.table == model.table_name + + id_col = extract_id_column(col) + return id_col if id_col + end + + nil + end + + def extract_id_column(col) + return col if id_expression?(col) + + return col.alias if col.is_a?(Sequel::SQL::AliasedExpression) && id_expression?(col.expression) + + nil + end + + def id_expression?(expr) + case expr + when Symbol + expr == :id || expr.to_s.end_with?('__id') + when Sequel::SQL::Identifier + expr.value == :id + when Sequel::SQL::QualifiedIdentifier + expr.column == :id + else + false + 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..b216740a80d --- /dev/null +++ b/spec/unit/lib/sequel/extensions/default_order_by_id_spec.rb @@ -0,0 +1,118 @@ +# frozen_string_literal: true + +require 'spec_helper' +require 'sequel/extensions/default_order_by_id' + +RSpec.describe 'Sequel::DefaultOrderById' do + let(:model_class) { VCAP::CloudController::Organization } + let(:db) { model_class.db } + + def capture_sql(&) + sqls = [] + db.loggers << (logger = Class.new do + define_method(:info) { |msg| sqls << msg if msg.include?('SELECT') } + define_method(:debug) { |_| } + define_method(:error) { |_| } + end.new) + yield + db.loggers.delete(logger) + sqls.last + end + + describe 'default ordering' do + it 'adds ORDER BY id to model queries' do + sql = capture_sql { model_class.dataset.all } + expect(sql).to match(/ORDER BY .id./) + end + end + + describe 'already_ordered?' do + it 'preserves explicit ORDER BY' do + sql = capture_sql { model_class.dataset.order(:name).all } + expect(sql).to match(/ORDER BY .name./) + end + end + + describe 'incompatible_with_order?' do + it 'skips for GROUP BY' do + sql = capture_sql { model_class.dataset.select_group(:status).select_append(Sequel.function(:max, :id).as(:id)).all } + expect(sql).not_to match(/ORDER BY/) + end + + it 'skips for compound queries (UNION)' do + ds1 = model_class.dataset.where(name: 'a') + ds2 = model_class.dataset.where(name: 'b') + sql = capture_sql { ds1.union(ds2, all: true, from_self: false).all } + expect(sql).not_to match(/ORDER BY/) + end + + it 'skips for DISTINCT ON' do + sql = capture_sql { model_class.dataset.distinct(:guid).all } + expect(sql).not_to match(/ORDER BY/) + end + + it 'skips for from_self (subquery)' do + sql = capture_sql { model_class.dataset.where(name: 'a').from_self.all } + expect(sql).not_to match(/ORDER BY/) + end + end + + describe 'not_a_data_query?' do + it 'skips for schema introspection (columns!)' do + sql = capture_sql { model_class.dataset.columns! } + expect(sql).not_to match(/ORDER BY/) + end + end + + describe 'model_has_id_primary_key?' do + it 'skips for models with non-id primary key' do + guid_pk_model = Class.new(Sequel::Model(db[:organizations])) do + set_primary_key :guid + end + sql = capture_sql { guid_pk_model.dataset.all } + expect(sql).not_to match(/ORDER BY/) + end + end + + describe 'find_id_column' do + context 'with SELECT *' do + it 'uses unqualified :id' do + sql = capture_sql { model_class.dataset.all } + expect(sql).to match(/ORDER BY .id./) + end + + it 'uses qualified column for JOIN to avoid ambiguity' do + sql = capture_sql { model_class.dataset.join(:spaces, organization_id: :id).all } + expect(sql).to match(/ORDER BY .organizations.\..id./) + end + end + + context 'with SELECT table.*' do + it 'uses unqualified :id' do + sql = capture_sql { model_class.dataset.select(Sequel::SQL::ColumnAll.new(:organizations)).join(:spaces, organization_id: :id).all } + expect(sql).to match(/ORDER BY .id./) + end + end + + context 'with qualified id in select list' do + it 'uses the qualified column' do + sql = capture_sql { model_class.dataset.select(:organizations__id, :organizations__name).all } + expect(sql).to match(/ORDER BY .organizations.\..id./) + end + end + + context 'with aliased id in select list' do + it 'uses the alias' do + sql = capture_sql { model_class.dataset.select(Sequel.as(:organizations__id, :org_id), :name).all } + expect(sql).to match(/ORDER BY .org_id./) + end + end + + context 'without id in select list' do + it 'skips ordering' do + sql = capture_sql { model_class.dataset.select(:guid, :name).all } + expect(sql).not_to match(/ORDER BY/) + end + end + end +end