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) }