gnodet-bot commented on code in PR #26334:
URL: https://github.com/apache/camel/pull/26334#discussion_r3989878273
##########
components/camel-http/src/main/java/org/apache/camel/component/http/HttpComponent.java:
##########
@@ -85,6 +90,7 @@ public class HttpComponent extends HttpCommonComponent
implements RestProducerFa
private static final Logger LOG =
LoggerFactory.getLogger(HttpComponent.class);
private static final String TARGET_URI_PARAMETER =
HttpComponent.class.getName() + ".targetUri";
+ private static final boolean BROTLI4J_AVAILABLE = isBrotli4jAvailable();
Review Comment:
🔴 **Critical — over-broad activation:** `isBrotli4jAvailable()` returns
`false` when brotli4j is not on the classpath at all (`ClassNotFoundException`
→ `false`). So `!BROTLI4J_AVAILABLE` is `true` for the vast majority of Camel
HTTP users who simply don't use brotli4j, and they all get the custom decoder
map (which is missing `x-gzip`).
The condition needs to distinguish between:
- **brotli4j absent** → do nothing, HttpClient's defaults are correct
- **brotli4j API present, native library missing** → install the workaround
As Croway suggested, the condition should be something like:
```java
private static final boolean BROTLI4J_UNUSABLE = isBrotliDecoderUnusable();
static boolean isBrotliDecoderUnusable() {
try {
// Check if brotli4j API is on the classpath
Class.forName("com.aayushatharva.brotli4j.Brotli4jLoader");
} catch (ClassNotFoundException e) {
return false; // Not on classpath at all — HttpClient defaults are
fine
}
// API is present, check if native library is loadable
return !isBrotli4jAvailable();
}
```
##########
components/camel-http/src/main/java/org/apache/camel/component/http/HttpComponent.java:
##########
@@ -664,6 +677,35 @@ protected HttpClientBuilder createHttpClientBuilder(
return clientBuilder;
}
+ static boolean isBrotli4jAvailable() {
+ try {
+ Class<?> loader =
Class.forName("com.aayushatharva.brotli4j.Brotli4jLoader", true,
+ HttpComponent.class.getClassLoader());
+ return (boolean) loader.getMethod("isAvailable").invoke(null);
+ } catch (Exception | LinkageError e) {
+ return false;
+ }
+ }
+
+ /**
+ * Builds a content decoder registry containing all HttpClient-supported
decoders except Brotli. Called when the
+ * Brotli4j API jar is on the classpath but the platform-native JNI
library is missing.
+ */
+ static LinkedHashMap<String, InputStreamFactory>
buildDecodersWithoutBrotli() {
+ LinkedHashMap<String, InputStreamFactory> decoders = new
LinkedHashMap<>();
+ for (ContentCoding cc : ContentCoding.values()) {
+ if (cc == ContentCoding.BROTLI) {
+ continue;
+ }
+ if (ContentCodecRegistry.decoder(cc) != null) {
+ final ContentCoding coding = cc;
+ decoders.put(cc.token(), in ->
ContentCodecRegistry.unwrap(coding,
+ new BasicHttpEntity(in, null)).getContent());
+ }
+ }
+ return decoders;
+ }
Review Comment:
🔴 **Critical — missing `x-gzip` alias:** `ContentCodecRegistry` does not
register a decoder for `ContentCoding.X_GZIP`. HttpClient's default
[`ContentCompressionExec`](https://github.com/apache/httpcomponents-client/blob/5.6.x/httpclient5/src/main/java/org/apache/hc/client5/http/impl/classic/ContentCompressionExec.java#L106-L110)
adds `x-gzip` as a separate alias of the `GZIP` decoder, but this method
doesn't replicate that.
Result: a server responding with `Content-Encoding: x-gzip` triggers
`HttpException: Unsupported Content-Encoding: x-gzip`.
Fix: after the loop, add the `x-gzip` alias:
```suggestion
static LinkedHashMap<String, InputStreamFactory>
buildDecodersWithoutBrotli() {
LinkedHashMap<String, InputStreamFactory> decoders = new
LinkedHashMap<>();
for (ContentCoding cc : ContentCoding.values()) {
if (cc == ContentCoding.BROTLI) {
continue;
}
if (ContentCodecRegistry.decoder(cc) != null) {
final ContentCoding coding = cc;
decoders.put(cc.token(), in ->
ContentCodecRegistry.unwrap(coding,
new BasicHttpEntity(in, null)).getContent());
}
}
// Replicate the x-gzip alias that ContentCompressionExec registers
by default
if (decoders.containsKey(ContentCoding.GZIP.token())) {
decoders.put(ContentCoding.X_GZIP.token(),
decoders.get(ContentCoding.GZIP.token()));
}
return decoders;
}
```
##########
components/camel-http/src/test/java/org/apache/camel/component/http/HttpBrotli4jAvailabilityTest.java:
##########
@@ -0,0 +1,109 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one or more
+ * contributor license agreements. See the NOTICE file distributed with
+ * this work for additional information regarding copyright ownership.
+ * The ASF licenses this file to You under the Apache License, Version 2.0
+ * (the "License"); you may not use this file except in compliance with
+ * the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an "AS IS" BASIS,
+ * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ * See the License for the specific language governing permissions and
+ * limitations under the License.
+ */
+package org.apache.camel.component.http;
+
+import java.util.LinkedHashMap;
+import java.util.concurrent.atomic.AtomicReference;
+
+import com.aayushatharva.brotli4j.Brotli4jLoader;
+import org.apache.hc.client5.http.entity.InputStreamFactory;
+import org.apache.hc.core5.http.HttpStatus;
+import org.apache.hc.core5.http.impl.bootstrap.HttpServer;
+import org.apache.hc.core5.http.impl.bootstrap.ServerBootstrap;
+import org.apache.hc.core5.http.io.HttpRequestHandler;
+import org.apache.hc.core5.http.io.entity.StringEntity;
+import org.junit.jupiter.api.Test;
+
+import static org.assertj.core.api.Assertions.assertThat;
+
+public class HttpBrotli4jAvailabilityTest extends BaseHttpTest {
+
+ private HttpServer localServer;
+ private final AtomicReference<String> capturedAcceptEncoding = new
AtomicReference<>();
+
+ @Override
+ public void setupResources() throws Exception {
+ HttpRequestHandler handler = (request, response, context) -> {
+ capturedAcceptEncoding.set(
+ request.getFirstHeader("Accept-Encoding") != null
+ ?
request.getFirstHeader("Accept-Encoding").getValue()
+ : null);
+ response.setCode(HttpStatus.SC_OK);
+ response.setEntity(new StringEntity(getExpectedContent()));
+ };
+
+ localServer = ServerBootstrap.bootstrap()
+ .setCanonicalHostName("localhost")
+ .register("/", handler)
+ .create();
+ localServer.start();
+ }
+
+ @Override
+ public void cleanupResources() throws Exception {
+ if (localServer != null) {
+ localServer.stop();
+ }
+ }
+
+ @Test
+ void brotli4jAvailabilityShouldMatchLoaderState() {
+ // The brotli4j API jar is on the test classpath (test-scope
dependency).
+ // Whether the native library loads depends on the test machine (e.g.
Homebrew
+ // brotli on macOS). What matters is that our reflective check returns
the same
+ // result as calling Brotli4jLoader.isAvailable() directly.
+ assertThat(HttpComponent.isBrotli4jAvailable())
+ .as("isBrotli4jAvailable() must agree with
Brotli4jLoader.isAvailable()")
+ .isEqualTo(Brotli4jLoader.isAvailable());
+ }
+
+ @Test
+ void brotli4jLoaderClassShouldBeOnClasspath() {
+ // Verify the API jar is actually on the test classpath, so the test
above
+ // is validating native-lib detection, not just a missing class.
+ try {
+ Class.forName("com.aayushatharva.brotli4j.Brotli4jLoader");
+ } catch (ClassNotFoundException e) {
+ throw new AssertionError("brotli4j API jar should be on the test
classpath as a test-scope dependency", e);
+ }
+ }
+
+ @Test
+ void buildDecodersWithoutBrotliShouldExcludeBr() {
+ LinkedHashMap<String, InputStreamFactory> decoders =
HttpComponent.buildDecodersWithoutBrotli();
+ assertThat(decoders).doesNotContainKey("br");
+ assertThat(decoders).containsKey("gzip");
+ assertThat(decoders).containsKey("deflate");
+ }
Review Comment:
⚠️ **Test gap:** This test doesn't verify `x-gzip` decompression. A test
that sends a `Content-Encoding: x-gzip` response and verifies successful
decompression would have caught the regression. Also,
`buildDecodersWithoutBrotliShouldExcludeBr` should assert `x-gzip` is present.
More broadly, on CI the native brotli4j jar is typically available (Maven
activates the OS-specific profiles for the test-scoped dependency), so
`Brotli4jLoader.isAvailable()` returns `true` and the "unavailable" branch of
`acceptEncodingShouldReflectBrotli4jAvailability` is never exercised. Consider
the approach Croway described: exclude the native transitive dependencies and
only include the `service` jar, forcing the "API present, native missing" state
on all CI platforms.
--
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]
