From c4ea2b27e04c12043bea7c7a09c5d74ce6a364a1 Mon Sep 17 00:00:00 2001 From: Philipp Thun Date: Thu, 23 Apr 2026 11:42:22 +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). Remove now-redundant explicit order: :id from lifecycle data model associations. Consolidate diego/lrp_constants require into cloud_controller/diego/constants. Introduce lightweight_db_spec_helper for Sequel extension specs that only need a bare DB connection without the full app bootstrap. Extract DbConnectionString to share connection logic between DbConfig and the new helper. Switch default_order_by_id and query_length_logging specs to use it. --- .../runtime/buildpack_lifecycle_data_model.rb | 3 +- .../runtime/cnb_lifecycle_data_model.rb | 3 +- lib/cloud_controller/db.rb | 2 + lib/cloud_controller/diego/constants.rb | 2 + .../diego/reporters/instances_reporter.rb | 2 +- lib/sequel/extensions/default_order_by_id.rb | 134 ++++++++++++++++ spec/lightweight_db_spec_helper.rb | 5 + spec/support/bootstrap/db_config.rb | 24 +-- .../support/bootstrap/db_connection_string.rb | 30 ++++ .../extensions/default_order_by_id_spec.rb | 145 ++++++++++++++++++ .../extensions/query_length_logging_spec.rb | 5 +- 11 files changed, 326 insertions(+), 29 deletions(-) create mode 100644 lib/sequel/extensions/default_order_by_id.rb create mode 100644 spec/lightweight_db_spec_helper.rb create mode 100644 spec/support/bootstrap/db_connection_string.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/cloud_controller/diego/constants.rb b/lib/cloud_controller/diego/constants.rb index 414418fd194..ff73236db23 100644 --- a/lib/cloud_controller/diego/constants.rb +++ b/lib/cloud_controller/diego/constants.rb @@ -1,3 +1,5 @@ +require 'diego/lrp_constants' + module VCAP::CloudController module Diego STAGING_DOMAIN = 'cf-app-staging'.freeze diff --git a/lib/cloud_controller/diego/reporters/instances_reporter.rb b/lib/cloud_controller/diego/reporters/instances_reporter.rb index 7874e720d1c..441e848f6c4 100644 --- a/lib/cloud_controller/diego/reporters/instances_reporter.rb +++ b/lib/cloud_controller/diego/reporters/instances_reporter.rb @@ -1,6 +1,6 @@ require 'utils/workpool' +require 'cloud_controller/diego/constants' require 'cloud_controller/diego/reporters/reporter_mixins' -require 'diego/lrp_constants' module VCAP::CloudController module Diego 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/lightweight_db_spec_helper.rb b/spec/lightweight_db_spec_helper.rb new file mode 100644 index 00000000000..a95babd668c --- /dev/null +++ b/spec/lightweight_db_spec_helper.rb @@ -0,0 +1,5 @@ +require 'lightweight_spec_helper' +require 'sequel' +require 'support/bootstrap/db_connection_string' + +DB = Sequel.connect(DbConnectionString.new.to_s) diff --git a/spec/support/bootstrap/db_config.rb b/spec/support/bootstrap/db_config.rb index 49bd018f4b4..b63465827c8 100644 --- a/spec/support/bootstrap/db_config.rb +++ b/spec/support/bootstrap/db_config.rb @@ -1,9 +1,10 @@ require 'cloud_controller/db' require 'cloud_controller/database_parts_parser' +require_relative 'db_connection_string' class DbConfig def initialize(connection_string: ENV.fetch('DB_CONNECTION_STRING', nil), db_type: ENV.fetch('DB', nil)) - @connection_string = connection_string || default_connection_string(db_type || 'postgres') + @connection_string = DbConnectionString.new(connection_string: connection_string, db_type: db_type).to_s initialize_environment_for_cc_spawning end @@ -49,25 +50,4 @@ def self.reset_environment def initialize_environment_for_cc_spawning ENV['DB_CONNECTION_STRING'] = connection_string end - - def default_connection_string(db_type) - "#{default_connection_prefix(db_type)}/#{default_name}" - end - - def default_connection_prefix(db_type) - default_connection_prefixes = { - 'mysql' => ENV['MYSQL_CONNECTION_PREFIX'] || 'mysql2://root:password@localhost:3306', - 'postgres' => ENV['POSTGRES_CONNECTION_PREFIX'] || 'postgres://postgres@localhost:5432' - } - - default_connection_prefixes[db_type] - end - - def default_name - if ENV['TEST_ENV_NUMBER'].presence - "cc_test_#{ENV.fetch('TEST_ENV_NUMBER')}" - else - 'cc_test_1' - end - end end diff --git a/spec/support/bootstrap/db_connection_string.rb b/spec/support/bootstrap/db_connection_string.rb new file mode 100644 index 00000000000..5a37b4d9d78 --- /dev/null +++ b/spec/support/bootstrap/db_connection_string.rb @@ -0,0 +1,30 @@ +class DbConnectionString + def initialize(connection_string: ENV.fetch('DB_CONNECTION_STRING', nil), db_type: ENV.fetch('DB', nil)) + @connection_string = connection_string || default_connection_string(db_type || 'postgres') + end + + def to_s + @connection_string + end + + private + + def default_connection_string(db_type) + "#{default_connection_prefix(db_type)}/#{default_name}" + end + + def default_connection_prefix(db_type) + { + 'mysql' => ENV['MYSQL_CONNECTION_PREFIX'] || 'mysql2://root:password@localhost:3306', + 'postgres' => ENV['POSTGRES_CONNECTION_PREFIX'] || 'postgres://postgres@localhost:5432' + }.fetch(db_type) + end + + def default_name + if ENV['TEST_ENV_NUMBER'].to_s.empty? + 'cc_test_1' + else + "cc_test_#{ENV.fetch('TEST_ENV_NUMBER')}" + end + end +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..827471a3646 --- /dev/null +++ b/spec/unit/lib/sequel/extensions/default_order_by_id_spec.rb @@ -0,0 +1,145 @@ +# frozen_string_literal: true + +require 'lightweight_db_spec_helper' +require 'sequel/extensions/default_order_by_id' + +DB.extension(:default_order_by_id) + +RSpec.describe 'Sequel::DefaultOrderById' do + let(:db) { DB } + + let(:model_class) { Class.new(Sequel::Model(db[:test_default_order_main])) } + + before(:all) do + DB.create_table?(:test_default_order_main) do + primary_key :id + String :name + String :guid + String :status + end + DB.create_table?(:test_default_order_join) do + primary_key :id + foreign_key :main_id, :test_default_order_main + end + end + + after(:all) do + DB.drop_table?(:test_default_order_join) + DB.drop_table?(:test_default_order_main) + end + + 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 + sqls.last + ensure + db.loggers.delete(logger) + 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 + skip if db.database_type != :postgres + + 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[:test_default_order_main])) 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(:test_default_order_join, main_id: :id).all } + expect(sql).to match(/ORDER BY .test_default_order_main.\..id./) + end + end + + context 'with SELECT table.*' do + it 'uses unqualified :id' do + sql = capture_sql { model_class.dataset.select(Sequel::SQL::ColumnAll.new(:test_default_order_main)).join(:test_default_order_join, main_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 + qualified_id = Sequel.qualify(:test_default_order_main, :id) + qualified_name = Sequel.qualify(:test_default_order_main, :name) + sql = capture_sql { model_class.dataset.select(qualified_id, qualified_name).all } + expect(sql).to match(/ORDER BY .test_default_order_main.\..id./) + end + end + + context 'with aliased id in select list' do + it 'uses the alias' do + qualified_id = Sequel.qualify(:test_default_order_main, :id) + sql = capture_sql { model_class.dataset.select(Sequel.as(qualified_id, :order_id), :name).all } + expect(sql).to match(/ORDER BY .order_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 diff --git a/spec/unit/lib/sequel/extensions/query_length_logging_spec.rb b/spec/unit/lib/sequel/extensions/query_length_logging_spec.rb index 584218e0d56..393e617c152 100644 --- a/spec/unit/lib/sequel/extensions/query_length_logging_spec.rb +++ b/spec/unit/lib/sequel/extensions/query_length_logging_spec.rb @@ -1,8 +1,9 @@ -require 'spec_helper' +require 'lightweight_db_spec_helper' +require 'sequel/extensions/query_length_logging' RSpec.describe Sequel::QueryLengthLogging do describe 'log_connection_yield' do - let(:db) { DbConfig.new.connection } + let(:db) { DB } let(:logs) { StringIO.new } let(:logger) { Logger.new(logs) }