bamaer commented on code in PR #9203:
URL: https://github.com/apache/hop/pull/9203#discussion_r4439716828
##########
plugins/transforms/json/src/main/java/org/apache/hop/pipeline/transforms/jsoninput/JsonInputDialog.java:
##########
@@ -1132,28 +1132,35 @@ public void widgetSelected(SelectionEvent
selectionEvent) {
// ///////////////////////////////////////////////////////////
}
+ /**
+ * Load previous-transform fields into the source combo. {@code setItems}
clears the current text,
+ * so the lookup runs first and the configured name is written back
afterwards. A failed lookup
+ * leaves that name in place. OK would otherwise store an empty source field.
+ */
private void setSourceStreamField(boolean isShowErrorDialog) {
+ String configured = wFieldValue.getText();
try {
- String value = wFieldValue.getText();
- wFieldValue.removeAll();
-
- IRowMeta r = pipelineMeta.getPrevTransformFields(variables,
transformName);
- if (r != null) {
- wFieldValue.setItems(r.getFieldNames());
- }
- if (value != null) {
- wFieldValue.setText(value);
+ IRowMeta row = pipelineMeta.getPrevTransformFields(variables,
transformName);
+ if (row != null) {
+ wFieldValue.setItems(row.getFieldNames());
}
- } catch (HopException ke) {
+ restoreSourceField(configured);
+ } catch (HopException exception) {
+ restoreSourceField(configured);
if (!isShowErrorDialog) {
return;
}
-
new ErrorDialog(
shell,
BaseMessages.getString(PKG,
"JsonInputDialog.FailedToGetFields.DialogTitle"),
BaseMessages.getString(PKG,
"JsonInputDialog.FailedToGetFields.DialogMessage"),
- ke);
+ exception);
+ }
+ }
+
+ private void restoreSourceField(String configured) {
Review Comment:
#9204 adds `ComboItems.setItemsKeepingText(CCombo, String[])`, which does
the same thing: read the text, replace the items, write the text back. If both
PRs are merged as they are, JSON Input keeps its own copy of that logic.
Options: rebase on #9204 and call the helper here, or switch JSON Input over in
#9204 after this PR is merged.
##########
plugins/transforms/json/src/test/java/org/apache/hop/pipeline/transforms/jsoninput/JsonInputSourceFieldDialogTest.java:
##########
@@ -0,0 +1,175 @@
+/*
+ * 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.jsoninput;
+
+import static org.junit.jupiter.api.Assertions.assertEquals;
+
+import java.lang.reflect.Field;
+import java.util.concurrent.atomic.AtomicReference;
+import org.apache.hop.core.exception.HopTransformException;
+import org.apache.hop.core.gui.plugin.GuiRegistry;
+import org.apache.hop.core.gui.plugin.GuiWidgetElement;
+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.IVariables;
+import org.apache.hop.core.variables.Variables;
+import org.apache.hop.i18n.BaseMessages;
+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.swtbot.swt.finder.SWTBot;
+import org.eclipse.swtbot.swt.finder.utils.SWTBotPreferences;
+import org.eclipse.swtbot.swt.finder.widgets.SWTBotCCombo;
+import org.junit.jupiter.api.AfterAll;
+import org.junit.jupiter.api.BeforeAll;
+import org.junit.jupiter.api.Tag;
+import org.junit.jupiter.api.Test;
+
+/** The source field stays put when the previous transform cannot supply its
fields. */
+@Tag("uitest")
+class JsonInputSourceFieldDialogTest extends SwtBotTestBase {
+
+ private static final String CONFIGURED_FIELD = "id_json";
+
+ /**
+ * JSON Input builds several tables before its event loop starts. SWTBot's 5
s default expires
+ * while that construction is still on the UI thread.
+ */
+ private static final long DIALOG_TIMEOUT_MS = 30_000L;
+
+ private static long defaultTimeout;
+
+ @BeforeAll
+ static void registerWidgets() {
+ GuiRegistry registry = GuiRegistry.getInstance();
+ for (Field field : JsonInputMeta.class.getDeclaredFields()) {
+ GuiWidgetElement element = field.getAnnotation(GuiWidgetElement.class);
+ if (element != null) {
+ registry.addGuiWidgetElement(JsonInputMeta.class.getName(), element,
field);
+ }
+ }
+ }
+
+ @BeforeAll
+ static void slowDownSwtBot() {
+ defaultTimeout = SWTBotPreferences.TIMEOUT;
+ SWTBotPreferences.TIMEOUT = DIALOG_TIMEOUT_MS;
+ }
+
+ @AfterAll
+ static void restoreSwtBotTimeout() {
+ SWTBotPreferences.TIMEOUT = defaultTimeout;
+ }
+
+ @Test
+ void failedLookupKeepsTheConfiguredSourceField() {
+ JsonInputMeta meta = sourceMeta();
+ withDialog(
+ parent ->
+ new JsonInputDialog(parent, new Variables(), meta, new
MissingFieldsPipeline(meta))
+ .open(),
+ bot -> {
+ dismissFieldLookupError();
Review Comment:
Minor: if a second error dialog appeared, this test would probably fail on
an SWTBot timeout rather than on an assertion. Only relevant if the description
keeps the "shown once" claim.
##########
plugins/transforms/json/src/main/java/org/apache/hop/pipeline/transforms/jsoninput/JsonInputDialog.java:
##########
@@ -1132,28 +1132,35 @@ public void widgetSelected(SelectionEvent
selectionEvent) {
// ///////////////////////////////////////////////////////////
}
+ /**
+ * Load previous-transform fields into the source combo. {@code setItems}
clears the current text,
+ * so the lookup runs first and the configured name is written back
afterwards. A failed lookup
+ * leaves that name in place. OK would otherwise store an empty source field.
+ */
Review Comment:
`wFieldValue` is created `READ_ONLY` and then `setEditable(true)`.
`CCombo.setItems` only clears the text when the combo is not editable (`if
(!text.getEditable()) text.setText("")`), so it leaves the text alone here. It
was `removeAll()` that cleared it. Suggest naming `removeAll()` in the comment
so nobody adds it back. The PR description makes the same statement about
`setItems`.
##########
plugins/transforms/json/src/test/java/org/apache/hop/pipeline/transforms/jsoninput/JsonInputSourceFieldDialogTest.java:
##########
@@ -0,0 +1,175 @@
+/*
+ * 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.jsoninput;
+
+import static org.junit.jupiter.api.Assertions.assertEquals;
+
+import java.lang.reflect.Field;
+import java.util.concurrent.atomic.AtomicReference;
+import org.apache.hop.core.exception.HopTransformException;
+import org.apache.hop.core.gui.plugin.GuiRegistry;
+import org.apache.hop.core.gui.plugin.GuiWidgetElement;
+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.IVariables;
+import org.apache.hop.core.variables.Variables;
+import org.apache.hop.i18n.BaseMessages;
+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.swtbot.swt.finder.SWTBot;
+import org.eclipse.swtbot.swt.finder.utils.SWTBotPreferences;
+import org.eclipse.swtbot.swt.finder.widgets.SWTBotCCombo;
+import org.junit.jupiter.api.AfterAll;
+import org.junit.jupiter.api.BeforeAll;
+import org.junit.jupiter.api.Tag;
+import org.junit.jupiter.api.Test;
+
+/** The source field stays put when the previous transform cannot supply its
fields. */
+@Tag("uitest")
+class JsonInputSourceFieldDialogTest extends SwtBotTestBase {
+
+ private static final String CONFIGURED_FIELD = "id_json";
+
+ /**
+ * JSON Input builds several tables before its event loop starts. SWTBot's 5
s default expires
+ * while that construction is still on the UI thread.
+ */
+ private static final long DIALOG_TIMEOUT_MS = 30_000L;
+
+ private static long defaultTimeout;
+
+ @BeforeAll
+ static void registerWidgets() {
+ GuiRegistry registry = GuiRegistry.getInstance();
+ for (Field field : JsonInputMeta.class.getDeclaredFields()) {
+ GuiWidgetElement element = field.getAnnotation(GuiWidgetElement.class);
+ if (element != null) {
+ registry.addGuiWidgetElement(JsonInputMeta.class.getName(), element,
field);
+ }
+ }
+ }
+
+ @BeforeAll
+ static void slowDownSwtBot() {
+ defaultTimeout = SWTBotPreferences.TIMEOUT;
+ SWTBotPreferences.TIMEOUT = DIALOG_TIMEOUT_MS;
+ }
+
+ @AfterAll
+ static void restoreSwtBotTimeout() {
+ SWTBotPreferences.TIMEOUT = defaultTimeout;
+ }
+
+ @Test
+ void failedLookupKeepsTheConfiguredSourceField() {
+ JsonInputMeta meta = sourceMeta();
+ withDialog(
+ parent ->
+ new JsonInputDialog(parent, new Variables(), meta, new
MissingFieldsPipeline(meta))
+ .open(),
+ bot -> {
+ dismissFieldLookupError();
+ SWTBot dialog = jsonDialog(bot);
+ assertEquals(CONFIGURED_FIELD, sourceCombo(dialog).getText());
+ dialog.button(buttonLabel("System.Button.OK")).click();
+ });
+ assertEquals(CONFIGURED_FIELD, meta.getFieldValue());
+ }
+
+ @Test
+ void loadedFieldsKeepANameThatIsNotInTheList() {
Review Comment:
This test also passes on the old code, because the old code restored the
text after a successful lookup. It's fine as a regression guard, but it doesn't
cover the reported bug.
--
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]
