diff --git a/config/locales/de.yml b/config/locales/de.yml index a6056ba9ad..cfa48aab49 100644 --- a/config/locales/de.yml +++ b/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: - "Für die Eigenschaft '%{property}' wird eine Ressource vom Typ '%{expected}' erwartet. Gesandt wurde der Typ '%{actual}'." + invalid_resource: "Für die Eigenschaft '%{property}' wird ein Link wie '%{expected}' erwartet. Gesandt wurde '%{actual}'." missing_content_type: "nicht angegeben" multiple_errors: "Die Integrität mehrerer Eigenschaften wurde verletzt." parse_error: "Die Abfrage enthielt kein valides JSON." diff --git a/config/locales/en.yml b/config/locales/en.yml index 8ae2a90856..e7a14abee4 100644 --- a/config/locales/en.yml +++ b/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: - "For property '%{property}' a resource of type '%{expected}' is expected but got a resource of type '%{actual}'." + invalid_resource: "For property '%{property}' a link like '%{expected}' is expected, but got '%{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." diff --git a/lib/api/errors/form/invalid_resource_link.rb b/lib/api/errors/form/invalid_resource_link.rb index 563cbacdb2..ffd94b1e59 100644 --- a/lib/api/errors/form/invalid_resource_link.rb +++ b/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, - actual: actual_resource) + expected: expected_link, + actual: actual_link) super(message) end diff --git a/lib/api/utilities/resource_link_parser.rb b/lib/api/utilities/resource_link_parser.rb index b007574d04..aecc8c6503 100644 --- a/lib/api/utilities/resource_link_parser.rb +++ b/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(?\\d)/(?\\w+)/(?#{SEGMENT_CHARACTER}*)\\z" - def self.resource_matcher - @@matcher ||= Regexp.compile(RESOURCE_REGEX) - end + class << self + def parse(resource_link) + 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) - match = resource_matcher.match(resource_link) + private - return nil unless match + def resource_matcher + @@matcher ||= Regexp.compile(RESOURCE_REGEX) + end - return { - version: match[:version], - namespace: match[:namespace], - id: match[:id] - } + def make_expected_link(version, namespace) + version = "v#{version}" || ':apiVersion' + namespace = namespace || ':resource' + "/api/#{version}/#{namespace}/:id" + end end end end diff --git a/lib/api/v3/utilities/custom_field_injector.rb b/lib/api/v3/utilities/custom_field_injector.rb index ad14211789..bfd848eec2 100644 --- a/lib/api/v3/utilities/custom_field_injector.rb +++ b/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 diff --git a/lib/api/v3/work_packages/form/work_package_attribute_links_representer.rb b/lib/api/v3/work_packages/form/work_package_attribute_links_representer.rb index 05bc708964..7c60ed93a2 100644 --- a/lib/api/v3/work_packages/form/work_package_attribute_links_representer.rb +++ b/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 - - if resource.nil? || resource[:namespace] != ns.to_s || - resource[:version] != '3' - 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 + ::API::Utilities::ResourceLinkParser.parse_id href, + property: property, + expected_version: '3', + expected_namespace: ns end end end diff --git a/spec/lib/api/utilities/resource_link_parser_spec.rb b/spec/lib/api/utilities/resource_link_parser_spec.rb index 960a72003d..5052cdd3a0 100644 --- a/spec/lib/api/utilities/resource_link_parser_spec.rb +++ b/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 diff --git a/spec/requests/api/v3/activities_api_spec.rb b/spec/requests/api/v3/activities_api_spec.rb index aadc6e8843..09724f78b4 100644 --- a/spec/requests/api/v3/activities_api_spec.rb +++ b/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 diff --git a/spec/requests/api/v3/support/response_examples.rb b/spec/requests/api/v3/support/response_examples.rb index 19ade61a91..2d3b735d00 100644 --- a/spec/requests/api/v3/support/response_examples.rb +++ b/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', - message + 'PropertyConstraintViolation' end shared_examples_for 'format error' do |message| diff --git a/spec/requests/api/v3/work_package_resource_spec.rb b/spec/requests/api/v3/work_package_resource_spec.rb index bfecd1fbd3..c7de1a4f91 100644 --- a/spec/requests/api/v3/work_package_resource_spec.rb +++ b/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', - 'Status ' + I18n.t('activerecord.errors.models.' \ + it_behaves_like 'constraint violation' do + 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', - I18n.t('api_v3.errors.invalid_resource', - property: 'Status', - expected: 'Status', - actual: 'User') + it_behaves_like 'constraint violation' do + let(:message) { + I18n.t('api_v3.errors.invalid_resource', + property: 'Status', + 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', - I18n.t('api_v3.errors.validation.' \ + it_behaves_like 'constraint violation' do + 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', - I18n.t('api_v3.errors.validation.' \ - 'invalid_user_assigned_to_work_package', - property: property.capitalize) + it_behaves_like 'constraint violation' do + let(:message) { + I18n.t('api_v3.errors.validation.invalid_user_assigned_to_work_package', + 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', - I18n.t('api_v3.errors.invalid_resource', - property: "#{property.capitalize}", - expected: 'User', - actual: 'Status') + it_behaves_like 'constraint violation' do + let(:message) { + I18n.t('api_v3.errors.invalid_resource', + property: "#{property.capitalize}", + 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', - I18n.t('api_v3.errors.validation.' \ - 'invalid_user_assigned_to_work_package', - property: "#{property.capitalize}") + it_behaves_like 'constraint violation' do + let(:message) { + I18n.t('api_v3.errors.validation.invalid_user_assigned_to_work_package', + 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 diff --git a/spec/requests/api/v3/work_packages/form/work_package_form_resource_spec.rb b/spec/requests/api/v3/work_packages/form/work_package_form_resource_spec.rb index 9772a1340a..c8e1ed3b8c 100644 --- a/spec/requests/api/v3/work_packages/form/work_package_form_resource_spec.rb +++ b/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', - I18n.t('api_v3.errors.invalid_resource', - property: 'Status', - expected: 'Status', - actual: 'User') + it_behaves_like 'constraint violation' do + let(:message) { + I18n.t('api_v3.errors.invalid_resource', + property: 'status', + 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', - I18n.t('api_v3.errors.invalid_resource', - property: "#{property.capitalize}", - expected: 'User', - actual: 'Status') + it_behaves_like 'constraint violation' do + let(:message) { + I18n.t('api_v3.errors.invalid_resource', + property: property, + 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.' \ - 'invalid_user_assigned_to_work_package', + I18n.t('api_v3.errors.validation.invalid_user_assigned_to_work_package', property: "#{property.capitalize}").to_json }