feat: log prepared HTTP request URLs through observers - #997
Conversation
Castiron custom code✅ No new custom-code files detected. 70 mixed files remain; 0 existing customizations changed. Compared 70 existing customizations unchanged
30 more in the full report. A changed generated baseline means this report cannot reliably identify which handwritten lines changed. Inspect the custom-code diffDownload the exact patch produced by this run (requires repository access): gh run download 34629845008 --repo openai/openai-java \
--name castiron-custom-code-34629845008-1 --dir /tmp/castiron-custom-code-34629845008-1
git apply --stat /tmp/castiron-custom-code-34629845008-1/custom-code.patch
cat /tmp/castiron-custom-code-34629845008-1/custom-code.patchOr reproduce it from an SDK checkout containing the vendored reporter: git fetch --no-tags origin 41bdd89bce7d88c14ce09f891b11bb7eb61ba534 eae6187e5414474dfe3c745ab426eee73467a345
python3 scripts/castiron/custom_code_report.py report \
--base 41bdd89bce7d88c14ce09f891b11bb7eb61ba534 \
--head eae6187e5414474dfe3c745ab426eee73467a345 --fetch --require-head-hash --public \
--out /tmp/castiron-custom-code-eae6187e5414
cat /tmp/castiron-custom-code-eae6187e5414/custom-code.patchThis is the current full custom patch for mixed files, not an attribution of only the handwritten lines changed by this PR. |
jbeckwith-oai
left a comment
jbeckwith-oai
left a comment
There was a problem hiding this comment.
Requesting changes for one confirmed behavioral compatibility regression: recompiling an existing Kotlin HttpClient by delegate wrapper can bypass its original execution overrides. Details and validation are in the inline comment.
Review conclusions:
- API contract:
HttpRequest.url()and default OkHttp wire construction remain unchanged, but the delegated-wrapper regression must be fixed. Legacy custom clients also intentionally lose URL logging in favor of<URL unavailable>; that remains an explicit compatibility tradeoff. - Generator ownership: None of the 16 changed files belongs to the verified generated snapshot
518b0539b6c7380ca65ceaedae379187c4545226. Generation metadata is unchanged. The custom-code budget passes at 2,157 / 3,000, unchanged from base. - Support matrix: No JVM/compiler floors or runtime dependencies change. Java 8 remains supported; exact-head CI passes its Java 8/25 runtime checks and build/Jackson/API checks. Those checks do not cover this delegated-wrapper behavior.
- API design: Reading the prepared URL from the transport is the right boundary and keeps core independent of OkHttp. Observation should be an explicit opt-in capability that preserves existing wrapper dispatch. A separate optional interface is one possible design; please agree on that API shape before expanding the implementation.
Validation: two independent read-only reviewers identified the same blocker; a separate offline counting-wrapper fixture confirmed it on Java 8 and 25, including against the complete Gradle-built PR JAR. Focused core HTTP and OkHttp tests passed locally (509 passed, 1 skipped), along with Kotlin lint, git diff --check, and the custom-code budget check. Security-focused inspection found no additional supported blocker.
Reviewed head eae6187e5414474dfe3c745ab426eee73467a345 against base 41bdd89bce7d88c14ce09f891b11bb7eb61ba534. No source changes were made.
| httpClient.execute( | ||
| loggingRequest, | ||
| requestOptions, | ||
| loggingObserver(request, observer), | ||
| ) |
There was a problem hiding this comment.
[P1] Preserve existing Kotlin delegated wrapper overrides
Calling the new three-argument overload here (and in executeAsync below) silently changes existing consumers using class Wrapper(private val delegate: HttpClient) : HttpClient by delegate with overrides of the original two-argument execute / executeAsync methods. After recompilation, Kotlin generates forwarding methods for the new overloads that call the delegate directly, skipping the wrapper's existing request customization, accounting, or lifecycle logic. This happens even at LogLevel.OFF because this dispatch is unconditional.
The defaults in HttpClient protect ordinary implementations and old binaries, but cannot protect against those newly generated forwarding methods. The existing legacy-client tests use explicit implementations and miss this case. See Kotlin's inheritance delegation semantics.
Confirmed with an offline counting wrapper on both Java 8 and Java 25, including the full Gradle-built PR JAR:
| Scenario | Sync override calls | Async override calls |
|---|---|---|
| Before PR | 1 | 1 |
| Old consumer binary running with PR | 1 | 1 |
| Same consumer source recompiled against PR | 0 | 0 |
Please preserve the original execution path unless a wrapper explicitly opts into observation, and add sync/async delegated-consumer regression coverage with logging off and enabled. A separate optional observation-capable interface is one possible remedy.
2 participants
Summary
Issue #886 reports that
HttpRequest.url()logs a path space as+even though OkHttp sends%20. The issue proposes changingHttpRequest.url(). This PR fixes the log by reading the default transport's prepared URL; it does not changeHttpRequest.url()or resolve that separate public-method request. PR #887 covers the narrower path-space change.RequestObserversupplies the prepared method and URL once per attempt, before dispatch, for sync and async calls. The SDK's retry, authentication, lifecycle, workload identity, and X.509 wrappers forward it. Core gains no OkHttp dependency. The reported URL describes the initial native request; later interceptors and redirects may change it.Differences covered
For new path segments, the complete measured character-level difference set is space (
+in core,%20in OkHttp) and these 15 characters (percent-escaped in core, literal in OkHttp):!,$,&,',(,),+,,,:,;,=,@,[,],~. For new query names and values, only space differs (+versus%20); both encode a literal+as%2B.The fixture also covers exact
./..segments, existing base queries and fragments, empty paths, scheme/host/IDN/IP/port normalization, authority separators and backslashes, raw base spaces/Unicode/controls and other unsafe characters, userinfo, and invalid URL rejection. It includes matching cases such as added Unicode, raw%, slash/backslash segments, and repeated query values. Core appends to the base string usingURLEncoder; OkHttp parses and builds a native URL. These differences can change the request target, not just its spelling.Before and after
For example:
With
LoggingHttpClientat INFO around the default OkHttp transport, the examples below show the request log's URL and the initial HTTP path/query target. Targets exclude fragments. Sending behavior does not change.Before this PR
https://example.comunless shown)/v1, segmenta bhttps://example.com/v1/a+b/v1/a%20b/v1, segmenta+bhttps://example.com/v1/a%2Bb/v1/a+b/v1, querya b=c dhttps://example.com/v1?a+b=c+d/v1?a%20b=c%20d/v1, segmentsa,..,bhttps://example.com/v1/a/../b/v1/b/v1?x=1, segmentusershttps://example.com/v1?x=1/users/v1/users?x=1/v1#section, segmentusershttps://example.com/v1#section/users/v1/usershttps://example.comhttps://example.com/HTTPS://EXAMPLE.COM:443/v1HTTPS://EXAMPLE.COM:443/v1/v1https://example.com:65536/v1After this PR
/v1, segmenta bhttps://example.com/v1/a%20b/v1/a%20b/v1, segmenta+bhttps://example.com/v1/a+b/v1/a+b/v1, querya b=c dhttps://example.com/v1?a%20b=c%20d/v1?a%20b=c%20d/v1, segmentsa,..,bhttps://example.com/v1/b/v1/b/v1?x=1, segmentusershttps://example.com/v1/users?x=1/v1/users?x=1/v1#section, segmentusershttps://example.com/v1/users#section/v1/usershttps://example.comhttps://example.com//HTTPS://EXAMPLE.COM:443/v1https://example.com/v1/v1https://example.com:65536/v1The full prepared URL can contain a fragment; the sent target cannot.
HttpRequest.url()still returns the core-rendered form shown in the first table.Custom HTTP clients
Existing custom
HttpClientimplementations continue to compile and send through their original methods. Without observer support, logging now shows--> GET <URL unavailable>instead of a potentially wrong URL, plus this guidance once per logging client:Implement and forward RequestObserver in execute and executeAsync to enable URL logging.The same applies to an old wrapper around OkHttp: its sent target is unchanged, but the wrapper's default observer overload reports no URL.To restore URL logging, a wrapper forwards the observer in both new overloads (shown here inside an existing
HttpClientimplementation):A custom transport instead reports its own prepared method and URL with
observer.onRequestStart(method, url)before dispatch, on each attempt. An override that drops the callback cannot provide a request log line. Async callbacks may run on another thread. SDK transports ignore observer runtime exceptions so observation cannot prevent dispatch.Verification and release
HttpRequest.url()tests retain and pass their current expectations.SKIP_MOCK_TESTS=true; the external OpenAPI mock server tests were skipped. The generated-code budget passes at 2,157 / 3,000 lines, unchanged from baseline.feat:). Please include security-focused review of URL/path and transport handling; tests use synthetic inputs. Existing opt-in DEBUG body logs can contain sensitive data.