gnodet-bot commented on code in PR #26268:
URL: https://github.com/apache/camel/pull/26268#discussion_r3988630529
##########
components/camel-sql/src/main/java/org/apache/camel/component/sql/stored/SqlStoredProducer.java:
##########
@@ -106,15 +106,27 @@ public void execute(StatementWrapper ps) throws
SQLException, DataAccessExceptio
private StatementWrapper createStatement(Exchange exchange) throws
SQLException {
String sql;
+ boolean fromHeader = false;
if (getEndpoint().isUseMessageBodyForTemplate()) {
sql = exchange.getIn().getBody(String.class);
} else {
- String templateHeader =
exchange.getIn().getHeader(SqlStoredConstants.SQL_STORED_TEMPLATE,
String.class);
- sql = templateHeader != null ? templateHeader : resolvedTemplate;
+ String templateHeader = getEndpoint().isAllowTemplateFromHeader()
+ ?
exchange.getIn().getHeader(SqlStoredConstants.SQL_STORED_TEMPLATE,
String.class) : null;
+ if (templateHeader != null) {
+ sql = templateHeader;
+ fromHeader = true;
+ } else {
+ sql = resolvedTemplate;
+ }
}
try {
- sql = SqlHelper.resolveQuery(getEndpoint().getCamelContext(), sql,
null);
+ // A header-supplied template is untrusted input, so it must not
be resolved as a file:/http: resource
+ // (SqlHelper.resolveQuery -> ResourceHelper does that) - resolve
placeholders only. The endpoint-configured
+ // template is already resolved in doInit/doStart.
+ sql = fromHeader
+ ? SqlHelper.resolvePlaceholders(sql, null)
+ : SqlHelper.resolveQuery(getEndpoint().getCamelContext(),
sql, null);
} catch (Exception e) {
Review Comment:
⚠️ **Redundant re-resolution of endpoint template on every message.** When
`fromHeader=false` and the body wasn't used, `sql` is `resolvedTemplate` —
which was already resolved through `SqlHelper.resolveQuery` in
`doInit`/`doStart` (inherited lifecycle). Calling `resolveQuery` again on an
already-resolved string is harmless (it won't have a scheme prefix), but it's
wasted work on every exchange: `ResourceHelper.hasScheme()` check +
`resolvePlaceholders()` stripping comments/blanks every time.
Pre-existing issue, not introduced by this PR, but now that the branching is
explicit this would be the natural place to short-circuit:
```suggestion
sql = fromHeader
? SqlHelper.resolvePlaceholders(sql, null)
: resolvedTemplate; // already resolved in
doInit/doStart — no need to re-resolve
```
This also eliminates the `catch (Exception e)` block for the non-header path
(the endpoint template can't throw here since it was already resolved at
startup).
##########
catalog/camel-catalog/src/generated/resources/org/apache/camel/catalog/components/sql.json:
##########
@@ -96,6 +96,7 @@
"schedulerProperties": { "index": 45, "kind": "parameter", "displayName":
"Scheduler Properties", "group": "scheduler", "label": "consumer,scheduler",
"required": false, "type": "object", "javaType":
"java.util.Map<java.lang.String, java.lang.Object>", "prefix": "scheduler.",
"multiValue": true, "deprecated": false, "autowired": false, "secret": false,
"description": "To configure additional properties when using a custom
scheduler or any of the Quartz, Spring based scheduler. This is a multi-value
option with prefix: scheduler." },
"startScheduler": { "index": 46, "kind": "parameter", "displayName":
"Start Scheduler", "group": "scheduler", "label": "consumer,scheduler",
"required": false, "type": "boolean", "javaType": "boolean", "deprecated":
false, "autowired": false, "secret": false, "defaultValue": true,
"description": "Whether the scheduler should be auto started." },
"timeUnit": { "index": 47, "kind": "parameter", "displayName": "Time
Unit", "group": "scheduler", "label": "consumer,scheduler", "required": false,
"type": "enum", "javaType": "java.util.concurrent.TimeUnit", "enum": [
"NANOSECONDS", "MICROSECONDS", "MILLISECONDS", "SECONDS", "MINUTES", "HOURS",
"DAYS" ], "deprecated": false, "autowired": false, "secret": false,
"defaultValue": "MILLISECONDS", "description": "Time unit for initialDelay and
delay options." },
- "useFixedDelay": { "index": 48, "kind": "parameter", "displayName": "Use
Fixed Delay", "group": "scheduler", "label": "consumer,scheduler", "required":
false, "type": "boolean", "javaType": "boolean", "deprecated": false,
"autowired": false, "secret": false, "defaultValue": true, "description":
"Controls if fixed delay or fixed rate is used. See ScheduledExecutorService in
JDK for details." }
+ "useFixedDelay": { "index": 48, "kind": "parameter", "displayName": "Use
Fixed Delay", "group": "scheduler", "label": "consumer,scheduler", "required":
false, "type": "boolean", "javaType": "boolean", "deprecated": false,
"autowired": false, "secret": false, "defaultValue": true, "description":
"Controls if fixed delay or fixed rate is used. See ScheduledExecutorService in
JDK for details." },
Review Comment:
💡 **`label` mismatch with the annotation.** The `@UriParam` on
`DefaultSqlEndpoint` uses `label = "security"`, but per @davsclaus's finding
this should be `label = "producer,security"` to prevent the DSL from surfacing
it on the consumer builder (where it's a no-op). Once the annotation is fixed,
this generated file needs a regen.
##########
components/camel-sql/src/test/java/org/apache/camel/component/sql/stored/SqlStoredAllowTemplateFromHeaderTest.java:
##########
@@ -0,0 +1,80 @@
+/*
+ * 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.sql.stored;
+
+import java.util.HashMap;
+import java.util.Map;
+
+import org.apache.camel.CamelExecutionException;
+import org.apache.camel.builder.RouteBuilder;
+import org.apache.camel.test.junit6.CamelTestSupport;
+import org.junit.jupiter.api.Test;
+import org.springframework.jdbc.datasource.embedded.EmbeddedDatabase;
+import org.springframework.jdbc.datasource.embedded.EmbeddedDatabaseBuilder;
+import org.springframework.jdbc.datasource.embedded.EmbeddedDatabaseType;
+
+import static org.junit.jupiter.api.Assertions.assertThrows;
+
+public class SqlStoredAllowTemplateFromHeaderTest extends CamelTestSupport {
+
+ private static final String PROC = "SUBNUMBERS(INTEGER :#num1,INTEGER
:#num2,OUT INTEGER resultofsum)";
+
+ private EmbeddedDatabase db;
+
+ @Override
+ public void doPreSetup() throws Exception {
+ db = new EmbeddedDatabaseBuilder()
+ .setName(getClass().getSimpleName())
+ .setType(EmbeddedDatabaseType.HSQL)
+ .addScript("sql/storedProcedureTest.sql").build();
+ }
+
+ @Override
+ public void doPostTearDown() throws Exception {
+ if (db != null) {
+ db.shutdown();
+ }
+ }
+
+ @Test
+ public void headerTemplateIgnoredByDefault() {
+ // allowTemplateFromHeader defaults to false, so the
CamelSqlStoredTemplate header must not override the
+ // endpoint-configured template; the (placeholder) endpoint template
is used instead and fails to parse,
+ // which is what confirms the header was ignored rather than executed.
+ Map<String, Object> params = new HashMap<>();
+ params.put("num1", 3);
+ params.put("num2", 1);
+ Map<String, Object> headers = new HashMap<>();
+ headers.put(SqlStoredConstants.SQL_STORED_TEMPLATE, PROC);
+ headers.put(SqlStoredConstants.SQL_STORED_PARAMETERS, params);
+
+ assertThrows(CamelExecutionException.class,
Review Comment:
🔴 **Confirming @davsclaus's finding: this test does not validate the gate.**
Setting `allowTemplateFromHeader=true` on the endpoint and re-running produces
the same `CamelExecutionException` — the exception comes from the endpoint's
own template (`"query"`) failing to parse, not from the header being ignored.
The test structure needs to match
`SqlRouteTest.testQueryFromHeaderIsIgnoredByDefault`: use a **valid** endpoint
template that produces a known result, send a different valid template via the
header, and assert the endpoint's result was used (proving the header was
ignored).
Also missing: a positive test proving `allowTemplateFromHeader=true`
actually enables the header override.
--
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]
