Route policies follow ups

app/actions/route_policy_create.rb

@@ -1,29 +1,46 @@
+require 'repositories/route_policy_event_repository'
+
 module VCAP::CloudController
   class RoutePolicyCreate
     class Error < StandardError
     end
 
+    def initialize(user_audit_info)
+      @user_audit_info = user_audit_info
+    end
+
     def create(route:, message:)
       validate_scope!(route.domain, message.source)
 
-      RoutePolicy.db.transaction do
+      route_policy = RoutePolicy.db.transaction do
         # Lock the parent route row to serialize concurrent creates.
         # SELECT ... FOR UPDATE on an empty policies table acquires no row locks,
         # so two concurrent transactions can both read [] and both pass cf:any
         # exclusivity validation. Locking the route row (which always exists)
         # ensures they serialize regardless of how many policies currently exist.
         Route.where(id: route.id).for_update.first or raise Error.new("Route '#{route.guid}' not found.")
 
-        existing_policies = RoutePolicy.where(route_id: route.id).all
-        validate_source_exclusivity(existing_policies, message.source)
-
-        RoutePolicy.create(
+        policy = RoutePolicy.create(
           source: message.source,
           route_id: route.id
         )
+
+        Repositories::RoutePolicyEventRepository.new.record_route_policy_create(
+          policy,
+          @user_audit_info,
+          { 'source' => message.source, 'route_guid' => route.guid }
+        )
+
+        policy
       end
+
+      route_policy.notify_diego
+
+      route_policy
     rescue Sequel::UniqueConstraintViolation
       raise Error.new("A route policy with source '#{message.source}' already exists for this route.")
+    rescue Sequel::ValidationFailed => e
+      raise Error.new(e.errors.full_messages.join(', '))
     end
 
     private
@@ -34,14 +51,5 @@ def validate_scope!(domain, source)
 
       raise Error.new("Source '#{source}' is not allowed: domain's route_policies_scope is 'space'.")
     end
-
-    def validate_source_exclusivity(locked_policies, source)
-      existing_sources = locked_policies.map(&:source)
-
-      # Enforce cf:any exclusivity: if new policy is cf:any, reject if route already has any policies;
-      # if route already has a cf:any policy, reject new policies.
-      raise Error.new("Cannot add 'cf:any' source when other route policies already exist for this route.") if source == 'cf:any' && existing_sources.any?
-      raise Error.new("Cannot add source '#{source}': route already has a 'cf:any' policy.") if existing_sources.include?('cf:any')
-    end
   end
 end

app/actions/route_policy_destroy.rb

@@ -0,0 +1,24 @@
+require 'repositories/route_policy_event_repository'
+
+module VCAP::CloudController
+  class RoutePolicyDestroy
+    def initialize(user_audit_info)
+      @user_audit_info = user_audit_info
+    end
+
+    def delete(route_policy)
+      RoutePolicy.db.transaction do
+        route_policy.destroy
+
+        Repositories::RoutePolicyEventRepository.new.record_route_policy_delete(
+          route_policy,
+          @user_audit_info
+        )
+      end
+
+      route_policy.notify_diego
+
+      nil
+    end
+  end
+end

app/actions/route_policy_update.rb

@@ -0,0 +1,23 @@
+require 'repositories/route_policy_event_repository'
+
+module VCAP::CloudController
+  class RoutePolicyUpdate
+    def initialize(user_audit_info)
+      @user_audit_info = user_audit_info
+    end
+
+    def update(route_policy, message)
+      RoutePolicy.db.transaction do
+        MetadataUpdate.update(route_policy, message)
+
+        Repositories::RoutePolicyEventRepository.new.record_route_policy_update(
+          route_policy,
+          @user_audit_info,
+          message.audit_hash
+        )
+      end
+
+      route_policy
+    end
+  end
+end

app/controllers/v3/route_policies_controller.rb

@@ -6,7 +6,8 @@
 require 'decorators/include_route_policy_source_decorator'
 require 'decorators/include_route_policy_route_decorator'
 require 'actions/route_policy_create'
-require 'repositories/route_policy_event_repository'
+require 'actions/route_policy_update'
+require 'actions/route_policy_destroy'
 require 'fetchers/label_selector_query_generator'
 
 class RoutePoliciesController < ApplicationController
@@ -53,13 +54,7 @@ def create
     route = find_and_authorize_route(message.route_guid)
     validate_route_domain(route)
 
-    route_policy = VCAP::CloudController::RoutePolicyCreate.new.create(route: route, message: message)
-
-    route_policy_event_repository.record_route_policy_create(
-      route_policy,
-      user_audit_info,
-      { 'source' => message.source, 'route_guid' => message.route_guid }
-    )
+    route_policy = VCAP::CloudController::RoutePolicyCreate.new(user_audit_info).create(route: route, message: message)
 
     render status: :created, json: Presenters::V3::RoutePolicyPresenter.new(route_policy)
   rescue VCAP::CloudController::RoutePolicyCreate::Error => e
@@ -75,15 +70,9 @@ def update
     message = RoutePolicyUpdateMessage.new(hashed_params[:body])
     unprocessable!(message.errors.full_messages) unless message.valid?
 
-    VCAP::CloudController::MetadataUpdate.update(route_policy, message)
-
-    route_policy_event_repository.record_route_policy_update(
-      route_policy.reload,
-      user_audit_info,
-      message.audit_hash
-    )
+    route_policy = VCAP::CloudController::RoutePolicyUpdate.new(user_audit_info).update(route_policy, message)
 
-    render status: :ok, json: Presenters::V3::RoutePolicyPresenter.new(route_policy.reload)
+    render status: :ok, json: Presenters::V3::RoutePolicyPresenter.new(route_policy)
   end
 
   def destroy
@@ -92,21 +81,13 @@ def destroy
 
     find_and_authorize_route_for_policy(route_policy)
 
-    route_policy.destroy
+    VCAP::CloudController::RoutePolicyDestroy.new(user_audit_info).delete(route_policy)
 
-    route_policy_event_repository.record_route_policy_delete(
-      route_policy,
-      user_audit_info
-    )
     head :no_content
   end
 
   private
 
-  def route_policy_event_repository
-    @route_policy_event_repository ||= Repositories::RoutePolicyEventRepository.new
-  end
-
   def find_and_authorize_route(route_guid)
     route = VCAP::CloudController::Route.find(guid: route_guid)
     resource_not_found!(:route) unless route && permission_queryer.can_read_from_space?(route.space.id, route.space.organization_id)

app/models/runtime/route_policy.rb

@@ -33,27 +33,29 @@ def source=(val)
     def validate
       validates_presence :source_type
       validates_presence :route_id
+      validate_cf_any_exclusivity
     end
 
-    def after_create
-      super
-      notify_processes_of_route_update
-    end
+    def notify_diego
+      return unless route
 
-    def after_destroy
-      super
-      notify_processes_of_route_update
+      route.apps.each do |process|
+        ProcessRouteHandler.new(process).notify_backend_of_route_update
+      end
     end
 
     private
 
-    def notify_processes_of_route_update
-      return unless route
+    def validate_cf_any_exclusivity
+      return unless route_id
+
+      siblings = RoutePolicy.where(route_id: route_id).exclude(id: id)
+      return if siblings.empty?
 
-      db.after_commit do
-        route.apps.each do |process|
-          ProcessRouteHandler.new(process).notify_backend_of_route_update
-        end
+      if source_type == 'any'
+        errors.add(:source, "'cf:any' cannot coexist with other route policies on the same route")
+      elsif siblings.where(source_type: 'any').any?
+        errors.add(:source, "cannot coexist with the existing 'cf:any' policy on this route")
       end
     end
   end