gnodet-bot commented on code in PR #28029:
URL: https://github.com/apache/camel/pull/28029#discussion_r4305104305
##########
core/camel-java-io/src/test/java/org/apache/camel/java/LwModelToJavaDumperTest.java:
##########
@@ -106,4 +107,30 @@ public void configure() {
.getExceptionType()).isNull();
}
}
+
+ /**
+ * An option set to its documented default value is written: when it is
not set, the option takes the value of a
+ * parent level (the rest configuration for a verb, the error handler for
the redelivery of an onException).
+ */
+ @Test
+ public void testDumpOptionsSetToTheirDefaultValue() throws Exception {
+ try (DefaultCamelContext context = new DefaultCamelContext()) {
+ context.addRoutes(new RouteBuilder() {
+ @Override
+ public void configure() {
+
rest("/api").get("/raw").bindingMode(RestBindingMode.off).to("direct:raw");
+
+ from("direct:start").routeId("redelivery")
+
.onException(IllegalStateException.class).maximumRedeliveries(1).redeliveryDelay(1000).end()
+ .to("mock:result");
+ }
+ });
+ Model model =
context.getCamelContextExtension().getContextPlugin(Model.class);
+ LwModelToJavaDumper dumper = new LwModelToJavaDumper();
+
+ assertThat(dumper.dumpModelAsJava(context,
model.getRestDefinitions().get(0))).contains(".bindingMode(\"off\")");
+ assertThat(dumper.dumpModelAsJava(context,
context.getRouteDefinition("redelivery")))
+ .contains(".redeliveryDelay(\"1000\")");
Review Comment:
⚠️ **The Java test only checks string containment — it does not verify
round-trip behavioral correctness.**
`assertThat(...).contains(".bindingMode(\"off\")")` confirms the string is
in the dump output. It does NOT confirm that loading the dump back and running
the route produces the same behavior as the original. That is the actual
regression this PR fixes, and the XML and YAML tests in this same PR both
verify it correctly (load → run → dump → reload → run → compare).
The Java DSL is the third dump format and deserves the same level of
verification. Add a round-trip test that:
1. Sets up a `RestConfiguration` with `bindingMode=json`
2. Defines a verb with `bindingMode=off`
3. Dumps via `LwModelToJavaDumper`, compiles and loads the result, runs the
route
4. Verifies the response is not JSON-marshalled (i.e. `{format=raw}` not
`{"format":"raw"}`)
If compiling the Java dump in a test is impractical, at minimum load-and-run
the redelivery scenario (equivalent to
`XmlDumpExplicitDefaultValuesTest.onExceptionRedeliveryDelayUnderErrorHandler`)
using the Java model APIs directly to exercise the redelivery path with the 1s
delay rather than the 30s error handler delay.
##########
core/camel-java-io/src/main/java/org/apache/camel/java/LwModelToJavaDumper.java:
##########
@@ -52,6 +56,17 @@
@JdkService(ModelToJavaDumper.FACTORY)
public class LwModelToJavaDumper implements ModelToJavaDumper {
+ /**
+ * Options that are written whenever they are set, even with their
documented default value: when they are not set,
+ * they take the value of a parent level (the rest or the rest
configuration for a rest or verb, the CamelContext
+ * for a route), and a dump that leaves them out runs differently once
loaded. The same holds for every option of a
+ * redelivery policy (an onException copies the error handler's policy)
and of a circuit breaker configuration (it
+ * is layered over the global one).
+ */
+ static final Set<String> INHERITED_OPTIONS = Set.of(
+ "bindingMode", "skipBindingOnErrorCode",
"clientRequestValidation", "clientResponseValidation", "enableCORS",
+ "enableNoContentResponse", "shutdownRoute", "shutdownRunningTask");
Review Comment:
⚠️ **`INHERITED_OPTIONS` is duplicated across three dumper files** —
`LwModelToJavaDumper`, `LwModelToXMLDumper`, and `LwModelToYAMLDumper` each
define the same `static final Set<String>` independently. If a new inherited
option is discovered in the future, it has to be added in three places and
there is no compiler enforcement that they stay in sync. A route that sets the
option explicitly would then be silently dropped by the Java dump but preserved
by the XML/YAML dump (or vice versa), producing inconsistent round-trip
behavior across formats.
Extract the constant to a shared location — the obvious candidate is a
package-private class or interface in `camel-core-model` (or wherever the
generated writers live), or at minimum a single class from which all three
import it:
```java
// in a new shared class, e.g. DumperConstants (same package or a common
parent):
/** Options whose value must always be written because "not set" means
"inherit from parent". */
public static final Set<String> INHERITED_OPTIONS = Set.of(
"bindingMode", "skipBindingOnErrorCode", "clientRequestValidation",
"clientResponseValidation", "enableCORS",
"enableNoContentResponse", "shutdownRoute", "shutdownRunningTask");
```
Each of the three dumpers then references
`DumperConstants.INHERITED_OPTIONS`.
--
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]
