Consolidate access rules migrations, fix RuboCop offenses, and clean up tests

- Collapse 4 migrations into 1 consolidated migration (20260407100001) that
  creates route_access_rules, route_access_rule_labels, and
  route_access_rule_annotations tables
- Remove name field from access rules per RFC updates
- Fix all RuboCop offenses: Style/CollectionQuerying (.count > 0 -> .any?),
  Migration/AddConstraintName (primary_key :id, name: :id),
  Metrics/BlockLength, Metrics/CyclomaticComplexity, and others
- Add stale resource detection in presenter (null data for deleted resources)
- Extract controller methods to reduce complexity
- Use relative class names within VCAP::CloudController module
- Fix test shadowing of rack-test app method (let(:app) -> let(:frontend_app))
- Fix Sequel validation assertion style (.include(:presence) not strings)

Author: rkoster

app/access/access_rule_access.rb

@@ -47,12 +47,12 @@ def read_for_update_with_token?(_)
       admin_user? || has_write_scope?
     end
 
-    def can_remove_related_object_with_token?(*args)
-      read_for_update_with_token?(*args)
+    def can_remove_related_object_with_token?(*)
+      read_for_update_with_token?(*)
     end
 
-    def read_related_object_for_update_with_token?(*args)
-      read_for_update_with_token?(*args)
+    def read_related_object_for_update_with_token?(*)
+      read_for_update_with_token?(*)
     end
 
     def update_with_token?(_)

app/controllers/v3/access_rules_controller.rb

@@ -39,24 +39,9 @@ def create
     message = AccessRuleCreateMessage.new(hashed_params[:body])
     unprocessable!(message.errors.full_messages) unless message.valid?
 
-    route = VCAP::CloudController::Route.find(guid: message.route_guid)
-    resource_not_found!(:route) unless route && permission_queryer.can_read_from_space?(route.space.id, route.space.organization_id)
-    unauthorized! unless permission_queryer.can_write_to_active_space?(route.space.id)
-    suspended! unless permission_queryer.is_space_active?(route.space.id)
-
-    if route.domain.internal?
-      unprocessable!('Cannot create access rules for routes on internal domains. Internal routes use container-to-container networking and bypass GoRouter.')
-    end
-    unprocessable!("Cannot create access rules for route '#{route.guid}': the route's domain does not have enforce_access_rules enabled.") unless route.domain.enforce_access_rules
-
-    # Enforce cf:any exclusivity: if route already has a cf:any rule, reject new rules;
-    # if new rule is cf:any, reject if route already has any rules.
-    existing_selectors = route.access_rules.map(&:selector)
-    unprocessable!("Cannot add 'cf:any' selector when other access rules already exist for this route.") if message.selector == 'cf:any' && existing_selectors.any?
-    unprocessable!("Cannot add selector '#{message.selector}': route already has a 'cf:any' rule.") if existing_selectors.include?('cf:any') && message.selector != 'cf:any'
-
-    # Uniqueness: selector must be unique per route
-    unprocessable!("An access rule with selector '#{message.selector}' already exists for this route.") if existing_selectors.include?(message.selector)
+    route = find_and_authorize_route(message.route_guid)
+    validate_route_domain(route)
+    validate_selector_exclusivity(route, message.selector)
 
     access_rule = VCAP::CloudController::RouteAccessRule.new(
       guid: SecureRandom.uuid,
@@ -102,6 +87,33 @@ def destroy
 
   private
 
+  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)
+    unauthorized! unless permission_queryer.can_write_to_active_space?(route.space.id)
+    suspended! unless permission_queryer.is_space_active?(route.space.id)
+    route
+  end
+
+  def validate_route_domain(route)
+    if route.domain.internal?
+      unprocessable!('Cannot create access rules for routes on internal domains. Internal routes use container-to-container networking and bypass GoRouter.')
+    end
+    unprocessable!("Cannot create access rules for route '#{route.guid}': the route's domain does not have enforce_access_rules enabled.") unless route.domain.enforce_access_rules
+  end
+
+  def validate_selector_exclusivity(route, selector)
+    existing_selectors = route.access_rules.map(&:selector)
+
+    # Enforce cf:any exclusivity: if route already has a cf:any rule, reject new rules;
+    # if new rule is cf:any, reject if route already has any rules.
+    unprocessable!("Cannot add 'cf:any' selector when other access rules already exist for this route.") if selector == 'cf:any' && existing_selectors.any?
+    unprocessable!("Cannot add selector '#{selector}': route already has a 'cf:any' rule.") if existing_selectors.include?('cf:any') && selector != 'cf:any'
+
+    # Uniqueness: selector must be unique per route
+    unprocessable!("An access rule with selector '#{selector}' already exists for this route.") if existing_selectors.include?(selector)
+  end
+
   def build_dataset(message)
     dataset = VCAP::CloudController::RouteAccessRule.dataset
 

app/decorators/include_access_rule_route_decorator.rb

@@ -14,12 +14,12 @@ def self.decorate(hash, access_rules)
       route_ids = access_rules.map(&:route_id).uniq
 
       # Fetch routes with their associations
-      routes = VCAP::CloudController::Route.where(id: route_ids).
+      routes = Route.where(id: route_ids).
                order(:created_at, :guid).
-               eager(VCAP::CloudController::Presenters::V3::RoutePresenter.associated_resources).all
+               eager(Presenters::V3::RoutePresenter.associated_resources).all
 
       # Present routes
-      hash[:included][:routes] = routes.map { |route| VCAP::CloudController::Presenters::V3::RoutePresenter.new(route).to_hash }
+      hash[:included][:routes] = routes.map { |route| Presenters::V3::RoutePresenter.new(route).to_hash }
 
       hash
     end

app/decorators/include_access_rule_selector_resource_decorator.rb

@@ -9,7 +9,7 @@ def self.match?(include_params)
       return false unless include_params
 
       # Match if any of: selector_resource, app, space, organization
-      (include_params & %w[selector_resource app space organization]).any?
+      include_params.intersect?(%w[selector_resource app space organization])
     end
 
     def self.decorate(hash, access_rules)
@@ -48,28 +48,28 @@ def self.decorate(hash, access_rules)
     private_class_method def self.fetch_and_present_apps(guids)
       return [] if guids.empty?
 
-      apps = VCAP::CloudController::AppModel.where(guid: guids).
+      apps = AppModel.where(guid: guids).
              order(:created_at, :guid).
-             eager(VCAP::CloudController::Presenters::V3::AppPresenter.associated_resources).all
-      apps.map { |app| VCAP::CloudController::Presenters::V3::AppPresenter.new(app).to_hash }
+             eager(Presenters::V3::AppPresenter.associated_resources).all
+      apps.map { |app| Presenters::V3::AppPresenter.new(app).to_hash }
     end
 
     private_class_method def self.fetch_and_present_spaces(guids)
       return [] if guids.empty?
 
-      spaces = VCAP::CloudController::Space.where(guid: guids).
+      spaces = Space.where(guid: guids).
                order(:created_at, :guid).
-               eager(VCAP::CloudController::Presenters::V3::SpacePresenter.associated_resources).all
-      spaces.map { |space| VCAP::CloudController::Presenters::V3::SpacePresenter.new(space).to_hash }
+               eager(Presenters::V3::SpacePresenter.associated_resources).all
+      spaces.map { |space| Presenters::V3::SpacePresenter.new(space).to_hash }
     end
 
     private_class_method def self.fetch_and_present_organizations(guids)
       return [] if guids.empty?
 
-      orgs = VCAP::CloudController::Organization.where(guid: guids).
+      orgs = Organization.where(guid: guids).
              order(:created_at, :guid).
-             eager(VCAP::CloudController::Presenters::V3::OrganizationPresenter.associated_resources).all
-      orgs.map { |org| VCAP::CloudController::Presenters::V3::OrganizationPresenter.new(org).to_hash }
+             eager(Presenters::V3::OrganizationPresenter.associated_resources).all
+      orgs.map { |org| Presenters::V3::OrganizationPresenter.new(org).to_hash }
     end
   end
 end

app/messages/access_rules_list_message.rb

@@ -18,7 +18,7 @@ class AccessRulesListMessage < ListMessage
     validates :selector_resource_guids, array: true, allow_nil: true
 
     def self.from_params(params)
-      super(params, %w[guids route_guids space_guids selectors selector_resource_guids include])
+      super(params, %w[route_guids space_guids selectors selector_resource_guids include])
     end
   end
 end

app/messages/domain_create_message.rb

@@ -106,17 +106,14 @@ def router_group_validation
     end
 
     def access_rules_scope_validation
-      if requested?(:access_rules_scope)
-        unless access_rules_scope.nil? || %w[any org space].include?(access_rules_scope)
-          errors.add(:access_rules_scope, "must be one of 'any', 'org', 'space'")
-        end
+      if requested?(:access_rules_scope) && !(access_rules_scope.nil? || %w[any org space].include?(access_rules_scope))
+        errors.add(:access_rules_scope, "must be one of 'any', 'org', 'space'")
       end
 
-      if requested?(:enforce_access_rules) && enforce_access_rules == true
-        if !requested?(:access_rules_scope) || access_rules_scope.nil?
-          errors.add(:access_rules_scope, 'is required when enforce_access_rules is true')
-        end
-      end
+      return unless requested?(:enforce_access_rules) && enforce_access_rules == true
+      return unless !requested?(:access_rules_scope) || access_rules_scope.nil?
+
+      errors.add(:access_rules_scope, 'is required when enforce_access_rules is true')
     end
 
     class Relationships < BaseMessage

app/presenters/v3/access_rule_presenter.rb

@@ -38,24 +38,28 @@ def build_relationships
           }
 
           # Extract resource GUID from selector and populate read-only relationships
+          # Only include the guid in data if the resource actually exists
           selector_match = access_rule.selector.match(/\Acf:(app|space|org):([0-9a-f-]+)\z/)
           if selector_match
             resource_type = selector_match[1]
             resource_guid = selector_match[2]
 
             case resource_type
             when 'app'
-              relationships[:app] = { data: { guid: resource_guid } }
+              app_exists = VCAP::CloudController::AppModel.where(guid: resource_guid).any?
+              relationships[:app] = { data: app_exists ? { guid: resource_guid } : nil }
               relationships[:space] = { data: nil }
               relationships[:organization] = { data: nil }
             when 'space'
+              space_exists = VCAP::CloudController::Space.where(guid: resource_guid).any?
               relationships[:app] = { data: nil }
-              relationships[:space] = { data: { guid: resource_guid } }
+              relationships[:space] = { data: space_exists ? { guid: resource_guid } : nil }
               relationships[:organization] = { data: nil }
             when 'org'
+              org_exists = VCAP::CloudController::Organization.where(guid: resource_guid).any?
               relationships[:app] = { data: nil }
               relationships[:space] = { data: nil }
-              relationships[:organization] = { data: { guid: resource_guid } }
+              relationships[:organization] = { data: org_exists ? { guid: resource_guid } : nil }
             end
           else
             # cf:any or malformed - all relationships are null

app/presenters/v3/route_presenter.rb

@@ -56,10 +56,10 @@ def to_hash
       @decorators.reduce(hash) { |memo, d| d.decorate(memo, [route]) }
     end
 
-    private
-
     INTERNAL_ROUTE_OPTIONS = %w[access_scope access_rules].freeze
 
+    private
+
     def route
       @resource
     end

db/migrations/20260407100001_create_route_access_rules.rb

@@ -2,23 +2,57 @@
   up do
     unless table_exists?(:route_access_rules)
       create_table :route_access_rules do
+        primary_key :id, name: :id
         String :guid, size: 255, null: false
-        primary_key :id
-        String :name, size: 255, null: false
         String :selector, size: 255, null: false
         Integer :route_id, null: false
         DateTime :created_at, null: false
         DateTime :updated_at, null: false
 
         index :guid, unique: true, name: :route_access_rules_guid_index
-        index %i[route_id name], unique: true, name: :route_access_rules_route_id_name_index
         index %i[route_id selector], unique: true, name: :route_access_rules_route_id_selector_index
         foreign_key [:route_id], :routes, on_delete: :cascade, name: :fk_route_access_rules_route_id
       end
     end
+
+    unless table_exists?(:route_access_rule_labels)
+      create_table :route_access_rule_labels do
+        primary_key :id, name: :id
+        String :guid, null: false, size: 255
+        String :resource_guid, null: false, size: 255
+        String :key_prefix, size: 253
+        String :key_name, null: false, size: 63
+        String :value, null: false, size: 63
+        DateTime :created_at, null: false
+        DateTime :updated_at
+
+        index :guid, unique: true, name: :route_access_rule_labels_guid_index
+        index :resource_guid, name: :route_access_rule_labels_resource_guid_index
+        index %i[resource_guid key_prefix key_name], unique: true, name: :route_access_rule_labels_compound_index
+        foreign_key [:resource_guid], :route_access_rules, key: :guid, on_delete: :cascade, name: :fk_route_access_rule_labels_resource_guid
+      end
+    end
+
+    unless table_exists?(:route_access_rule_annotations)
+      create_table :route_access_rule_annotations do
+        primary_key :id, name: :id
+        String :guid, null: false, size: 255
+        String :resource_guid, null: false, size: 255
+        String :key_prefix, size: 253
+        String :key, null: false, size: 1000
+        String :value, size: 5000
+        DateTime :created_at, null: false
+        DateTime :updated_at
+
+        index :guid, unique: true, name: :route_access_rule_annotations_guid_index
+        index :resource_guid, name: :route_access_rule_annotations_resource_guid_index
+        index %i[resource_guid key], unique: true, name: :route_access_rule_annotations_key_index
+        foreign_key [:resource_guid], :route_access_rules, key: :guid, on_delete: :cascade, name: :fk_route_access_rule_annotations_resource_guid
+      end
+    end
   end
 
   down do
-    drop_table(:route_access_rules) if table_exists?(:route_access_rules)
+    %i[route_access_rule_annotations route_access_rule_labels route_access_rules].each { |t| drop_table(t) if table_exists?(t) }
   end
 end