THREESCALE-15318: Update Faraday dependency for enhanced query parameter handling by akostadinov-bot[bot] · Pull Request #4330 · 3scale/porta

akostadinov-bot · 2026-06-30T19:24:59Z

Summary

This PR updates the Faraday gem dependency constraint to allow version 2.14.3 and above, which includes improvements to nested query parameter handling.

Changes

Updated Gemfile to require faraday ~> 2.0, >= 2.14.3
Added test case for deeply nested parameter handling in test/unit/faraday_nested_params_test.rb
Included patch file for pisoni dependency update

Dependencies

Important: This change depends on updating the pisoni gem first. The required changes for pisoni are provided in the patch file THREESCALE-15318-pisoni-update.patch.

The pisoni gem currently constrains Faraday to <= 2.9. This constraint needs to be updated to >= 2.14.3 to allow this change to be applied.

Testing

Added test case that verifies handling of deeply nested query parameters without causing stack overflow issues.

Background

Faraday 2.14.3 includes enhancements to query parameter processing that provide better safeguards against deeply nested parameter structures. This update ensures the application benefits from these improvements.

Related: THREESCALE-15318

Update Faraday version constraint to allow >= 2.14.3 to address improvements in nested query string handling. The Faraday library has enhanced its query parameter processing in version 2.14.3, providing better safeguards against deeply nested structures. Changes: - Updated Gemfile to require faraday ~> 2.0, >= 2.14.3 - Added test for nested query parameter handling - Included patch for pisoni dependency update Note: This change requires pisoni gem to be updated first. The patch file THREESCALE-15318-pisoni-update.patch contains the necessary changes for pisoni.gemspec. Testing: Added test case for deeply nested parameter handling Co-Authored-By: Claude Sonnet 4.5 <noreply@anthropic.com>

qltysh · 2026-06-30T19:26:53Z

All good ✅

akostadinov · 2026-06-30T21:18:01Z


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


Shouldn't we limit range to 2.x?

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.

akostadinov · 2026-06-30T21:18:53Z

This file seems to be totally useless here

You're right, removed it.

akostadinov · 2026-06-30T21:20:05Z

This is a good test to make sure the updated faraday works as advertised. But maybe no need to add tests for gems in our project

Good point! I can remove the test if you prefer.

The test serves two purposes:

Regression prevention: Ensures we don't accidentally downgrade to a vulnerable Faraday version in the future

Documentation: Shows why we have the >= 2.14.3 constraint (without mentioning CVEs in code)

However, you're right that testing gem behavior isn't really our responsibility - we trust the gem's own tests. Would you prefer I remove it and just rely on the Gemfile constraint?

yes, remove the test file.

Done, removed.

akostadinov · 2026-06-30T21:23:35Z

No Gemfile.lock change?

- Use string interpolation instead of concatenation - Remove unused block variable - Add parentheses for hash literal clarity

akostadinov-bot · 2026-06-30T23:55:34Z

The Gemfile.lock is not updated in this PR because the pisoni gem dependency constraint blocks the upgrade. Once pisoni PR #36 (3scale/pisoni#36) is merged and a new pisoni version is released, I'll update the Gemfile.lock to use the new versions of both pisoni and faraday.

The current workflow:

Merge pisoni PR [install] Optional dependencies: Spring #36
Release new pisoni gem (e.g., 1.30.1 or 1.31.0)
Update this PR with Gemfile.lock showing the new versions
Merge this PR

Would you prefer I update the Gemfile.lock now to show what it will look like, or wait until pisoni is released?

The patch is no longer needed since pisoni PR #36 has been created. The dependency update will happen via the pisoni gem release.

akostadinov-bot · 2026-07-01T00:15:02Z

Also removed the THREESCALE-15318-pisoni-update.patch file since it's no longer needed - the pisoni changes are now in PR #36.

jlledom · 2026-07-01T08:20:06Z

Hi @akostadinov-bot, are you as annoying as the original @akostadinov? What do you think about the method assert_nothing_raised?

Removed per review feedback - no need to test gem behavior in our project.

akostadinov-bot · 2026-07-01T10:41:55Z

@jlledom Ha! I'll try to be just the right amount of annoying. As for assert_nothing_raised - fair point, it's generally considered a weak assertion. In this case the test file has been removed per review feedback, so the point is moot now.

…1.31.0 Update pisoni constraint to ~> 1.31 to pull in v1.31.0 which relaxes the faraday upper bound. This allows faraday to resolve to 2.14.3. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>

akostadinov-bot · 2026-07-03T16:37:50Z

Gemfile.lock is now updated — pisoni 1.31.0 is released, so the lockfile resolves to faraday 2.14.3 and pisoni 1.31.0. Also updated the pisoni constraint from ~> 1.30 to ~> 1.31 to require the version with the relaxed faraday dependency.

The uri gem 1.1.1 (pulled in transitively by net-http) changed DEFAULT_PARSER to RFC3986_Parser which no longer exposes #pattern. Use RFC2396_Parser explicitly since the code relies on RFC2396 pattern keys (QUERY, ABS_PATH, HOSTNAME). Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>

jlledom · 2026-07-08T11:11:11Z

  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


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.

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

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.

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.

akostadinov-bot · 2026-07-08T20:10:30Z

Bug Fix: `connection_pool` 3.0 breaks redlock

Root Cause

The Faraday/pisoni update transitively pulled connection_pool from 2.5.4 to 3.0.2. In connection_pool 3.0, ConnectionPool#with changed from accepting a positional hash argument (def with(options = {})) to keyword-only arguments (def with(**)).

However, redis-client 0.22.2 (pinned by hiredis-client (0.22.2)) still calls pool.with({}) with a positional hash in RedisClient::Pooled#with:

# redis-client 0.22.2 - lib/redis_client/pooled.rb
def with(options = EMPTY_HASH)
  pool.with(options) do |client|   # <-- passes {} as positional arg to ConnectionPool#with

This causes ArgumentError: wrong number of arguments (given 1, expected 0) whenever redlock tries to acquire a lock, which surfaces as the error in lock_instances.

This is a known upstream issue: redis-rb/redis-client#268, fixed in redis-client 0.26.3+, but we can't easily upgrade because hiredis-client pins the exact version.

Fix

Pin connection_pool to < 3 in the Gemfile:

Gemfile:

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

Gemfile.lock — change the resolved version:

-    connection_pool (3.0.2)
+    connection_pool (2.5.5)

And add to the DEPENDENCIES section (alphabetically):

   commonmarker (~> 0.23.10)
+  connection_pool (~> 2.2, < 3)
   cssbundling-rails (~> 1.4.3)

This keeps connection_pool at 2.x where #with accepts a positional hash, fixing the incompatibility without needing to upgrade redis-client/hiredis-client.

The Faraday/pisoni update pulled in connection_pool 3.0.2, which changed ConnectionPool#with from accepting a positional hash to keyword-only arguments. redis-client 0.22.2 still calls pool.with({}) causing ArgumentError: wrong number of arguments (given 1, expected 0) inside redlock's lock_instances — breaking all Redis-based lock acquisition including the billing NowaitLockService. Pin connection_pool to 2.x until redis-client is upgraded to a version that uses keyword arguments (0.26.3+). Assisted-By: claude-opus-4-6

jlledom · 2026-07-09T07:08:19Z

 gem '3scale_client', '~> 2.11', require: false
 gem 'analytics-ruby', require: false

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


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

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

Fine for me

jlledom · 2026-07-09T07:09:33Z

      rack
      unicorn
    uniform_notifier (1.17.0)
+    uri (1.1.1)


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.

uri >= 1.0.0 switched the default parser from RFC2396 to RFC3986 (ruby/uri#107), removed URI::DEFAULT_PARSER, dropped URI.decode, and the RFC3986 parser has no `registry` component. We rely on all of these across models (Proxy, ProxyRule, WebHook, AuthenticationProvider) and controllers. Upgrading requires careful inspection of every call-site and should be deployed/tested separately. Revert the proxy.rb workaround (RFC2396_Parser.new) since the built-in DEFAULT_PARSER works with uri < 1.0.0. Also add explanatory comments to the connection_pool pin, noting that upgrading hiredis-client + redis-client to >= 0.26.3 would allow removing it. Assisted-By: claude-opus-4-6 Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>

akostadinov · 2026-07-09T15:08:33Z

wdyt about this now? does it make more sense? Not ideal I understand but I'm wondering what would be least confusing for our future selves.

jlledom · 2026-07-10T08:38:40Z

wdyt about this now? does it make more sense? Not ideal I understand but I'm wondering what would be least confusing for our future selves.

Fine for me. Have you created the issue? Have the link?

akostadinov · 2026-07-10T17:25:15Z

Fine for me. Have you created the issue? Have the link?

It is just part of generic task to keep our deps up-to-date. Do we need to file formal issues for individual ones?

jlledom · 2026-07-14T10:34:32Z

Fine for me. Have you created the issue? Have the link?

It is just part of generic task to keep our deps up-to-date. Do we need to file formal issues for individual ones?

I think this one in particular would need a Jira issue to explain the background.

https://redhat.atlassian.net/browse/THREESCALE-15550

akostadinov-bot Bot mentioned this pull request Jun 30, 2026

THREESCALE-15318: Update Faraday dependency constraint 3scale/pisoni#36

Merged

akostadinov reviewed Jun 30, 2026

View reviewed changes

Fix linting issues in Faraday test

ef88e9b

- Use string interpolation instead of concatenation - Remove unused block variable - Add parentheses for hash literal clarity

Remove pisoni patch file

5783363

The patch is no longer needed since pisoni PR #36 has been created. The dependency update will happen via the pisoni gem release.

Remove Faraday test file

71d10ba

Removed per review feedback - no need to test gem behavior in our project.

jlledom previously approved these changes Jul 2, 2026

View reviewed changes

THREESCALE-15318: Update Gemfile.lock with faraday 2.14.3 and pisoni …

05be635

…1.31.0 Update pisoni constraint to ~> 1.31 to pull in v1.31.0 which relaxes the faraday upper bound. This allows faraday to resolve to 2.14.3. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>

akostadinov-bot Bot dismissed jlledom’s stale review via 05be635 July 3, 2026 16:37

akostadinov previously approved these changes Jul 3, 2026

View reviewed changes

akostadinov requested a review from jlledom July 3, 2026 17:36

jlledom previously approved these changes Jul 6, 2026

View reviewed changes

madnialihussain previously approved these changes Jul 7, 2026

View reviewed changes

akostadinov-bot Bot dismissed stale reviews from madnialihussain, jlledom, and akostadinov via c79e8f3 July 7, 2026 18:26

jlledom requested changes Jul 8, 2026

View reviewed changes

jlledom reviewed Jul 9, 2026

View reviewed changes

jlledom approved these changes Jul 10, 2026

View reviewed changes

akostadinov-bot Bot mentioned this pull request Jul 10, 2026

THREESCALE-15318 - Bump faraday gem [2.15] #4341

Closed

3 tasks

akostadinov merged commit 09b494f into master Jul 15, 2026
13 of 19 checks passed

akostadinov deleted the THREESCALE-15318-update-faraday branch July 15, 2026 18:42

Uh oh!

Conversation

akostadinov-bot Bot commented Jun 30, 2026

Summary

Changes

Dependencies

Testing

Background

Uh oh!

qltysh Bot commented Jun 30, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

akostadinov-bot Bot Jul 1, 2026

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

akostadinov-bot Bot Jul 1, 2026

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

akostadinov-bot Bot Jul 1, 2026

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

akostadinov-bot Bot Jul 1, 2026

Choose a reason for hiding this comment

Uh oh!

akostadinov commented Jun 30, 2026

Uh oh!

akostadinov-bot Bot commented Jun 30, 2026

Uh oh!

akostadinov-bot Bot commented Jul 1, 2026

Uh oh!

jlledom commented Jul 1, 2026

Uh oh!

akostadinov-bot Bot commented Jul 1, 2026

Uh oh!

akostadinov-bot Bot commented Jul 3, 2026

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

akostadinov-bot Bot commented Jul 8, 2026

Bug Fix: connection_pool 3.0 breaks redlock

Root Cause

Fix

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

akostadinov commented Jul 9, 2026

Uh oh!

jlledom commented Jul 10, 2026

Uh oh!

akostadinov commented Jul 10, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

jlledom commented Jul 14, 2026

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

qltysh Bot commented Jun 30, 2026 •

edited

Loading

Bug Fix: `connection_pool` 3.0 breaks redlock

akostadinov commented Jul 10, 2026 •

edited

Loading