Skip to content

Commit 8a12d24

Browse files
committed
fix(test): close three latent OkHttp-port gaps flagged in review
- OBPReq.toOkHttpRequest: dedupe headers by name (case-insensitive) before sending, keeping the last value. reqHeaders accumulated via <:</addHeader without removing same-named entries, so a caller-set header plus a helper default (e.g. makePostRequest's Content-Type) would be sent twice on the wire instead of the helper default overriding, as the pre-port Map-based header merge did. - OBPReq./ path-segment encoding: percent-encode '%' itself (first, before the other reserved characters). Without it, a literal '%' in a test id/label either makes HttpUrl.parse reject the segment as an invalid escape, or — if it happens to look like a valid escape (e.g. "%2F") — gets silently decoded server-side into the wrong resource. - SendServerRequests.getAPIResponse: restore the retry-once-on-IOException the old dispatch-based client had for a corrupted pooled connection (one test's error response polluting a connection OkHttp's own RetryAndFollowUpInterceptor won't recover from). Broadened from the old code's single string-matched exception to java.io.IOException generally, since OkHttp surfaces this as various IOException subtypes rather than one specific message. All three are additive/defensive — none change behaviour for any currently-passing test. Verified with two full local runs (run_tests_parallel.sh, JDK 25): 2921 tests, 0 failures, 0 errors.
1 parent 34ea57b commit 8a12d24

2 files changed

Lines changed: 33 additions & 4 deletions

File tree

obp-api/src/test/scala/code/setup/OBPReq.scala

Lines changed: 19 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -25,8 +25,12 @@ case class OBPReq(
2525
// Percent-encode characters that must not appear unencoded in a URI path segment.
2626
// '/' is the path delimiter — encoding it prevents a URL-valued provider string
2727
// (e.g. "http://localhost:8016") from being split into multiple path segments.
28-
// This replicates dispatch's addPathPart percent-encoding behaviour.
29-
val encodedSeg = seg.replace("/", "%2F").replace("?", "%3F").replace("#", "%23")
28+
// '%' must be encoded FIRST — otherwise a literal '%' in a test id/label (e.g. a
29+
// view name "50%") would either make HttpUrl.parse reject the segment as an invalid
30+
// escape, or — if it happens to look like a valid escape (e.g. "%2F") — get silently
31+
// decoded server-side into the character it "escapes", resolving to the wrong
32+
// resource. This replicates dispatch's addPathPart percent-encoding behaviour.
33+
val encodedSeg = seg.replace("%", "%25").replace("/", "%2F").replace("?", "%3F").replace("#", "%23")
3034
copy(baseUrl = s"$cleanBase/$encodedSeg")
3135
}
3236

@@ -87,7 +91,19 @@ case class OBPReq(
8791
.url(urlBuilder.build())
8892
.method(method.toUpperCase, requestBody)
8993

90-
reqHeaders.foreach { case (k, v) => builder.addHeader(k, v) }
94+
// Dedupe by header name (HTTP header names are case-insensitive), keeping the LAST
95+
// value for a given name. reqHeaders accumulates via <:</addHeader without removing
96+
// same-named entries, so e.g. makePostRequest's own Content-Type default appended
97+
// after a caller-set Content-Type would otherwise be sent twice on the wire. "Last
98+
// wins" matches the Content-Type lookup above (reqHeaders.toMap already takes the
99+
// last occurrence) and restores the override semantics the old Map-based header
100+
// merge had before the OkHttp port.
101+
val dedupedHeaders = {
102+
val seen = scala.collection.mutable.LinkedHashMap[String, (String, String)]()
103+
reqHeaders.foreach { case (k, v) => seen(k.toLowerCase) = (k, v) }
104+
seen.values
105+
}
106+
dedupedHeaders.foreach { case (k, v) => builder.addHeader(k, v) }
91107
builder.build()
92108
}
93109
}

obp-api/src/test/scala/code/setup/SendServerRequests.scala

Lines changed: 14 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -116,7 +116,20 @@ trait SendServerRequests {
116116
}
117117
}.toOption
118118

119-
private def getAPIResponse(req: OBPReq): APIResponse = executeRequest(req)
119+
private def getAPIResponse(req: OBPReq): APIResponse =
120+
try {
121+
executeRequest(req)
122+
} catch {
123+
case _: java.io.IOException =>
124+
// Concurrent shards/tests share OBPReq.client's connection pool; one test's error
125+
// response can corrupt a pooled connection, surfacing as a broken status line on
126+
// the next request that reuses it. OkHttp does not retry this itself
127+
// (RetryAndFollowUpInterceptor.recover() refuses to recover a ProtocolException).
128+
// Retry once with a fresh connection after a brief delay — the same recovery the
129+
// old dispatch-based client had for the same "invalid version format" symptom.
130+
Thread.sleep(100)
131+
executeRequest(req)
132+
}
120133

121134
private def getAPIResponseAsync(req: OBPReq): Future[APIResponse] =
122135
Future { scala.concurrent.blocking { getAPIResponse(req) } }(ExecutionContext.global)

0 commit comments

Comments
 (0)