You are viewing a plain text version of this content. The canonical link for it is here.
Posted to commits@hop.apache.org by ha...@apache.org on 2026/08/07 12:00:07 UTC
(hop) branch main updated: Update transform should give clear error, fixes #4772 (#7831)
This is an automated email from the ASF dual-hosted git repository.
hansva pushed a commit to branch main
in repository https://gitbox.apache.org/repos/asf/hop.git
The following commit(s) were added to refs/heads/main by this push:
new 64aa6cbcf2 Update transform should give clear error, fixes #4772 (#7831)
64aa6cbcf2 is described below
commit 64aa6cbcf258abadbead85201c30d64537f0382a
Author: Hans Van Akelyen <ha...@gmail.com>
AuthorDate: Fri Aug 7 14:00:01 2026 +0200
Update transform should give clear error, fixes #4772 (#7831)
---
.../hop/pipeline/transforms/update/Update.java | 19 +-
.../hop/pipeline/transforms/update/UpdateMeta.java | 11 +
.../update/messages/messages_en_US.properties | 1 +
.../transforms/update/UpdateDialogTest.java | 370 ++++++++++++++++++
.../pipeline/transforms/update/UpdateMetaTest.java | 39 ++
.../hop/pipeline/transforms/update/UpdateTest.java | 425 +++++++++++++++++++++
6 files changed, 863 insertions(+), 2 deletions(-)
diff --git a/plugins/transforms/update/src/main/java/org/apache/hop/pipeline/transforms/update/Update.java b/plugins/transforms/update/src/main/java/org/apache/hop/pipeline/transforms/update/Update.java
index a64af7442a..c18a130cac 100644
--- a/plugins/transforms/update/src/main/java/org/apache/hop/pipeline/transforms/update/Update.java
+++ b/plugins/transforms/update/src/main/java/org/apache/hop/pipeline/transforms/update/Update.java
@@ -358,7 +358,20 @@ public class Update extends BaseTransform<UpdateMeta, UpdateData> {
return true;
}
- public void setLookup(IRowMeta rowMeta) throws HopDatabaseException {
+ /**
+ * The lookup keys drive the WHERE clause of both the lookup SELECT and the UPDATE. Without a
+ * single key we would generate a statement ending in a dangling WHERE, so fail with a
+ * configuration error instead of letting the database report invalid SQL.
+ */
+ private void verifyLookupKeys() throws HopTransformException {
+ if (meta.getLookupField().getLookupKeys().isEmpty()) {
+ throw new HopTransformException(BaseMessages.getString(PKG, "Update.Exception.NoKeyFields"));
+ }
+ }
+
+ public void setLookup(IRowMeta rowMeta) throws HopException {
+ verifyLookupKeys();
+
data.lookupParameterRowMeta = new RowMeta();
data.lookupReturnRowMeta = new RowMeta();
@@ -437,7 +450,9 @@ public class Update extends BaseTransform<UpdateMeta, UpdateData> {
}
// Lookup certain fields in a table
- public void prepareUpdate(IRowMeta rowMeta) throws HopDatabaseException {
+ public void prepareUpdate(IRowMeta rowMeta) throws HopException {
+ verifyLookupKeys();
+
DatabaseMeta databaseMeta = getPipelineMeta().findDatabase(meta.getConnection(), variables);
data.updateParameterRowMeta = new RowMeta();
diff --git a/plugins/transforms/update/src/main/java/org/apache/hop/pipeline/transforms/update/UpdateMeta.java b/plugins/transforms/update/src/main/java/org/apache/hop/pipeline/transforms/update/UpdateMeta.java
index 79c7148e0e..695d1537ea 100644
--- a/plugins/transforms/update/src/main/java/org/apache/hop/pipeline/transforms/update/UpdateMeta.java
+++ b/plugins/transforms/update/src/main/java/org/apache/hop/pipeline/transforms/update/UpdateMeta.java
@@ -416,6 +416,17 @@ public class UpdateMeta extends BaseTransformMeta<Update, UpdateData> {
remarks.add(cr);
}
+ // Without lookup keys we cannot build the WHERE clause of the UPDATE statement, not even when
+ // the lookup is skipped.
+ if (lookupField.getLookupKeys().isEmpty()) {
+ cr =
+ new CheckResult(
+ ICheckResult.TYPE_RESULT_ERROR,
+ BaseMessages.getString(PKG, "UpdateMeta.CheckResult.MissingKeyFields"),
+ transformMeta);
+ remarks.add(cr);
+ }
+
// See if we have input streams leading to this transform!
if (input.length > 0) {
cr =
diff --git a/plugins/transforms/update/src/main/resources/org/apache/hop/pipeline/transforms/update/messages/messages_en_US.properties b/plugins/transforms/update/src/main/resources/org/apache/hop/pipeline/transforms/update/messages/messages_en_US.properties
index c27b64a57a..1c532e8fab 100644
--- a/plugins/transforms/update/src/main/resources/org/apache/hop/pipeline/transforms/update/messages/messages_en_US.properties
+++ b/plugins/transforms/update/src/main/resources/org/apache/hop/pipeline/transforms/update/messages/messages_en_US.properties
@@ -18,6 +18,7 @@
Update.Description=Update data in a database table based upon keys
Update.Exception.FieldRequired=Field [{0}] is required and couldn''t be found\!
Update.Exception.KeyCouldNotFound=Entry to update with following key could not be found\:
+Update.Exception.NoKeyFields=No key fields are specified. At least one key is required to build the WHERE clause of the UPDATE statement, also when the Skip lookup option is enabled.
Update.Init.ConnectionMissing=Database connection is missing for transform [{0}]\!
Update.Log.CheckingRow=Checking row\:
Update.Log.ConnectedToDB=Connected to database...
diff --git a/plugins/transforms/update/src/test/java/org/apache/hop/pipeline/transforms/update/UpdateDialogTest.java b/plugins/transforms/update/src/test/java/org/apache/hop/pipeline/transforms/update/UpdateDialogTest.java
new file mode 100644
index 0000000000..03c2b4d854
--- /dev/null
+++ b/plugins/transforms/update/src/test/java/org/apache/hop/pipeline/transforms/update/UpdateDialogTest.java
@@ -0,0 +1,370 @@
+/*
+ * 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.update;
+
+import static org.junit.jupiter.api.Assertions.assertEquals;
+import static org.junit.jupiter.api.Assertions.assertFalse;
+import static org.junit.jupiter.api.Assertions.assertNotNull;
+import static org.junit.jupiter.api.Assertions.assertTrue;
+
+import java.util.ArrayList;
+import java.util.List;
+import java.util.Map;
+import java.util.function.Consumer;
+import org.apache.hop.core.database.DatabaseMeta;
+import org.apache.hop.core.exception.HopException;
+import org.apache.hop.core.plugins.PluginRegistry;
+import org.apache.hop.core.plugins.TransformPluginType;
+import org.apache.hop.core.variables.Variables;
+import org.apache.hop.metadata.serializer.memory.MemoryMetadataProvider;
+import org.apache.hop.pipeline.PipelineMeta;
+import org.apache.hop.pipeline.transform.TransformMeta;
+import org.apache.hop.ui.testing.SwtBotTestBase;
+import org.eclipse.swt.widgets.Shell;
+import org.eclipse.swt.widgets.Table;
+import org.eclipse.swtbot.swt.finder.SWTBot;
+import org.eclipse.swtbot.swt.finder.waits.DefaultCondition;
+import org.eclipse.swtbot.swt.finder.widgets.SWTBotShell;
+import org.eclipse.swtbot.swt.finder.widgets.SWTBotTable;
+import org.junit.jupiter.api.Tag;
+import org.junit.jupiter.api.Test;
+
+/**
+ * End-to-end SWTBot coverage for the Update transform's {@link UpdateDialog}. The dialog runs its
+ * own blocking event loop in {@code open()}, so {@link SwtBotTestBase#withDialog} pumps it on the
+ * UI thread while the assertions drive it from a worker thread.
+ *
+ * <p>Both grids are Hop {@link org.apache.hop.ui.core.widget.TableView}s whose cell editors only
+ * materialise on click, so cell content is staged directly on the underlying SWT {@link Table} (on
+ * the UI thread) the way {@code ConstantDialogTest} does it. The real OK/Cancel buttons are then
+ * clicked so the dialog's own {@code ok()}/{@code cancel()} logic is what gets exercised.
+ *
+ * <p>Tagged {@code uitest} so it is skipped on headless machines; run with {@code mvn -pl
+ * plugins/transforms/update -Puitest test}.
+ */
+@Tag("uitest")
+class UpdateDialogTest extends SwtBotTestBase {
+
+ private static final String TRANSFORM_NAME = "update";
+ private static final String DIALOG_TITLE = "Update";
+ private static final String CONNECTION_NAME = "test-db";
+
+ // Tab order as built by open(): General, Lookup keys, Fields.
+ private static final int KEYS_TAB = 1;
+ private static final int FIELDS_TAB = 2;
+
+ // Text widgets in creation order. The connection is a CCombo, not a Text, so it does not shift
+ // these; address it with ccomboBox(0).
+ private static final int TRANSFORM_NAME_TEXT = 0;
+ private static final int SCHEMA_TEXT = 1;
+ private static final int TABLE_TEXT = 2;
+ private static final int COMMIT_TEXT = 3;
+ private static final int FLAG_FIELD_TEXT = 4;
+
+ // Check boxes in creation order, all on the General tab.
+ private static final int BATCH_CHECK = 0;
+ private static final int SKIP_LOOKUP_CHECK = 1;
+ private static final int ERROR_IGNORED_CHECK = 2;
+
+ // Grid layout: column 0 holds the row number, the mapped columns follow.
+ private static final int KEY_TABLE_FIELD_COLUMN = 1;
+ private static final int KEY_CONDITION_COLUMN = 2;
+ private static final int KEY_STREAM_COLUMN = 3;
+ private static final int KEY_STREAM2_COLUMN = 4;
+ private static final int UPDATE_TABLE_FIELD_COLUMN = 1;
+ private static final int UPDATE_STREAM_COLUMN = 2;
+
+ @Test
+ void existingMetaIsShownInTheDialog() throws Exception {
+ UpdateMeta meta =
+ metaWith(
+ List.of(
+ new UpdateKeyField("id_stream", "id", "=", ""),
+ new UpdateKeyField("from_stream", "day", "BETWEEN", "to_stream")),
+ List.of(new UpdateField("name", "name_stream")));
+ meta.setCommitSize("500");
+ meta.setUseBatchUpdate(true);
+ meta.setErrorIgnored(true);
+ meta.setIgnoreFlagField("was_found");
+
+ withDialog(
+ openerFor(meta),
+ bot -> {
+ SWTBot dialog = dialogBot(bot);
+
+ assertEquals(TRANSFORM_NAME, dialog.text(TRANSFORM_NAME_TEXT).getText());
+ assertEquals(CONNECTION_NAME, dialog.ccomboBox(0).getText());
+ assertEquals("myschema", dialog.text(SCHEMA_TEXT).getText());
+ assertEquals("mytable", dialog.text(TABLE_TEXT).getText());
+ assertEquals("500", dialog.text(COMMIT_TEXT).getText());
+ assertEquals("was_found", dialog.text(FLAG_FIELD_TEXT).getText());
+ assertTrue(dialog.checkBox(BATCH_CHECK).isChecked());
+ assertFalse(dialog.checkBox(SKIP_LOOKUP_CHECK).isChecked());
+ assertTrue(dialog.checkBox(ERROR_IGNORED_CHECK).isChecked());
+
+ SWTBotTable keys = grid(dialog, KEYS_TAB);
+ assertEquals("id", keys.cell(0, KEY_TABLE_FIELD_COLUMN));
+ assertEquals("=", keys.cell(0, KEY_CONDITION_COLUMN));
+ assertEquals("id_stream", keys.cell(0, KEY_STREAM_COLUMN));
+ assertEquals("day", keys.cell(1, KEY_TABLE_FIELD_COLUMN));
+ assertEquals("BETWEEN", keys.cell(1, KEY_CONDITION_COLUMN));
+ assertEquals("from_stream", keys.cell(1, KEY_STREAM_COLUMN));
+ assertEquals("to_stream", keys.cell(1, KEY_STREAM2_COLUMN));
+
+ SWTBotTable fields = grid(dialog, FIELDS_TAB);
+ assertEquals("name", fields.cell(0, UPDATE_TABLE_FIELD_COLUMN));
+ assertEquals("name_stream", fields.cell(0, UPDATE_STREAM_COLUMN));
+
+ dialog.button(buttonLabel("System.Button.Cancel")).click();
+ });
+ }
+
+ @Test
+ void okWritesTheEditedDialogBackToMeta() throws Exception {
+ UpdateMeta meta =
+ metaWith(
+ List.of(new UpdateKeyField("old_stream", "old_key", "=", "")),
+ List.of(new UpdateField("old_field", "old_field_stream")));
+
+ withDialog(
+ openerFor(meta),
+ bot -> {
+ SWTBot dialog = dialogBot(bot);
+
+ dialog.text(SCHEMA_TEXT).setText("newschema");
+ dialog.text(TABLE_TEXT).setText("newtable");
+ dialog.text(COMMIT_TEXT).setText("250");
+ dialog.checkBox(BATCH_CHECK).click();
+
+ setCells(
+ grid(dialog, KEYS_TAB).widget,
+ 0,
+ Map.of(
+ KEY_TABLE_FIELD_COLUMN, "id",
+ KEY_CONDITION_COLUMN, ">=",
+ KEY_STREAM_COLUMN, "id_stream",
+ KEY_STREAM2_COLUMN, ""));
+ setCells(
+ grid(dialog, FIELDS_TAB).widget,
+ 0,
+ Map.of(
+ UPDATE_TABLE_FIELD_COLUMN, "name",
+ UPDATE_STREAM_COLUMN, "name_stream"));
+
+ dialog.button(buttonLabel("System.Button.OK")).click();
+ });
+
+ assertEquals(CONNECTION_NAME, meta.getConnection());
+ assertEquals("newschema", meta.getLookupField().getSchemaName());
+ assertEquals("newtable", meta.getLookupField().getTableName());
+ assertEquals("250", meta.getCommitSizeVar());
+ assertTrue(meta.isUseBatchUpdate());
+
+ assertEquals(1, meta.getLookupField().getLookupKeys().size());
+ UpdateKeyField key = meta.getLookupField().getLookupKeys().get(0);
+ assertEquals("id", key.getKeyLookup());
+ assertEquals(">=", key.getKeyCondition());
+ assertEquals("id_stream", key.getKeyStream());
+
+ assertEquals(1, meta.getLookupField().getUpdateFields().size());
+ UpdateField field = meta.getLookupField().getUpdateFields().get(0);
+ assertEquals("name", field.getUpdateLookup());
+ assertEquals("name_stream", field.getUpdateStream());
+ }
+
+ /**
+ * Skipping the lookup means no SELECT is issued, so there is no lookup that could fail and no key
+ * to flag as found. The dialog has to clear and disable both options, and the cleared state is
+ * what gets saved. This is the UI half of the confusion behind issue #4772.
+ */
+ @Test
+ void skipLookupClearsAndDisablesTheLookupFailureOptions() throws Exception {
+ UpdateMeta meta =
+ metaWith(
+ List.of(new UpdateKeyField("id_stream", "id", "=", "")),
+ List.of(new UpdateField("name", "name_stream")));
+ meta.setErrorIgnored(true);
+ meta.setIgnoreFlagField("was_found");
+
+ withDialog(
+ openerFor(meta),
+ bot -> {
+ SWTBot dialog = dialogBot(bot);
+ assertTrue(dialog.checkBox(ERROR_IGNORED_CHECK).isChecked(), "precondition");
+ assertEquals("was_found", dialog.text(FLAG_FIELD_TEXT).getText(), "precondition");
+
+ dialog.checkBox(SKIP_LOOKUP_CHECK).click();
+
+ assertFalse(dialog.checkBox(ERROR_IGNORED_CHECK).isChecked());
+ assertFalse(dialog.checkBox(ERROR_IGNORED_CHECK).isEnabled());
+ assertEquals("", dialog.text(FLAG_FIELD_TEXT).getText());
+ assertFalse(dialog.text(FLAG_FIELD_TEXT).isEnabled());
+
+ dialog.button(buttonLabel("System.Button.OK")).click();
+ });
+
+ assertTrue(meta.isSkipLookup());
+ assertFalse(meta.isErrorIgnored());
+ assertEquals("", meta.getIgnoreFlagField());
+ }
+
+ /** Rows the user blanked out are dropped rather than saved as empty keys/fields. */
+ @Test
+ void blankGridRowsAreNotSaved() throws Exception {
+ UpdateMeta meta =
+ metaWith(
+ List.of(
+ new UpdateKeyField("id_stream", "id", "=", ""),
+ new UpdateKeyField("other_stream", "other", "=", "")),
+ List.of(
+ new UpdateField("name", "name_stream"), new UpdateField("label", "label_stream")));
+
+ withDialog(
+ openerFor(meta),
+ bot -> {
+ SWTBot dialog = dialogBot(bot);
+
+ setCells(
+ grid(dialog, KEYS_TAB).widget,
+ 1,
+ Map.of(
+ KEY_TABLE_FIELD_COLUMN, "",
+ KEY_CONDITION_COLUMN, "",
+ KEY_STREAM_COLUMN, "",
+ KEY_STREAM2_COLUMN, ""));
+ setCells(
+ grid(dialog, FIELDS_TAB).widget,
+ 1,
+ Map.of(UPDATE_TABLE_FIELD_COLUMN, "", UPDATE_STREAM_COLUMN, ""));
+
+ dialog.button(buttonLabel("System.Button.OK")).click();
+ });
+
+ assertEquals(1, meta.getLookupField().getLookupKeys().size());
+ assertEquals("id", meta.getLookupField().getLookupKeys().get(0).getKeyLookup());
+ assertEquals(1, meta.getLookupField().getUpdateFields().size());
+ assertEquals("name", meta.getLookupField().getUpdateFields().get(0).getUpdateLookup());
+ }
+
+ @Test
+ void cancelLeavesMetaUntouched() throws Exception {
+ UpdateMeta meta =
+ metaWith(
+ List.of(new UpdateKeyField("id_stream", "id", "=", "")),
+ List.of(new UpdateField("name", "name_stream")));
+
+ withDialog(
+ openerFor(meta),
+ bot -> {
+ SWTBot dialog = dialogBot(bot);
+
+ dialog.text(TABLE_TEXT).setText("discarded");
+ dialog.checkBox(SKIP_LOOKUP_CHECK).click();
+ setCells(
+ grid(dialog, KEYS_TAB).widget, 0, Map.of(KEY_TABLE_FIELD_COLUMN, "discarded_key"));
+
+ dialog.button(buttonLabel("System.Button.Cancel")).click();
+ });
+
+ assertEquals("mytable", meta.getLookupField().getTableName());
+ assertFalse(meta.isSkipLookup());
+ assertEquals("id", meta.getLookupField().getLookupKeys().get(0).getKeyLookup());
+ }
+
+ /**
+ * {@code createShell()} registers the dialog's shell well before {@code open()} reaches the event
+ * loop that puts it on screen, so the worker can find a shell that is not visible yet. Waiting
+ * for visibility before activating keeps that race out of the tests - this dialog builds three
+ * tabs, two grids and a background field lookup - and turns a dialog that fails to open into a
+ * clear message rather than an activation timeout.
+ */
+ private SWTBot dialogBot(SWTBot bot) {
+ SWTBotShell dialogShell = bot.shell(DIALOG_TITLE);
+ bot.waitUntil(
+ new DefaultCondition() {
+ @Override
+ public boolean test() {
+ return dialogShell.isOpen() && dialogShell.isVisible();
+ }
+
+ @Override
+ public String getFailureMessage() {
+ return "The " + DIALOG_TITLE + " dialog never became visible";
+ }
+ });
+ return dialogShell.activate().bot();
+ }
+
+ /**
+ * Activates the tab holding a grid and returns it. Only the visible tab's table is reachable, so
+ * after activating there is exactly one table to find.
+ */
+ private SWTBotTable grid(SWTBot dialog, int tabIndex) {
+ dialog.cTabItem(tabIndex).activate();
+ return dialog.table(0);
+ }
+
+ private Consumer<Shell> openerFor(UpdateMeta meta) throws HopException {
+ PipelineMeta pipelineMeta = pipelineWith(meta);
+ return parent -> new UpdateDialog(parent, new Variables(), meta, pipelineMeta).open();
+ }
+
+ /**
+ * Stages cell text on one grid row from the UI thread. The dialog's TableView installs its
+ * editors on click, so there is no SWTBot path to type into a cell; writing the item text leaves
+ * the same state the editors would, and {@code ok()} then reads it for real.
+ */
+ private void setCells(Table table, int row, Map<Integer, String> valuesByColumn) {
+ display.syncExec(
+ () -> valuesByColumn.forEach((column, value) -> table.getItem(row).setText(column, value)));
+ }
+
+ private static UpdateMeta metaWith(List<UpdateKeyField> keys, List<UpdateField> fields) {
+ UpdateMeta meta = new UpdateMeta();
+ // Same starting point a transform dropped on the canvas gets; notably it is what fills in the
+ // commit size, which getData() reads without a null guard.
+ meta.setDefault();
+ meta.setConnection(CONNECTION_NAME);
+ UpdateLookupField lookupField = new UpdateLookupField();
+ lookupField.setSchemaName("myschema");
+ lookupField.setTableName("mytable");
+ lookupField.setLookupKeys(new ArrayList<>(keys));
+ lookupField.setUpdateFields(new ArrayList<>(fields));
+ meta.setLookupField(lookupField);
+ return meta;
+ }
+
+ /**
+ * The dialog warns - through a native MessageBox that SWTBot cannot dismiss - when the configured
+ * connection cannot be resolved, so the pipeline gets a real (driverless "NONE") connection of
+ * that name to look up.
+ */
+ private static PipelineMeta pipelineWith(UpdateMeta meta) throws HopException {
+ DatabaseMeta databaseMeta = new DatabaseMeta();
+ databaseMeta.setName(CONNECTION_NAME);
+ MemoryMetadataProvider metadataProvider = new MemoryMetadataProvider();
+ metadataProvider.getSerializer(DatabaseMeta.class).save(databaseMeta);
+
+ String pluginId = PluginRegistry.getInstance().getPluginId(TransformPluginType.class, meta);
+ assertNotNull(pluginId, "Update transform must be registered via HopEnvironment.init()");
+ PipelineMeta pipelineMeta = new PipelineMeta();
+ pipelineMeta.setMetadataProvider(metadataProvider);
+ pipelineMeta.addTransform(new TransformMeta(pluginId, TRANSFORM_NAME, meta));
+ return pipelineMeta;
+ }
+}
diff --git a/plugins/transforms/update/src/test/java/org/apache/hop/pipeline/transforms/update/UpdateMetaTest.java b/plugins/transforms/update/src/test/java/org/apache/hop/pipeline/transforms/update/UpdateMetaTest.java
index 258ff1f723..fa6d12f02d 100644
--- a/plugins/transforms/update/src/test/java/org/apache/hop/pipeline/transforms/update/UpdateMetaTest.java
+++ b/plugins/transforms/update/src/test/java/org/apache/hop/pipeline/transforms/update/UpdateMetaTest.java
@@ -18,6 +18,7 @@
package org.apache.hop.pipeline.transforms.update;
import static org.junit.jupiter.api.Assertions.assertEquals;
+import static org.junit.jupiter.api.Assertions.assertTrue;
import static org.junit.jupiter.api.Assertions.fail;
import java.util.ArrayList;
@@ -29,11 +30,15 @@ import java.util.Random;
import java.util.UUID;
import org.apache.commons.lang3.builder.EqualsBuilder;
import org.apache.hop.core.HopEnvironment;
+import org.apache.hop.core.ICheckResult;
import org.apache.hop.core.exception.HopException;
import org.apache.hop.core.logging.ILoggingObject;
import org.apache.hop.core.plugins.PluginRegistry;
import org.apache.hop.core.plugins.TransformPluginType;
+import org.apache.hop.core.row.RowMeta;
+import org.apache.hop.core.variables.Variables;
import org.apache.hop.junit.rules.RestoreHopEngineEnvironmentExtension;
+import org.apache.hop.metadata.serializer.memory.MemoryMetadataProvider;
import org.apache.hop.pipeline.Pipeline;
import org.apache.hop.pipeline.PipelineMeta;
import org.apache.hop.pipeline.engines.local.LocalPipelineEngine;
@@ -247,6 +252,40 @@ class UpdateMetaTest implements IInitializer<ITransformMeta> {
}
}
+ /**
+ * The lookup keys build the WHERE clause of the UPDATE, so an empty key grid can only produce
+ * invalid SQL - check() has to say so at design time rather than let the database complain at
+ * runtime. See <a href="https://github.com/apache/hop/issues/4772">issue #4772</a>.
+ */
+ @Test
+ void checkReportsMissingKeyFields() {
+ UpdateLookupField lookupField = new UpdateLookupField();
+ lookupField.setTableName("table_name");
+ lookupField.setLookupKeys(new ArrayList<>());
+ lookupField.setUpdateFields(List.of(new UpdateField("name", "name")));
+ umi.setLookupField(lookupField);
+
+ List<ICheckResult> remarks = new ArrayList<>();
+ umi.check(
+ remarks,
+ null,
+ transformMeta,
+ new RowMeta(),
+ new String[] {"previous transform"},
+ new String[0],
+ null,
+ new Variables(),
+ new MemoryMetadataProvider());
+
+ assertTrue(
+ remarks.stream()
+ .anyMatch(
+ r ->
+ r.getType() == ICheckResult.TYPE_RESULT_ERROR
+ && r.getText().contains("No key fields are specified")),
+ "check() should flag the empty key grid, got: " + remarks);
+ }
+
// Call the allocate method on the LoadSaveTester meta class
@Override
public void modify(ITransformMeta someMeta) {
diff --git a/plugins/transforms/update/src/test/java/org/apache/hop/pipeline/transforms/update/UpdateTest.java b/plugins/transforms/update/src/test/java/org/apache/hop/pipeline/transforms/update/UpdateTest.java
new file mode 100644
index 0000000000..10d3e5d0ec
--- /dev/null
+++ b/plugins/transforms/update/src/test/java/org/apache/hop/pipeline/transforms/update/UpdateTest.java
@@ -0,0 +1,425 @@
+/*
+ * 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.update;
+
+import static org.junit.jupiter.api.Assertions.assertEquals;
+import static org.junit.jupiter.api.Assertions.assertFalse;
+import static org.junit.jupiter.api.Assertions.assertThrows;
+import static org.junit.jupiter.api.Assertions.assertTrue;
+import static org.mockito.ArgumentMatchers.any;
+import static org.mockito.ArgumentMatchers.anyBoolean;
+import static org.mockito.ArgumentMatchers.anyString;
+import static org.mockito.Mockito.mock;
+import static org.mockito.Mockito.never;
+import static org.mockito.Mockito.times;
+import static org.mockito.Mockito.verify;
+import static org.mockito.Mockito.when;
+
+import java.sql.Connection;
+import java.sql.PreparedStatement;
+import java.util.List;
+import org.apache.hop.core.QueueRowSet;
+import org.apache.hop.core.database.Database;
+import org.apache.hop.core.database.DatabaseMeta;
+import org.apache.hop.core.exception.HopException;
+import org.apache.hop.core.exception.HopTransformException;
+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.pipeline.transforms.mock.TransformMockHelper;
+import org.junit.jupiter.api.AfterEach;
+import org.junit.jupiter.api.BeforeEach;
+import org.junit.jupiter.api.Test;
+import org.mockito.ArgumentCaptor;
+
+/**
+ * Covers the SQL the Update transform builds and the decisions it makes per row. The transform is
+ * driven with a mocked {@link Database} so no server is needed: {@code data.db} is injected
+ * directly rather than through {@code init()}, which would open a real connection.
+ */
+class UpdateTest {
+
+ private static final String SCHEMA_TABLE = "\"db_name\".\"table_name\"";
+
+ private TransformMockHelper<UpdateMeta, UpdateData> helper;
+ private Database db;
+ private Connection connection;
+ private PreparedStatement lookupStatement;
+ private PreparedStatement updateStatement;
+
+ @BeforeEach
+ void setUp() throws Exception {
+ helper = new TransformMockHelper<>("update", UpdateMeta.class, UpdateData.class);
+ when(helper.logChannelFactory.create(any(), any())).thenReturn(helper.iLogChannel);
+ when(helper.pipeline.isRunning()).thenReturn(true);
+
+ DatabaseMeta databaseMeta = mock(DatabaseMeta.class);
+ when(databaseMeta.quoteField(anyString())).thenAnswer(i -> "\"" + i.getArgument(0) + "\"");
+ when(databaseMeta.stripCR(any(StringBuilder.class)))
+ .thenAnswer(i -> i.getArgument(0).toString().replace("\n", " ").replace("\r", " "));
+ when(helper.pipelineMeta.findDatabase(anyString(), any())).thenReturn(databaseMeta);
+
+ lookupStatement = mock(PreparedStatement.class);
+ updateStatement = mock(PreparedStatement.class);
+ connection = mock(Connection.class);
+ // Keyed on the statement itself rather than call order: with "skip lookup" on, prepareUpdate()
+ // is the only statement prepared.
+ when(connection.prepareStatement(anyString()))
+ .thenAnswer(
+ invocation ->
+ invocation.getArgument(0, String.class).startsWith("SELECT")
+ ? lookupStatement
+ : updateStatement);
+
+ db = mock(Database.class);
+ when(db.getConnection()).thenReturn(connection);
+ }
+
+ @AfterEach
+ void tearDown() {
+ helper.cleanUp();
+ }
+
+ // ---------------------------------------------------------------- SQL generation
+
+ @Test
+ void lookupSqlUsesEveryKeyCondition() throws Exception {
+ Update update = updateTransform(metaWith(false, allConditionKeys()), newData());
+
+ update.setLookup(wideRowMeta());
+
+ assertEquals(
+ "SELECT \"name\" FROM "
+ + SCHEMA_TABLE
+ + " WHERE ( ( \"id\" = ? ) ) AND ( ( \"day\" BETWEEN ? AND ? ) ) "
+ + " AND ( ( \"deleted\" IS NULL ) ) AND ( ( \"created\" IS NOT NULL ) ) "
+ + " AND ( ( \"note\" IS NULL AND ? IS NULL ) OR ( \"note\" = ? ) ) ",
+ capturePreparedSql());
+ }
+
+ @Test
+ void updateSqlUsesEveryKeyCondition() throws Exception {
+ Update update = updateTransform(metaWith(false, allConditionKeys()), newData());
+
+ update.prepareUpdate(wideRowMeta());
+
+ assertEquals(
+ "UPDATE "
+ + SCHEMA_TABLE
+ + " SET \"name\" = ? WHERE ( ( \"id\" = ? ) ) AND ( ( \"day\" BETWEEN ? AND ? ) ) "
+ + "AND ( ( \"deleted\" IS NULL ) ) AND ( ( \"created\" IS NOT NULL ) ) "
+ + "AND ( ( \"note\" IS NULL AND ? IS NULL ) OR ( \"note\" = ? ) ) ",
+ capturePreparedSql());
+ }
+
+ /** Every bound key contributes a parameter; IS NULL / IS NOT NULL bind nothing. */
+ @Test
+ void parameterRowMetaMatchesTheBoundPlaceholders() throws Exception {
+ UpdateData data = newData();
+ Update update = updateTransform(metaWith(false, allConditionKeys()), data);
+
+ update.setLookup(wideRowMeta());
+ update.prepareUpdate(wideRowMeta());
+
+ // "= ~NULL" binds its stream field twice (IS NULL check plus equality check); the second copy
+ // is a clone, which the row meta auto-renames to keep the names unique.
+ assertEquals(
+ List.of("id", "day", "to_day", "note", "note_1"),
+ data.lookupParameterRowMeta.getValueMetaList().stream().map(v -> v.getName()).toList());
+ assertEquals(
+ List.of("name", "id", "day", "to_day", "note", "note_1"),
+ data.updateParameterRowMeta.getValueMetaList().stream().map(v -> v.getName()).toList());
+ }
+
+ // ------------------------------------------------------- issue #4772: no keys configured
+
+ /**
+ * The lookup keys build the WHERE clause of both statements. An empty key grid used to produce
+ * SQL ending in a dangling WHERE, which the database rejected with a confusing syntax error. See
+ * <a href="https://github.com/apache/hop/issues/4772">issue #4772</a>.
+ */
+ @Test
+ void prepareUpdateWithoutKeysFails() {
+ Update update = updateTransform(metaWith(true, List.of()), newData());
+
+ assertThrows(HopTransformException.class, () -> update.prepareUpdate(simpleRowMeta()));
+ }
+
+ /** Not specific to "skip lookup": the lookup SELECT is just as broken without keys. */
+ @Test
+ void setLookupWithoutKeysFails() {
+ Update update = updateTransform(metaWith(false, List.of()), newData());
+
+ assertThrows(HopTransformException.class, () -> update.setLookup(simpleRowMeta()));
+ }
+
+ /** Skipping the lookup skips the SELECT, not the WHERE clause of the UPDATE. */
+ @Test
+ void skippingTheLookupStillFiltersOnTheKeys() throws Exception {
+ Update update = updateTransform(metaWith(true, List.of(idKey())), newData());
+
+ update.prepareUpdate(simpleRowMeta());
+
+ assertEquals(
+ "UPDATE " + SCHEMA_TABLE + " SET \"name\" = ? WHERE ( ( \"id\" = ? ) ) ",
+ capturePreparedSql());
+ }
+
+ // ---------------------------------------------------------------- per-row behaviour
+
+ @Test
+ void skippingTheLookupUpdatesWithoutSelecting() throws Exception {
+ Update update = runRows(metaWith(true, List.of(idKey())), row("1", "Alice"));
+
+ verify(db, never()).getLookup(any(PreparedStatement.class));
+ verify(db).insertRow(updateStatement, false, true);
+ assertEquals(1, update.getLinesUpdated());
+ assertEquals(0, update.getLinesSkipped());
+ }
+
+ @Test
+ void unchangedValuesAreSkippedInsteadOfUpdated() throws Exception {
+ when(db.getLookup(lookupStatement)).thenReturn(new Object[] {"Alice"});
+ when(db.getReturnRowMeta()).thenReturn(returnRowMeta());
+
+ Update update = runRows(metaWith(false, List.of(idKey())), row("1", "Alice"));
+
+ verify(db, never()).insertRow(any(PreparedStatement.class), anyBoolean(), anyBoolean());
+ assertEquals(0, update.getLinesUpdated());
+ assertEquals(1, update.getLinesSkipped());
+ }
+
+ @Test
+ void changedValuesAreUpdated() throws Exception {
+ when(db.getLookup(lookupStatement)).thenReturn(new Object[] {"Bob"});
+ when(db.getReturnRowMeta()).thenReturn(returnRowMeta());
+
+ Update update = runRows(metaWith(false, List.of(idKey())), row("1", "Alice"));
+
+ verify(db).insertRow(updateStatement, false, true);
+ assertEquals(1, update.getLinesUpdated());
+ assertEquals(0, update.getLinesSkipped());
+ }
+
+ /** With "ignore lookup failure" the row survives, flagged as not found. */
+ @Test
+ void missingKeyIsFlaggedWhenLookupFailureIsIgnored() throws Exception {
+ when(db.getLookup(lookupStatement)).thenReturn(null);
+
+ UpdateMeta meta = metaWith(false, List.of(idKey()));
+ meta.setErrorIgnored(true);
+ meta.setIgnoreFlagField("was_found");
+
+ QueueRowSet output = new QueueRowSet();
+ Update update = runRows(meta, output, row("1", "Alice"));
+
+ verify(db, never()).insertRow(any(PreparedStatement.class), anyBoolean(), anyBoolean());
+ assertEquals(0, update.getLinesUpdated());
+ Object[] emitted = output.getRow();
+ assertEquals(Boolean.FALSE, emitted[2], "the flag field must report the key was not found");
+ }
+
+ /** Without that option a missing key is an error, not a silent no-op. */
+ @Test
+ void missingKeyIsAnErrorByDefault() throws Exception {
+ when(db.getLookup(lookupStatement)).thenReturn(null);
+
+ Update update = runRows(metaWith(false, List.of(idKey())), row("1", "Alice"));
+
+ verify(db, never()).insertRow(any(PreparedStatement.class), anyBoolean(), anyBoolean());
+ assertEquals(1, update.getErrors());
+ }
+
+ @Test
+ void aKeyFieldMissingFromTheInputIsRejected() {
+ UpdateKeyField key = new UpdateKeyField("not_in_stream", "id", "=", "");
+ Update update =
+ transformFor(metaWith(false, List.of(key)), row("1", "Alice"), new QueueRowSet());
+
+ assertThrows(HopTransformException.class, update::processRow);
+ }
+
+ @Test
+ void anUpdateFieldMissingFromTheInputIsRejected() {
+ UpdateMeta meta = metaWith(false, List.of(idKey()));
+ meta.getLookupField().setUpdateFields(List.of(new UpdateField("name", "not_in_stream")));
+ Update update = transformFor(meta, row("1", "Alice"), new QueueRowSet());
+
+ assertThrows(HopTransformException.class, update::processRow);
+ }
+
+ @Test
+ void noInputEndsTheTransform() throws Exception {
+ QueueRowSet input = new QueueRowSet();
+ input.setDone();
+ Update update = updateTransform(metaWith(true, List.of(idKey())), newData());
+ update.setInputRowMeta(simpleRowMeta());
+ update.addRowSetToInputRowSets(input);
+ update.addRowSetToOutputRowSets(new QueueRowSet());
+
+ assertFalse(update.processRow());
+ }
+
+ // ---------------------------------------------------------------- lifecycle
+
+ @Test
+ void initFailsWithoutAConnection() {
+ UpdateMeta meta = metaWith(true, List.of(idKey()));
+ meta.setConnection(null);
+
+ assertFalse(updateTransform(meta, newData()).init());
+ }
+
+ @Test
+ void disposeCommitsAndReleasesTheStatements() throws Exception {
+ UpdateData data = newData();
+ data.db = db;
+ data.prepStatementLookup = lookupStatement;
+ data.prepStatementUpdate = updateStatement;
+ when(db.isAutoCommit()).thenReturn(false);
+
+ updateTransform(metaWith(true, List.of(idKey())), data).dispose();
+
+ verify(db).emptyAndCommit(updateStatement, false);
+ verify(db).closePreparedStatement(lookupStatement);
+ verify(db).disconnect();
+ }
+
+ @Test
+ void batchCompleteCommitsWithoutClosingTheStatements() throws Exception {
+ UpdateData data = newData();
+ data.db = db;
+ data.prepStatementUpdate = updateStatement;
+ when(db.isAutoCommit()).thenReturn(false);
+
+ updateTransform(metaWith(true, List.of(idKey())), data).batchComplete();
+
+ verify(db).commit();
+ verify(db, never()).disconnect();
+ }
+
+ @Test
+ void aFailedRowRollsBackOnDispose() throws Exception {
+ when(db.getLookup(lookupStatement)).thenReturn(null);
+ when(db.isAutoCommit()).thenReturn(false);
+
+ Update update = runRows(metaWith(false, List.of(idKey())), row("1", "Alice"));
+ assertTrue(update.getErrors() > 0, "precondition: the row must have failed");
+
+ update.dispose();
+
+ verify(db).rollback();
+ verify(db, never()).emptyAndCommit(any(PreparedStatement.class), anyBoolean());
+ }
+
+ // ---------------------------------------------------------------- fixture
+
+ private Update runRows(UpdateMeta meta, Object[] row) throws HopException {
+ return runRows(meta, new QueueRowSet(), row);
+ }
+
+ private Update runRows(UpdateMeta meta, QueueRowSet output, Object[] row) throws HopException {
+ Update update = transformFor(meta, row, output);
+ while (update.processRow()) {
+ // drain the input
+ }
+ return update;
+ }
+
+ private Update transformFor(UpdateMeta meta, Object[] row, QueueRowSet output) {
+ UpdateData data = newData();
+
+ QueueRowSet input = new QueueRowSet();
+ input.putRow(simpleRowMeta(), row);
+ input.setDone();
+
+ Update update = updateTransform(meta, data);
+ update.setInputRowMeta(simpleRowMeta());
+ update.addRowSetToInputRowSets(input);
+ update.addRowSetToOutputRowSets(output);
+ return update;
+ }
+
+ private Update updateTransform(UpdateMeta meta, UpdateData data) {
+ return new Update(helper.transformMeta, meta, data, 0, helper.pipelineMeta, helper.pipeline);
+ }
+
+ private UpdateData newData() {
+ UpdateData data = new UpdateData();
+ data.schemaTable = SCHEMA_TABLE;
+ data.db = db;
+ return data;
+ }
+
+ private String capturePreparedSql() throws Exception {
+ ArgumentCaptor<String> sql = ArgumentCaptor.forClass(String.class);
+ verify(connection, times(1)).prepareStatement(sql.capture());
+ return sql.getValue();
+ }
+
+ private static UpdateMeta metaWith(boolean skipLookup, List<UpdateKeyField> keys) {
+ UpdateMeta meta = new UpdateMeta();
+ meta.setDefault();
+ meta.setConnection("db");
+ meta.setSkipLookup(skipLookup);
+ UpdateLookupField lookupField = new UpdateLookupField();
+ lookupField.setTableName("table_name");
+ lookupField.setLookupKeys(keys);
+ lookupField.setUpdateFields(List.of(new UpdateField("name", "name")));
+ meta.setLookupField(lookupField);
+ return meta;
+ }
+
+ private static UpdateKeyField idKey() {
+ return new UpdateKeyField("id", "id", "=", "");
+ }
+
+ private static List<UpdateKeyField> allConditionKeys() {
+ return List.of(
+ new UpdateKeyField("id", "id", "=", ""),
+ new UpdateKeyField("day", "day", "BETWEEN", "to_day"),
+ new UpdateKeyField("", "deleted", "IS NULL", ""),
+ new UpdateKeyField("", "created", "IS NOT NULL", ""),
+ new UpdateKeyField("note", "note", "= ~NULL", ""));
+ }
+
+ private static IRowMeta simpleRowMeta() {
+ IRowMeta rowMeta = new RowMeta();
+ rowMeta.addValueMeta(new ValueMetaString("id"));
+ rowMeta.addValueMeta(new ValueMetaString("name"));
+ return rowMeta;
+ }
+
+ private static IRowMeta wideRowMeta() {
+ IRowMeta rowMeta = simpleRowMeta();
+ rowMeta.addValueMeta(new ValueMetaString("day"));
+ rowMeta.addValueMeta(new ValueMetaString("to_day"));
+ rowMeta.addValueMeta(new ValueMetaString("note"));
+ return rowMeta;
+ }
+
+ private static IRowMeta returnRowMeta() {
+ IRowMeta rowMeta = new RowMeta();
+ rowMeta.addValueMeta(new ValueMetaString("name"));
+ return rowMeta;
+ }
+
+ private static Object[] row(String id, String name) {
+ return new Object[] {id, name};
+ }
+}