gnodet commented on code in PR #27685:
URL: https://github.com/apache/camel/pull/27685#discussion_r4241453328
##########
dsl/camel-jbang/camel-jbang-core/src/main/java/org/apache/camel/dsl/jbang/core/commands/ai/EndpointChecks.java:
##########
@@ -505,6 +508,15 @@ static void checkSqlNamedParameters(
if (query == null || query.isBlank()) {
return;
}
+ // ON CONFLICT (id) DO UPDATE SET country = :#excluded.country:
EXCLUDED is PostgreSQL's row being inserted, as
+ // a named parameter it is a key looked up in the body and the
headers, which is not there (CAMEL-25557)
+ Matcher excluded = EXCLUDED_PARAMETER.matcher(query);
+ if (excluded.find()) {
Review Comment:
🐛 **Bug: only the first `:#excluded.x` is reported.**
With `if (excluded.find())`, only the first match in the query is flagged. A
query like
```sql
SET country = :#excluded.country, orders = :#excluded.orders
```
will silently skip `:#excluded.orders`. `BARE_NAMED_PARAMETER` won't catch
it either — its capture group requires `[A-Za-z_]` immediately after `:`, so
`#` never matches.
Change to a `while` loop:
```suggestion
while (excluded.find()) {
```
##########
dsl/camel-jbang/camel-jbang-core/src/test/java/org/apache/camel/dsl/jbang/core/commands/ai/SqlDialectChecksTest.java:
##########
@@ -0,0 +1,126 @@
+/*
+ * 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.dsl.jbang.core.commands.ai;
+
+import java.io.IOException;
+import java.nio.file.Files;
+import java.nio.file.Path;
+import java.util.List;
+
+import org.apache.camel.catalog.DefaultCamelCatalog;
+import org.junit.jupiter.api.Test;
+import org.junit.jupiter.api.io.TempDir;
+
+import static org.assertj.core.api.Assertions.assertThat;
+
+/**
+ * CAMEL-25557: an insert-or-update in the dialect of another database than
the datasource, and the row being inserted
+ * written as a named parameter, as the local model wrote them for the
service-sql example (a PostgreSQL datasource).
+ */
+class SqlDialectChecksTest {
+
+ private static final String CREATE_TABLE
+ = """
+ - route:
+ id: create-table
+ from:
+ uri: timer:setup?repeatCount=1
+ steps:
+ - to:
+ uri: sql
+ parameters:
+ query: "CREATE TABLE IF NOT EXISTS customers
(id varchar(10) PRIMARY KEY, country varchar(2), orders integer)"
+ """;
+
+ private static List<String> validate(Path dir, String yaml) {
+ return SourceValidator.validate("sql.camel.yaml", yaml, new
DefaultCamelCatalog(), null, dir);
+ }
+
+ @Test
+ void mysqlUpsertOnPostgres(@TempDir Path dir) throws IOException {
+ Files.writeString(dir.resolve("application.properties"),
+
"spring.datasource.url=jdbc:postgresql://localhost:5432/test\n");
+ List<String> msgs = validate(dir,
+ CREATE_TABLE + """
+ - route:
+ id: register-customers
+ from:
+ uri: timer:orders?repeatCount=1
+ steps:
+ - to:
+ uri: "sql:INSERT INTO customers (id,
country, orders) VALUES (:#customer, :#country, 1) ON DUPLICATE KEY UPDATE
orders = orders + 1, country = VALUES(country)"
+ """);
+ assertThat(msgs).singleElement().satisfies(m -> assertThat(m)
+ .startsWith("Line 16: sql: ON DUPLICATE KEY UPDATE is MySQL,
and the datasource of application.properties"
+ + " is PostgreSQL")
+ .contains("write INSERT INTO customers (id, country, orders)
VALUES (:#customer, :#country, 1) ON CONFLICT"
+ + " (id) DO UPDATE SET orders = customers.orders +
1, country = EXCLUDED.country"));
+ }
+
+ @Test
+ void postgresUpsertOnMysql(@TempDir Path dir) throws IOException {
+ Files.writeString(dir.resolve("application.properties"),
"spring.datasource.url=jdbc:mysql://localhost:3306/test\n");
+ List<String> msgs = validate(dir,
+ """
+ - route:
+ from:
+ uri: timer:orders?repeatCount=1
+ steps:
+ - to:
+ uri: sql
+ parameters:
+ query: "INSERT INTO customers (id,
orders) VALUES (:#customer, 1) ON CONFLICT (id) DO UPDATE SET orders =
customers.orders + 1"
+ """);
+ assertThat(msgs).singleElement().satisfies(m -> assertThat(m)
+ .contains("ON CONFLICT ... DO UPDATE is PostgreSQL, and the
datasource of application.properties is MySQL")
+ .contains("write INSERT INTO customers (id, orders) VALUES
(:#customer, 1) ON DUPLICATE KEY UPDATE orders ="
+ + " orders + 1"));
+ }
+
+ @Test
+ void theRightDialectOrNoDatasourceSaysNothing(@TempDir Path dir) throws
IOException {
+ String upsert
+ = """
+ - route:
+ from:
+ uri: timer:orders?repeatCount=1
+ steps:
+ - to:
+ uri: "sql:INSERT INTO customers (id,
orders) VALUES (:#customer, 1) ON CONFLICT (id) DO UPDATE SET orders =
customers.orders + 1"
+ """;
+ assertThat(validate(dir, upsert)).isEmpty();
+ Files.writeString(dir.resolve("application.properties"),
"spring.datasource.url=jdbc:postgresql://db/test\n");
+ assertThat(validate(dir, upsert)).isEmpty();
+ }
+
+ @Test
+ void theRowBeingInsertedIsNotANamedParameter() {
+ List<String> msgs = SourceValidator.validate("sql.camel.yaml",
+ """
+ - route:
+ from:
+ uri: timer:orders?repeatCount=1
+ steps:
+ - to:
+ uri: "sql:INSERT INTO customers (id,
country) VALUES (:#customer, :#country) ON CONFLICT (id) DO UPDATE SET country
= :#excluded.country"
+ """,
+ new DefaultCamelCatalog(), null);
+ assertThat(msgs).anySatisfy(m -> assertThat(m)
+ .contains("sql: :#excluded.country is a named parameter")
+ .contains("write EXCLUDED.country without :#"));
+ }
+}
Review Comment:
💡 **Missing test: dual-datasource case (`datasource()` returns `null`).**
When `application.properties` contains both a PostgreSQL and a MySQL JDBC
URL, `found.size() != 1` and all dialect checks are silently disabled. Please
add a test that verifies no errors are produced in that scenario, so the
behaviour is intentional and documented.
##########
dsl/camel-jbang/camel-jbang-core/src/main/java/org/apache/camel/dsl/jbang/core/commands/ai/SqlDialectChecks.java:
##########
@@ -0,0 +1,177 @@
+/*
+ * 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.dsl.jbang.core.commands.ai;
+
+import java.io.IOException;
+import java.nio.charset.StandardCharsets;
+import java.nio.file.Files;
+import java.nio.file.Path;
+import java.util.ArrayList;
+import java.util.LinkedHashSet;
+import java.util.List;
+import java.util.Locale;
+import java.util.Set;
+import java.util.regex.Matcher;
+import java.util.regex.Pattern;
+
+/**
+ * An insert-or-update written in the dialect of another database than the
datasource of the application: the statement
+ * fails with a syntax error for every message, which a model reads as a
problem of its route and rewrites in yet
+ * another dialect (the service-sql example of the local-model benchmark, a
PostgreSQL datasource, failed half of its
+ * runs with ON DUPLICATE KEY UPDATE). The datasource is read from
application.properties.
+ */
+final class SqlDialectChecks {
+
+ private enum Product {
+ POSTGRESQL("PostgreSQL"),
+ MYSQL("MySQL");
+
+ private final String title;
+
+ Product(String title) {
+ this.title = title;
+ }
+ }
+
+ private static final Pattern JDBC_URL =
Pattern.compile("jdbc:(postgresql|mysql|mariadb):");
+ /**
+ * The statement of a sql: endpoint: in the uri (uri: sql:INSERT ...) or
as its query option (query: INSERT ...).
+ */
+ private static final Pattern STATEMENT
+ =
Pattern.compile("^\\s*(?:-\\s+)?(?:uri|query):\\s*[\"']?(?:sql:)?(.*?)[\"']?\\s*$");
+ private static final Pattern INSERT_INTO =
Pattern.compile("(?i)\\bINSERT\\s+INTO\\s+(\\w+)\\s*\\(([^)]*)\\)");
+ private static final Pattern ON_DUPLICATE =
Pattern.compile("(?i)\\bON\\s+DUPLICATE\\s+KEY\\s+UPDATE\\s+(.*)$");
+ private static final Pattern ON_CONFLICT
+ =
Pattern.compile("(?i)\\bON\\s+CONFLICT\\s*(?:\\(([^)]*)\\))?\\s*DO\\s+UPDATE\\s+SET\\s+(.*)$");
+
+ private SqlDialectChecks() {
+ }
+
+ /** The messages for the statements of the YAML route written in the
dialect of another database. */
+ static List<String> validate(String content, Path directory) {
+ Product product = datasource(directory);
+ if (product == null || content == null) {
+ return List.of();
+ }
+ List<String> answer = new ArrayList<>();
+ String[] lines = content.split("\n", -1);
+ for (int i = 0; i < lines.length; i++) {
+ Matcher m = STATEMENT.matcher(lines[i]);
+ if (!m.matches()) {
+ continue;
+ }
+ String sql = m.group(1).replaceAll("\\?\\w+=.*$", "");
+ Matcher insert = INSERT_INTO.matcher(sql);
+ if (!insert.find()) {
+ continue;
+ }
+ String table = insert.group(1);
+ String head = sql.substring(0, insertEnd(sql)).strip();
+ Matcher dup = ON_DUPLICATE.matcher(sql);
+ Matcher conflict = ON_CONFLICT.matcher(sql);
+ if (product == Product.POSTGRESQL && dup.find()) {
+ String key = primaryKey(content, table);
+ answer.add("Line " + (i + 1) + ": sql: ON DUPLICATE KEY UPDATE
is MySQL, and the datasource of"
+ + " application.properties is PostgreSQL, where the
statement fails with a syntax error:"
+ + " write " + head + " ON CONFLICT (" + (key !=
null ? key : "<its primary key column>")
+ + ") DO UPDATE SET " +
toPostgres(dup.group(1).strip(), table)
+ + " (the row already in the table is " + table +
".column, the row being inserted is"
+ + " EXCLUDED.column, both without :#)");
+ } else if (product == Product.MYSQL && conflict.find()) {
+ answer.add("Line " + (i + 1) + ": sql: ON CONFLICT ... DO
UPDATE is PostgreSQL, and the datasource of"
+ + " application.properties is " + product.title +
", where the statement fails with a"
+ + " syntax error: write " + head + " ON DUPLICATE
KEY UPDATE "
+ + toMysql(conflict.group(2).strip(), table)
+ + " (VALUES(column) is the row being inserted)");
+ }
+ }
+ return answer;
+ }
+
+ /** The end of INSERT INTO ... VALUES (...): where the upsert clause
starts. */
+ private static int insertEnd(String sql) {
+ Matcher m =
Pattern.compile("(?i)\\s+ON\\s+(DUPLICATE|CONFLICT)\\b").matcher(sql);
+ return m.find() ? m.start() : sql.length();
+ }
+
+ /** orders = orders + 1, VALUES(country) as orders = customers.orders + 1,
EXCLUDED.country. */
+ static String toPostgres(String assignments, String table) {
+ List<String> parts = new ArrayList<>();
+ for (String a : assignments.split(",(?![^(]*\\))")) {
+ int eq = a.indexOf('=');
+ if (eq < 0) {
+ parts.add(a.strip());
+ continue;
+ }
+ String column = a.substring(0, eq).strip();
+ String value = a.substring(eq + 1).strip()
+ .replaceAll("(?i)\\bVALUES\\s*\\(\\s*(\\w+)\\s*\\)",
"EXCLUDED.$1");
+ value = value.replaceAll("(?<![.\\w:#])\\b" +
Pattern.quote(column) + "\\b(?!\\s*\\()", table + "." + column);
+ parts.add(column + " = " + value);
+ }
+ return String.join(", ", parts);
+ }
+
+ /** orders = customers.orders + 1, EXCLUDED.country as orders = orders +
1, VALUES(country). */
+ static String toMysql(String assignments, String table) {
+ return assignments.replaceAll("(?i)\\bEXCLUDED\\.(\\w+)", "VALUES($1)")
+ .replaceAll("\\b" + Pattern.quote(table) + "\\.(\\w+)", "$1");
+ }
+
+ /** The primary key column of the table, from a CREATE TABLE in the same
file; null when not there. */
+ static String primaryKey(String content, String table) {
+ Matcher m =
Pattern.compile("(?i)CREATE\\s+TABLE\\s+(?:IF\\s+NOT\\s+EXISTS\\s+)?" +
Pattern.quote(table)
+ + "\\s*\\((.*?)\\)\\s*[\"']?\\s*$",
+ Pattern.MULTILINE).matcher(content);
Review Comment:
⚠️ **`MULTILINE` without `DOTALL` — `(.*?)` cannot span newlines.**
With only `Pattern.MULTILINE`, `.` still doesn't match `\n`. A multi-line
`CREATE TABLE` body won't be captured and `primaryKey()` returns `null`,
producing the `<its primary key column>` placeholder in the suggestion. In the
YAML-inline context this is unlikely to bite, but consider `Pattern.MULTILINE |
Pattern.DOTALL` to handle it correctly.
--
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]
