Fix remaining test failure, add detailed manifest route options error messages

Author: hoffmaen

app/actions/manifest_route_update.rb

@@ -92,16 +92,15 @@ def find_or_create_valid_route(app, manifest_route, user_audit_info)
               domain: existing_domain,
               manifest_triggered: true
             )
+          elsif route.space.guid != app.space_guid
+            raise InvalidRoute.new('Routes cannot be mapped to destinations in different spaces')
           elsif manifest_route[:options] && route[:options] != manifest_route[:options]
             # remove nil values from options
             manifest_route[:options] = manifest_route[:options].compact
             message = RouteUpdateMessage.new({
                                                'options' => manifest_route[:options]
                                              })
             route = RouteUpdate.new.update(route:, message:)
-
-          elsif route.space.guid != app.space_guid
-            raise InvalidRoute.new('Routes cannot be mapped to destinations in different spaces')
           end
 
           return route

app/messages/manifest_routes_update_message.rb

@@ -13,14 +13,8 @@ def validate(record)
           return
         end
 
-        if contains_invalid_route_options?(record.routes)
-          record.errors.add(:routes, message: 'contains invalid route options')
-          return
-        end
-
-        return unless contains_invalid_lb_algo?(record.routes)
-
-        record.errors.add(:routes, message: 'contains an invalid loadbalancing-algorithm option')
+        contains_invalid_route_options?(record)
+        contains_invalid_lb_algo?(record)
         nil
       end
 
@@ -32,23 +26,34 @@ def contains_non_route_hash_values?(routes)
         routes.any? { |r| !(r.is_a?(Hash) && r[:route].present?) }
       end
 
-      def contains_invalid_route_options?(routes)
+      def contains_invalid_route_options?(record)
+        routes = record.routes
         routes.any? do |r|
           next unless r[:options]
 
           return true unless r[:options].is_a?(Hash)
 
           return false if r[:options].empty?
 
-          return r[:options].keys.all? { |key| RouteOptionsMessage::VALID_MANIFEST_ROUTE_OPTIONS.exclude?(key) }
+          r[:options].each_key do |key|
+            RouteOptionsMessage::VALID_MANIFEST_ROUTE_OPTIONS.exclude?(key) &&
+              record.errors.add(:base,
+                                message: "Route '#{r[:route]}' contains invalid route option '#{key}'. \
+Valid keys: '#{RouteOptionsMessage::VALID_MANIFEST_ROUTE_OPTIONS.join(', ')}'")
+          end
         end
       end
 
-      def contains_invalid_lb_algo?(routes)
-        routes.any? do |r|
+      def contains_invalid_lb_algo?(record)
+        routes = record.routes
+        routes.each do |r|
           next unless r[:options] && r[:options][:'loadbalancing-algorithm']
 
-          return true if r[:options][:'loadbalancing-algorithm'] && RouteOptionsMessage::VALID_LOADBALANCING_ALGORITHMS.exclude?(r[:options][:'loadbalancing-algorithm'])
+          lb_algo = r[:options][:'loadbalancing-algorithm']
+          RouteOptionsMessage::VALID_LOADBALANCING_ALGORITHMS.exclude?(lb_algo) &&
+            record.errors.add(:base,
+                              message: "Route '#{r[:route]}' contains invalid load-balancing algorithm '#{lb_algo}'. \
+Valid algorithms: '#{RouteOptionsMessage::VALID_LOADBALANCING_ALGORITHMS.join(', ')}'")
         end
       end
     end

spec/request/space_manifests_spec.rb

@@ -646,7 +646,8 @@
           post "/v3/spaces/#{space.guid}/actions/apply_manifest", yml_manifest, yml_headers(user_header)
 
           expect(last_response).to have_status_code(422)
-          expect(last_response).to have_error_message('Routes contains invalid route options')
+          expect(last_response).to have_error_message("For application '#{app1_model.name}': \
+Route 'https://#{route.host}.#{route.domain.name}' contains invalid route option 'doesnt-exist'. Valid keys: 'loadbalancing-algorithm'")
         end
       end
 
@@ -811,7 +812,8 @@
             post "/v3/spaces/#{space.guid}/actions/apply_manifest", yml_manifest, yml_headers(user_header)
 
             expect(last_response).to have_status_code(422)
-            expect(last_response).to have_error_message('Routes contains an invalid loadbalancing-algorithm option')
+            expect(last_response).to have_error_message("For application '#{app1_model.name}': \
+Route 'https://#{route.host}.#{route.domain.name}' contains invalid load-balancing algorithm 'unsupported-lb-algorithm'. Valid algorithms: 'round-robin, least-connections'")
           end
         end
 

spec/unit/lib/cloud_controller/app_manifest/manifest_route_spec.rb

@@ -117,16 +117,6 @@ module VCAP::CloudController
           end
         end
 
-        context 'when options is set to null' do
-          let(:route) { 'http://example.com' }
-
-          it 'is invalid' do
-            options = nil
-            manifest_route = ManifestRoute.parse(route, options)
-            expect(manifest_route.valid?).to be(false)
-          end
-        end
-
         context 'when there is an invalid loadbalancing-algorithm' do
           let(:route) { 'http://example.com' }
           let(:options) { { 'loadbalancing-algorithm': 'invalid' } }

spec/unit/messages/manifest_routes_update_message_spec.rb

@@ -225,22 +225,6 @@ module VCAP::CloudController
         end
       end
 
-      context 'when a route contains route-options: null' do
-        let(:body) do
-          { 'routes' =>
-              [
-                { 'route' => 'existing.example.com',
-                  'options' => nil }
-              ] }
-        end
-
-        it 'returns false' do
-          msg = ManifestRoutesUpdateMessage.new(body)
-
-          expect(msg.valid?).to be(false)
-        end
-      end
-
       context 'when a route contains invalid route options' do
         let(:body) do
           { 'routes' =>
@@ -254,42 +238,45 @@ module VCAP::CloudController
           msg = ManifestRoutesUpdateMessage.new(body)
 
           expect(msg.valid?).to be(false)
+          expect(msg.errors.full_messages).to include("Route 'existing.example.com' contains invalid route option 'invalid'. Valid keys: 'loadbalancing-algorithm'")
         end
       end
 
-      context 'when a route contains a route option key: null' do
+      context 'when a route contains a valid loadbalancing-algorithm' do
         let(:body) do
           { 'routes' =>
               [
                 { 'route' => 'existing.example.com',
                   'options' => {
-                    'loadbalancing-algorithm' => nil
+                    'loadbalancing-algorithm' => 'round-robin'
                   } }
               ] }
         end
 
-        it 'returns false' do
+        it 'returns true' do
           msg = ManifestRoutesUpdateMessage.new(body)
 
-          expect(msg.valid?).to be(false)
+          expect(msg.valid?).to be(true)
         end
       end
 
-      context 'when a route contains a valid loadbalancing-algorithm' do
+      context 'when a route contains an invalid loadbalancing-algorithm' do
         let(:body) do
           { 'routes' =>
-              [
-                { 'route' => 'existing.example.com',
-                  'options' => {
-                    'loadbalancing-algorithm' => 'round-robin'
-                  } }
-              ] }
+            [
+              { 'route' => 'existing.example.com',
+                'options' => {
+                  'loadbalancing-algorithm' => 'sushi'
+                } }
+            ] }
         end
 
-        it 'returns true' do
+        it 'returns false' do
           msg = ManifestRoutesUpdateMessage.new(body)
 
-          expect(msg.valid?).to be(true)
+          expect(msg.valid?).to be(false)
+          expect(msg.errors.full_messages).to include("Route 'existing.example.com' contains invalid load-balancing algorithm 'sushi'. \
+Valid algorithms: 'round-robin, least-connections'")
         end
       end
     end