Update code to work with new docker compose

.github/workflows/ci.yml

@@ -1,8 +1,9 @@
 name: ci
 
 on:
-  push:
-  workflow_dispatch:
+  - push
+  - workflow_dispatch
+  - pull_request
 
 env:
   COMPOSE_DOCKER_CLI_BUILD: 1
@@ -37,7 +38,7 @@ jobs:
             database-password: 'root'
             database-port: '3306'
             docker-compose-file: 'docker-compose.ci.mysql.yml'
-          - ruby-version: '2.4.2'
+          - ruby-version: '3.3.5'
             database-adapter: 'postgresql'
             database-user: 'postgres'
             database-password: 'postgres'
@@ -57,7 +58,7 @@ jobs:
             docker-compose-file: 'docker-compose.ci.postgresql.yml'
     steps:
       - name: Checkout code
-        uses: actions/checkout@v2
+        uses: actions/checkout@v4
       - name: Set up Ruby
         uses: ruby/setup-ruby@v1
         with:
@@ -66,8 +67,8 @@ jobs:
       - name: Start stack
         run: |
           cd docker
-          docker-compose -p it -f ${{ matrix.docker-compose-file }} up --no-start
-          docker start it_db_1
+          docker compose -p it -f ${{ matrix.docker-compose-file }} up --no-start
+          docker start it-db-1
       - name: Wait for MySQL
         if: ${{ matrix.docker-compose-file == 'docker-compose.ci.mysql.yml' }}
         run: |
@@ -100,7 +101,7 @@ jobs:
         # Sometimes it gets stuck (if Kill Bill starts when the DB isn't ready?)
         timeout-minutes: 4
         run: |
-          docker start it_killbill_1
+          docker start it-killbill-1
           count=0
           until $(curl --connect-timeout 10 --max-time 30 --output /dev/null --silent --fail http://${KB_ADDRESS}:${KB_PORT}/1.0/healthcheck); do
             if [[ "$count" == "180" ]]; then
@@ -148,10 +149,10 @@ jobs:
           echo "[DEBUG] docker ps -a"
           docker ps -a
           echo "[DEBUG] killbill env"
-          docker exec it_killbill_1 env || true
+          docker exec it-killbill-1 env || true
           echo "[DEBUG] db env"
-          docker exec it_db_1 env || true
+          docker exec it-db-1 env || true
           echo "[DEBUG] killbill logs"
-          docker logs -t --details it_killbill_1 || true
+          docker logs -t --details it-killbill-1 || true
           echo "[DEBUG] db logs"
-          docker logs -t --details it_db_1 || true
+          docker logs -t --details it-db-1 || true

docker/docker-compose.ci.mysql.yml

@@ -3,7 +3,7 @@ version: '3.8'
 services:
   killbill:
     network_mode: host
-    image: killbill/killbill:0.22.20
+    image: killbill/killbill:0.24.0
     environment:
       - KILLBILL_CATALOG_URI=SpyCarAdvanced.xml
       - KILLBILL_DAO_URL=jdbc:mysql://127.0.0.1:3306/killbill
@@ -16,6 +16,6 @@ services:
       - db
   db:
     network_mode: host
-    image: killbill/mariadb:0.22
+    image: killbill/mariadb:0.24
     environment:
       - MYSQL_ROOT_PASSWORD=root

docker/docker-compose.ci.postgresql.yml

@@ -3,7 +3,7 @@ version: '3.8'
 services:
   killbill:
     network_mode: host
-    image: killbill/killbill:0.22.20
+    image: killbill/killbill:0.24.0
     environment:
       - KILLBILL_CATALOG_URI=SpyCarAdvanced.xml
       - KILLBILL_DAO_URL=jdbc:postgresql://127.0.0.1:5432/killbill
@@ -16,6 +16,6 @@ services:
       - db
   db:
     network_mode: host
-    image: killbill/postgresql:0.22
+    image: killbill/postgresql:0.24
     environment:
       - POSTGRES_PASSWORD=postgres

lib/killbill_client/api/net_http_adapter.rb

@@ -40,14 +40,75 @@ def build_uri(relative_uri, options)
           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
+
+          # Check if this is an absolute URI (has scheme) by looking for protocol pattern
+          is_absolute_uri = path_part.match?(/\A[a-z][a-z0-9+.-]*:\/\//i)
+
+          if is_absolute_uri
+            # This is an absolute URI, parse it carefully
+            begin
+              # Parse the URI components manually to handle spaces properly
+              if path_part.match(/\A([a-z][a-z0-9+.-]*):\/\/([^\/]+)(\/.*)?/i)
+                scheme = $1
+                authority = $2  # host:port
+                path = $3 || '/'
+
+                # Encode only the path segments, not the scheme or authority
+                if path && path != '/'
+                  path_segments = path.split('/')
+                  encoded_segments = path_segments.map do |segment|
+                    # Skip encoding if the segment is already encoded (contains %XX patterns)
+                    if segment.match?(/%[0-9A-Fa-f]{2}/)
+                      segment
+                    else
+                      unsafe_regex = /[^a-zA-Z0-9\-_.!~*'()]/
+                      if unsafe_regex.match?(segment)
+                        CGI.escape(segment).gsub('+', '%20')
+                      else
+                        segment
+                      end
+                    end
+                  end
+                  encoded_path = encoded_segments.join('/')
+                else
+                  encoded_path = path
+                end
+
+                encoded_relative_uri = "#{scheme}://#{authority}#{encoded_path}"
+                encoded_relative_uri += "?#{query_part}" if query_part
+              else
+                # Fallback: treat as relative if parsing fails
+                is_absolute_uri = false
+              end
+            rescue
+              # Fallback: treat as relative if any error occurs
+              is_absolute_uri = false
+            end
+          end
+
+          unless is_absolute_uri
+            # This is a relative URI, encode path segments individually
+            path_segments = path_part.split('/')
+            encoded_segments = path_segments.map do |segment|
+              # Skip encoding if the segment is already encoded (contains %XX patterns)
+              if segment.match?(/%[0-9A-Fa-f]{2}/)
+                segment
+              else
+                # Only encode segments that contain unsafe characters
+                unsafe_regex = /[^a-zA-Z0-9\-_.!~*'()]/
+                if unsafe_regex.match?(segment)
+                  # Use CGI.escape and replace + with %20 for URL path encoding
+                  CGI.escape(segment).gsub('+', '%20')
+                else
+                  segment
+                end
+              end
+            end
+            encoded_path = encoded_segments.join('/')
+            encoded_relative_uri = query_part ? "#{encoded_path}?#{query_part}" : encoded_path
+          end
 
-          if URI(encoded_relative_uri).scheme.nil?
+          if !is_absolute_uri
             uri = (options[:base_uri] || KillBillClient::API.base_uri)
             uri = URI.parse(uri) unless uri.is_a?(URI)
             # Note: make sure to keep the full path (if any) from URI::HTTP, for non-ROOT deployments
@@ -80,17 +141,25 @@ def encode_params(options = {})
           # so remove with from global hash and insert them under :params
           plugin_properties = options.delete :pluginProperty
           if plugin_properties && plugin_properties.size > 0
+            options[:params] ||= {}
             options[:params][:pluginProperty] = plugin_properties.map { |p| "#{CGI.escape p.key.to_s}=#{CGI.escape p.value.to_s}" }
           end
 
           control_plugin_names = options.delete(:controlPluginNames)
-          options[:params][:controlPluginName] = control_plugin_names if control_plugin_names
+          if control_plugin_names
+            options[:params] ||= {}
+            options[:params][:controlPluginName] = control_plugin_names
+          end
 
           return nil unless (options[:params] && !options[:params].empty?)
 
-          options[:params][:withStackTrace] = true if (options[:return_full_stacktraces] || KillBillClient.return_full_stacktraces)
+          if (options[:return_full_stacktraces] || KillBillClient.return_full_stacktraces)
+            options[:params][:withStackTrace] = true
+          end
+
+          pairs = options[:params].filter_map { |key, value|
+            next if value.nil?
 
-          pairs = options[:params].map { |key, value|
             # If the value is an array, we 'demultiplex' into several
             if value.is_a? Array
               internal_pairs = value.map do |simple_value|
@@ -102,6 +171,7 @@ def encode_params(options = {})
             end
           }
           pairs.flatten!
+          return nil if pairs.empty?
           "?#{pairs.join '&'}"
         end
 

spec/killbill_client/http_adapter_spec.rb

@@ -149,11 +149,11 @@
     end
 
     context 'with path encoding' do
-      it 'should handle properly encoded URIs (with double encoding)' do
+      it 'should handle already encoded URIs without 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')
+        expect(uri.to_s).to eq('http://example.com:8080/1.0/kb/accounts/my%20account%20with%20spaces')
       end
 
       it 'should not encode safe characters in path' do
@@ -163,11 +163,11 @@
         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
+      it 'should handle already encoded URI with query string without 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')
+        expect(uri.to_s).to eq('http://example.com:8080/1.0/kb/accounts/my%20account?search=test%20value')
       end
     end
 
@@ -238,6 +238,123 @@
         expect(uri.query).to include('controlPluginName=plugin2')
       end
     end
+
+    context 'with spaces in path segments' do
+      it 'should encode spaces in relative URI path segments' do
+        relative_uri = '/1.0/kb/accounts/search/Kill Bill Client'
+        options = {}
+        uri = http_adapter.send(:build_uri, relative_uri, options)
+        expect(uri.to_s).to eq('http://example.com:8080/1.0/kb/accounts/search/Kill%20Bill%20Client')
+      end
+
+      it 'should handle absolute URI with spaces in path' do
+        relative_uri = 'http://127.0.0.1:8080/1.0/kb/accounts/search/Kill Bill Client'
+        options = {}
+        uri = http_adapter.send(:build_uri, relative_uri, options)
+        expect(uri.to_s).to eq('http://127.0.0.1:8080/1.0/kb/accounts/search/Kill%20Bill%20Client')
+      end
+
+      it 'should preserve scheme and host when encoding spaces in absolute URI' do
+        relative_uri = 'https://api.example.com:9090/search/My Test Account'
+        options = {}
+        uri = http_adapter.send(:build_uri, relative_uri, options)
+        expect(uri.to_s).to eq('https://api.example.com:9090/search/My%20Test%20Account')
+        expect(uri.scheme).to eq('https')
+        expect(uri.host).to eq('api.example.com')
+        expect(uri.port).to eq(9090)
+      end
+    end
+
+    context 'with nil values in query parameters' do
+      it 'should skip nil values in params' do
+        relative_uri = '/1.0/kb/accounts'
+        options = {
+          params: {
+            account: nil,
+            withStackTrace: true,
+            limit: 10
+          }
+        }
+        uri = http_adapter.send(:build_uri, relative_uri, options)
+        expect(uri.query).not_to include('account=')
+        expect(uri.query).to include('withStackTrace=true')
+        expect(uri.query).to include('limit=10')
+      end
+
+      it 'should handle all nil values in params' do
+        relative_uri = '/1.0/kb/accounts'
+        options = {
+          params: {
+            account: nil,
+            tenant: nil
+          }
+        }
+        uri = http_adapter.send(:build_uri, relative_uri, options)
+        expect(uri.query).to be_nil
+      end
+
+      it 'should handle mixed nil and valid values' do
+        relative_uri = '/1.0/kb/accounts'
+        options = {
+          params: {
+            account: nil,
+            withStackTrace: true,
+            offset: 0,
+            search: nil,
+            limit: 50
+          }
+        }
+        uri = http_adapter.send(:build_uri, relative_uri, options)
+        expect(uri.query).not_to include('account=')
+        expect(uri.query).not_to include('search=')
+        expect(uri.query).to include('withStackTrace=true')
+        expect(uri.query).to include('offset=0')
+        expect(uri.query).to include('limit=50')
+      end
+    end
+  end
+
+  describe '#encode_params' do
+    let(:http_adapter) { DummyForHTTPAdapter.new }
+
+    it 'should filter out nil values from params' do
+      options = {
+        params: {
+          account: nil,
+          withStackTrace: true,
+          limit: 10,
+          search: nil
+        }
+      }
+      query_string = http_adapter.send(:encode_params, options)
+      expect(query_string).to include('withStackTrace=true')
+      expect(query_string).to include('limit=10')
+      expect(query_string).not_to include('account=')
+      expect(query_string).not_to include('search=')
+    end
+
+    it 'should return nil when all params are nil' do
+      options = {
+        params: {
+          account: nil,
+          tenant: nil
+        }
+      }
+      query_string = http_adapter.send(:encode_params, options)
+      expect(query_string).to be_nil
+    end
+
+    it 'should return nil when params hash is empty' do
+      options = { params: {} }
+      query_string = http_adapter.send(:encode_params, options)
+      expect(query_string).to be_nil
+    end
+
+    it 'should handle options without params key' do
+      options = { account: nil, withStackTrace: true }
+      query_string = http_adapter.send(:encode_params, options)
+      expect(query_string).to be_nil
+    end
   end
 end