Raise new UniquenessError on duplicate app names

We recently did some explorations into how we can better use v3 API error
`title`s + `code`s to allow clients to distinguish between 422s, which are
returned in many cases, without needing to resort to the `detail`
field. The result was that we wanted to add new categories of 422
errors, identified by their `title`/`code`. These errors will still have
a `detail` field that contains their original, longer message.

The first category that we're introducing is the `UniquenessError` (code
`10016`). We now raise it when trying to create an app with a name that
already exists in the target space. Previously, this had title
`UnprocessableEntity` (code `10008`), which did not give any more useful
information than the 422 HTTP status code already gave. The error
`detail` is unchanged.

This commit also introduces some patterns and helpers that will
hopefully be useful as we do more of this kind of thing: we have
`api_error!` and `v3_api_error!` methods that can raise new `ApiError`s
and `V3::ApiErrors`, respectively. And we have corresponding
`raise_api_error` and `raise_v3_api_error` RSpec matchers to make it
easier to assert that we raised the right error.

[finishes #173506370]

Authored-by: Reid Mitchell <rmitchell@pivotal.io>

Author: reidmit

app/actions/app_create.rb

@@ -1,9 +1,12 @@
 require 'process_create'
 require 'models/helpers/process_types'
 require 'actions/labels_update'
+require 'cloud_controller/errors/api_error_helpers'
 
 module VCAP::CloudController
   class AppCreate
+    include CloudController::Errors::ApiErrorHelpers
+
     class InvalidApp < StandardError; end
 
     def initialize(user_audit_info)
@@ -25,7 +28,7 @@ def create(message, lifecycle)
 
         MetadataUpdate.update(app, message)
 
-        raise CloudController::Errors::ApiError.new_from_details('CustomBuildpacksDisabled') if using_disabled_custom_buildpack?(app)
+        api_error!(:CustomBuildpacksDisabled) if using_disabled_custom_buildpack?(app)
 
         ProcessCreate.new(@user_audit_info).create(app, {
           guid: app.guid,
@@ -42,6 +45,10 @@ def create(message, lifecycle)
 
       app
     rescue Sequel::ValidationFailed => e
+      if e.errors.on([:space_guid, :name])
+        v3_api_error!(:UniquenessError, e.message)
+      end
+
       raise InvalidApp.new(e.message)
     end
 

errors/v3.yml

@@ -1,3 +1,8 @@
+10016:
+  name: UniquenessError
+  http_code: 422
+  message: "%s"
+
 270010:
   name: ServiceBrokerNotRemovable
   http_code: 422
@@ -6,4 +11,4 @@
 270011:
   name: ServiceBrokerGone
   http_code: 422
-  message: "The service broker was removed before the synchronization completed"
+  message: "The service broker was removed before the synchronization completed"

lib/cloud_controller/errors/api_error_helpers.rb

@@ -0,0 +1,13 @@
+module CloudController
+  module Errors
+    module ApiErrorHelpers
+      def api_error!(name, *args)
+        raise CloudController::Errors::ApiError.new_from_details(name, *args)
+      end
+
+      def v3_api_error!(name, *args)
+        raise CloudController::Errors::V3::ApiError.new_from_details(name, *args)
+      end
+    end
+  end
+end

spec/support/matchers/raise_api_error.rb

@@ -0,0 +1,51 @@
+# Allows making assertions like:
+#   .to raise_api_error(:UniquenessError, "some-message")
+# or like:
+#   .to raise_api_error(:UniquenessError)
+# If only the error name is given, the additional arguments will not be considered
+# when matching API errors. This allows slightly more relaxed assertions.
+RSpec::Matchers.define :raise_api_error do |api_error_name, *api_error_args|
+  build_api_error_matcher(CloudController::Errors::ApiError, api_error_name, *api_error_args)
+end
+
+# Similar to the above, but looks for a Errors::V3::ApiError instead of Errors::ApiError
+RSpec::Matchers.define :raise_v3_api_error do |api_error_name, *api_error_args|
+  build_api_error_matcher(CloudController::Errors::V3::ApiError, api_error_name, *api_error_args)
+end
+
+def build_api_error_matcher(error_class, api_error_name, *api_error_args)
+  supports_block_expectations
+
+  match do |actual|
+    actual.call
+  rescue error_class => actual_api_error
+    @actual_api_error = actual_api_error
+    expected_error = error_class.new_from_details(api_error_name, *api_error_args)
+
+    if api_error_args.empty?
+      expected_error.name == actual_api_error.name
+    else
+      expected_error == actual_api_error
+    end
+  rescue => other_error
+    @other_error = other_error
+  end
+
+  failure_message do
+    message = "expected block to raise a #{error_class} with:\n" \
+      "  name: #{api_error_name}\n" \
+      "  args: #{api_error_args}\nbut "
+
+    message += if @actual_api_error
+                 "it raised a #{error_class} with:\n" \
+                   "  name: #{@actual_api_error.name}\n" \
+                   "  args: #{@actual_api_error.args}"
+               elsif @other_error
+                 "it raised #{@other_error}"
+               else
+                 'it did not raise'
+               end
+
+    message
+  end
+end

spec/unit/actions/app_create_spec.rb

@@ -20,19 +20,19 @@ module VCAP::CloudController
       let(:message) do
         AppCreateMessage.new(
           {
-            name:                  'my-app',
-            relationships:         relationships,
+            name: 'my-app',
+            relationships: relationships,
             environment_variables: environment_variables,
-            lifecycle:             lifecycle_request,
+            lifecycle: lifecycle_request,
             metadata: {
-                labels: {
-                    release: 'stable',
-                    'seriouseats.com/potato': 'mashed'
-                },
-                annotations: {
-                  superhero: 'Bummer-boy',
-                  superpower: 'Bums you out',
-                }
+              labels: {
+                release: 'stable',
+                'seriouseats.com/potato': 'mashed'
+              },
+              annotations: {
+                superhero: 'Bummer-boy',
+                superpower: 'Bums you out',
+              }
             }
           })
       end
@@ -141,7 +141,7 @@ module VCAP::CloudController
           it 'raises an error' do
             expect {
               app_create.create(message, lifecycle)
-            }.to raise_error(CloudController::Errors::ApiError, /Custom buildpacks are disabled/)
+            }.to raise_api_error(:CustomBuildpacksDisabled)
           end
 
           it 'does not create an app' do
@@ -168,6 +168,16 @@ module VCAP::CloudController
           app_create.create(message, lifecycle)
         }.to raise_error(AppCreate::InvalidApp)
       end
+
+      context 'when the app name is a duplicate within the space' do
+        let!(:existing_app) { AppModel.make(space: space, name: 'my-app') }
+
+        it 'fails the right way' do
+          expect {
+            app_create.create(message, lifecycle)
+          }.to raise_v3_api_error(:UniquenessError)
+        end
+      end
     end
   end
 end