mattcasters commented on code in PR #8787: URL: https://github.com/apache/hop/pull/8787#discussion_r4210072400
########## 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. + * + * <p>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". + * + * <p>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()); Review Comment: **[suggestion]** Mockito 5's `any()` and `any(IRowMeta.class)` do not match null. The no-placeholder path calls `openQuery(sql, null, null, ...)`, so both stubs in `setUp` miss, the mock returns null, and `doQuery` takes the "Couldn't open Query" path (`setErrors(1)`, `stopAll()`). The `verify` calls still pass, so these tests never show that the transform succeeds. The comment above this stub says the query opens successfully. `legacyEmptyLookupDrainsWithoutCollectingOrBinding` is the same scenario as `sqlWithoutParametersDoesNotBindIncomingRows`. **Suggestion:** Stub with `nullable(IRowMeta.class)` and `nullable(Object[].class)`, assert that `processRow()` returns true and `getErrors()` stays 0, and drop the duplicate test. ########## 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(); Review Comment: **[suggestion]** `resolveSql()` is invoked here, outside the try/catch that `doQuery` uses for the same call. A missing SQL file (or any other `HopException` from `getEffectiveSql`) used to be logged as "Could not get SQL", with `setErrors(1)`, `stopAll()`, and `processRow` returning false. On this new path the exception leaves `processRow` and the engine reports it as an unexpected error. This only happens when the lookup is empty and named parameters are off, which is the path this fix adds. **Suggestion:** Catch `HopException` around this call the same way `doQuery` does, and return false before draining. ########## docs/hop-user-manual/modules/ROOT/pages/pipeline/transforms/tableinput.adoc: ########## @@ -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. Review Comment: **[suggestion]** This sentence says that with no bind placeholder the incoming rows are only drained and the query runs once. That is only the assemble-all branch (`executeEachInputRow` off, empty lookup, named parameters off) in `TableInput.processRow`. When *Execute for each row* is on, that branch is skipped. The first row still calls `doQuery`, and `determineDoneReading` calls `doQuery` again for every later row. `TableInputSql.prepare` drops the bind, so there is no ORA-17003, but a multi-row predecessor repeats the whole result set. Lines 88–89 say the same thing ("only drained", "before the query runs"). **Suggestion:** Limit the "drained / runs once" wording to *Execute for each row* being off. If a single execution is the intended contract for every mode, also drain without re-querying when `countPositionalPlaceholders` is 0. -- 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]
