Croway commented on code in PR #29282:
URL: https://github.com/apache/camel/pull/29282#discussion_r4486429650
##########
components/camel-jdbc/src/main/java/org/apache/camel/component/jdbc/DefaultJdbcPrepareStatementStrategy.java:
##########
@@ -118,10 +119,26 @@ public void remove() {
}
} else {
- // just use a regular iterator
- return
exchange.getContext().getTypeConverter().convertTo(Iterator.class,
- headerMap != null ? headerMap.values() : null);
+ // plain ? placeholders are bound by position, so the values are
only taken from the JDBC_PARAMETERS header
+ // (and not from all the message headers, as their order does not
relate to the query)
+ return createPositionalIterator(query, exchange);
+ }
+ }
+
+ private Iterator<?> createPositionalIterator(String query, Exchange
exchange) {
+ Object parameters =
exchange.getIn().getHeader(JdbcConstants.JDBC_PARAMETERS);
+ if (parameters == null) {
+ throw new RuntimeExchangeException(
+ "Cannot set the ? placeholders in query [" + query + "]:
set their values, in order, as a List in the "
+ + JdbcConstants.JDBC_PARAMETERS
+ + " header, or use named
parameters (:?name)",
+ exchange);
+ } else if (parameters instanceof Map<?, ?> map) {
+ return map.values().iterator();
}
+ // a List, Iterable or array gives its values in order, and any other
value (a String is not split) is
+ // the one value
+ return ObjectHelper.createIterator(parameters, null, true);
Review Comment:
**Medium: a `byte[]` for one `?` binds only its first byte, and the exchange
succeeds**
`ObjectHelper.createIterator` iterates a primitive array element by element,
so a `byte[]` becomes a sequence of `Byte` values. `populateStatement` stops
after `expectedParams` values and its count check passes, so the rest of the
array is dropped without an error.
Reproduced on H2 (the camel-jdbc test setup) with this PR:
```java
// create table blobs (id int auto_increment primary key, data
varbinary(100))
template.request("direct:start", e -> { //
jdbc:testdb?useHeadersAsParameters=true
e.getIn().setBody("insert into blobs (data) values (?)");
e.getIn().setHeader(JdbcConstants.JDBC_PARAMETERS, new byte[] { 1, 2, 3,
4, 5 });
});
// select length(data), data from blobs -> len=1, data=[1], no exception
```
On `main` the same exchange fails with `Number of parameters mismatch.
Expected: 1, was:0`. So this turns a loud failure into silent data loss for a
BLOB/VARBINARY insert, and a `where hash = ?` lookup compares against a single
byte.
Suggestion: only an `Object[]` or a `Collection`/`Iterable` gives several
values. A primitive array such as `byte[]` is the one value.
##########
components/camel-jdbc/src/main/java/org/apache/camel/component/jdbc/DefaultJdbcPrepareStatementStrategy.java:
##########
@@ -118,10 +119,26 @@ public void remove() {
}
} else {
- // just use a regular iterator
- return
exchange.getContext().getTypeConverter().convertTo(Iterator.class,
- headerMap != null ? headerMap.values() : null);
+ // plain ? placeholders are bound by position, so the values are
only taken from the JDBC_PARAMETERS header
+ // (and not from all the message headers, as their order does not
relate to the query)
+ return createPositionalIterator(query, exchange);
+ }
+ }
+
+ private Iterator<?> createPositionalIterator(String query, Exchange
exchange) {
Review Comment:
**Low-medium: more values than placeholders are dropped silently**
`List.of("cust1", "jstrachan", "unused")` for `select * from customer where
id = ? and name = ?` returns the row with no error (reproduced). The PR
description keeps this because `populateStatement` is shared with the named
path. But `createPopulateIterator` already receives `expectedParams`, so the
positional path can check the size itself without touching `populateStatement`:
```java
private Iterator<?> createPositionalIterator(String query, int
expectedParams, Exchange exchange) {
...
List<?> values = ...; // the Map values, the Collection, the Object[],
or the single value
if (values.size() != expectedParams) {
throw new RuntimeExchangeException("Query [" + query + "] has " +
expectedParams + " ? placeholders but the "
+ JdbcConstants.JDBC_PARAMETERS +
" header has " + values.size() + " values",
exchange);
}
return values.iterator();
}
```
Positional values now come only from an explicit header, so a size mismatch
is a mistake in the route (a list built for another version of the query, an
off-by-one). Failing is the safer default, and this check would also have
caught the `byte[]` case above (5 values for 1 placeholder).
##########
components/camel-jdbc/src/main/java/org/apache/camel/component/jdbc/DefaultJdbcPrepareStatementStrategy.java:
##########
@@ -55,19 +56,19 @@ public Iterator<?> createPopulateIterator(
final String query, final String preparedQuery, final int
expectedParams,
final Exchange exchange, final Object value)
throws SQLException {
- Map<?, ?> map = null;
- if (exchange.getIn().hasHeaders()) {
- if (exchange.getIn().getHeader(JdbcConstants.JDBC_PARAMETERS) !=
null) {
- // header JDBC_PARAMETERS takes precedence over regular headers
- map =
exchange.getIn().getHeader(JdbcConstants.JDBC_PARAMETERS, Map.class);
- } else {
- map = exchange.getIn().getHeaders();
+ if (hasNamedParameters(query)) {
+ Map<?, ?> map = null;
+ if (exchange.getIn().hasHeaders()) {
+ if (exchange.getIn().getHeader(JdbcConstants.JDBC_PARAMETERS)
!= null) {
+ // header JDBC_PARAMETERS takes precedence over regular
headers
+ map =
exchange.getIn().getHeader(JdbcConstants.JDBC_PARAMETERS, Map.class);
Review Comment:
**Low-medium: the `List` that positional queries now need breaks a later
named query in the same route**
The producer leaves `CamelJdbcParameters` on the message. When a later jdbc
call in the same route uses named parameters, this line converts the `List` to
a `Map` and gets `null` (there is no List-to-Map converter). The lookup then
fails, although the header it needs is set. Reproduced:
```java
from("direct:twoCalls")
.setBody(constant("select * from customer where id = ?"))
.to("jdbc:testdb?useHeadersAsParameters=true")
.setBody(constant("select * from customer where id = :?customer"))
.to("jdbc:testdb?useHeadersAsParameters=true");
// headers: CamelJdbcParameters = List.of("cust1"), customer = "cust2"
// -> RuntimeExchangeException: Cannot find key [customer] in message body
or headers to use when setting
// named parameter in query [select * from customer where id = :?customer]
```
Before this PR a positional query took its values from the headers, so no
`List` was left in this header to trip over. Suggestion: on the named path, use
`CamelJdbcParameters` only when it is a `Map`. Otherwise fall back to the
message headers, or fail with a message that says the header is not a `Map`.
--
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]
