Skip to content

THREESCALE-15318: Update Faraday dependency for enhanced query parameter handling #4330

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
akostadinov merged 8 commits into from
Jul 15, 2026
+35 −17
Merged

THREESCALE-15318: Update Faraday dependency for enhanced query parameter handling #4330

Show file tree
Hide file tree
Changes from 7 commits
Commits
Show all changes
8 commits
Select commit Hold shift + click to select a range
b76cdec
THREESCALE-15318: Update Faraday dependency constraint
Jun 30, 2026
ef88e9b
Fix linting issues in Faraday test
Jun 30, 2026
5783363
Remove pisoni patch file
Jul 1, 2026
71d10ba
Remove Faraday test file
Jul 1, 2026
05be635
THREESCALE-15318: Update Gemfile.lock with faraday 2.14.3 and pisoni …
Jul 3, 2026
c79e8f3
Fix URI::DEFAULT_PARSER.pattern compatibility with uri gem 1.1.1
Jul 7, 2026
4b520b7
Pin connection_pool < 3 to fix redlock ArgumentError
akostadinov-bot[bot] Jul 8, 2026
4d8c766
Pin uri < 1.0.0, add comments for version constraints
akostadinov-bot[bot] Jul 9, 2026
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
5 changes: 3 additions & 2 deletions Gemfile
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -43,7 +43,7 @@ gem 'sorted_set', '~> 1.0'
gem 'i18n'

# Apisonator client
gem 'pisoni', '~> 1.30'
gem 'pisoni', '~> 1.31'

gem '3scale_time_range', '0.0.6'

Expand Down Expand Up @@ -79,8 +79,9 @@ gem 'mysql2', '~> 0.5.3'
gem '3scale_client', '~> 2.11', require: false
gem 'analytics-ruby', require: false

gem 'connection_pool', '~> 2.2', '< 3'

@jlledom jlledom Jul 9, 2026

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

What if you just remove this and leave the reference to connection_pool un the lockfile untouched? I don't like the idea of adding dependencies to our Gemfile when they are not our dependencies

@akostadinov akostadinov Jul 9, 2026

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Then the next bundle install may bring it back. I'd rather put a comment unless you give me a better alternative.

@jlledom jlledom Jul 9, 2026

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Fine for me

gem 'dalli'
gem 'faraday', '~> 2.0', '<= 2.9'
gem 'faraday', '~> 2.0', '>= 2.14.3'

@akostadinov akostadinov Jun 30, 2026

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Shouldn't we limit range to 2.x?

@akostadinov-bot akostadinov-bot Bot Jul 1, 2026

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The ~> 2.0 constraint already limits to 2.x (it's equivalent to ">= 2.0, < 3.0"). So faraday '~> 2.0', '>= 2.14.3' means any 2.x version starting from 2.14.3.

gem 'mimemagic', '~> 0.3.10'
gem 'nokogiri', '~> 1.18.9', force_ruby_platform: true
gem 'secure_headers', '~> 6.3.0'
Expand Down
35 changes: 20 additions & 15 deletions Gemfile.lock
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -272,7 +272,7 @@ GEM
colorize (0.8.1)
commonmarker (0.23.10)
concurrent-ruby (1.3.5)
connection_pool (2.5.4)
connection_pool (2.5.5)
crack (1.0.0)
bigdecimal
rexml
Expand Down Expand Up @@ -399,14 +399,15 @@ GEM
factory_bot (~> 6.2.0)
railties (>= 5.0.0)
fakefs (1.8.0)
faraday (2.8.1)
base64
faraday-net_http (>= 2.0, < 3.1)
ruby2_keywords (>= 0.0.4)
faraday-net_http (3.0.2)
faraday-net_http_persistent (2.1.0)
faraday (2.14.3)
faraday-net_http (>= 2.0, < 3.5)
json
logger
faraday-net_http (3.4.4)
net-http (~> 0.5)
faraday-net_http_persistent (2.3.1)
faraday (~> 2.5)
net-http-persistent (~> 4.0)
net-http-persistent (>= 4.0.4, < 5)
ffi (1.17.0)
ffi-compiler (1.0.1)
ffi (>= 1.0.0)
Expand Down Expand Up @@ -454,7 +455,7 @@ GEM
rails-dom-testing (>= 1, < 3)
railties (>= 4.2.0)
thor (>= 0.14, < 2.0)
json (2.13.2)
json (2.20.0)
json_schemer (2.4.0)
bigdecimal
hana (~> 1.3)
Expand Down Expand Up @@ -550,9 +551,11 @@ GEM
mutex_m (0.3.0)
mysql2 (0.5.6)
n_plus_one_control (0.6.2)
net-http (0.9.1)
uri (>= 0.11.1)
net-http-digest_auth (1.4.1)
net-http-persistent (4.0.2)
connection_pool (~> 2.2)
net-http-persistent (4.0.8)
connection_pool (>= 2.2.4, < 4)
net-imap (0.5.7)
date
net-protocol
Expand Down Expand Up @@ -597,8 +600,8 @@ GEM
ruby-rc4
ttfunk
pg (1.3.5)
pisoni (1.30.0)
faraday (~> 2.0, <= 2.9)
pisoni (1.31.0)
faraday (~> 2.0, >= 2.14.3)
faraday-net_http_persistent (~> 2.1)
injectedlogger (= 0.0.13)
json (~> 2.7, >= 2.7.1)
Expand Down Expand Up @@ -973,6 +976,7 @@ GEM
rack
unicorn
uniform_notifier (1.17.0)
uri (1.1.1)

@jlledom jlledom Jul 9, 2026

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

You can remove this one and let bundle pick the builtin ruby 3 gem which is 0.13.3, This way you solve the URI parser problem.

version_gem (1.1.3)
webmock (3.24.0)
addressable (>= 2.8.0)
Expand Down Expand Up @@ -1044,6 +1048,7 @@ DEPENDENCIES
coffee-rails (~> 5.0)
colorize
commonmarker (~> 0.23.10)
connection_pool (~> 2.2, < 3)
cssbundling-rails (~> 1.4.3)
cucumber (~> 7.0)
cucumber-rails (~> 3.0.0)
Expand All @@ -1058,7 +1063,7 @@ DEPENDENCIES
escape_utils
factory_bot_rails (~> 6.2)
fakefs
faraday (~> 2.0, <= 2.9)
faraday (~> 2.0, >= 2.14.3)
formtastic (~> 5.0)
hashie
hiredis-client
Expand Down Expand Up @@ -1097,7 +1102,7 @@ DEPENDENCIES
open_id_authentication
pdf-inspector
pg (~> 1.3.5)
pisoni (~> 1.30)
pisoni (~> 1.31)
prawn
prawn-svg
prawn-table!
Expand Down
2 changes: 1 addition & 1 deletion app/models/proxy.rb
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -29,7 +29,7 @@ class Proxy < ApplicationRecord # rubocop:disable Metrics/ClassLength

validates :error_status_no_match, :error_status_auth_missing, :error_status_auth_failed, :error_status_limits_exceeded, presence: true

uri_pattern = URI::DEFAULT_PARSER.pattern
uri_pattern = URI::RFC2396_Parser.new.pattern

@jlledom jlledom Jul 8, 2026

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I faced this same problem in #4325. And this solution doesn't work.

The problem is uri switched the default parser to RFC3986 (ruby/uri#107) at all effects. That means that changing this single line causes an inconsistency because the rest of the gem is still defaulting to RFC3986 for all other operations. For instance, all calls to URI.parse would use a different pattern, where the registry field doesn't exist. All other URI methods will use DEFAULT_PARSER internally...

In my PR, what I did it to remove the reference to the uri gem from the lockfile. It works because ruby 3 already includes uri 0.13.3 builtin, which still uses the old parser and satisfies faraday dependenciy, which is >= 0.11

Another problem is the URI.decode method raises an undefined method error. This was broken when we updated ruby to version 3 in porta.

As you may see, the issue is bigger than expected and deservers its own Jira ticket for investigation and careful testing. For this pr, just don't touch the uri gem.

@akostadinov akostadinov Jul 9, 2026

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Makes sense. But I'd add the dependency with a comment to Gemfile, otherwise the nect bundle install may bring it back as it happened right now.

Also is there any reason not to use the newer RFC parser? We can in general switch the default back too as evident in the issue you linked to

URI.parser = URI::RFC2396_PARSER

@jlledom jlledom Jul 9, 2026

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think it's not gonna be so easy.

URI.parser = URI::RFC2396_PARSER

This wouldn't work if we are mentioning DEFAULT_PARSER somewhere else in the code, like in the problematic call to .pattern in the proxy module. Also we are using the registry field which doesn't exist in the new RFC, and calling the decode metho which is undefined. Maybe changing a couple of lines would fix it, but it would require some time to test and investigate, to ensure we are not breaking anything. I think is not something to do quick and dirty in the middle of an unrelated PR.

@akostadinov akostadinov Jul 9, 2026

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I already said that I agree that a separate JIRA is needed. The question was whether we actually need the old parser or is it just how it worked for a long time so we don't want to fix it as it ain't broken.

The DEFAULT_PARSER call we don't use elsewhere according to grep. Idk why the other two things didn't manifest. But anyway, just capturing existing knowledge for the JIRA.


URI_OR_LOCALHOST = /\A(https?:\/\/([a-zA-Z0-9._:\/?-])+|.*localhost.*)\Z/
OPTIONAL_QUERY_FORMAT = "(?:\\?(#{uri_pattern.fetch(:QUERY)}))?"
Expand Down