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

Author: philippthun

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

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

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

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"
@@ -83,7 +85,7 @@ def many_to_many(name, opts={})
         self.name.split('::').last.underscore.pluralize.to_sym
 
       define_to_many_methods(name, singular_name, ids_attr, guids_attr)
-      super
+      super(name, opts, &block)
     end
 
     # Override one_to_many in order to add an override the default Sequel
@@ -93,15 +95,17 @@ 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"
 
       opts[:reciprocal] ||= self.name.split('::').last.underscore.to_sym
 
       define_to_many_methods(name, singular_name, ids_attr, guids_attr)
-      super
+      super(name, opts, &block)
     end
 
     private

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_<relation> 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_<relation> method that takes an object' do
       @d1.add_name @n1
       expect(@d1.names).to include(@n1)