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