Merge pull request #112 from tungleduyxyz/fix_url_encoding_error

Fix issue with URL encoding

Author: pierre

lib/killbill_client/api/net_http_adapter.rb

@@ -36,8 +36,17 @@ def net_http
         }
 
         def build_uri(relative_uri, options)
-          # Need to encode in case of spaces (e.g. /1.0/kb/security/users/Mad Max/roles)
-          encoded_relative_uri = URI::DEFAULT_PARSER.regexp[:UNSAFE].match?(relative_uri) ? relative_uri : URI::DEFAULT_PARSER.escape(relative_uri)
+          # Split the URI into path and query parts
+          uri_parts = relative_uri.split('?', 2)
+          path_part = uri_parts[0]
+          query_part = uri_parts[1]
+          unsafe_regex = /[^a-zA-Z0-9\-_.!~*'():\/]/
+          # Only encode the path part, not the query string
+          encoded_path = unsafe_regex.match?(path_part) ? CGI.escape(path_part) : path_part
+
+          # Reconstruct the relative_uri with encoded path
+          encoded_relative_uri = query_part ? "#{encoded_path}?#{query_part}" : encoded_path
+
           if URI(encoded_relative_uri).scheme.nil?
             uri = (options[:base_uri] || KillBillClient::API.base_uri)
             uri = URI.parse(uri) unless uri.is_a?(URI)
@@ -49,7 +58,19 @@ def build_uri(relative_uri, options)
             uri = encoded_relative_uri
             uri = URI.parse(uri) unless uri.is_a?(URI)
           end
-          uri += encode_params(options).to_s
+
+          query_params = encode_params(options)
+          if query_params && !query_params.empty?
+            # encode_params returns "?param=value", so remove the leading "?"
+            params_without_question = query_params[1..-1]
+            if uri.query && !uri.query.empty?
+              # If there's already a query string, append with &
+              uri.query = uri.query + '&' + params_without_question
+            else
+              # If no existing query string, set it directly
+              uri.query = params_without_question
+            end
+          end
 
           uri
         end

spec/killbill_client/http_adapter_spec.rb

@@ -106,6 +106,139 @@
     uri = http_adapter.send(:build_uri, KillBillClient::Model::Account::KILLBILL_API_ACCOUNTS_PREFIX, options)
     expect(uri).to eq(URI.parse("#{KillBillClient::API.base_uri.to_s}/1.0/kb/accounts"))
   end
+
+  describe '#build_uri' do
+    let(:http_adapter) { DummyForHTTPAdapter.new }
+
+    before do
+      KillBillClient.url = 'http://example.com:8080'
+    end
+
+    context 'with basic relative URI' do
+      it 'should combine base URI with relative URI' do
+        relative_uri = '/1.0/kb/accounts'
+        options = {}
+        uri = http_adapter.send(:build_uri, relative_uri, options)
+        expect(uri.to_s).to eq('http://example.com:8080/1.0/kb/accounts')
+      end
+
+      it 'should handle relative URI without leading slash' do
+        relative_uri = '1.0/kb/accounts'
+        options = {}
+        uri = http_adapter.send(:build_uri, relative_uri, options)
+        expect(uri.to_s).to eq('http://example.com:8080/1.0/kb/accounts')
+      end
+    end
+
+    context 'with custom base_uri in options' do
+      it 'should use custom base_uri from options' do
+        relative_uri = '/1.0/kb/accounts'
+        options = { base_uri: 'https://custom.example.com:9090' }
+        uri = http_adapter.send(:build_uri, relative_uri, options)
+        expect(uri.to_s).to eq('https://custom.example.com:9090/1.0/kb/accounts')
+      end
+    end
+
+    context 'with absolute URI' do
+      it 'should use absolute URI as-is' do
+        relative_uri = 'https://absolute.example.com/api/test'
+        options = {}
+        uri = http_adapter.send(:build_uri, relative_uri, options)
+        expect(uri.to_s).to eq('https://absolute.example.com/api/test')
+      end
+    end
+
+    context 'with path encoding' do
+      it 'should handle properly encoded URIs (with double encoding)' do
+        relative_uri = '/1.0/kb/accounts/my%20account%20with%20spaces'
+        options = {}
+        uri = http_adapter.send(:build_uri, relative_uri, options)
+        expect(uri.to_s).to eq('http://example.com:8080/%2F1.0%2Fkb%2Faccounts%2Fmy%2520account%2520with%2520spaces')
+      end
+
+      it 'should not encode safe characters in path' do
+        relative_uri = '/1.0/kb/accounts/abc-1234'
+        options = {}
+        uri = http_adapter.send(:build_uri, relative_uri, options)
+        expect(uri.to_s).to eq('http://example.com:8080/1.0/kb/accounts/abc-1234')
+      end
+
+      it 'should handle properly encoded URI with query string (with double encoding)' do
+        relative_uri = '/1.0/kb/accounts/my%20account?search=test%20value'
+        options = {}
+        uri = http_adapter.send(:build_uri, relative_uri, options)
+        expect(uri.to_s).to eq('http://example.com:8080/%2F1.0%2Fkb%2Faccounts%2Fmy%2520account?search=test%20value')
+      end
+    end
+
+    context 'with query parameters in options' do
+      it 'should add simple query parameters' do
+        relative_uri = '/1.0/kb/accounts'
+        options = { params: { accountId: '123', limit: 10 } }
+        uri = http_adapter.send(:build_uri, relative_uri, options)
+        expect(uri.query).to include('accountId=123')
+        expect(uri.query).to include('limit=10')
+      end
+
+      it 'should handle array parameters' do
+        relative_uri = '/1.0/kb/accounts'
+        options = { params: { tags: ['premium', 'business'] } }
+        uri = http_adapter.send(:build_uri, relative_uri, options)
+        expect(uri.query).to include('tags=premium')
+        expect(uri.query).to include('tags=business')
+      end
+
+      it 'should merge with existing query string in relative URI' do
+        relative_uri = '/1.0/kb/accounts?existing=value'
+        options = { params: { new: 'param' } }
+        uri = http_adapter.send(:build_uri, relative_uri, options)
+        expect(uri.query).to include('existing=value')
+        expect(uri.query).to include('new=param')
+      end
+
+      it 'should handle empty params hash' do
+        relative_uri = '/1.0/kb/accounts'
+        options = { params: {} }
+        uri = http_adapter.send(:build_uri, relative_uri, options)
+        expect(uri.query).to be_nil
+      end
+
+      it 'should encode special characters in query parameters' do
+        relative_uri = '/1.0/kb/accounts'
+        options = { params: { search: 'test & special chars' } }
+        uri = http_adapter.send(:build_uri, relative_uri, options)
+        expect(uri.query).to include('search=test+%26+special+chars')
+      end
+    end
+
+    context 'with plugin properties' do
+      it 'should handle pluginProperty option' do
+        contract_property = KillBillClient::Model::PluginPropertyAttributes.new
+        contract_property.key = :contractId
+        contract_property.value = 'test-123'
+        relative_uri = '/1.0/kb/accounts'
+        options = {
+          params: {},
+          pluginProperty: [contract_property]
+        }
+        uri = http_adapter.send(:build_uri, relative_uri, options)
+        expect(uri.query).to include('pluginProperty=contractId%3Dtest-123')
+      end
+    end
+
+    context 'with control plugin names' do
+      it 'should handle controlPluginNames option' do
+        relative_uri = '/1.0/kb/accounts'
+        options = {
+          params: {},
+          controlPluginNames: ['plugin1', 'plugin2']
+        }
+        uri = http_adapter.send(:build_uri, relative_uri, options)
+        expect(uri.query).to include('controlPluginName=plugin1')
+        expect(uri.query).to include('controlPluginName=plugin2')
+      end
+    end
+  end
 end
 
 class DummyForHTTPAdapter