8277: Add a transport-agnostic HTTP test client and consolidate test HTTP construction - #8302
Open
codeforgreen wants to merge 15 commits into
Open
8277: Add a transport-agnostic HTTP test client and consolidate test HTTP construction#8302codeforgreen wants to merge 15 commits into
codeforgreen wants to merge 15 commits into
Conversation
…grate into RestfulServerExtension. Migrate some of the existing tests to this new test API. [Co-Authored with Claude Sonnet 5]
[Co-Authored with Claude Opus-5 and Claude Sonnet-5]
…directs Three gaps blocked migrating tests onto the new HTTP test framework: - HttpTestResponse held the body as a String, so any binary payload (Binary resources, images, gzipped NDJSON) was silently lossy. The body is now captured as byte[], with getBodyBytes() for binary and a UTF-8-decoding getBody() for the textual case. The String constructor is kept for callers whose body is known to be text. - Neither ParsedHttpResponse's contentType() nor CDR's getResponseContentType() had an equivalent here, so neither could be retired. Added contentType(), which strips Content-Type parameters and lower-cases the result. - Redirect behaviour was inherited from whichever client a test happened to hold, and the two ecosystems disagree: hapi's HttpClientExtension follows redirects, CDR's SmileTestHttpClient does not. Added a followRedirects field to IHttpTestTransport.Request, set via HttpTestRequest.followRedirects(boolean) or withoutRedirects(), and applied by both transports, so a test that cares can say so rather than inherit it. Also gave HttpClientExtension and BaseRestServerHelper HttpTestRequest entry points. HttpClientExtension's raw getClient()/execute() are documented as the older path but deliberately not annotated @deprecated: 127 callers remain, so the annotation would emit 127 warnings now and buy nothing. It belongs with the call-site migration. Verified: 167 tests green in hapi-fhir-test-utilities. The 4 new contract cases run against both the 4.x and 5.x transports (32 total), so the two cannot drift on the new behaviour. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
…sion.request hapi-fhir-test-utilities built its Apache 4.x test client in two places. They were not the same recipe: HttpClientExtension sized the pool to 99 connections and set a 30s socket timeout, while BaseJettyServerExtension set only the 5s connection TTL and so inherited HttpClient's default of two connections per route. Any test making concurrent calls through RestfulServerExtension was therefore queueing on itself. Both now delegate to a new TestHttpClientFactory, which carries the fuller config. Beyond removing the duplication this lifts the per-route limit for every RestfulServerExtension consumer, so a test that was accidentally serialised by the old limit may now genuinely run in parallel. Also added HttpServletExtension.request(path) — the non-FHIR counterpart to RestfulServerExtension.fhirRequest(path), for the servlet tests that assert on raw HTTP rather than on a parsed resource. Verified: 167 tests green in hapi-fhir-test-utilities, and hapi-fhir-structures-r4 test-compiles against the change (it is the heaviest RestfulServerExtension consumer). Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
These were near-exact ancestors of HttpTestResponse: the same eager capture of status, reason, headers and body, and the same HTTP-wire-format toString. The only thing they had that HttpTestResponse lacked was contentType(), which landed in the previous commit; statusReason() was already covered by getReasonPhrase(). They had a single consumer between them, so BulkPatchProviderTest moves in the same commit — four call sites onto ourFhirServer.fhirRequest(...). Its contentType() assertions carry over unchanged, which is what demonstrates the new method matches the one being deleted. The rest of that file's raw-Apache call sites are deliberately untouched; those belong to the call-site migration, not to the foundation. Verified: 21 tests green in BulkPatchProviderTest. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Each of the BaseResourceProvider*Test bases already holds a
RestfulServerExtension, which gained fhirRequest(path) with the framework. This
adds a protected delegate on each base so the 157 subclasses can call
fhirRequest("/Patient") directly rather than myServer.fhirRequest(...).
Only the entry point moves here. The subclasses keep using ourHttpClient until
they are migrated, which is call-site work rather than foundation.
Verified: all five modules test-compile, as does hapi-fhir-jpaserver-test-r4,
which holds the 95 subclasses of the R4 base.
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
hapi-fhir-jpaserver-test-utilities carried two abstract classes with the same name: the live ca.uhn.fhir.jpa.provider.BaseResourceProviderR4Test, and a legacy ca.uhn.fhir.jpa.provider.r4 one that hand-rolled its own Jetty server and HTTP client. Nothing imported the legacy class. It was reachable only by same-package binding, and exactly one test resolved to it that way: ComboUniqueSearchParameterDateOffsetR4Test, which sits in ca.uhn.fhir.jpa.provider.r4 and extends the name without an import. That is a trap — the class a test inherits from depends on which package it happens to be declared in. Added the explicit import so that test binds to the live base, then deleted the legacy class. Verified: ComboUniqueSearchParameterDateOffsetR4Test passes 5/5 against the live base, so the two were behaviourally equivalent for its purposes. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
codeforgreen
requested review from
jamesagnew,
michaelabuckley and
tadgh
as code owners
August 26, 2026 21:25
Collaborator
|
Formatting check succeeded! |
codeforgreen
changed the title
codeforgreen
changed the title
…ssy binary asserts Addresses GH-8277-code-review-1 findings 1, 3, and 4 (finding 2 is documented as an accepted, unavoidable limitation rather than fixed — see below). - followRedirects(true) was a silent no-op against a client built with redirects disabled (e.g. via HttpClientExtension.dontFollowRedirects() or CDR's SmileTestHttpClient), because Apache HttpClient decides redirect handling when the client is built, not per request, and no request-level config can override that. Verified empirically against a live server before and after. Both transports now detect the mismatch — a 3xx that a normal redirect strategy would have followed, returned despite followRedirects(true) being requested — and throw IllegalStateException instead of silently handing back the unfollowed response. - The related finding (setting followRedirects at all replaces the request's RequestConfig wholesale, discarding any of the client's own default timeouts/ compression/cookie settings) has no fix: Apache HttpClient exposes no public way to read a client's configured defaults back, so there is nothing to merge onto. Documented explicitly on HttpTestRequest.followRedirects() rather than left as a silent surprise. - BaseRestServerHelper.fhirRequest() built its own client via HttpClientBuilder.create().build() in the same PR that collapsed every other such duplicate onto TestHttpClientFactory. Now uses it too. - Four migrated binary-response assertions did status.getBody().getBytes(UTF_8)).containsExactly(...), the exact String-round-trip lossiness getBodyBytes() was added to this PR to fix. They passed only because their fixtures are ASCII-safe. Switched to getBodyBytes(). Verified: 167 tests green in hapi-fhir-test-utilities (including two new redirect-behavior cases proving both directions), 84 green across OperationServerR4Test and ResponseHighlighterInterceptorTest, hapi-fhir-jpaserver-test-r4 test-compiles. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
Msg.code() is a production convention; ApacheHttp4TestTransport and ApacheHttp5TestTransport are test infrastructure (ca.uhn.fhir.test.utilities), never invoked by a running production system, despite living in src/main so other modules' tests can depend on them. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
…t references
Two related documentation changes across the six new test-infrastructure
classes.
Trimmed the Javadoc. It had grown to explain the reasoning behind each design
choice at length, which buried what a caller actually needs to know. Kept the
load-bearing facts — the redirect limitations, why the body is bytes, why the
response is fully buffered — and cut the narration around them. Net 43 lines
lighter with nothing material lost.
Removed references to a specific downstream product from hapi-fhir code. Three
Javadoc comments named a particular consumer and its client class to illustrate
that clients disagree on redirect defaults. That point stands on its own, so
they now say so generically.
Also sharpened the IHttpTestTransport class comment to state plainly that it is
test infrastructure and not the production IHttpClient SPI beneath
IGenericClient. The distinction was already noted, but was easy to miss and the
two are genuinely easy to confuse.
Verified: 167 tests green in hapi-fhir-test-utilities, and every {@link} target
in these six files resolves under javadoc:javadoc. That goal still fails overall
on a pre-existing malformed entity in StringToIntegerListArgumentConverter,
which this branch does not touch.
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
The redirect control added in an earlier commit had no callers anywhere. Nothing in hapi-fhir or in the downstream consumer used followRedirects/withoutRedirects except the two tests written to exercise them, so it was speculative API — and one direction of it could not work at all. Apache HttpClient fixes redirect handling when the client is built, so followRedirects(true) against a client built with redirects disabled was inherently a no-op. That drove an IllegalStateException guard, an isRedirectStatus helper duplicated across both transports, and most of the Javadoc caveats on the feature. Removed followRedirects(boolean) and kept withoutRedirects(), which is the direction that works and the one a test actually wants — assert on a 302 and its Location header rather than follow it. The tri-state Boolean on IHttpTestTransport.Request becomes a plain boolean disableRedirects. The guard and its helper are gone from both transports, along with ~40 lines of code and the caveat explaining why half the API could not be trusted. Also closed the coverage gaps this exposed. Core logic in the module now sits at 100% lines for HttpTestRequest, HttpTestResponse and TestHttpClientFactory, and 93%/92% for the two transports, up from 86%: - TestHttpClientFactoryTest: the redirect choice at client-build time, including create(false), which was entirely untested despite being one half of the redirect story - withUtf8Charset: both branches, so a caller-supplied charset is no longer silently assumed to work - The HttpClient 5.x to(...) overloads, which no test called - HEAD and both put(...) overloads in the transport contract What remains uncovered is the catch(IOException) path in each transport and the null-body branch of one convenience constructor. Verified: 180 tests green in hapi-fhir-test-utilities, up from 167. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
codeforgreen
changed the title
…test bases HttpTestHeader becomes a top-level record instead of HttpTestResponse.HeaderEntry, so the request half of the API no longer borrows a type from the response half. withoutRedirects() copies the client's default RequestConfig before overriding redirects. A request-level RequestConfig replaces the client default rather than merging with it, so the previous code silently dropped timeouts, cookie spec, compression and proxy settings for that request. Both the 4.x and 5.x transports now do this, falling back to RequestConfig.DEFAULT for a client that is not Configurable. TestHttpClientFactory takes the socket timeout as a parameter, with NO_SOCKET_TIMEOUT for callers whose tests drive slow endpoints; BaseJettyServerExtension passes it to keep the unbounded read it had before it moved onto the factory. BaseRestServerHelper.stop() and BaseJettyServerExtension.stopServer() release the server and the client even when the other one throws, so a failed close no longer strands a port for the rest of the JVM. HttpTestResponse replaces its String-bodied constructor with fromText() -- the byte[] and String overloads were ambiguous for a null body -- and renames contentType() to getContentType(). The identical fhirRequest() override is removed from the five BaseResourceProvider*Test bases; RestfulServerExtension.fhirRequest() is inherited directly. The two BaseResourceProviderR4Test classes collapse into the ca.uhn.fhir.jpa.provider one, with a deprecated subclass left at the old ca.uhn.fhir.jpa.provider.r4 name for one release. New coverage: BaseRestServerHelperTest, HttpClientExtensionTest and its EchoServlet; FhirHttpTransportContractTest becomes HttpTestTransportContractTest. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Adds a transport-agnostic HTTP test client and uses it to replace the ad-hoc raw-Apache-HttpClient idiom used across thousands of test call sites in hapi-fhir. This is the foundation layer — mass call-site migration across the rest of the codebase follows separately.
What this PR adds
A transport-agnostic request/response API
HttpTestRequest— a fluent request builder naming no HTTP client library (withHeader,withBasicAuth,withPreferHandling,get/post/put/patch/delete/options/head).HttpTestResponse— an eagerly-buffered response withassertStatus(int),getBody(),getHeader(name),getHeaders(name),getAllHeaders(), and an HTTP-wire-formattoString(). The body is stored asbyte[]withgetBodyBytes()alongside a UTF-8-decodinggetBody(), so binary payloads (Binary resources, images, gzipped NDJSON) survive intact rather than being silently corrupted by a String round-trip.contentType()— Content-Type minus parameters, lower-cased.withoutRedirects()— returns a3xxas-is instead of following it, so a test can assert on the status andLocationheader. (A per-requestfollowRedirects(true)existed briefly but was removed: Apache HttpClient fixes redirect handling when the client is built, so it could never override a client built with redirects disabled, and nothing called it anyway.)IHttpTestTransportplusApacheHttp4TestTransport/ApacheHttp5TestTransport— the seam that lets one API serve both Apache HttpClient majors.FhirHttpTransportContractTestruns every case against both transports, so they cannot drift.Entry points
RestfulServerExtension.fhirRequest(path),HttpServletExtension.request(path),BaseRestServerHelper.fhirRequest(path), andfhirRequest(path)on the fiveBaseResourceProvider*Testbases (reaching 157 subclasses). Subclasses themselves are not migrated in this PR.Duplicate client construction removed
HttpClientExtensionandBaseJettyServerExtensioneach built the 4.x client separately. Both now use a newTestHttpClientFactory. Worth knowing: the two recipes were not the same —BaseJettyServerExtensionset no pool sizing, so it inherited HttpClient's default of 2 connections per route. EveryRestfulServerExtensionconsumer now gets 99. Tests that were accidentally serialised by the old limit may now genuinely run in parallel.Dead and superseded code removed
FhirHttpRequest/FhirHttpResponse(renamed toHttpTestRequest/HttpTestResponse) andParsedHttpResponse/CloseableHttpResponseUtil(ancestors ofHttpTestResponse), migrating their one consumer,BulkPatchProviderTest.ca.uhn.fhir.jpa.provider.r4.BaseResourceProviderR4Test. It shared a name with the live class one package up and was reachable only by same-package binding — exactly one test resolved to it that way.Migrated onto the new API:
OperationServerDstu2Test,OperationServerDstu3Test,OperationServerR4Test,ResponseHighlighterInterceptorTest,RequestValidatingInterceptorDstu3Test/R4Test,ResponseValidatingInterceptorDstu3Test/R4Test,AuthorizationInterceptorWriteResponseJpaR4Test,BulkPatchProviderTest.For the reviewer
The commits closing the client's own gaps and collapsing the client-construction recipes are the ones worth close attention; the migrated tests are mechanical proof the API works at scale.
Verified
hapi-fhir-test-utilities, including 100% line and branch coverage onHttpTestRequestandTestHttpClientFactory, 100%/90% onHttpTestResponse, and 93%/88% and 92%/88% on the two transportsBulkPatchProviderTest; 5 green in the reboundComboUniqueSearchParameterDateOffsetR4Testhapi-fhir-structures-r4,hapi-fhir-jpaserver-test-r4and the five JPA test modules all test-compile🤖 Generated with Claude Code