From 9a90b801b19bc92c335d7e3bfbf1b663a27774e8 Mon Sep 17 00:00:00 2001 From: Tung Le Date: Wed, 25 Jun 2025 17:27:04 +0700 Subject: [PATCH] Update code to work with new docker compose --- .github/workflows/ci.yml | 23 ++-- docker/docker-compose.ci.mysql.yml | 4 +- docker/docker-compose.ci.postgresql.yml | 4 +- lib/killbill_client/api/net_http_adapter.rb | 90 ++++++++++++-- spec/killbill_client/http_adapter_spec.rb | 125 +++++++++++++++++++- 5 files changed, 217 insertions(+), 29 deletions(-) diff --git a/.github/workflows/ci.yml b/.github/workflows/ci.yml index a0d7fe9..91fcace 100644 --- a/.github/workflows/ci.yml +++ b/.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 diff --git a/docker/docker-compose.ci.mysql.yml b/docker/docker-compose.ci.mysql.yml index 5d019de..42230ac 100644 --- a/docker/docker-compose.ci.mysql.yml +++ b/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 diff --git a/docker/docker-compose.ci.postgresql.yml b/docker/docker-compose.ci.postgresql.yml index 8890892..08f5a4d 100644 --- a/docker/docker-compose.ci.postgresql.yml +++ b/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 diff --git a/lib/killbill_client/api/net_http_adapter.rb b/lib/killbill_client/api/net_http_adapter.rb index fcd5a15..c5359fc 100644 --- a/lib/killbill_client/api/net_http_adapter.rb +++ b/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 diff --git a/spec/killbill_client/http_adapter_spec.rb b/spec/killbill_client/http_adapter_spec.rb index 568d8b8..e3ee273 100644 --- a/spec/killbill_client/http_adapter_spec.rb +++ b/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