Support httprb v6 while keeping compatibility with v4/v5

[vyntheral]

No description provided.

[coveralls]

Coverage Report for CI Build 26630532238

Coverage remained the same at 100.0%

Details

• Coverage remained the same as the base build.
• Patch coverage: 272 of 272 lines across 5 files are fully covered (100%).
• No coverage regressions found.

Uncovered Changes

No uncovered changes found.

Coverage Regressions

No coverage regressions found.

---

Coverage Stats

Relevant Lines:	836
Covered Lines:	836
Line Coverage:	100.0%
Coverage Strength:	59.84 hits per line

---

💛 - Coveralls

[tycooon]

Не мешало бы завести отдельный гемфайл с 6-й версией http и добавить прогон rspec с ним.

[tycooon]

Чет такое себе – мутировать глобально левые классы всякие 🤔

[tycooon]

А где оно юзается?

[tycooon]

Code Review: PR #24 — Support httprb v6 while keeping compatibility with v4/v5

Overview

Adds an [EzClient::HttprbCompatibility](https://claude.ai/epitaxy/lib/ezclient/httprb_compatibility.rb) module that papers over API differences between http gem v4/v5 and v6 (positional-hash vs keyword initialize, removed HTTP::Client#build_request, missing []/[]= on HTTP::Request/HTTP::Response). Refactors [Request#perform_request](https://claude.ai/epitaxy/lib/ezclient/request.rb) to manually carry cookies across redirects on v6, and monkey-patches HTTP::Response.from_webmock + HTTP::Response::Streamer#readpartial in [spec/spec_helper.rb](https://claude.ai/epitaxy/spec/spec_helper.rb) so WebMock works against v6.

Critical

• The v6 code paths are not actually exercised against httprb v6. Gemfile.lock pins http (5.2.0), and the CI matrix only varies Ruby version — no http matrix. The "v6 code path" specs work by stubbing client_supports_build_request? = false and stub_const("HTTP::Request::Builder", …). That tests branch selection inside HttprbCompatibility, not behaviour against real v6. The PR claims compatibility it never validates. Add a second Gemfile (Appraisal or a BUNDLE_GEMFILE matrix entry) that resolves to http >= 6 and run the full suite against it before merging — that's the only way to know the spec_helper monkey patches, the manual cookie handling, and the LegacyHashInitializer actually compose correctly.

• HttprbCompatibility.install! mutates third-party classes at require time. Loading ezclient prepends LegacyHashInitializer onto HTTP::Response, HTTP::Request, HTTP::Redirector and includes LegacyHeaderAccessors onto HTTP::Response/HTTP::Request globally. Any host app or gem touching these classes inherits the changes. For an "HTTP wrapper" gem this is invasive — at minimum, document it; ideally, scope the patches more narrowly or move them behind an explicit opt-in.

Issues

• EzClient::HTTP_CLIENT_SUPPORTS_BUILD_REQUEST at [lib/ezclient.rb:16](lib/ezclient.rb:16) is dead code. Not referenced in lib/ or spec/. Remove it (or use it instead of re-calling HttprbCompatibility.client_supports_build_request?).

• Duplicated header-accessor logic. LegacyHeaderAccessors is included into HTTP::Request at load time, then [define_api_auth_header_accessors](https://claude.ai/epitaxy/lib/ezclient/request.rb:82) re-adds the same []/[]= as singleton methods per request. Its unless request.respond_to?(:[]) guards never fire on a real HTTP::Request because install! already added them — the only consumer of that branch is the contrived spec that mocks respond_to? to return false. Pick one (the class-level include is fine) and delete the other plus its spec.

• Cookie persistence across redirects has fragile edges in [perform_redirects_with_cookies](https://claude.ai/epitaxy/lib/ezclient/request.rb:135):

  • store_response_cookies treats cookie.value == "" as deletion. Real expiry uses Max-Age=0 or a past Expires, and an empty-string value is legitimate for some cookies. HTTP::CookieJar already drops expired cookies — let it do the work and remove the heuristic.
  • store_request_cookies rebuilds cookies from the request Cookie header via HTTP::Cookie.cookie_value_to_hash, hard-coding path: request.uri.path and domain: request.host. Cookies the caller passed with broader scope get narrowed.
  • apply_cookies joins "#{name}=#{value}" without URL-encoding. Prefer cookie_jar.cookies(request.uri) so the jar handles scoping + encoding.

• spec/spec_helper.rb rewrites WebMock and HTTP::Response::Streamer globally for the suite. The Streamer#readpartial patch changes behaviour for all response bodies, not just WebMock-stubbed ones. Acceptable as a stopgap, but worth a tracking link to a WebMock issue so this can be removed later.

• Local options shadows the options instance method in [perform_redirects_with_cookies](lib/ezclient/request.rb:138-141). Rename (redirect_opts) to avoid the trap.

• response_body_requires_eof_error? is misnamed — it's just a "v6 or later" predicate. Rename to httprb_v6_or_later? (and reuse it instead of duplicating the version check anywhere else).

Positive notes

• The split of v5 fast-path vs v6 fallback in [perform_redirects](https://claude.ai/epitaxy/lib/ezclient/request.rb:128) is readable.
• The if options[:params] guard added in [build_request_opts](https://claude.ai/epitaxy/lib/ezclient/request.rb:99) fixes the previous "set opts[:form] = nil" behaviour, which is an improvement under v6's stricter validation.
• Adding basic_auth/cookies to the PersistentClient delegators is the right fix for the persistent-client path that the new spec at [spec/ezclient_spec.rb:242](spec/ezclient_spec.rb:242) covers.

Recommendation

Don't merge until CI actually runs against http >= 6 and that job is green. Then trim the dead constant, the duplicate header-accessor path, and the empty-string cookie heuristic. The shape of the change is right; the verification gap is the blocker.

[tycooon]

Still worth addressing

• request_cookie_values instantiates HTTP::Cookie with hardcoded dummy domain: "example.com", path: "/" just to call .cookie_value ([request.rb:194-198](lib/ezclient/request.rb)). The dummy domain reads like a copy-paste artifact and obscures the intent (URL-encode the value). Either:

  • Drop a comment explaining that the dummy domain/path are required by HTTP::Cookie.new and unused by cookie_value, or
  • Just format with the gem's quoter directly (HTTP::Cookie::Scanner.quote(value) if accessible), or
  • Inline a small helper that produces "#{name}=#{encoded_value}" without the throwaway object.

• expired_cookie_names is a shared mutable array threaded through three helper methods (perform_redirects_with_cookies, store_response_cookies, apply_cookies → cookie_header_for). Hard to follow. Consider wrapping the state in a small RedirectCookieState value object, or computing expired? lazily off the jar instead of side-tracking names.

• Edge case worth a spec: what happens when the user provides a Set-Cookie-format value (semicolons, quoted) in options[:cookies]? The current path round-trips it through HTTP::Cookie.cookie_value_to_hash → HTTP::Cookie.new → cookie_value. Probably fine, but no test exercises pathological cookie values.