Copilot commented on code in PR #23127:
URL: https://github.com/apache/camel/pull/23127#discussion_r3221768013
##########
components/camel-xslt/src/main/java/org/apache/camel/component/xslt/XsltComponent.java:
##########
@@ -94,6 +95,11 @@ public void setAllowTemplateFromHeader(boolean
allowTemplateFromHeader) {
}
public boolean isContentCache() {
+ return contentCache != null ? contentCache : true;
+ }
+
+ @Override
+ public Boolean getContentCache() {
return contentCache;
}
Review Comment:
`XsltComponent` now exposes *both* `isContentCache(): boolean` and
`getContentCache(): Boolean` for the same property name, while the setter is
`setContentCache(Boolean)`. This can confuse JavaBeans introspection/binders
(some prefer `isXxx` for booleans) and may result in a read-only `contentCache`
property or failed binding in frameworks that rely on standard property
descriptors. A concrete fix is to restore JavaBeans compatibility by adding an
overload `setContentCache(boolean)` delegating to the `Boolean` setter (or
alternatively rename `isContentCache()` to something like
`isContentCacheEnabled()` / `isContentCacheEffective()` so the bean property is
unambiguous).
##########
components/camel-xslt/src/main/java/org/apache/camel/component/xslt/XsltComponent.java:
##########
@@ -102,7 +108,8 @@ public boolean isContentCache() {
* stylesheet file on each message processing. This is good for
development. A cached stylesheet can be forced to
* reload at runtime via JMX using the clearCachedStylesheet operation.
*/
- public void setContentCache(boolean contentCache) {
+ @Override
+ public void setContentCache(Boolean contentCache) {
this.contentCache = contentCache;
}
Review Comment:
`XsltComponent` now exposes *both* `isContentCache(): boolean` and
`getContentCache(): Boolean` for the same property name, while the setter is
`setContentCache(Boolean)`. This can confuse JavaBeans introspection/binders
(some prefer `isXxx` for booleans) and may result in a read-only `contentCache`
property or failed binding in frameworks that rely on standard property
descriptors. A concrete fix is to restore JavaBeans compatibility by adding an
overload `setContentCache(boolean)` delegating to the `Boolean` setter (or
alternatively rename `isContentCache()` to something like
`isContentCacheEnabled()` / `isContentCacheEffective()` so the bean property is
unambiguous).
##########
core/camel-main/src/test/java/org/apache/camel/main/MainDevModeContentCacheTest.java:
##########
@@ -0,0 +1,117 @@
+/*
+ * 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.main;
+
+import java.util.Map;
+
+import org.apache.camel.Endpoint;
+import org.apache.camel.spi.ContentCacheAware;
+import org.apache.camel.support.DefaultComponent;
+import org.junit.jupiter.api.Test;
+
+import static org.junit.jupiter.api.Assertions.assertEquals;
+import static org.junit.jupiter.api.Assertions.assertNull;
+
+public class MainDevModeContentCacheTest {
+
+ @Test
+ public void shouldDisableContentCacheWhenRoutesReloadEnabled() {
+ Main main = new Main();
+ main.configure().withRoutesReloadEnabled(true);
+ main.bind("dummy", new TestContentCacheComponent());
+
+ main.start();
+ try {
+ TestContentCacheComponent c =
main.getCamelContext().getComponent("dummy", TestContentCacheComponent.class);
+ assertEquals(Boolean.FALSE, c.getContentCache(),
+ "contentCache should be auto-disabled when
routesReloadEnabled=true");
+ } finally {
+ main.stop();
+ }
+ }
+
+ @Test
+ public void shouldLeaveContentCacheUnsetWhenRoutesReloadDisabled() {
+ Main main = new Main();
+ // routesReloadEnabled defaults to false
+ main.bind("dummy", new TestContentCacheComponent());
+
+ main.start();
+ try {
+ TestContentCacheComponent c =
main.getCamelContext().getComponent("dummy", TestContentCacheComponent.class);
+ assertNull(c.getContentCache(),
+ "contentCache should remain unset when routesReloadEnabled
is not active");
+ } finally {
+ main.stop();
+ }
+ }
+
+ @Test
+ public void shouldRespectExplicitContentCacheTrue() {
+ Main main = new Main();
+ main.configure().withRoutesReloadEnabled(true);
+ TestContentCacheComponent component = new TestContentCacheComponent();
+ component.setContentCache(Boolean.TRUE);
+ main.bind("dummy", component);
+
+ main.start();
+ try {
+ TestContentCacheComponent c =
main.getCamelContext().getComponent("dummy", TestContentCacheComponent.class);
+ assertEquals(Boolean.TRUE, c.getContentCache(),
+ "explicit user setting must not be overridden by dev-mode
auto-flip");
+ } finally {
+ main.stop();
+ }
+ }
+
+ @Test
+ public void shouldRespectExplicitContentCacheFalse() {
+ Main main = new Main();
+ main.configure().withRoutesReloadEnabled(true);
+ TestContentCacheComponent component = new TestContentCacheComponent();
+ component.setContentCache(Boolean.FALSE);
+ main.bind("dummy", component);
+
+ main.start();
+ try {
+ TestContentCacheComponent c =
main.getCamelContext().getComponent("dummy", TestContentCacheComponent.class);
+ assertEquals(Boolean.FALSE, c.getContentCache());
+ } finally {
+ main.stop();
+ }
+ }
+
+ public static final class TestContentCacheComponent extends
DefaultComponent implements ContentCacheAware {
+
+ private Boolean contentCache;
+
+ @Override
+ public Boolean getContentCache() {
+ return contentCache;
+ }
+
+ @Override
+ public void setContentCache(Boolean contentCache) {
+ this.contentCache = contentCache;
+ }
+
+ @Override
+ protected Endpoint createEndpoint(String uri, String remaining,
Map<String, Object> parameters) {
+ return null;
Review Comment:
`DefaultComponent#createEndpoint(...)` is expected to return a non-null
`Endpoint`. Returning `null` can cause hard-to-diagnose NPEs if the test is
later extended (or if Camel behavior changes to create an endpoint during
startup). Prefer throwing `UnsupportedOperationException` (to fail fast) or
returning a minimal stub `Endpoint` implementation.
##########
core/camel-main/src/main/java/org/apache/camel/main/DevModeContentCacheStrategy.java:
##########
@@ -0,0 +1,45 @@
+/*
+ * 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.main;
+
+import org.apache.camel.Component;
+import org.apache.camel.spi.ContentCacheAware;
+import org.apache.camel.support.LifecycleStrategySupport;
+import org.slf4j.Logger;
+import org.slf4j.LoggerFactory;
+
+/**
+ * Lifecycle strategy that disables content caching on resource-based
components when routes-reload is enabled. Lets
+ * users edit a resource file (e.g. an XSLT stylesheet) and see the change
applied live, without having to set
+ * {@code contentCache=false} on every endpoint.
+ *
+ * Only flips components that have not been explicitly configured by the user
(i.e.
+ * {@link ContentCacheAware#getContentCache()} returns {@code null}); explicit
user settings are preserved.
+ */
+public class DevModeContentCacheStrategy extends LifecycleStrategySupport {
+
+ private static final Logger LOG =
LoggerFactory.getLogger(DevModeContentCacheStrategy.class);
+
+ @Override
+ public void onComponentAdd(String name, Component component) {
+ if (component instanceof ContentCacheAware aware &&
aware.getContentCache() == null) {
+ aware.setContentCache(Boolean.FALSE);
+ LOG.info("Routes-reload is enabled: disabling contentCache on
component '{}' for live resource reload",
+ name);
+ }
Review Comment:
The PR description states that explicit user settings “on the component,
endpoint, or URI are always respected”, but the added tests only cover explicit
*component-level* settings. Add a unit test that exercises endpoint/URI
configuration overriding the auto-flipped component default (e.g., create a
minimal component+endpoint pair where the endpoint has `contentCache` set from
URI parameters and assert it remains `true` when `routesReloadEnabled=true`).
##########
core/camel-main/src/main/java/org/apache/camel/main/DevModeContentCacheStrategy.java:
##########
@@ -0,0 +1,45 @@
+/*
+ * 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.main;
+
+import org.apache.camel.Component;
+import org.apache.camel.spi.ContentCacheAware;
+import org.apache.camel.support.LifecycleStrategySupport;
+import org.slf4j.Logger;
+import org.slf4j.LoggerFactory;
+
+/**
+ * Lifecycle strategy that disables content caching on resource-based
components when routes-reload is enabled. Lets
+ * users edit a resource file (e.g. an XSLT stylesheet) and see the change
applied live, without having to set
+ * {@code contentCache=false} on every endpoint.
+ *
+ * Only flips components that have not been explicitly configured by the user
(i.e.
+ * {@link ContentCacheAware#getContentCache()} returns {@code null}); explicit
user settings are preserved.
+ */
+public class DevModeContentCacheStrategy extends LifecycleStrategySupport {
+
+ private static final Logger LOG =
LoggerFactory.getLogger(DevModeContentCacheStrategy.class);
+
+ @Override
+ public void onComponentAdd(String name, Component component) {
+ if (component instanceof ContentCacheAware aware &&
aware.getContentCache() == null) {
+ aware.setContentCache(Boolean.FALSE);
+ LOG.info("Routes-reload is enabled: disabling contentCache on
component '{}' for live resource reload",
Review Comment:
Logging this at `INFO` will emit one line per `ContentCacheAware` component
added, which can be noisy in dev mode (and especially as more components adopt
the SPI). Consider lowering to `DEBUG`, or logging once with a summary (or
guard with a “log once per component name” mechanism) to reduce log volume
while keeping the behavior discoverable.
--
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]
