From 62aec1227b7ab547a68a0717c7430e4da4519a36 Mon Sep 17 00:00:00 2001 From: Philipp Thun Date: Tue, 7 Apr 2026 16:37:24 +0200 Subject: [PATCH] Add default order by id to one_to_many and many_to_many associations Ensures deterministic ordering for all association queries by adding a default `order: :id` to the vcap_relations plugin. This fixes sporadic test failures in parallel test runs where database query order was non-deterministic. Explicit order options still override this default, and callers can use `.order(:field)` on the dataset to override as needed. Associations with a block (custom dataset transformation) do not get the default order, as the transformation may not be compatible with ordering by id. Also removes now-redundant explicit `order: :id` from two models and fixes orgs_visibility to order by service_plan_id (the only column in its select list). --- .../runtime/buildpack_lifecycle_data_model.rb | 3 +- .../runtime/cnb_lifecycle_data_model.rb | 3 +- app/models/services/service_plan.rb | 2 +- lib/sequel_plugins/vcap_relations.rb | 8 ++++-- .../lib/sequel_plugins/vcap_relations_spec.rb | 28 +++++++++++++++++++ 5 files changed, 37 insertions(+), 7 deletions(-) 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/app/models/services/service_plan.rb b/app/models/services/service_plan.rb index 50fbe006bdc..1be2c4ebac1 100644 --- a/app/models/services/service_plan.rb +++ b/app/models/services/service_plan.rb @@ -11,7 +11,7 @@ class ServicePlan < Sequel::Model add_association_dependencies service_plan_visibilities: :destroy - one_to_many(:orgs_visibility, clone: :service_plan_visibilities) { |ds| ds.select(:service_plan_id).distinct } + one_to_many(:orgs_visibility, clone: :service_plan_visibilities, order: :service_plan_id) { |ds| ds.select(:service_plan_id).distinct } one_to_many :labels, class: 'VCAP::CloudController::ServicePlanLabelModel', key: :resource_guid, primary_key: :guid add_association_dependencies labels: :destroy diff --git a/lib/sequel_plugins/vcap_relations.rb b/lib/sequel_plugins/vcap_relations.rb index 8a88acef4a2..a3181f1fc75 100644 --- a/lib/sequel_plugins/vcap_relations.rb +++ b/lib/sequel_plugins/vcap_relations.rb @@ -56,7 +56,9 @@ def one_through_one(name, opts={}) # # See the default many_to_many implementation for a description of the args # and return values. - def many_to_many(name, opts={}) + def many_to_many(name, opts={}, &block) + opts[:order] ||= :id unless block + singular_name = name.to_s.singularize ids_attr = "#{singular_name}_ids" guids_attr = "#{singular_name}_guids" @@ -93,7 +95,9 @@ def many_to_many(name, opts={}) # # See the default one_to_many implementation for a description of the args # and return values. - def one_to_many(name, opts={}) + def one_to_many(name, opts={}, &block) + opts[:order] ||= :id unless block + singular_name = name.to_s.singularize ids_attr = "#{singular_name}_ids" guids_attr = "#{singular_name}_guids" diff --git a/spec/unit/lib/sequel_plugins/vcap_relations_spec.rb b/spec/unit/lib/sequel_plugins/vcap_relations_spec.rb index df0494f98b1..1cd24248986 100644 --- a/spec/unit/lib/sequel_plugins/vcap_relations_spec.rb +++ b/spec/unit/lib/sequel_plugins/vcap_relations_spec.rb @@ -29,6 +29,20 @@ def define_model(name) expect(@o.dogs).to be_empty end + it 'orders by id by default' do + expect(@o.dogs_dataset.sql).to match(/ORDER BY .id.$/) + end + + it 'allows overriding the default order' do + owner_klass.one_to_many :dogs, order: Sequel.desc(:id) + + expect(@o.dogs_dataset.sql).to match(/ORDER BY .id. DESC$/) + end + + it 'allows overriding the default order via dataset' do + expect(@o.dogs_dataset.order(:created_at).sql).to match(/ORDER BY .created_at.$/) + end + it 'adds a add_ method that takes an object' do d = dog_klass.create @o.add_dog d @@ -167,6 +181,20 @@ def define_model(name) expect(@d1.names).to be_empty end + it 'orders by id by default' do + expect(@d1.names_dataset.sql).to match(/ORDER BY .id.$/) + end + + it 'allows overriding the default order' do + dog_klass.many_to_many :names, order: Sequel.desc(:id) + + expect(@d1.names_dataset.sql).to match(/ORDER BY .id. DESC$/) + end + + it 'allows overriding the default order via dataset' do + expect(@d1.names_dataset.order(:created_at).sql).to match(/ORDER BY .created_at.$/) + end + it 'adds a add_ method that takes an object' do @d1.add_name @n1 expect(@d1.names).to include(@n1)