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

@@ -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(?<version>\\d)/(?<namespace>\\w+)/(?<id>#{SEGMENT_CHARACTER}*)\\z"
+      RESOURCE_REGEX = "/api/v(?<version>\\d)/(?<namespace>\\w+)/(?<id>#{SEGMENT_CHARACTER}+)\\z"
+      SO_REGEX = "/api/v(?<version>\\d)/string_objects/?\\?value=(?<id>#{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

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

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