gnodet-bot commented on code in PR #26677:
URL: https://github.com/apache/camel/pull/26677#discussion_r4061473380
##########
components/camel-opa/src/test/java/org/apache/camel/component/opa/OpaWasmIT.java:
##########
@@ -97,20 +146,16 @@ void keepsTheEntrypointAcrossPooledReuse() {
assertThat(out.getMessage().getHeader(OpaConstants.DECISION))
.as("message %d was still decided by authz/decision", i)
.isInstanceOf(Map.class);
Review Comment:
⚠️ **Dropped assertion weakens the test's actual purpose.**
The original test asserted both that the result *is* a `Map` and that it
*contains* `"allow": true`. The second assertion is what actually proves the
correct entrypoint was used — `authz/decision` (which returns `{"allow": true,
...}`) rather than `authz/allow` (which returns a bare boolean). The
`isInstanceOf(Map.class)` check alone doesn't detect the regression this test
was written to guard against: a pool reset that puts the entrypoint back to 0,
causing subsequent exchanges to be evaluated by the wrong rule.
With only the `isInstanceOf` check, if a pool reset drops back to entrypoint
0 (`authz/allow`, a bare boolean), the exchange would set `DECISION_ALLOW` and
leave `DECISION` as `null` — which fails `isInstanceOf(Map.class)`. But if
entrypoint 0 *happens* to be `authz/decision` (because the bundle was built
with `authz/decision` as the first `-e`), the Map check passes even for the
wrong rule. The `containsEntry("allow", true)` assertion should be restored.
```suggestion
.isInstanceOf(Map.class);
assertThat(out.getMessage().getHeader(OpaConstants.DECISION,
Map.class)).containsEntry("allow", true);
```
##########
components/camel-opa/src/test/java/org/apache/camel/component/opa/OpaWasmIT.java:
##########
@@ -25,45 +29,82 @@
import java.util.stream.IntStream;
import org.apache.camel.Exchange;
-import org.apache.camel.ResolveEndpointFailedException;
+import org.apache.camel.test.infra.opa.services.OpaWasmBundleBuilder;
import org.apache.camel.test.junit6.CamelTestSupport;
+import org.junit.jupiter.api.BeforeAll;
import org.junit.jupiter.api.Test;
-import org.junit.jupiter.api.Timeout;
+import org.junit.jupiter.api.io.TempDir;
import static org.assertj.core.api.Assertions.assertThat;
-import static org.assertj.core.api.Assertions.assertThatThrownBy;
/**
- * In-process evaluation of a bundle produced by {@code opa build -t wasm}.
+ * In-process evaluation of a bundle compiled from the very {@code authz.rego}
that {@link OpaIT} uploads to a real OPA
+ * server.
* <p/>
- * The bundle here was compiled from the same {@code authz.rego} the REST
tests use, so the assertions double as a check
- * that a route sees the same decision whichever engine evaluated it.
+ * Sharing one policy between the two classes is the point rather than a
convenience: the component promises that a
+ * route sees the same decision whichever engine evaluated it, and that
promise is only tested if both engines are asked
+ * about the same rules. It also closes the way that promise was broken before
- a compiled bundle committed beside the
+ * Rego drifted from it, and the two suites asserted opposite things about
{@code authz/decision} while both stayed
+ * green (CAMEL-24741). Nothing is committed now; the bundle is built from the
policy under test.
*/
-public class OpaWasmEvaluatorTest extends CamelTestSupport {
+public class OpaWasmIT extends CamelTestSupport {
- private static final String WASM =
"opa:authz/allow?evaluationMode=wasm&policyBundle=classpath:authz.wasm";
+ @TempDir
+ static Path bundles;
+
+ private static String authz;
+ private static String roles;
+
+ private static String resource(String name) throws Exception {
+ try (InputStream in = OpaWasmIT.class.getResourceAsStream(name)) {
Review Comment:
💡 **`getResourceAsStream()` can return `null` — silent NPE in `@BeforeAll`.**
`Class.getResourceAsStream()` returns `null` when the resource is not found.
The `try`-with-resources block will then call `null.readAllBytes()` →
`NullPointerException` inside `compileBundles()`, surfacing as a confusing
`@BeforeAll` failure with a stack trace pointing at `in.readAllBytes()` rather
than the missing resource name.
```suggestion
try (InputStream in = OpaWasmIT.class.getResourceAsStream(name)) {
if (in == null) {
throw new IllegalArgumentException("Test resource not found:
" + name);
}
return new String(in.readAllBytes(), StandardCharsets.UTF_8);
```
##########
components/camel-opa/src/test/java/org/apache/camel/component/opa/OpaWasmIT.java:
##########
@@ -121,54 +166,8 @@ void appliesTheDataDocumentPackedInTheBundle() {
}
@Test
- void failsClosedOnAnUndefinedDecisionJustLikeTheRestEngine() {
- // authz/strict_allow has no default, so for mallory the rule is
undefined. The WASM ABI returns an
- // empty array where the REST client raises an error; both must reach
the route the same way.
- Exchange out = template.request(
-
"opa:authz/strict_allow?evaluationMode=wasm&policyBundle=classpath:authz.wasm",
- e -> e.getMessage().setHeader("user", "mallory"));
-
-
assertThat(out.getException()).isInstanceOf(OpaPolicyEvaluationException.class);
-
assertThat(out.getMessage().getHeader(OpaConstants.DECISION_ALLOW)).isNull();
- }
-
- @Test
- void acceptsTheBundleTarballOpaBuildActuallyEmits() {
- Exchange out = template.request(
-
"opa:authz/allow?evaluationMode=wasm&policyBundle=classpath:authz-bundle.tar.gz",
- e -> e.getMessage().setHeader("user", "alice"));
-
- assertThat(out.getException()).isNull();
-
assertThat(out.getMessage().getHeader(OpaConstants.DECISION_ALLOW)).isEqualTo(true);
- }
-
- @Test
- void requiresAPolicyBundle() {
- // the check runs when the endpoint starts, so a misconfiguration
fails fast rather than once per message
- assertThatThrownBy(() ->
template.request("opa:authz/allow?evaluationMode=wasm", e -> {
- }))
- .isInstanceOf(ResolveEndpointFailedException.class)
- .hasMessageContaining("policyBundle is required");
- }
-
- @Test
- void rejectsAPoolSizeBelowOne() {
- // the pool rejects it as well, but as "maxSize must be positive" -
its own parameter rather than the
- // option that was set, which is what the operator has to go looking
for
- assertThatThrownBy(() -> template.request(
-
"opa:authz/allow?evaluationMode=wasm&policyBundle=classpath:authz.wasm&poolSize=0",
e -> {
- }))
- .isInstanceOf(ResolveEndpointFailedException.class)
- .hasMessageContaining("poolSize must be at least 1");
- }
-
- @Test
- @Timeout(60)
void keepsThePoolUsableAfterRepeatedEvaluationFailures() {
Review Comment:
⚠️ **`@Timeout(60)` removed — pool deadlock in this test now hangs the suite
indefinitely.**
The original test had `@Timeout(60)` for a specific reason: this test
exercises a correctness invariant of the pool (`template.request()` must
unblock after a failure, not hold the permit). If the pool is broken and
`request()` wedges, the test will block forever without the timeout annotation,
freezing the entire test run with no signal about why.
The timeout should be restored on this test, and if `OpaWasmIT` is now an IT
(running under `-Pit`), the annotation is just as valid there.
```suggestion
@Test
@org.junit.jupiter.api.Timeout(60)
void keepsThePoolUsableAfterRepeatedEvaluationFailures() {
```
--
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]
