diff --git a/docs/hop-user-manual/modules/ROOT/pages/pipeline/transforms/tableinput.adoc b/docs/hop-user-manual/modules/ROOT/pages/pipeline/transforms/tableinput.adoc index 43115c673cc..74da090b402 100644 --- a/docs/hop-user-manual/modules/ROOT/pages/pipeline/transforms/tableinput.adoc +++ b/docs/hop-user-manual/modules/ROOT/pages/pipeline/transforms/tableinput.adoc @@ -85,6 +85,14 @@ Prefer *named parameters* `{openfield}fieldName{closefield}` so the SQL does not *Execute for each row* is the usual case: run the query once per incoming row. Disable it only for the legacy assemble-all path, where every incoming row is concatenated into one parameter list. That is how positional SQL such as `WHERE bar IN (?,?,?)` can be filled from three one-field rows. +When the SQL has no bind placeholder at all (no `{openfield}fieldName{closefield}` and no positional `?`), Table Input never binds incoming rows to the statement. + +With *Execute for each row* **off**, the incoming hops are still consumed so an upstream transform can complete before the query runs (header or sequencing hops), but their rows are only drained, never concatenated into a parameter list and never bound, and the query runs once without parameters. +With *Execute for each row* **on**, the query runs once per incoming row (the usual contract), still with no parameter bound. +Binding values to a statement without placeholders fails on every driver (for example `ORA-17003 Invalid column index` on Oracle), so this is what keeps those pipelines working. + +NOTE: This restores the 2.19 behavior for SQL without placeholders and keeps the 2.20 "optional lookup" feature for SQL with placeholders: named and positional parameters are still read from every incoming hop (even with an empty *Insert data from transform*). + If two hops into Table Input have different layouts, Hop still warns about mixing rows. That check is unchanged. .Named parameter with an informational hop from the previous transform: @@ -208,7 +216,7 @@ TIP: If you are getting unexpected query results, try clearing the database cach TIP: A cartesian join transform will combine a different number of fields from multiple table inputs without requiring key join fields. -TIP: When *Execute for each row* is off, Table Input waits until every incoming hop has completed so it can concatenate the parameter rows. +TIP: When *Execute for each row* is off, Table Input waits until every incoming hop has completed so it can concatenate the parameter rows. When the SQL has no bind placeholder, the incoming rows are only drained so an upstream header or sequencing transform can complete before the query runs. TIP: For better performance with large datasets you can use indexed columns in `WHERE` clauses and avoid `SELECT *` and only retrieve needed fields. @@ -271,8 +279,8 @@ On the *Fields* tab you can define the output schema instead of asking the datab |SQL tab|SQL statement, optional SQL file, `Get SQL select statement`, `Use named parameters`, and `Insert field...` for `{openfield}fieldName{closefield}`. |Use named parameters|When enabled, `{openfield}fieldName{closefield}` in SQL is bound to an incoming field. Off for existing Table Input metadata, on for new transforms. Disable if curly braces are used for something else. Checking this option also enables *Execute for each row* when hops exist, and selects the incoming transform when there is only one. |Replace variables in script?|Enable to substitute variables (e.g., `+${param}+`) in your SQL before execution. -|Insert data from transform|Optional. Names the hop that should be treated as informational (parameter rows, not the SQL result). Parameter values are read from **all** incoming hops. Leave empty when you only need the connected hops as parameter sources. -|Execute for each row?|When incoming hops exist, run the SQL query once for each incoming row. Disable to concatenate all incoming rows into a single parameter list (legacy `WHERE bar IN (?,?,?)` from several one-field rows). Incoming hops with different layouts still produce the mixed-layout warning. +|Insert data from transform|Optional. Names the hop that should be treated as informational (parameter rows, not the SQL result). Parameter values are read from **all** incoming hops. Leave empty when you only need the connected hops as parameter sources. When the SQL has no bind placeholder, the incoming rows are never bound (only drained when *Execute for each row* is off). +|Execute for each row?|When incoming hops exist, run the SQL query once for each incoming row. Disable to concatenate all incoming rows into a single parameter list (legacy `WHERE bar IN (?,?,?)` from several one-field rows). Incoming hops with different layouts still produce the mixed-layout warning. When the SQL has no bind placeholder, nothing is ever bound: with this option **off** the incoming rows are only drained and the query runs once without parameters; with it **on** the query runs once per incoming row, still without parameters. |Limit size|Maximum number of rows to return from the query. `0` means no limit. See <> below. |Specify output fields|Define the output field list instead of reading column metadata from the database. |Validate specified fields|When specifying fields, fail if the query result names or types do not match the list. diff --git a/plugins/transforms/tableinput/pom.xml b/plugins/transforms/tableinput/pom.xml index 75b5bf38ec4..3334c02e8e8 100644 --- a/plugins/transforms/tableinput/pom.xml +++ b/plugins/transforms/tableinput/pom.xml @@ -29,4 +29,29 @@ jar Hop Plugins Transforms Table Input + + + + org.apache.hop + hop-libs-jdbc + ${project.version} + pom + import + + + + + + + com.h2database + h2 + test + + + org.apache.hop + hop-databases-h2 + ${project.version} + test + + diff --git a/plugins/transforms/tableinput/src/main/java/org/apache/hop/pipeline/transforms/tableinput/TableInput.java b/plugins/transforms/tableinput/src/main/java/org/apache/hop/pipeline/transforms/tableinput/TableInput.java index 1be26baf71e..6320edcbf80 100644 --- a/plugins/transforms/tableinput/src/main/java/org/apache/hop/pipeline/transforms/tableinput/TableInput.java +++ b/plugins/transforms/tableinput/src/main/java/org/apache/hop/pipeline/transforms/tableinput/TableInput.java @@ -98,9 +98,40 @@ public boolean processRow() throws HopException { if (isDetailed()) { logDetailed("Reading all parameter rows from incoming hops"); } - RowMetaAndData assembled = readAllParameterRows(); - parameters = assembled.getData(); - parametersMeta = assembled.getRowMeta(); + + // Legacy sequencing contract (2.19): when there is no info stream (empty lookup) and no + // named parameters, the incoming rows are not parameters at all. They are only consumed + // so an upstream "header" transform can complete before the extraction runs. Consuming + // them as parameter rows would bind them to a statement without placeholders (ORA-17003 on + // Oracle). Drain them without collecting. + // + if (Utils.isEmpty(meta.getLookup()) && !meta.isUseNamedParameters()) { + String resolved; + try { + resolved = resolveSql(); + } catch (HopException e) { + logError("Could not get SQL: " + e.getMessage()); + setErrors(1); + stopAll(); + return false; + } + if (TableInputSql.countPositionalPlaceholders(resolved) == 0) { + while (getRow() != null) { + // consume the sequencing row(s); nothing is bound + } + parameters = new Object[] {}; + parametersMeta = new RowMeta(); + } else { + RowMetaAndData assembled = readAllParameterRows(); + parameters = assembled.getData(); + parametersMeta = assembled.getRowMeta(); + } + } else { + RowMetaAndData assembled = readAllParameterRows(); + parameters = assembled.getData(); + parametersMeta = assembled.getRowMeta(); + } + if (parameters == null) { parameters = new Object[] {}; } @@ -205,22 +236,27 @@ private void closePreviousQuery() throws HopDatabaseException { } } + private String resolveSql() throws HopException { + String sql = meta.getEffectiveSql(variables); + if (meta.isVariableReplacementActive()) { + sql = resolve(sql); + } + return sql; + } + private boolean doQuery(IRowMeta parametersMeta, Object[] parameters) throws HopException { boolean success = true; // Open the query with the optional parameters received from the source transforms. String sql; try { - sql = meta.getEffectiveSql(variables); + sql = resolveSql(); } catch (HopException e) { logError("Could not get SQL: " + e.getMessage()); setErrors(1); stopAll(); return false; } - if (meta.isVariableReplacementActive()) { - sql = resolve(sql); - } TableInputSql.Bound bound; try { diff --git a/plugins/transforms/tableinput/src/main/java/org/apache/hop/pipeline/transforms/tableinput/TableInputSql.java b/plugins/transforms/tableinput/src/main/java/org/apache/hop/pipeline/transforms/tableinput/TableInputSql.java index 142370f1ab4..34ac13e0461 100644 --- a/plugins/transforms/tableinput/src/main/java/org/apache/hop/pipeline/transforms/tableinput/TableInputSql.java +++ b/plugins/transforms/tableinput/src/main/java/org/apache/hop/pipeline/transforms/tableinput/TableInputSql.java @@ -166,12 +166,17 @@ public static Parsed parse(String sql) throws HopException { } /** - * Bind incoming row values to the named parameters in {@code parsed}. When the SQL has no named - * parameters the original parameter metadata and data are passed through (positional {@code ?}). + * Bind incoming row values to the named parameters in {@code parsed}. When the SQL has no + * parameters at all (no named, no positional {@code ?}) the bound statement must not receive any + * parameter metadata or data: binding values to a statement without placeholders fails on every + * driver (ORA-17003 on Oracle, "Parameter index out of range" on H2). */ public static Bound bind(Parsed parsed, IRowMeta parametersMeta, Object[] parameters) throws HopException { if (!parsed.hasNamedParameters()) { + if (parsed.getPositionalParameterCount() == 0) { + return new Bound(parsed.getJdbcSql(), null, null); + } return new Bound(parsed.getJdbcSql(), parametersMeta, parameters); } if (parametersMeta == null) { @@ -205,6 +210,80 @@ public static Bound prepare(String sql, IRowMeta parametersMeta, Object[] parame return bind(parse(sql), parametersMeta, parameters); } + /** + * Count real JDBC positional placeholders ({@code ?}) outside string literals, quoted + * identifiers, comments and Hop variables. Used when named parameters are disabled so SQL is + * passed through unchanged; values must only be bound when the statement really has placeholders + * (otherwise every driver fails: ORA-17003 on Oracle, "Parameter index out of range" on H2). + */ + public static int countPositionalPlaceholders(String sql) { + if (sql == null) { + return 0; + } + int count = 0; + int i = 0; + final int n = sql.length(); + while (i < n) { + char c = sql.charAt(i); + char next = (i + 1 < n) ? sql.charAt(i + 1) : 0; + + if (c == '-' && next == '-') { + int end = indexOfNewline(sql, i + 2); + if (end < 0) { + break; + } + i = end; + continue; + } + if (c == '/' && next == '*') { + int end = sql.indexOf("*/", i + 2); + if (end < 0) { + break; + } + i = end + 2; + continue; + } + if (c == '\'') { + i = skipQuoted(sql, i, '\''); + continue; + } + if (c == '"') { + i = skipQuoted(sql, i, '"'); + continue; + } + if (c == '$' && next == '{') { + int end = sql.indexOf('}', i + 2); + if (end < 0) { + break; + } + i = end + 1; + continue; + } + if (c == '?') { + count++; + } + i++; + } + return count; + } + + private static int skipQuoted(String sql, int start, char quote) { + int j = start + 1; + final int n = sql.length(); + while (j < n) { + char ch = sql.charAt(j); + if (ch == quote) { + if (j + 1 < n && sql.charAt(j + 1) == quote) { + j += 2; + continue; + } + return j + 1; + } + j++; + } + return n; + } + /** * Bind named parameters only when {@code useNamedParameters} is true. Otherwise the SQL is passed * through unchanged so existing {@code {braces}} in queries stay literal. @@ -213,6 +292,9 @@ public static Bound prepare( boolean useNamedParameters, String sql, IRowMeta parametersMeta, Object[] parameters) throws HopException { if (!useNamedParameters) { + if (countPositionalPlaceholders(sql) == 0) { + return new Bound(sql, null, null); + } return new Bound(sql, parametersMeta, parameters); } return prepare(sql, parametersMeta, parameters); diff --git a/plugins/transforms/tableinput/src/test/java/org/apache/hop/pipeline/transforms/tableinput/TableInputSqlTest.java b/plugins/transforms/tableinput/src/test/java/org/apache/hop/pipeline/transforms/tableinput/TableInputSqlTest.java index eb5f425b144..9483e5674ab 100644 --- a/plugins/transforms/tableinput/src/test/java/org/apache/hop/pipeline/transforms/tableinput/TableInputSqlTest.java +++ b/plugins/transforms/tableinput/src/test/java/org/apache/hop/pipeline/transforms/tableinput/TableInputSqlTest.java @@ -166,11 +166,43 @@ void prepareDisabledLeavesBracesLiteral() throws Exception { TableInputSql.Bound bound = TableInputSql.prepare(false, sql, incoming, row); + // No positional placeholder: the SQL is passed through and nothing is bound, otherwise the + // driver fails (ORA-17003 / "Parameter index out of range") on a statement without binds. assertEquals(sql, bound.getJdbcSql()); + assertEquals(null, bound.getParameterMeta()); + assertEquals(null, bound.getParameterData()); + } + + @Test + void prepareDisabledStillBindsWhenPositionalPlaceholdersExist() throws Exception { + IRowMeta incoming = new RowMeta(); + incoming.addValueMeta(new ValueMetaString("key")); + Object[] row = new Object[] {"10"}; + + TableInputSql.Bound bound = + TableInputSql.prepare(false, "SELECT * FROM t WHERE id = ?", incoming, row); + + assertEquals("SELECT * FROM t WHERE id = ?", bound.getJdbcSql()); assertEquals(incoming, bound.getParameterMeta()); assertArrayEquals(row, bound.getParameterData()); } + @Test + void countPositionalPlaceholdersIgnoresLiteralsCommentsAndVariables() { + assertEquals( + 0, + TableInputSql.countPositionalPlaceholders( + "SELECT '?' AS a, \"q?\" AS b FROM t -- ?\n WHERE x = '${VAR}'")); + assertEquals( + 3, + TableInputSql.countPositionalPlaceholders( + "SELECT * FROM t WHERE id = ? AND name = ? AND x = ?")); + assertEquals( + 1, + TableInputSql.countPositionalPlaceholders( + "SELECT * FROM t WHERE id = ? -- comment\n AND /* ? */ name = '?'")); + } + @Test void parseAllowsSpacesInFieldName() throws Exception { TableInputSql.Parsed parsed = TableInputSql.parse("SELECT * FROM t WHERE id = {from date}"); diff --git a/plugins/transforms/tableinput/src/test/java/org/apache/hop/pipeline/transforms/tableinput/TableInputTest.java b/plugins/transforms/tableinput/src/test/java/org/apache/hop/pipeline/transforms/tableinput/TableInputTest.java new file mode 100644 index 00000000000..ec9607c9913 --- /dev/null +++ b/plugins/transforms/tableinput/src/test/java/org/apache/hop/pipeline/transforms/tableinput/TableInputTest.java @@ -0,0 +1,261 @@ +/* + * 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.hop.pipeline.transforms.tableinput; + +import static org.mockito.ArgumentMatchers.anyBoolean; +import static org.mockito.ArgumentMatchers.isNull; +import static org.mockito.ArgumentMatchers.notNull; +import static org.mockito.ArgumentMatchers.nullable; +import static org.mockito.Mockito.any; +import static org.mockito.Mockito.anyInt; +import static org.mockito.Mockito.anyString; +import static org.mockito.Mockito.doNothing; +import static org.mockito.Mockito.doReturn; +import static org.mockito.Mockito.eq; +import static org.mockito.Mockito.mock; +import static org.mockito.Mockito.never; +import static org.mockito.Mockito.spy; +import static org.mockito.Mockito.times; +import static org.mockito.Mockito.verify; + +import java.sql.PreparedStatement; +import java.sql.ResultSet; +import java.util.UUID; +import org.apache.hop.core.HopClientEnvironment; +import org.apache.hop.core.database.Database; +import org.apache.hop.core.database.DatabaseMeta; +import org.apache.hop.core.row.IRowMeta; +import org.apache.hop.core.row.RowMeta; +import org.apache.hop.core.row.value.ValueMetaString; +import org.apache.hop.core.variables.Variables; +import org.apache.hop.databases.h2.H2DatabaseMeta; +import org.apache.hop.metadata.serializer.memory.MemoryMetadataProvider; +import org.apache.hop.pipeline.PipelineMeta; +import org.apache.hop.pipeline.engines.local.LocalPipelineEngine; +import org.apache.hop.pipeline.transform.TransformMeta; +import org.junit.jupiter.api.BeforeAll; +import org.junit.jupiter.api.BeforeEach; +import org.junit.jupiter.api.Test; + +/** + * Unit tests for the Table Input query/parameter path. + * + *

These tests reproduce the regression where a hop into a Table Input whose SQL has no + * placeholders was still consumed as parameter rows in 2.20 (Issue #2722 / PR #8034): the incoming + * row was bound to a PreparedStatement without parameters, which fails on Oracle with ORA-17003 and + * on H2 with "Parameter index out of range". + * + *

The fix keeps the 2.20 "optional lookup" feature (collect parameter rows from any incoming + * hop, named parameters) while restoring the legacy behavior for SQL without placeholders: no + * collection, no bind, and the incoming hops are still drained so the sequence (header transform + * then extraction) keeps working. + */ +class TableInputTest { + + private static final String CONNECTION_NAME = "table-input-test"; + + private Variables variables; + private TableInput tableInput; + private TableInputMeta meta; + private TableInputData data; + private Database db; + + @BeforeAll + static void initHop() throws Exception { + HopClientEnvironment.init(); + org.apache.hop.core.database.DatabasePluginType.getInstance() + .registerClassPathPlugin(H2DatabaseMeta.class); + } + + @BeforeEach + void setUp() throws Exception { + variables = new Variables(); + + DatabaseMeta databaseMeta = connectionTo("mem:" + UUID.randomUUID()); + // Force the H2 driver to load and validate the connection name before the transform uses it. + try (Database ignored = + new Database( + new org.apache.hop.core.logging.LoggingObject("TableInputTest"), + variables, + databaseMeta)) { + // connecting is enough + } + + meta = new TableInputMeta(); + meta.setConnection(CONNECTION_NAME); + meta.setUseNamedParameters(false); + meta.setSql("SELECT 1"); + + data = new TableInputData(); + db = mock(Database.class); + data.db = db; + doReturn(new RowMeta()).when(db).getReturnRowMeta(); + doReturn(null).when(db).getRow(any(ResultSet.class)); + doReturn(databaseMeta).when(db).getDatabaseMeta(); + // The no-placeholder path calls openQuery(sql, null, null, ...) so the stubs must match null: + // on Mockito 5, any(IRowMeta.class) / any(Object[].class) (InstanceOf) reject null; the + // nullable() variants accept it. + doReturn(mock(ResultSet.class)) + .when(db) + .openQuery( + anyString(), + nullable(IRowMeta.class), + nullable(Object[].class), + anyInt(), + anyBoolean()); + + PipelineMeta pipelineMeta = new PipelineMeta(); + MemoryMetadataProvider metadataProvider = new MemoryMetadataProvider(); + metadataProvider.getSerializer(DatabaseMeta.class).save(databaseMeta); + pipelineMeta.setMetadataProvider(metadataProvider); + TransformMeta transformMeta = new TransformMeta("table input", meta); + pipelineMeta.addTransform(transformMeta); + // dispatch() in the BaseTransform constructor walks previous/next transforms; none exist here. + + tableInput = + spy( + new TableInput( + transformMeta, meta, data, 1, pipelineMeta, spy(new LocalPipelineEngine()))); + doReturn(transformMeta).when(tableInput).getTransformMeta(); + doReturn(false).when(tableInput).isRowLevel(); + doReturn(false).when(tableInput).isDebug(); + doNothing().when(tableInput).logDetailed(any()); + doNothing().when(tableInput).logBasic(any()); + doNothing().when(tableInput).logDebug(any()); + doNothing().when(tableInput).logError(any()); + // doQuery() reads through data.db; make the query "open" successfully with no rows. + // (single stub above already matches both the null-params and bound-params paths) + } + + private DatabaseMeta connectionTo(String database) { + DatabaseMeta databaseMeta = + new DatabaseMeta( + CONNECTION_NAME, "H2", "Native", "", database + ";DB_CLOSE_DELAY=-1", "", "", ""); + databaseMeta.setSupportsTimestampDataType(true); + return databaseMeta; + } + + private void stubHeaderRow() throws Exception { + // One incoming row (the "header" row), then end of input. + IRowMeta headerRowMeta = new RowMeta(); + headerRowMeta.addValueMeta(new ValueMetaString("header_flag")); + doReturn(new Object[] {"H"}, (Object) null).when(tableInput).getRow(); + doReturn(headerRowMeta).when(tableInput).getInputRowMeta(); + } + + private static void assertQueryWithoutBind(Database db, String sql) throws Exception { + // The query must run with no parameter metadata/data at all: + verify(db, times(1)) + .openQuery(eq(sql), isNull(IRowMeta.class), isNull(Object[].class), anyInt(), eq(false)); + // ... and no attempt to bind values to a statement: + verify(db, never()) + .setValues(any(IRowMeta.class), any(Object[].class), any(PreparedStatement.class)); + } + + /** + * The transform must succeed: an empty result set makes processRow() return false immediately + * (done), so the meaningful assertion is that no error was recorded and the pipeline was not + * stopped. + */ + private static void assertSucceeded(TableInput tableInput) { + org.junit.jupiter.api.Assertions.assertEquals(0, tableInput.getErrors()); + verify(tableInput, never()).stopAll(); + } + + /** + * Regression test: an incoming hop into a Table Input whose SQL has NO placeholder (e.g. a + * header/sequencing hop) must NOT be bound to the statement. Before the fix, 2.20 collected the + * incoming row (optional lookup) and passed it to the PreparedStatement, which fails with + * ORA-17003 / H2 "Parameter index out of range" because the SQL has no bind variables. + */ + @Test + void sqlWithoutParametersDoesNotBindIncomingRows() throws Exception { + stubHeaderRow(); + + tableInput.processRow(); + + assertQueryWithoutBind(db, "SELECT 1"); + // The header row must still be drained so upstream transforms can complete (sequencing). + verify(tableInput, times(2)).getRow(); // one header row + the null terminator + assertSucceeded(tableInput); + } + + /** + * The 2.20 feature must keep working: named parameters {field} are still bound even when the + * lookup (Insert data from transform) is empty, as long as the SQL declares them. + */ + @Test + void namedParametersStillBindEvenWithoutLookup() throws Exception { + meta.setUseNamedParameters(true); + meta.setSql("SELECT ID, NAME FROM T WHERE NAME = {name}"); + stubHeaderRow(); + IRowMeta incoming = new RowMeta(); + incoming.addValueMeta(new ValueMetaString("name")); + doReturn(incoming).when(tableInput).getInputRowMeta(); + + tableInput.processRow(); + + verify(db, times(1)) + .openQuery( + eq("SELECT ID, NAME FROM T WHERE NAME = ?"), + notNull(IRowMeta.class), + notNull(Object[].class), + anyInt(), + eq(false)); + assertSucceeded(tableInput); + } + + /** + * Legacy positional placeholders (IN (?,?,?)) must still bind values from the collected incoming + * rows even with no lookup and named parameters off. + */ + @Test + void positionalQuestionMarksStillBindWithoutLookup() throws Exception { + meta.setUseNamedParameters(false); + meta.setSql("SELECT ID FROM T WHERE BAR IN (?,?,?)"); + stubHeaderRow(); + IRowMeta incoming = new RowMeta(); + incoming.addValueMeta(new ValueMetaString("bar")); + incoming.addValueMeta(new ValueMetaString("bar")); + incoming.addValueMeta(new ValueMetaString("bar")); + doReturn(incoming).when(tableInput).getInputRowMeta(); + + tableInput.processRow(); + + verify(db, times(1)) + .openQuery( + eq("SELECT ID FROM T WHERE BAR IN (?,?,?)"), + notNull(IRowMeta.class), + notNull(Object[].class), + anyInt(), + eq(false)); + assertSucceeded(tableInput); + } + + /** A '?' inside a string literal is not a bind placeholder and must not trigger a bind. */ + @Test + void literalQuestionMarksAreNotPlaceholders() throws Exception { + stubHeaderRow(); + meta.setSql("SELECT '?' AS Q FROM T WHERE X = 1"); + + tableInput.processRow(); + + assertQueryWithoutBind(db, "SELECT '?' AS Q FROM T WHERE X = 1"); + assertSucceeded(tableInput); + } +}