From 70e79993757f1d6dc8694c02ec67435aa3a14314 Mon Sep 17 00:00:00 2001 From: Jan Sandbrink Date: Fri, 27 Mar 2015 10:08:05 +0100 Subject: [PATCH] fix parsing of string objects - accept them since format has changed - actually revert the URL escaping that has to be performed --- lib/api/utilities/resource_link_parser.rb | 46 ++++++-- .../utilities/resource_link_parser_spec.rb | 100 ++++++++++++------ .../api/v3/work_package_resource_spec.rb | 34 ++++++ 3 files changed, 137 insertions(+), 43 deletions(-) diff --git a/lib/api/utilities/resource_link_parser.rb b/lib/api/utilities/resource_link_parser.rb index 9821511bcf..7821d330b4 100644 --- a/lib/api/utilities/resource_link_parser.rb +++ b/lib/api/utilities/resource_link_parser.rb @@ -33,19 +33,17 @@ module API # N.B. valid characters for URL path segments as of # http://tools.ietf.org/html/rfc3986#section-3.3 SEGMENT_CHARACTER = '(\w|[-~!$&\'\(\)*+\.,:;=@]|%[0-9A-Fa-f]{2})' - RESOURCE_REGEX = "/api/v(?\\d)/(?\\w+)/(?#{SEGMENT_CHARACTER}*)\\z" + RESOURCE_REGEX = "/api/v(?\\d)/(?\\w+)/(?#{SEGMENT_CHARACTER}+)\\z" + SO_REGEX = "/api/v(?\\d)/string_objects/?\\?value=(?#{SEGMENT_CHARACTER}*)\\z" class << self def parse(resource_link) - match = resource_matcher.match(resource_link) - - return nil unless match + # string objects have a quite different format from the usual resources (query-parameter) + # we therefore have a specific regex to deal with them and a generic one for all others + result = parse_string_object resource_link + result = parse_resource resource_link unless result - { - version: match[:version], - namespace: match[:namespace], - id: match[:id] - } + result end def parse_id(resource_link, @@ -74,8 +72,36 @@ module API private + def parse_resource(resource_link) + match = resource_matcher.match(resource_link) + + return nil unless match + + { + version: match[:version], + namespace: match[:namespace], + id: ::URI.unescape(match[:id]) + } + end + + def parse_string_object(resource_link) + match = string_object_matcher.match(resource_link) + + return nil unless match + + { + version: match[:version], + namespace: 'string_objects', + id: ::URI.unescape(match[:id]) + } + end + def resource_matcher - @matcher ||= Regexp.compile(RESOURCE_REGEX) + @resource_matcher ||= Regexp.compile(RESOURCE_REGEX) + end + + def string_object_matcher + @string_object_matcher ||= Regexp.compile(SO_REGEX) end # returns whether expectation and actual are identical diff --git a/spec/lib/api/utilities/resource_link_parser_spec.rb b/spec/lib/api/utilities/resource_link_parser_spec.rb index c77e7c0766..aee0878b04 100644 --- a/spec/lib/api/utilities/resource_link_parser_spec.rb +++ b/spec/lib/api/utilities/resource_link_parser_spec.rb @@ -52,48 +52,82 @@ describe ::API::Utilities::ResourceLinkParser do end end - describe 'accepts a simple resource' do - it_behaves_like 'accepts resource link' do - let(:result) { subject.parse '/api/v3/statuses/12' } - let(:version) { '3' } - let(:namespace) { 'statuses' } - let(:id) { '12' } + describe 'generic resource links' do + describe 'accepts a simple resource' do + it_behaves_like 'accepts resource link' do + let(:result) { subject.parse '/api/v3/statuses/12' } + let(:version) { '3' } + let(:namespace) { 'statuses' } + let(:id) { '12' } + end end - end - describe 'accepts all valid segment characters as id' do - it_behaves_like 'accepts resource link' do - let(:result) { subject.parse '/api/v3/string_object/foo-2_~!$&\'()*+.,:;=@%Fa' } - let(:version) { '3' } - let(:namespace) { 'string_object' } - let(:id) { 'foo-2_~!$&\'()*+.,:;=@%Fa' } + describe 'accepts and parses all valid segment characters as id' do + it_behaves_like 'accepts resource link' do + let(:result) { subject.parse '/api/v3/string_object/foo-2_~!$&\'()*+.,:;=@%40' } + let(:version) { '3' } + let(:namespace) { 'string_object' } + let(:id) { 'foo-2_~!$&\'()*+.,:;=@@' } + end end - end - describe 'accepts resource with empty id segment' do - it_behaves_like 'accepts resource link' do - let(:result) { subject.parse '/api/v3/string_object/' } - let(:version) { '3' } - let(:namespace) { 'string_object' } - let(:id) { '' } + describe 'rejects resources with empty id segment' do + it_behaves_like 'rejects resource link' do + let(:result) { subject.parse '/api/v3/statuses/' } + end end - end - describe 'rejects resource with missing id segment' do - it_behaves_like 'rejects resource link' do - let(:result) { subject.parse '/api/v3/string_object' } + describe 'rejects resource with missing id segment' do + it_behaves_like 'rejects resource link' do + let(:result) { subject.parse '/api/v3/statuses' } + end + end + + describe 'rejects the api root' do + it_behaves_like 'rejects resource link' do + let(:result) { subject.parse '/api/v3/' } + end end - end - describe 'rejects the api root' do - it_behaves_like 'rejects resource link' do - let(:result) { subject.parse '/api/v3/' } + describe 'rejects nested resources' do + it_behaves_like 'rejects resource link' do + let(:result) { subject.parse '/api/v3/statuses/imaginary/' } + end end end - describe 'rejects nested resources' do - it_behaves_like 'rejects resource link' do - let(:result) { subject.parse '/api/v3/statuses/imaginary/' } + describe 'string object resource' do + describe 'accepts a simple string' do + it_behaves_like 'accepts resource link' do + let(:result) { subject.parse '/api/v3/string_objects/foobar' } + let(:version) { '3' } + let(:namespace) { 'string_objects' } + let(:id) { 'foobar' } + end + end + + describe 'accepts and parses all valid segment characters as value' do + it_behaves_like 'accepts resource link' do + let(:result) { subject.parse '/api/v3/string_objects?value=foo-2_~!$&\'()*+.,:;=@%40' } + let(:version) { '3' } + let(:namespace) { 'string_objects' } + let(:id) { 'foo-2_~!$&\'()*+.,:;=@@' } + end + end + + describe 'accepts string objects with empty value parameter' do + it_behaves_like 'accepts resource link' do + let(:result) { subject.parse '/api/v3/string_objects?value=' } + let(:version) { '3' } + let(:namespace) { 'string_objects' } + let(:id) { '' } + end + end + + describe 'rejects resource with missing value parameter' do + it_behaves_like 'rejects resource link' do + let(:result) { subject.parse '/api/v3/string_objects' } + end end end end @@ -103,8 +137,8 @@ describe ::API::Utilities::ResourceLinkParser 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('') + it 'parses an empty value as empty string' do + expect(subject.parse_id('/api/v3/string_objects?value=', property: 'foo')).to eql('') end it 'accepts on matching version' do diff --git a/spec/requests/api/v3/work_package_resource_spec.rb b/spec/requests/api/v3/work_package_resource_spec.rb index 635717cfe7..f86193bfa0 100644 --- a/spec/requests/api/v3/work_package_resource_spec.rb +++ b/spec/requests/api/v3/work_package_resource_spec.rb @@ -751,6 +751,40 @@ h4. things we like end end + context 'list custom field' do + let(:custom_field) { + FactoryGirl.create(:work_package_custom_field, + field_format: 'list', + is_required: false, + possible_values: target_value) + } + let(:target_value) { 'Low No. of specialc#aracters!' } + let(:value_link) { api_v3_paths.string_object target_value } + let(:value_parameter) { + { _links: { custom_field.accessor_name.camelize(:lower) => { href: value_link } } } + } + let(:params) { valid_params.merge(value_parameter) } + + before do + allow(User).to receive(:current).and_return current_user + work_package.project.work_package_custom_fields << custom_field + work_package.type.custom_fields << custom_field + end + + context 'valid' do + include_context 'patch request' + + it { expect(response.status).to eq(200) } + + it 'should respond with the work package assigned to the new value' do + expect(subject.body).to be_json_eql(value_link.to_json) + .at_path("_links/#{custom_field.accessor_name.camelize(:lower)}/href") + end + + it_behaves_like 'lock version updated' + end + end + describe 'update with read-only attributes' do describe 'single read-only violation' do context 'created and updated' do