Skip to content

repository_ctx: avoid duplicate Accept headers when user specifies one #28455

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Open
ijuren8 wants to merge 2 commits into
base: master
Choose a base branch
from
+32 −2
Open

repository_ctx: avoid duplicate Accept headers when user specifies one #28455

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
1fe716e
Extract `Accept` header handling into a reusable method and apply it …
ijuren8 Jan 27, 2026
eeb2cd5
Add test for handling `Accept` header overrides in HTTP requests
ijuren8 Jan 28, 2026
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
6 changes: 6 additions & 0 deletions src/main/java/com/google/devtools/build/lib/remote/http/AbstractHttpHandler.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -92,6 +92,12 @@ protected void addUserAgentHeader(HttpRequest request) {
request.headers().set(HttpHeaderNames.USER_AGENT, USER_AGENT_VALUE);
}

protected void addAcceptHeaders(HttpRequest request) {
if (request.headers().get(HttpHeaderNames.ACCEPT) == null) {
request.headers().add(HttpHeaderNames.ACCEPT, "*/*");
}
Comment on lines +96 to +98
Copy link

@gemini-code-assist gemini-code-assist bot Jan 27, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

high

The new addAcceptHeaders method changes the behavior from unconditionally setting the Accept header (using set) to conditionally adding it only if it's not already present. While this doesn't alter functionality for the current call sites, it introduces a subtle semantic change for a protected method. To maintain the original, simpler semantics and avoid potential confusion for future developers using this method, it's recommended to use set unconditionally. This makes the method's behavior more explicit: it ensures the Accept header is set to */*.

    request.headers().set(HttpHeaderNames.ACCEPT, "*/*");

Copy link
Author

@ijuren8 ijuren8 Jan 27, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Regarding set vs add: in this context both behave the same due to the surrounding if check, so the choice is largely stylistic. The method conditionally adds a default header, and using add matches the existing add* methods in the request-building chain.

Using set here could also be read as enforcing a value, even though the guard already prevents overwriting. A conditional setAccept(...) would feel a bit odd, whereas an add* method that conditionally calls .add() aligns better with the intent and naming.

That said, I’m happy to switch to set if that’s preferred for consistency or readability.

}

protected String constructPath(URI uri, String hash, boolean isCas) {
StringBuilder builder = new StringBuilder();
builder.append(uri.getPath());
Expand Down
2 changes: 1 addition & 1 deletion src/main/java/com/google/devtools/build/lib/remote/http/HttpDownloadHandler.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -163,6 +163,7 @@ public void write(ChannelHandlerContext ctx, Object msg, ChannelPromise promise)
addCredentialHeaders(request, cmd.uri());
addExtraRemoteHeaders(request);
addUserAgentHeader(request);
addAcceptHeaders(request);
ctx.writeAndFlush(request)
.addListener(
(f) -> {
Expand All @@ -186,7 +187,6 @@ private HttpRequest buildRequest(String path, String host) {
new DefaultFullHttpRequest(HttpVersion.HTTP_1_1, HttpMethod.GET, path);
httpRequest.headers().set(HttpHeaderNames.HOST, host);
httpRequest.headers().set(HttpHeaderNames.CONNECTION, HttpHeaderValues.KEEP_ALIVE);
httpRequest.headers().set(HttpHeaderNames.ACCEPT, "*/*");
httpRequest.headers().set(HttpHeaderNames.ACCEPT_ENCODING, ACCEPT_ENCODING);
return httpRequest;
}
Expand Down
2 changes: 1 addition & 1 deletion src/main/java/com/google/devtools/build/lib/remote/http/HttpUploadHandler.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -102,6 +102,7 @@ public void write(ChannelHandlerContext ctx, Object msg, ChannelPromise promise)
addCredentialHeaders(request, cmd.uri());
addExtraRemoteHeaders(request);
addUserAgentHeader(request);
addAcceptHeaders(request);
HttpChunkedInput body = buildBody(cmd);
ctx.writeAndFlush(request)
.addListener(
Expand Down Expand Up @@ -136,7 +137,6 @@ public void exceptionCaught(ChannelHandlerContext ctx, Throwable t) {
private HttpRequest buildRequest(String path, String host, long contentLength) {
HttpRequest request = new DefaultHttpRequest(HttpVersion.HTTP_1_1, HttpMethod.PUT, path);
request.headers().set(HttpHeaderNames.HOST, host);
request.headers().set(HttpHeaderNames.ACCEPT, "*/*");
request.headers().set(HttpHeaderNames.CONTENT_LENGTH, contentLength);
request.headers().set(HttpHeaderNames.CONNECTION, HttpHeaderValues.KEEP_ALIVE);
return request;
Expand Down
24 changes: 24 additions & 0 deletions src/test/java/com/google/devtools/build/lib/remote/http/AbstractHttpHandlerTest.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -134,6 +134,30 @@ public void extraHeadersAreIncluded() throws Exception {
HttpRequest request = ch.readOutbound();
assertThat(request.headers().get("key1")).isEqualTo("value1");
assertThat(request.headers().get("key2")).isEqualTo("value2");
assertThat(request.headers().get(HttpHeaderNames.ACCEPT)).isEqualTo("*/*");
}

@Test
public void extraHeadersOverridesDefaultAccept() throws Exception {
URI uri = new URI("http://does.not.exist:8080/foo");
ImmutableList<Entry<String, String>> remoteHeaders =
ImmutableList.of(
Maps.immutableEntry("key1", "value1"),
Maps.immutableEntry("key2", "value2"),
Maps.immutableEntry("Accept", "application/octet-stream")
);

EmbeddedChannel ch =
new EmbeddedChannel(new HttpDownloadHandler(/* credentials= */ null, remoteHeaders));
DownloadCommand cmd =
new DownloadCommand(uri, /* casDownload= */ true, DIGEST, new ByteArrayOutputStream());
ChannelPromise writePromise = ch.newPromise();
ch.writeOneOutbound(cmd, writePromise);

HttpRequest request = ch.readOutbound();
assertThat(request.headers().get("key1")).isEqualTo("value1");
assertThat(request.headers().get("key2")).isEqualTo("value2");
assertThat(request.headers().get(HttpHeaderNames.ACCEPT)).isEqualTo("application/octet-stream");
}

@Test
Expand Down