From 55905d5bef22cab663e6fab611facd22e79fdc8c Mon Sep 17 00:00:00 2001 From: Romain Manni-Bucau Date: Wed, 7 Oct 2026 15:08:35 +0200 Subject: [PATCH 1/3] fix(tableinput): don't bind incoming rows when the SQL has no placeholders PR #8034 (Issue #2722) made Table Input read parameter rows from every incoming hop, even with an empty "Insert data from transform". Pipelines that feed a Table Input for sequencing only (e.g. a header transform before the extraction) now had their header row bound to the statement even though the SQL has no bind variable, failing on Oracle with ORA-17003 "Invalid column index" and on H2 with "Parameter index out of range". Fix both sides of the regression while keeping the 2.20 feature: - TableInputSql: count real positional placeholders (?) outside string literals, quoted identifiers, comments and Hop variables. prepare() with named parameters disabled and bind() now return a Bound with null parameter metadata/data when the SQL has no placeholder, so Database runs a plain statement instead of a PreparedStatement. - TableInput: in the assemble-all branch, when the lookup is empty and named parameters are disabled and the SQL has no placeholder, drain the incoming rows without collecting or binding them. Upstream transforms still complete (sequencing contract of 2.19) and consumesMainInput() stays true so the unconsumed-main-input check keeps passing. - Named and positional parameters are still read from every incoming hop (optional lookup, PR #8034) when the SQL declares them. Tests: new TableInputTest with an in-memory H2 connection reproduces the failure before the fix (openQuery receives the header row on "SELECT 1") and guards named/positional binding, literal '?' handling and the legacy drain-without-bind path. TableInputSqlTest updated accordingly. Documentation: tableinput.adoc now describes the no-placeholder behavior and the 2.19/2.20 compatibility. --- .../pages/pipeline/transforms/tableinput.adoc | 12 +- plugins/transforms/tableinput/pom.xml | 25 ++ .../transforms/tableinput/TableInput.java | 42 ++- .../transforms/tableinput/TableInputSql.java | 86 +++++- .../tableinput/TableInputSqlTest.java | 32 +++ .../transforms/tableinput/TableInputTest.java | 262 ++++++++++++++++++ 6 files changed, 447 insertions(+), 12 deletions(-) create mode 100644 plugins/transforms/tableinput/src/test/java/org/apache/hop/pipeline/transforms/tableinput/TableInputTest.java 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..3b3e3c8b1eb 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,12 @@ 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. +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. +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 +214,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 +277,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 only drained, never bound. +|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, the incoming rows are only drained, never bound, and the query runs once 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..c35bfcbb291 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,32 @@ 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 = resolveSql(); + 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 +228,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..ed203906abd --- /dev/null +++ b/plugins/transforms/tableinput/src/test/java/org/apache/hop/pipeline/transforms/tableinput/TableInputTest.java @@ -0,0 +1,262 @@ +/* + * 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.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(); + doReturn(mock(ResultSet.class)) + .when(db) + .openQuery(anyString(), any(IRowMeta.class), any(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. + doReturn(mock(ResultSet.class)) + .when(db) + .openQuery(anyString(), any(), any(), anyInt(), anyBoolean()); + } + + 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)); + } + + /** + * 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 + } + + /** + * 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)); + } + + /** + * 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)); + } + + /** 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"); + } + + /** + * Legacy sequencing contract: with an empty lookup and named parameters off, the incoming rows + * must be drained (so a header transform can complete) but NOT assembled as parameters and NOT + * bound. + */ + @Test + void legacyEmptyLookupDrainsWithoutCollectingOrBinding() throws Exception { + stubHeaderRow(); + + tableInput.processRow(); + + // The header row was consumed: + verify(tableInput, times(2)).getRow(); + // ... but nothing was bound: + verify(db, never()) + .setValues(any(IRowMeta.class), any(Object[].class), any(PreparedStatement.class)); + // ... and the query ran with no parameters: + verify(db, times(1)) + .openQuery( + eq("SELECT 1"), isNull(IRowMeta.class), isNull(Object[].class), anyInt(), eq(false)); + } +} From c5add36b7e34134556a06ccb5b4972a0dbb5cfb4 Mon Sep 17 00:00:00 2001 From: Romain Manni-Bucau Date: Wed, 7 Oct 2026 23:32:27 +0200 Subject: [PATCH 2/3] fix(tableinput): apply review feedback from PR #8787 - TableInput: catch HopException around resolveSql() in the drain branch (empty lookup, named parameters off) so a missing SQL file is reported as "Could not get SQL" with setErrors(1) + stopAll() instead of an unexpected engine error, mirroring doQuery(). - Docs: qualify the drain-without-bind wording with *Execute for each row* off; with it on the query runs once per incoming row, still with no parameter bound. - Tests: single nullable(IRowMeta/Object[]) openQuery stub (matches the null-params path), assertSucceeded() (errors == 0, no stopAll()), drop the duplicate legacyEmptyLookup test. --- .../pages/pipeline/transforms/tableinput.adoc | 8 +-- .../transforms/tableinput/TableInput.java | 10 +++- .../transforms/tableinput/TableInputTest.java | 50 +++++++++---------- 3 files changed, 38 insertions(+), 30 deletions(-) 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 3b3e3c8b1eb..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 @@ -86,7 +86,9 @@ 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. -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. + +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*). @@ -277,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. When the SQL has no bind placeholder, the incoming rows are only drained, never bound. -|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, the incoming rows are only drained, never bound, and the query runs once without parameters. +|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/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 c35bfcbb291..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 @@ -106,7 +106,15 @@ public boolean processRow() throws HopException { // Oracle). Drain them without collecting. // if (Utils.isEmpty(meta.getLookup()) && !meta.isUseNamedParameters()) { - String resolved = resolveSql(); + 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 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 index ed203906abd..244af016987 100644 --- 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 @@ -106,9 +106,17 @@ void setUp() throws Exception { 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(), any(IRowMeta.class), any(Object[].class), anyInt(), anyBoolean()); + .openQuery( + anyString(), + nullable(IRowMeta.class), + nullable(Object[].class), + anyInt(), + anyBoolean()); PipelineMeta pipelineMeta = new PipelineMeta(); MemoryMetadataProvider metadataProvider = new MemoryMetadataProvider(); @@ -130,9 +138,7 @@ void setUp() throws Exception { doNothing().when(tableInput).logDebug(any()); doNothing().when(tableInput).logError(any()); // doQuery() reads through data.db; make the query "open" successfully with no rows. - doReturn(mock(ResultSet.class)) - .when(db) - .openQuery(anyString(), any(), any(), anyInt(), anyBoolean()); + // (single stub above already matches both the null-params and bound-params paths) } private DatabaseMeta connectionTo(String database) { @@ -160,6 +166,16 @@ private static void assertQueryWithoutBind(Database db, String sql) throws Excep .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 @@ -175,6 +191,7 @@ void sqlWithoutParametersDoesNotBindIncomingRows() throws Exception { 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); } /** @@ -199,6 +216,7 @@ void namedParametersStillBindEvenWithoutLookup() throws Exception { notNull(Object[].class), anyInt(), eq(false)); + assertSucceeded(tableInput); } /** @@ -225,6 +243,7 @@ void positionalQuestionMarksStillBindWithoutLookup() throws Exception { notNull(Object[].class), anyInt(), eq(false)); + assertSucceeded(tableInput); } /** A '?' inside a string literal is not a bind placeholder and must not trigger a bind. */ @@ -236,27 +255,6 @@ void literalQuestionMarksAreNotPlaceholders() throws Exception { tableInput.processRow(); assertQueryWithoutBind(db, "SELECT '?' AS Q FROM T WHERE X = 1"); - } - - /** - * Legacy sequencing contract: with an empty lookup and named parameters off, the incoming rows - * must be drained (so a header transform can complete) but NOT assembled as parameters and NOT - * bound. - */ - @Test - void legacyEmptyLookupDrainsWithoutCollectingOrBinding() throws Exception { - stubHeaderRow(); - - tableInput.processRow(); - - // The header row was consumed: - verify(tableInput, times(2)).getRow(); - // ... but nothing was bound: - verify(db, never()) - .setValues(any(IRowMeta.class), any(Object[].class), any(PreparedStatement.class)); - // ... and the query ran with no parameters: - verify(db, times(1)) - .openQuery( - eq("SELECT 1"), isNull(IRowMeta.class), isNull(Object[].class), anyInt(), eq(false)); + assertSucceeded(tableInput); } } From 08aae05583718c40ed4283454ede53afaedd9e68 Mon Sep 17 00:00:00 2001 From: Romain Manni-Bucau Date: Wed, 7 Oct 2026 23:53:50 +0200 Subject: [PATCH 3/3] [build] fix compilation --- .../hop/pipeline/transforms/tableinput/TableInputTest.java | 1 + 1 file changed, 1 insertion(+) 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 index 244af016987..ec9607c9913 100644 --- 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 @@ -20,6 +20,7 @@ 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;