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
@@ -33,7 +34,7 @@ def show
     message = RoutePolicyShowMessage.from_params(query_params)
     unprocessable!(message.errors.full_messages) unless message.valid?
 
-    route_policy = VCAP::CloudController::RoutePolicy.find(guid: hashed_params[:guid])
+    route_policy = find_route_policy_with_route_and_space(hashed_params[:guid])
     resource_not_found!(:route_policy) unless route_policy
 
     route = route_policy.route
@@ -53,58 +54,42 @@ 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
     unprocessable!(e.message)
   end
 
   def update
-    route_policy = VCAP::CloudController::RoutePolicy.find(guid: hashed_params[:guid])
+    route_policy = find_route_policy_with_route_and_space(hashed_params[:guid])
     resource_not_found!(:route_policy) unless route_policy
 
     find_and_authorize_route_for_policy(route_policy)
 
     message = RoutePolicyUpdateMessage.new(hashed_params[:body])
     unprocessable!(message.errors.full_messages) unless message.valid?
 
-    VCAP::CloudController::MetadataUpdate.update(route_policy, message)
+    route_policy = VCAP::CloudController::RoutePolicyUpdate.new(user_audit_info).update(route_policy, message)
 
-    route_policy_event_repository.record_route_policy_update(
-      route_policy.reload,
-      user_audit_info,
-      message.audit_hash
-    )
-
-    render status: :ok, json: Presenters::V3::RoutePolicyPresenter.new(route_policy.reload)
+    render status: :ok, json: Presenters::V3::RoutePolicyPresenter.new(route_policy)
   end
 
   def destroy
-    route_policy = VCAP::CloudController::RoutePolicy.find(guid: hashed_params[:guid])
+    route_policy = find_route_policy_with_route_and_space(hashed_params[:guid])
     resource_not_found!(:route_policy) unless route_policy
 
     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
+  def find_route_policy_with_route_and_space(guid)
+    VCAP::CloudController::RoutePolicy.eager_graph(route: :space).where(Sequel[:route_policies][:guid] => guid).all[0]
   end
 
   def find_and_authorize_route(route_guid)

app/decorators/include_route_policies_decorator.rb

@@ -2,14 +2,15 @@ module VCAP::CloudController
   class IncludeRoutePoliciesDecorator
     class << self
       def match?(include)
-        include&.any? { |i| %w[route_policies].include?(i) }
+        include&.include?('route_policies')
       end
 
       def decorate(hash, routes)
         hash[:included] ||= {}
         route_ids = routes.map(&:id).uniq
         route_policies = RoutePolicy.where(route_id: route_ids).
-                         eager(:route, :labels, :annotations).all
+                         order(:created_at, :guid).
+                         eager(Presenters::V3::RoutePolicyPresenter.associated_resources).all
 
         hash[:included][:route_policies] = route_policies.map { |rp| Presenters::V3::RoutePolicyPresenter.new(rp).to_hash }
         hash

app/decorators/include_route_policy_route_decorator.rb

@@ -3,25 +3,24 @@ class IncludeRoutePolicyRouteDecorator
     # Handles `?include=route` for GET /v3/route_policies
     # Includes the route resources associated with the route policies
 
-    def self.match?(include_params)
-      include_params&.include?('route')
-    end
+    class << self
+      def match?(include_params)
+        include_params&.include?('route')
+      end
 
-    def self.decorate(hash, route_policies)
-      hash[:included] ||= {}
+      def decorate(hash, route_policies)
+        hash[:included] ||= {}
 
-      # Collect all unique route IDs from route policies
-      route_ids = route_policies.map(&:route_id).uniq
+        route_ids = route_policies.map(&:route_id).uniq
 
-      # Fetch routes with their associations
-      routes = Route.where(id: route_ids).
-               order(:created_at, :guid).
-               eager(Presenters::V3::RoutePresenter.associated_resources).all
+        routes = Route.where(id: route_ids).
+                 order(:created_at, :guid).
+                 eager(Presenters::V3::RoutePresenter.associated_resources).all
 
-      # Present routes
-      hash[:included][:routes] = routes.map { |route| Presenters::V3::RoutePresenter.new(route).to_hash }
+        hash[:included][:routes] = routes.map { |route| Presenters::V3::RoutePresenter.new(route).to_hash }
 
-      hash
+        hash
+      end
     end
   end
 end

app/decorators/include_route_policy_source_decorator.rb

@@ -4,68 +4,68 @@ class IncludeRoutePolicySourceDecorator
     # Stale/missing resources (source GUIDs that no longer exist) are silently absent.
     # Resources the current user cannot read are also silently absent.
 
-    def self.match?(include_params)
-      return false unless include_params
-
-      include_params.include?('source')
-    end
+    class << self
+      def match?(include_params)
+        include_params&.include?('source')
+      end
 
-    def self.decorate(hash, route_policies)
-      hash[:included] ||= {}
+      def decorate(hash, route_policies)
+        hash[:included] ||= {}
 
-      # Collect all GUIDs by type
-      app_guids   = []
-      space_guids = []
-      org_guids   = []
+        app_guids   = []
+        space_guids = []
+        org_guids   = []
 
-      route_policies.each do |policy|
-        next if policy.source_type == 'any'
+        route_policies.each do |policy|
+          next if policy.source_type == 'any'
 
-        case policy.source_type
-        when 'app'   then app_guids   << policy.source_guid
-        when 'space' then space_guids << policy.source_guid
-        when 'org'   then org_guids   << policy.source_guid
+          case policy.source_type
+          when 'app'   then app_guids   << policy.source_guid
+          when 'space' then space_guids << policy.source_guid
+          when 'org'   then org_guids   << policy.source_guid
+          end
         end
-      end
 
-      permission_queryer = Permissions.new(SecurityContext.current_user)
+        permission_queryer = Permissions.new(SecurityContext.current_user)
 
-      # Fetch and present resources, filtering by what the current user can read
-      hash[:included][:apps] = fetch_and_present_apps(app_guids.uniq, permission_queryer)
-      hash[:included][:spaces] = fetch_and_present_spaces(space_guids.uniq, permission_queryer)
-      hash[:included][:organizations] = fetch_and_present_organizations(org_guids.uniq, permission_queryer)
+        hash[:included][:apps] = fetch_and_present_apps(app_guids.uniq, permission_queryer)
+        hash[:included][:spaces] = fetch_and_present_spaces(space_guids.uniq, permission_queryer)
+        hash[:included][:organizations] = fetch_and_present_organizations(org_guids.uniq, permission_queryer)
 
-      hash
-    end
+        hash
+      end
 
-    private_class_method def self.fetch_and_present_apps(guids, permission_queryer)
-      return [] if guids.empty?
+      private
 
-      apps = AppModel.where(guid: guids)
-      apps = apps.where(space_guid: permission_queryer.readable_space_guids_query) unless permission_queryer.can_read_globally?
-      apps.order(:created_at, :guid).
-        eager(Presenters::V3::AppPresenter.associated_resources).all.
-        map { |app| Presenters::V3::AppPresenter.new(app).to_hash }
-    end
+      def fetch_and_present_apps(guids, permission_queryer)
+        return [] if guids.empty?
 
-    private_class_method def self.fetch_and_present_spaces(guids, permission_queryer)
-      return [] if guids.empty?
+        apps = AppModel.where(guid: guids)
+        apps = apps.where(space_guid: permission_queryer.readable_space_guids_query) unless permission_queryer.can_read_globally?
+        apps.order(:created_at, :guid).
+          eager(Presenters::V3::AppPresenter.associated_resources).all.
+          map { |app| Presenters::V3::AppPresenter.new(app).to_hash }
+      end
 
-      spaces = Space.where(guid: guids)
-      spaces = spaces.where(guid: permission_queryer.readable_space_guids_query) unless permission_queryer.can_read_globally?
-      spaces.order(:created_at, :guid).
-        eager(Presenters::V3::SpacePresenter.associated_resources).all.
-        map { |space| Presenters::V3::SpacePresenter.new(space).to_hash }
-    end
+      def fetch_and_present_spaces(guids, permission_queryer)
+        return [] if guids.empty?
 
-    private_class_method def self.fetch_and_present_organizations(guids, permission_queryer)
-      return [] if guids.empty?
+        spaces = Space.where(guid: guids)
+        spaces = spaces.where(guid: permission_queryer.readable_space_guids_query) unless permission_queryer.can_read_globally?
+        spaces.order(:created_at, :guid).
+          eager(Presenters::V3::SpacePresenter.associated_resources).all.
+          map { |space| Presenters::V3::SpacePresenter.new(space).to_hash }
+      end
+
+      def fetch_and_present_organizations(guids, permission_queryer)
+        return [] if guids.empty?
 
-      orgs = Organization.where(guid: guids)
-      orgs = orgs.where(guid: permission_queryer.readable_org_guids_query) unless permission_queryer.can_read_globally?
-      orgs.order(:created_at, :guid).
-        eager(Presenters::V3::OrganizationPresenter.associated_resources).all.
-        map { |org| Presenters::V3::OrganizationPresenter.new(org).to_hash }
+        orgs = Organization.where(guid: guids)
+        orgs = orgs.where(guid: permission_queryer.readable_org_guids_query) unless permission_queryer.can_read_globally?
+        orgs.order(:created_at, :guid).
+          eager(Presenters::V3::OrganizationPresenter.associated_resources).all.
+          map { |org| Presenters::V3::OrganizationPresenter.new(org).to_hash }
+      end
     end
   end
 end

app/messages/domain_create_message.rb

@@ -127,7 +127,7 @@ def route_policies_scope_validation
       end
 
       return unless requested?(:enforce_route_policies) && enforce_route_policies == true
-      return unless !requested?(:route_policies_scope) || route_policies_scope.nil?
+      return if route_policies_scope.present?
 
       errors.add(:route_policies_scope, 'is required when enforce_route_policies is true')
     end

app/messages/route_policies_list_message.rb

@@ -14,6 +14,7 @@ class RoutePoliciesListMessage < MetadataListMessage
     validates_with NoAdditionalParamsValidator
     validates_with IncludeParamValidator, valid_values: %w[source route]
 
+    validates :route_guids, array: true, allow_nil: true
     validates :space_guids, array: true, allow_nil: true
     validates :source_guids, array: true, allow_nil: true
     validates :sources, array: true, allow_nil: true