oscerd commented on code in PR #143:
URL: https://github.com/apache/camel-kit/pull/143#discussion_r4441957532
##########
knowledge/mcp/src/main/java/org/apache/camel/kit/knowledge/mcp/IndexDownloader.java:
##########
@@ -568,9 +603,41 @@ private InputStream openStream(URI uri) throws
IOException, InterruptedException
}
private HttpResponse<InputStream> send(HttpRequest request) throws
IOException, InterruptedException {
+ final int maxRedirects = 5;
long deadline = System.nanoTime() +
request.timeout().orElseThrow().toNanos();
- return httpClient.send(request, info ->
HttpResponse.BodySubscribers.mapping(
- HttpResponse.BodySubscribers.ofPublisher(), publisher -> new
TimedBody(publisher, deadline)));
+ HttpRequest current = request;
+ for (int hop = 0;; hop++) {
+ HttpResponse<InputStream> response = httpClient.send(current, info
-> HttpResponse.BodySubscribers.mapping(
+ HttpResponse.BodySubscribers.ofPublisher(), publisher ->
new TimedBody(publisher, deadline)));
+ int status = response.statusCode();
+ if (status != 301 && status != 302 && status != 303 && status !=
307 && status != 308) {
+ return response;
+ }
+ // Validate the redirect target's transport BEFORE following it; a
304 is not a redirect and
+ // is returned above for the conditional-manifest path.
+ String target =
response.headers().firstValue("Location").orElse(null);
+ response.body().close();
+ if (hop >= maxRedirects) {
+ throw new IOException("Too many index redirects starting from
" + request.uri());
+ }
+ if (target == null) {
+ throw new IOException("Index redirect without a Location
header from " + current.uri());
+ }
+ URI next;
+ try {
+ next = current.uri().resolve(target);
+ } catch (IllegalArgumentException e) {
+ throw new IOException("Invalid index redirect target: " +
target, e);
+ }
+ requireFetchableUri(next);
+ long remaining = deadline - System.nanoTime();
+ if (remaining <= 0) {
+ throw new HttpTimeoutException("Index redirect handling timed
out from " + request.uri());
+ }
+ HttpRequest.Builder builder =
HttpRequest.newBuilder(next).timeout(Duration.ofNanos(remaining)).GET();
+ current.headers().firstValue("Accept").ifPresent(accept ->
builder.header("Accept", accept));
+ current = builder.build();
Review Comment:
Good catch — the manual redirect loop in 182dcc9 rebuilt the hop request
with `Accept` only, so the `If-None-Match` that `fetchManifest()` sets was lost
on the first hop and every redirected resolution re-downloaded. Fixed in
b1ac1242: `send()` now re-sends `If-None-Match` together with `Accept` on every
validated hop — an explicit list of the two request headers the downloader
sets; nothing else crosses a hop.
Regression test `manifestRedirectKeepsConditionalRevalidation`: a loopback
redirector 302s every path to the `HttpFixture` backend; the first resolve
installs v1 (200, `ETag: "v1"`); the second resolve must reach the target with
`If-None-Match: "v1"`, receive 304, reuse the installed directory and leave the
download count at 1. It fails on 182dcc9 (`expected: <"v1"> but was: <null>`)
and passes with the fix.
`IndexDownloaderTest` 68 and `IndexDownloaderTimeoutTest` 10 pass
(knowledge/mcp: 112 passed, 4 existing skips); `-Psourcecheck validate` clean;
full reactor `clean install -DskipTests` green.
_AI-generated by Claude Code (Claude Fable 5.1) on behalf of @oscerd._
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
