Filter active service bindings per instance on dataset level

- 'active' means latest in state 'create succeeded' (or without any
  operation)
- filtering is done in SQL, not Ruby
- no duplicate code where filtering is needed

Author: philippthun

app/actions/service_credential_binding_app_create.rb

@@ -112,7 +112,8 @@ def event_repository
       end
 
       def max_bindings_per_app_service_instance
-        1
+        # TODO: revert !!!
+        2
         # NOTE: This is hard-coded to 1 for now to preserve the old uniqueness behavior.
         # TODO: Once the DB migration that drops the unique constraints for service bindings has been released,
         #       this should be switched to read from config:

app/models/runtime/helpers/service_operation_mixin.rb

@@ -1,5 +1,9 @@
 module VCAP::CloudController
   module ServiceOperationMixin
+    CREATE = 'create'.freeze
+    UPDATE = 'update'.freeze
+    DELETE = 'delete'.freeze
+
     INITIAL = 'initial'.freeze
     IN_PROGRESS = 'in progress'.freeze
     SUCCEEDED = 'succeeded'.freeze
@@ -56,15 +60,15 @@ def last_operation?
     end
 
     def create?
-      last_operation&.type == 'create'
+      last_operation&.type == CREATE
     end
 
     def update?
-      last_operation&.type == 'update'
+      last_operation&.type == UPDATE
     end
 
     def delete?
-      last_operation&.type == 'delete'
+      last_operation&.type == DELETE
     end
 
     def initial?
@@ -83,4 +87,18 @@ def failed?
       last_operation&.state == FAILED
     end
   end
+
+  module ServiceOperationDatasetMixin
+    def create_succeeded
+      last_operation_table = Sequel[last_operation_association]
+
+      no_operation = Sequel.expr(last_operation_table[:id] => nil)
+      create_and_succeeded = Sequel.expr(last_operation_table[:type] => ServiceOperationMixin::CREATE) &
+                             Sequel.expr(last_operation_table[:state] => ServiceOperationMixin::SUCCEEDED)
+
+      association_left_join(last_operation_association).
+        where(no_operation | create_and_succeeded).
+        select_all(first_source_alias)
+    end
+  end
 end

app/models/services/service_binding.rb

@@ -130,5 +130,26 @@ def save_with_new_operation(last_operation, attributes: {})
         service_binding_operation(reload: true)
       end
     end
+
+    dataset_module ServiceOperationDatasetMixin
+
+    dataset_module do
+      def last_operation_association
+        :service_binding_operation
+      end
+
+      # 'active' means latest in state 'create succeeded' (or without any operation)
+      def active_per_instance
+        create_succeeded.
+          from_self.
+          select_append do
+            row_number.function.over(
+              partition: :service_instance_guid,
+              order: [Sequel.desc(:created_at), Sequel.desc(:id)]
+            ).as(:_rn)
+          end.
+          from_self.where(_rn: 1)
+      end
+    end
   end
 end

app/presenters/system_environment/system_env_presenter.rb

@@ -3,9 +3,9 @@
 
 class SystemEnvPresenter
   def initialize(app_or_process)
+    @app_or_process = app_or_process
     @service_binding_k8s_enabled = app_or_process.service_binding_k8s_enabled
     @file_based_vcap_services_enabled = app_or_process.file_based_vcap_services_enabled
-    @service_bindings = app_or_process.service_bindings
   end
 
   def system_env
@@ -22,14 +22,8 @@ def vcap_services
   private
 
   def service_binding_env_variables
-    latest_bindings = @service_bindings.
-                      select(&:create_succeeded?).
-                      group_by(&:service_instance_guid).
-                      values.
-                      map { |list| list.max_by(&:created_at) }
-
     services_hash = {}
-    latest_bindings.each do |service_binding|
+    @app_or_process.service_bindings_dataset.active_per_instance.each do |service_binding|
       service_name = service_binding_label(service_binding)
       services_hash[service_name.to_sym] ||= []
       services_hash[service_name.to_sym] << service_binding_env_values(service_binding)

lib/cloud_controller/diego/service_binding_files_builder.rb

@@ -13,7 +13,6 @@ def initialize(app_or_process)
         @app_or_process = app_or_process
         @service_binding_k8s_enabled = app_or_process.service_binding_k8s_enabled
         @file_based_vcap_services = app_or_process.file_based_vcap_services_enabled
-        @service_bindings = app_or_process.service_bindings
       end
 
       def build
@@ -27,21 +26,14 @@ def build
 
       private
 
-      # rubocop:disable Metrics/CyclomaticComplexity
       def build_service_binding_k8s
         return nil unless @service_binding_k8s_enabled
 
         service_binding_files = {}
         names = Set.new # to check for duplicate binding names
         total_bytesize = 0 # to check the total bytesize
 
-        latest_bindings = @service_bindings.
-                          select(&:create_succeeded?).
-                          group_by(&:service_instance_guid).
-                          values.
-                          map { |list| list.max_by(&:created_at) }
-
-        latest_bindings.each do |service_binding|
+        @app_or_process.service_bindings_dataset.active_per_instance.each do |service_binding|
           sb_hash = ServiceBindingPresenter.new(service_binding, include_instance: true).to_hash
           name = sb_hash[:name]
           raise IncompatibleBindings.new("Invalid binding name: '#{name}'. Name must match #{binding_naming_convention.inspect}") unless valid_binding_name?(name)
@@ -58,8 +50,6 @@ def build_service_binding_k8s
           total_bytesize += add_file(service_binding_files, name, 'type', label)
           total_bytesize += add_file(service_binding_files, name, 'provider', label)
         end
-        # rubocop:enable Metrics/CyclomaticComplexity
-
         raise IncompatibleBindings.new("Bindings exceed the maximum allowed bytesize of #{MAX_ALLOWED_BYTESIZE}: #{total_bytesize}") if total_bytesize > MAX_ALLOWED_BYTESIZE
 
         service_binding_files.values

spec/unit/lib/cloud_controller/diego/service_binding_files_builder_spec.rb

@@ -18,10 +18,11 @@ module VCAP::CloudController::Diego
     end
 
     context 'when there are multiple bindings with the same name for the same app and service instance' do
-      before do
-        # TODO: Remove skip when the service bindings unique constraints are removed
-        skip 'this test can be enabled when the service bindings unique constraints are removed and max_bindings_per_app_service_instance can be configured'
-      end
+      # TODO: include !!!
+      # before do
+      #   # TODO: Remove skip when the service bindings unique constraints are removed
+      #   skip 'this test can be enabled when the service bindings unique constraints are removed and max_bindings_per_app_service_instance can be configured'
+      # end
 
       let(:newer_binding_created_at) { Time.now.utc - 2.minutes }
 
@@ -39,9 +40,19 @@ module VCAP::CloudController::Diego
       end
 
       it 'uses the most recent binding' do
-        expect(service_binding_files.find { |f| f.path == "#{directory}/binding-guid" }).to have_attributes(content: newer_binding.guid)
+        expect(service_binding_files.find { |f| f.path == "#{directory}/binding_guid" }).to have_attributes(content: newer_binding.guid)
         expect(service_binding_files.find { |f| f.path == "#{directory}/name" }).to have_attributes(content: name || 'binding-name')
-        expect(service_binding_files.find { |f| f.path == "#{directory}/binding-name" }).to have_attributes(content: 'binding-name') if name.nil?
+        expect(service_binding_files.find { |f| f.path == "#{directory}/binding_name" }).to have_attributes(content: 'binding-name') if name.nil?
+      end
+
+      context 'when the bindings have the same created_at timestamp' do
+        let(:newer_binding_created_at) { binding_created_at }
+
+        it 'uses the most recent binding determined by highest id' do
+          expect(service_binding_files.find { |f| f.path == "#{directory}/binding_guid" }).to have_attributes(content: newer_binding.guid)
+          expect(service_binding_files.find { |f| f.path == "#{directory}/name" }).to have_attributes(content: name || 'binding-name')
+          expect(service_binding_files.find { |f| f.path == "#{directory}/binding_name" }).to have_attributes(content: 'binding-name') if name.nil?
+        end
       end
     end
   end