Revert "v3 🐞: default route in manifest no longer errors when meant to be ignored"

* this breaks mysql unit tests

This reverts commit 805220c.

Author: cwlbraa

app/actions/app_apply_manifest.rb

@@ -9,7 +9,6 @@
 
 module VCAP::CloudController
   class AppApplyManifest
-    class Error < StandardError; end
     class NoDefaultDomain < StandardError; end
     class ServiceBindingError < StandardError; end
 
@@ -106,7 +105,6 @@ def update_routes(app, message)
       end
 
       if update_message.default_route && existing_routes.empty?
-        validate_name_dns_compliant!(app.name)
         domain_name = get_default_domain_name(app)
 
         route = "#{app.name}.#{domain_name}"
@@ -123,18 +121,6 @@ def get_default_domain_name(app)
       domain_name
     end
 
-    def validate_name_dns_compliant!(name)
-      prefix = 'Failed to create default route from app name:'
-
-      if name.present? && name.length > 63
-        error!(prefix + ' Host cannot exceed 63 characters')
-      end
-
-      unless name&.match(/\A[\w\-]+\z/)
-        error!(prefix + ' Host must be either "*" or contain only alphanumeric characters, "_", or "-"')
-      end
-    end
-
     def create_service_bindings(manifest_service_bindings_message, app)
       action = ServiceBindingCreate.new(@user_audit_info, manifest_triggered: true)
       manifest_service_bindings_message.manifest_service_bindings.each do |manifest_service_binding|
@@ -176,9 +162,5 @@ def volume_services_enabled?
     def logger
       @logger ||= Steno.logger('cc.action.app_apply_manifest')
     end
-
-    def error!(message)
-      raise Error.new(message)
-    end
   end
 end

app/jobs/app_apply_manifest_action_job.rb

@@ -23,7 +23,6 @@ def perform
              SidecarUpdate::InvalidSidecar,
              ManifestRouteUpdate::InvalidRoute,
              Route::InvalidOrganizationRelation,
-             AppApplyManifest::Error,
              AppApplyManifest::ServiceBindingError => e
 
         error = CloudController::Errors::ApiError.new_from_details('UnprocessableEntity', e.message)

app/jobs/space_apply_manifest_action_job.rb

@@ -22,7 +22,6 @@ def perform
                ProcessUpdate::InvalidProcess,
                ManifestRouteUpdate::InvalidRoute,
                Route::InvalidOrganizationRelation,
-               AppApplyManifest::Error,
                AppApplyManifest::ServiceBindingError => e
 
           app_name = AppModel.find(guid: app_guid)&.name

app/messages/named_app_manifest_message.rb

@@ -6,5 +6,18 @@ class NamedAppManifestMessage < AppManifestMessage
     register_allowed_keys [:name]
 
     validates :name, presence: { message: 'must not be empty' }, string: true
+    validate :validate_name_dns_compliant!, if: -> { default_route }
+
+    def validate_name_dns_compliant!
+      prefix = 'Failed to create default route from app name:'
+
+      if name.present? && name.length > 63
+        errors.add(prefix, 'Host cannot exceed 63 characters')
+      end
+
+      unless name&.match(/\A[\w\-]+\z/)
+        errors.add(prefix, 'Host must be either "*" or contain only alphanumeric characters, "_", or "-"')
+      end
+    end
   end
 end

spec/request/space_manifests_spec.rb

@@ -309,7 +309,7 @@
     end
 
     context 'when the app name is not a valid host name and the default-route flag is set to true' do
-      let(:app1_model) { VCAP::CloudController::AppModel.make(name: 'a' * 64, space: space) }
+      let(:app1_model) { VCAP::CloudController::AppModel.make(name: '!' * 64, space: space) }
       let(:yml_manifest) do
         {
           'applications' => [
@@ -319,51 +319,16 @@
         }.to_yaml
       end
 
-      it 'returns a 202 but fails on the job' do
+      it 'returns a 422 with an informative message' do
         expect {
           post "/v3/spaces/#{space.guid}/actions/apply_manifest", yml_manifest, yml_headers(user_header)
         }.to change { VCAP::CloudController::AppModel.count }.by(0)
 
-        expect(last_response).to have_status_code(202)
-        expect(last_response.status).to eq(202), last_response.body
-
-        job_guid = VCAP::CloudController::PollableJobModel.last.guid
-        expect(last_response.headers['Location']).to match(%r(/v3/jobs/#{job_guid}))
-
-        Delayed::Worker.new.work_off
-        job = VCAP::CloudController::PollableJobModel.find(guid: job_guid)
-        errors = YAML.safe_load(job.cf_api_error)['errors']
-        expect(errors.length).to eq 1
-        expect(errors[0]['detail']).to include('Host cannot exceed 63 characters')
-      end
-
-      context 'and routes are provided in the manifest' do
-        let(:yml_manifest) do
-          {
-            'applications' => [
-              { 'name' => app1_model.name,
-                'default-route' => true,
-                'routes' => [{ 'route' => "http://#{route.host}.#{shared_domain.name}" }] },
-            ]
-          }.to_yaml
-        end
-
-        it 'returns a 202 and succeeds' do
-          expect {
-            post "/v3/spaces/#{space.guid}/actions/apply_manifest", yml_manifest, yml_headers(user_header)
-          }.to change { VCAP::CloudController::AppModel.count }.by(0)
-
-          expect(last_response).to have_status_code(202)
-          expect(last_response.status).to eq(202), last_response.body
-
-          job_guid = VCAP::CloudController::PollableJobModel.last.guid
-          expect(last_response.headers['Location']).to match(%r(/v3/jobs/#{job_guid}))
-
-          Delayed::Worker.new.work_off
-          job = VCAP::CloudController::PollableJobModel.find(guid: job_guid)
-          expect(job.complete?).to be true
-          expect(job.cf_api_error).to be nil
-        end
+        expect(last_response).to have_status_code(422)
+        expect(last_response).to include_error_message(
+          /Failed to create default route from app name: Host cannot exceed 63 characters/)
+        expect(last_response).to include_error_message(
+          /Failed to create default route from app name: Host must be either "\*" or contain only alphanumeric characters, "_", or "-"/)
       end
     end
 

spec/unit/actions/app_apply_manifest_spec.rb

@@ -553,28 +553,6 @@ module VCAP::CloudController
               end
             end
 
-            context 'when the app name has special characters' do
-              let(:message) { AppManifestMessage.create_from_yml({ name: 'blah!@#', default_route: true }) }
-              let(:app_model) { AppModel.make(name: 'blah!@#') }
-              it 'fails with a useful error message' do
-                expect {
-                  app_apply_manifest.apply(app_model.guid, message)
-                }.to raise_error(AppApplyManifest::Error,
-                  /Failed to create default route from app name: Host must be either "\*" or contain only alphanumeric characters, "_", or "-"/)
-              end
-            end
-
-            context 'when the app name is too long' do
-              let(:app_name) { 'a' * 100 }
-              let(:message) { AppManifestMessage.create_from_yml({ name: app_name, default_route: true }) }
-              let(:app_model) { AppModel.make(name: app_name * 100) }
-              it 'fails with a useful error' do
-                expect {
-                  app_apply_manifest.apply(app_model.guid, message)
-                }.to raise_error(AppApplyManifest::Error, 'Failed to create default route from app name: Host cannot exceed 63 characters')
-              end
-            end
-
             context 'when there is no shared domain' do
               let(:domain) { PrivateDomain.make(owning_organization: app.organization) }
 

spec/unit/messages/named_app_manifest_message_spec.rb

@@ -24,6 +24,76 @@ module VCAP::CloudController
           expect(message.errors.full_messages[0]).to match(/^Name must not be empty/)
         end
       end
+
+      context 'when the name is too long' do
+        let(:params) { { name: 'x' * 64, 'default-route': default_route } }
+
+        context 'when default-route is true' do
+          let(:default_route) { true }
+
+          it 'is not valid' do
+            message = NamedAppManifestMessage.create_from_yml(params)
+
+            expect(message).to_not be_valid
+            expect(message.errors.full_messages[0]).to match(/Host cannot exceed 63 characters/)
+          end
+        end
+
+        context 'when default-route is false' do
+          let(:default_route) { false }
+
+          it 'is valid' do
+            message = NamedAppManifestMessage.create_from_yml(params)
+
+            expect(message).to be_valid
+          end
+        end
+
+        context 'when default-route is not set' do
+          let(:default_route) { nil }
+
+          it 'is valid' do
+            message = NamedAppManifestMessage.create_from_yml(params)
+
+            expect(message).to be_valid
+          end
+        end
+      end
+
+      context 'when the name contains special characters' do
+        let(:params) { { name: '%%%', 'default-route': default_route } }
+
+        context 'when default-route is true' do
+          let(:default_route) { true }
+
+          it 'is not valid' do
+            message = NamedAppManifestMessage.create_from_yml(params)
+
+            expect(message).to_not be_valid
+            expect(message.errors.full_messages[0]).to match(/Host must be either "\*" or contain only alphanumeric characters, "_", or "-"/)
+          end
+        end
+
+        context 'when default-route is false' do
+          let(:default_route) { false }
+
+          it 'is valid' do
+            message = NamedAppManifestMessage.create_from_yml(params)
+
+            expect(message).to be_valid
+          end
+        end
+
+        context 'when default-route is not set' do
+          let(:default_route) { nil }
+
+          it 'is valid' do
+            message = NamedAppManifestMessage.create_from_yml(params)
+
+            expect(message).to be_valid
+          end
+        end
+      end
     end
   end
 end