extract method into ResourceLinkParser

- similar code was already used in two different places
- now the error message makes sense for both occurences

config/locales/de.yml

@@ -1680,8 +1680,7 @@ de:
       invalid_content_type:
         "Es wird der CONTENT-TYPE '%{content_type}' erwartet, es wurde aber der CONTENT-TYP '%{actual}' gesandt."
       invalid_user_status_transition: "Für diesen Nutzeraccount ist die angeforderte Statusänderung nicht zulässig."
-      invalid_resource:
+      invalid_resource: "Für die Eigenschaft '%{property}' wird ein Link wie '%{expected}' erwartet. Gesandt wurde '%{actual}'."
-        "Für die Eigenschaft '%{property}' wird eine Ressource vom Typ '%{expected}' erwartet. Gesandt wurde der Typ '%{actual}'."
       missing_content_type: "nicht angegeben"
       multiple_errors: "Die Integrität mehrerer Eigenschaften wurde verletzt."
       parse_error: "Die Abfrage enthielt kein valides JSON."

config/locales/en.yml

@@ -1669,8 +1669,7 @@ en:
       code_404: "The requested resource could not be found."
       code_409: "Couldn\'t update the resource because of conflicting modifications."
       invalid_content_type: "Expected CONTENT-TYPE to be '%{content_type}' but got '%{actual}'."
-      invalid_resource:
+      invalid_resource: "For property '%{property}' a link like '%{expected}' is expected, but got '%{actual}'."
-        "For property '%{property}' a resource of type '%{expected}' is expected but got a resource of type '%{actual}'."
       invalid_user_status_transition: "The current user account status does not allow this operation."
       missing_content_type: "not specified"
       multiple_errors: "Multiple fields violated their constraints."

lib/api/errors/form/invalid_resource_link.rb

@@ -31,11 +31,11 @@ module API
   module Errors
     module Form
       class InvalidResourceLink < StandardError
-        def initialize(property_name, expected_resource, actual_resource = :unknown)
+        def initialize(property_name, expected_link, actual_link)
           message = I18n.t('api_v3.errors.invalid_resource',
                            property: property_name,
-                           expected: expected_resource,
+                           expected: expected_link,
-                           actual: actual_resource)
+                           actual: actual_link)
 
           super(message)
         end

lib/api/utilities/resource_link_parser.rb

@@ -35,20 +35,55 @@ module API
       SEGMENT_CHARACTER = '(\w|[-~!$&\'\(\)*+\.,:;=@]|%[0-9A-Fa-f]{2})'
       RESOURCE_REGEX = "/api/v(?<version>\\d)/(?<namespace>\\w+)/(?<id>#{SEGMENT_CHARACTER}*)\\z"
 
-      def self.resource_matcher
+      class << self
-        @@matcher ||= Regexp.compile(RESOURCE_REGEX)
+        def parse(resource_link)
-      end
+          match = resource_matcher.match(resource_link)
+
+          return nil unless match
+
+          return {
+            version: match[:version],
+            namespace: match[:namespace],
+            id: match[:id]
+          }
+        end
+
+        def parse_id(resource_link,
+                     property: nil,
+                     expected_version: nil,
+                     expected_namespace: nil)
+          raise ArgumentError unless resource_link && property
+
+          resource = parse(resource_link)
+
+          if resource
+            version_valid = expected_version.nil? || expected_version.to_s == resource[:version]
+            namespace_valid = expected_namespace.nil? ||
+                              expected_namespace.to_s == resource[:namespace]
+          end
+
+          unless resource && version_valid && namespace_valid
+            fail ::API::Errors::Form::InvalidResourceLink.new(property,
+                                                              make_expected_link(
+                                                                expected_version,
+                                                                expected_namespace),
+                                                              resource_link)
+          end
+
+          resource[:id]
+        end
 
-      def self.parse(resource_link)
+        private
-        match = resource_matcher.match(resource_link)
 
-        return nil unless match
+        def resource_matcher
+          @@matcher ||= Regexp.compile(RESOURCE_REGEX)
+        end
 
-        return {
+        def make_expected_link(version, namespace)
-          version: match[:version],
+          version = "v#{version}" || ':apiVersion'
-          namespace: match[:namespace],
+          namespace = namespace || ':resource'
-          id: match[:id]
+          "/api/#{version}/#{namespace}/:id"
-        }
+        end
       end
     end
   end

lib/api/v3/utilities/custom_field_injector.rb

@@ -112,18 +112,12 @@ module API
                             href = link_object['href']
                             return nil unless href
 
-                            resource = ::API::Utilities::ResourceLinkParser.parse href
+                            value = ::API::Utilities::ResourceLinkParser.parse_id(
+                              href,
+                              property: property,
+                              expected_version: '3',
+                              expected_namespace: expected_namespace)
 
-                            if resource.nil? || resource[:namespace] != expected_namespace ||
-                               resource[:version] != '3'
-                              actual_namespace = resource ? resource[:namespace] : nil
-
-                              fail ::API::Errors::Form::InvalidResourceLink.new(property,
-                                                                                expected_namespace,
-                                                                                actual_namespace)
-                            end
-
-                            value = resource[:id] || ''
                             represented.custom_field_values = { custom_field.id => value }
                           }
         end

lib/api/v3/work_packages/form/work_package_attribute_links_representer.rb

@@ -125,21 +125,10 @@ module API
           def parse_resource(property, ns, href)
             return nil unless href
 
-            resource = ::API::Utilities::ResourceLinkParser.parse href
+            ::API::Utilities::ResourceLinkParser.parse_id href,
-
+                                                          property: property,
-            if resource.nil? || resource[:namespace] != ns.to_s ||
+                                                          expected_version: '3',
-               resource[:version] != '3'
+                                                          expected_namespace: ns
-              actual_ns = resource ? resource[:namespace] : nil
-
-              property_localized = I18n.t("attributes.#{property}")
-              expected_localized = I18n.t("attributes.#{ns.to_s.singularize}")
-              actual_localized = I18n.t("attributes.#{actual_ns.to_s.singularize}")
-              fail ::API::Errors::Form::InvalidResourceLink.new(property_localized,
-                                                                expected_localized,
-                                                                actual_localized)
-            end
-
-            resource ? resource[:id] : nil
           end
         end
       end

spec/lib/api/utilities/resource_link_parser_spec.rb

@@ -31,7 +31,7 @@ require 'spec_helper'
 describe ::API::Utilities::ResourceLinkParser do
   subject { described_class }
 
-  describe('#parse') do
+  describe '#parse' do
     shared_examples_for 'accepts resource link' do
       it 'parses the version' do
         expect(result[:version]).to eql(version)
@@ -97,4 +97,50 @@ describe ::API::Utilities::ResourceLinkParser do
       end
     end
   end
+
+  describe '#parse_id' do
+    it 'parses the id' do
+      expect(subject.parse_id('/api/v3/statuses/14', property: 'foo')).to eql('14')
+    end
+
+    it 'parses an empty id as empty string' do
+      expect(subject.parse_id('/api/v3/string_objects/', property: 'foo')).to eql('')
+    end
+
+    it 'accepts on matching version' do
+      expect {
+        subject.parse_id('/api/v3/statuses/14', property: 'foo', expected_version: '3')
+      }.to_not raise_error
+    end
+
+    it 'accepts on matching version as integer' do
+      expect {
+        subject.parse_id('/api/v3/statuses/14', property: 'foo', expected_version: 3)
+      }.to_not raise_error
+    end
+
+    it 'accepts on matching namespace' do
+      expect {
+        subject.parse_id('/api/v3/statuses/14', property: 'foo', expected_namespace: 'statuses')
+      }.to_not raise_error
+    end
+
+    it 'accepts on matching namespace as symbol' do
+      expect {
+        subject.parse_id('/api/v3/statuses/14', property: 'foo', expected_namespace: :statuses)
+      }.to_not raise_error
+    end
+
+    it 'raises on version mismatch' do
+      expect {
+        subject.parse_id('/api/v4/statuses/14', property: 'foo', expected_version: '3')
+      }.to raise_error(::API::Errors::Form::InvalidResourceLink)
+    end
+
+    it 'raises on namespace mismatch' do
+      expect {
+        subject.parse_id('/api/v3/types/14', property: 'foo', expected_namespace: 'statuses')
+      }.to raise_error(::API::Errors::Form::InvalidResourceLink)
+    end
+  end
 end

spec/requests/api/v3/activities_api_spec.rb

@@ -52,7 +52,9 @@ describe API::V3::Activities::ActivitiesAPI, type: :request do
   shared_examples_for 'invalid activity request' do |message|
     before { allow(User).to receive(:current).and_return(admin) }
 
-    it_behaves_like 'constraint violation', message
+    it_behaves_like 'constraint violation' do
+      let(:message) { message }
+    end
   end
 
   describe 'PATCH /api/v3/activities/:activityId' do

spec/requests/api/v3/support/response_examples.rb

@@ -110,11 +110,10 @@ shared_examples_for 'update conflict' do
                   I18n.t('api_v3.errors.code_409')
 end
 
-shared_examples_for 'constraint violation' do |message|
+shared_examples_for 'constraint violation' do
   it_behaves_like 'error response',
                   422,
-                  'PropertyConstraintViolation',
+                  'PropertyConstraintViolation'
-                  message
 end
 
 shared_examples_for 'format error' do |message|

spec/requests/api/v3/work_package_resource_spec.rb

@@ -448,9 +448,13 @@ h4. things we like
         context 'invalid status' do
           include_context 'patch request'
 
-          it_behaves_like 'constraint violation',
+          it_behaves_like 'constraint violation' do
-                          'Status ' + I18n.t('activerecord.errors.models.' \
+            let(:message) {
+              'Status ' + I18n.t('activerecord.errors.models.' \
                           'work_package.attributes.status_id.status_transition_invalid')
+            }
+          end
+
         end
 
         context 'wrong resource' do
@@ -458,11 +462,14 @@ h4. things we like
 
           include_context 'patch request'
 
-          it_behaves_like 'constraint violation',
+          it_behaves_like 'constraint violation' do
-                          I18n.t('api_v3.errors.invalid_resource',
+            let(:message) {
-                                 property: 'Status',
+              I18n.t('api_v3.errors.invalid_resource',
-                                 expected: 'Status',
+                     property: 'Status',
-                                 actual: 'User')
+                     expected: 'Status',
+                     actual: 'User')
+            }
+          end
         end
       end
 
@@ -553,20 +560,25 @@ h4. things we like
             context 'user doesn\'t exist' do
               let(:user_href) { '/api/v3/users/909090' }
 
-              it_behaves_like 'constraint violation',
+              it_behaves_like 'constraint violation' do
-                              I18n.t('api_v3.errors.validation.' \
+                let(:message) {
+                  I18n.t('api_v3.errors.validation.' \
                                      'invalid_user_assigned_to_work_package',
-                                     property: property.capitalize)
+                         property: property.capitalize)
+                }
+              end
             end
 
             context 'user is not visible' do
               let(:invalid_user) { FactoryGirl.create(:user) }
               let(:user_href) { "/api/v3/users/#{invalid_user.id}" }
 
-              it_behaves_like 'constraint violation',
+              it_behaves_like 'constraint violation' do
-                              I18n.t('api_v3.errors.validation.' \
+                let(:message) {
-                                     'invalid_user_assigned_to_work_package',
+                  I18n.t('api_v3.errors.validation.invalid_user_assigned_to_work_package',
-                                     property: property.capitalize)
+                         property: property.capitalize)
+                }
+              end
             end
 
             context 'wrong resource' do
@@ -574,11 +586,14 @@ h4. things we like
 
               include_context 'patch request'
 
-              it_behaves_like 'constraint violation',
+              it_behaves_like 'constraint violation' do
-                              I18n.t('api_v3.errors.invalid_resource',
+                let(:message) {
-                                     property: "#{property.capitalize}",
+                  I18n.t('api_v3.errors.invalid_resource',
-                                     expected: 'User',
+                         property: "#{property.capitalize}",
-                                     actual: 'Status')
+                         expected: 'User',
+                         actual: 'Status')
+                }
+              end
             end
 
             context 'group assignement disabled' do
@@ -587,10 +602,12 @@ h4. things we like
               include_context 'setup group membership', false
               include_context 'patch request'
 
-              it_behaves_like 'constraint violation',
+              it_behaves_like 'constraint violation' do
-                              I18n.t('api_v3.errors.validation.' \
+                let(:message) {
-                                     'invalid_user_assigned_to_work_package',
+                  I18n.t('api_v3.errors.validation.invalid_user_assigned_to_work_package',
-                                     property: "#{property.capitalize}")
+                         property: "#{property.capitalize}")
+                }
+              end
             end
           end
         end
@@ -741,7 +758,9 @@ h4. things we like
 
           include_context 'patch request'
 
-          it_behaves_like 'constraint violation', "Subject can't be blank"
+          it_behaves_like 'constraint violation' do
+            let(:message) { "Subject can't be blank" }
+          end
         end
 
         context 'multiple invalid attributes' do

spec/requests/api/v3/work_packages/form/work_package_form_resource_spec.rb

@@ -375,11 +375,14 @@ describe 'API v3 Work package form resource', type: :request do
 
                   include_context 'post request'
 
-                  it_behaves_like 'constraint violation',
+                  it_behaves_like 'constraint violation' do
-                                  I18n.t('api_v3.errors.invalid_resource',
+                    let(:message) {
-                                         property: 'Status',
+                      I18n.t('api_v3.errors.invalid_resource',
-                                         expected: 'Status',
+                             property: 'status',
-                                         actual: 'User')
+                             expected: '/api/v3/statuses/:id',
+                             actual: status_link)
+                    }
+                  end
                 end
               end
             end
@@ -468,19 +471,21 @@ describe 'API v3 Work package form resource', type: :request do
 
                     include_context 'post request'
 
-                    it_behaves_like 'constraint violation',
+                    it_behaves_like 'constraint violation' do
-                                    I18n.t('api_v3.errors.invalid_resource',
+                      let(:message) {
-                                           property: "#{property.capitalize}",
+                        I18n.t('api_v3.errors.invalid_resource',
-                                           expected: 'User',
+                               property: property,
-                                           actual: 'Status')
+                               expected: '/api/v3/users/:id',
+                               actual: user_link)
+                      }
+                    end
                   end
 
                   context 'group assignement disabled' do
                     let(:user_link) { "/api/v3/users/#{group.id}" }
                     let(:error_message_path) { "_embedded/validationErrors/#{property}/message" }
                     let(:error_message) {
-                      I18n.t('api_v3.errors.validation.' \
+                      I18n.t('api_v3.errors.validation.invalid_user_assigned_to_work_package',
-                             'invalid_user_assigned_to_work_package',
                              property: "#{property.capitalize}").to_json
                     }