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