This is an automated email from the ASF dual-hosted git repository.
mattcasters 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 d811c61030 Issue #8804 : Stop context action filters from logging a
null GUI plugin (#8805)
d811c61030 is described below
commit d811c61030d846c4ac8efa516a86f0553eec9bb5
Author: Matt Casters <[email protected]>
AuthorDate: Wed Oct 14 18:44:49 2026 +0200
Issue #8804 : Stop context action filters from logging a null GUI plugin
(#8805)
* Issue #8804 : Stop context action filters from logging a null GUI plugin
The filter id called getClass() on the class-name string, so the log said
java.lang.String. getInstance() is null when the clicked graph is not the
active tab, and that null was logged once per action. Use the graph the
context already holds, and prepare each filter once.
* Issue #8804 : Drop the context-filter changes already fixed on main
#8716 already stores the filter id from the plugin class name and finds the
clicked graph through getPipelineGraph() and getWorkflowGraph(). This leaves
the action lambda, which still has to use the graph that was clicked, and
the
one-time filter resolution.
---
.../gui/plugin/action/GuiActionLambdaBuilder.java | 38 ++++--
.../plugin/action/GuiActionLambdaBuilderTest.java | 140 +++++++++++++++++++++
.../ui/hopgui/context/BaseGuiContextHandler.java | 82 ++++++++----
.../context/BaseGuiContextHandlerFilterTest.java | 126 +++++++++++++++++++
4 files changed, 352 insertions(+), 34 deletions(-)
diff --git
a/core/src/main/java/org/apache/hop/core/gui/plugin/action/GuiActionLambdaBuilder.java
b/core/src/main/java/org/apache/hop/core/gui/plugin/action/GuiActionLambdaBuilder.java
index efe42feb8d..919162ec86 100644
---
a/core/src/main/java/org/apache/hop/core/gui/plugin/action/GuiActionLambdaBuilder.java
+++
b/core/src/main/java/org/apache/hop/core/gui/plugin/action/GuiActionLambdaBuilder.java
@@ -64,21 +64,35 @@ public class GuiActionLambdaBuilder<T> {
"Unable to find class '" + guiAction.getGuiPluginClassName() +
"'", e);
}
- Object guiPlugin;
-
- try {
- Method getInstanceMethod =
guiPluginClass.getDeclaredMethod("getInstance");
- guiPlugin = getInstanceMethod.invoke(null, (Object[]) null);
- } catch (Exception nsme) {
- // On the rebound we'll try to simply construct a new instance...
- // This makes the plugins even simpler.
- //
+ // The caller passes the graph the click happened on. getInstance() only
sees the active
+ // tab and returns null when that graph is not the active file, which is
how the UI tests
+ // drive a canvas.
+ Object guiPlugin = null;
+ if (guiPluginClass.isInstance(refresher)) {
+ guiPlugin = refresher;
+ } else {
try {
- guiPlugin = guiPluginClass.getDeclaredConstructor().newInstance();
- } catch (Exception e) {
- throw nsme;
+ Method getInstanceMethod =
guiPluginClass.getDeclaredMethod("getInstance");
+ guiPlugin = getInstanceMethod.invoke(null, (Object[]) null);
+ } catch (Exception nsme) {
+ // On the rebound we'll try to simply construct a new instance...
+ // This makes the plugins even simpler.
+ //
+ try {
+ guiPlugin = guiPluginClass.getDeclaredConstructor().newInstance();
+ } catch (Exception e) {
+ throw nsme;
+ }
}
}
+ if (guiPlugin == null) {
+ throw new HopException(
+ "No instance of "
+ + guiAction.getGuiPluginClassName()
+ + " for action "
+ + action.getId()
+ + ". getInstance() returned null.");
+ }
Method method =
guiPluginClass.getMethod(action.getGuiPluginMethodName(),
methodParameter.getClass());
diff --git
a/core/src/test/java/org/apache/hop/core/gui/plugin/action/GuiActionLambdaBuilderTest.java
b/core/src/test/java/org/apache/hop/core/gui/plugin/action/GuiActionLambdaBuilderTest.java
new file mode 100644
index 0000000000..1485d2a4cd
--- /dev/null
+++
b/core/src/test/java/org/apache/hop/core/gui/plugin/action/GuiActionLambdaBuilderTest.java
@@ -0,0 +1,140 @@
+/*
+ * 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.core.gui.plugin.action;
+
+import static org.junit.jupiter.api.Assertions.assertThrows;
+import static org.junit.jupiter.api.Assertions.assertTrue;
+
+import org.apache.hop.core.exception.HopRuntimeException;
+import org.apache.hop.core.gui.plugin.IGuiRefresher;
+import org.junit.jupiter.api.Test;
+
+class GuiActionLambdaBuilderTest {
+
+ @Test
+ void actionRunsOnTheRefresherWhenGetInstanceReturnsNull() throws Exception {
+ ClickedGraph.active = null;
+ ClickedGraph graph = new ClickedGraph();
+ GuiAction action =
+ new GuiAction(
+ "mark",
+ GuiActionType.Modify,
+ "mark",
+ "mark",
+ null,
+ ClickedGraph.class.getName(),
+ "mark");
+
+ GuiAction built =
+ new GuiActionLambdaBuilder<Marker>().createLambda(action, new
Marker(), graph);
+ built.getActionLambda().executeAction(false, false);
+
+ assertTrue(graph.marked, "the action runs on the graph that was clicked");
+ assertTrue(graph.updated, "that graph is refreshed afterwards");
+ }
+
+ @Test
+ void actionUsesGetInstanceWhenTheRefresherIsADifferentClass() throws
Exception {
+ Plugin.instance.marked = false;
+ OtherRefresher refresher = new OtherRefresher();
+ GuiAction action =
+ new GuiAction(
+ "mark", GuiActionType.Modify, "mark", "mark", null,
Plugin.class.getName(), "mark");
+
+ GuiAction built =
+ new GuiActionLambdaBuilder<Marker>().createLambda(action, new
Marker(), refresher);
+ built.getActionLambda().executeAction(false, false);
+
+ assertTrue(Plugin.instance.marked, "a plugin action still uses its
singleton");
+ assertTrue(refresher.updated);
+ }
+
+ @Test
+ void missingPluginInstanceFailsWhenTheActionIsBuilt() {
+ GuiAction action =
+ new GuiAction(
+ "mark",
+ GuiActionType.Modify,
+ "mark",
+ "mark",
+ null,
+ MissingPlugin.class.getName(),
+ "mark");
+
+ HopRuntimeException exception =
+ assertThrows(
+ HopRuntimeException.class,
+ () ->
+ new GuiActionLambdaBuilder<Marker>()
+ .createLambda(action, new Marker(), new OtherRefresher()));
+ assertTrue(exception.getMessage().contains(MissingPlugin.class.getName()));
+ }
+
+ public static final class Marker {}
+
+ /** Stands in for a pipeline or workflow graph that is not the active tab. */
+ public static final class ClickedGraph implements IGuiRefresher {
+ static ClickedGraph active;
+ boolean marked;
+ boolean updated;
+
+ public static ClickedGraph getInstance() {
+ return active;
+ }
+
+ public void mark(Marker marker) {
+ marked = marker != null;
+ }
+
+ @Override
+ public void updateGui() {
+ updated = true;
+ }
+ }
+
+ /** {@code getInstance()} returns null and the refresher is a different
class. */
+ public static final class MissingPlugin {
+ public static MissingPlugin getInstance() {
+ return null;
+ }
+
+ public void mark(Marker marker) {}
+ }
+
+ public static final class Plugin {
+ static final Plugin instance = new Plugin();
+ boolean marked;
+
+ public static Plugin getInstance() {
+ return instance;
+ }
+
+ public void mark(Marker marker) {
+ marked = marker != null;
+ }
+ }
+
+ private static final class OtherRefresher implements IGuiRefresher {
+ boolean updated;
+
+ @Override
+ public void updateGui() {
+ updated = true;
+ }
+ }
+}
diff --git
a/ui/src/main/java/org/apache/hop/ui/hopgui/context/BaseGuiContextHandler.java
b/ui/src/main/java/org/apache/hop/ui/hopgui/context/BaseGuiContextHandler.java
index 1fb360517d..30302b559a 100644
---
a/ui/src/main/java/org/apache/hop/ui/hopgui/context/BaseGuiContextHandler.java
+++
b/ui/src/main/java/org/apache/hop/ui/hopgui/context/BaseGuiContextHandler.java
@@ -62,31 +62,15 @@ public abstract class BaseGuiContextHandler<T extends
IGuiContextHandler> {
List<GuiActionFilter> actionFilters =
GuiRegistry.getInstance().getGuiContextActionFilters(getContextId());
- // Evaluate all the actions and see if any of the filters remove it from
the list...
+ // A filter that cannot be created (for example getInstance() returned
null) used to throw
+ // once per action and print a full stack trace every time. Resolve each
filter once. A
+ // failure leaves every action visible, which is what the per-action catch
did before.
//
+ List<ResolvedActionFilter> resolvedFilters =
resolveActionFilters(actionFilters);
+
List<GuiAction> filteredActions = new ArrayList<>();
for (GuiAction action : actions) {
- boolean retain = true;
- if (actionFilters != null) {
- for (GuiActionFilter actionFilter : actionFilters) {
- boolean retainAction = false;
- try {
- retainAction = evaluateActionFilter(action, actionFilter);
- if (!retainAction) {
- retain = false;
- break;
- }
- } catch (HopException e) {
- LogChannel.UI.logError(
- "Error filtering out action "
- + action.getId()
- + " with filter "
- + actionFilter.getId(),
- e);
- }
- }
- }
- if (retain) {
+ if (isActionRetained(action, resolvedFilters)) {
filteredActions.add(action);
}
}
@@ -99,6 +83,47 @@ public abstract class BaseGuiContextHandler<T extends
IGuiContextHandler> {
return actions;
}
+ private List<ResolvedActionFilter>
resolveActionFilters(List<GuiActionFilter> actionFilters) {
+ if (actionFilters == null) {
+ return Collections.emptyList();
+ }
+ List<ResolvedActionFilter> resolvedFilters = new ArrayList<>();
+ for (GuiActionFilter actionFilter : actionFilters) {
+ try {
+ Object guiPlugin = getFilterObject(actionFilter);
+ Method method = getFilterMethod(guiPlugin.getClass(), actionFilter);
+ resolvedFilters.add(new ResolvedActionFilter(actionFilter, guiPlugin,
method));
+ } catch (HopException e) {
+ LogChannel.UI.logError(
+ "Error preparing action filter "
+ + actionFilter.getId()
+ + ". Actions stay visible because this filter could not be
created.",
+ e);
+ }
+ }
+ return resolvedFilters;
+ }
+
+ private boolean isActionRetained(GuiAction action,
List<ResolvedActionFilter> resolvedFilters) {
+ for (ResolvedActionFilter resolvedFilter : resolvedFilters) {
+ try {
+ boolean retainAction =
+ (boolean) resolvedFilter.method.invoke(resolvedFilter.guiPlugin,
action.getId(), this);
+ if (!retainAction) {
+ return false;
+ }
+ } catch (Exception e) {
+ LogChannel.UI.logError(
+ "Error filtering out action "
+ + action.getId()
+ + " with filter "
+ + resolvedFilter.actionFilter.getId(),
+ e);
+ }
+ }
+ return true;
+ }
+
public ClassLoader findClassLoader(GuiActionFilter actionFilter) {
if (actionFilter.getClassLoader() != null) {
return actionFilter.getClassLoader();
@@ -238,4 +263,17 @@ public abstract class BaseGuiContextHandler<T extends
IGuiContextHandler> {
e);
}
}
+
+ /** A filter whose plugin instance and method were loaded once for this
context. */
+ private static final class ResolvedActionFilter {
+ private final GuiActionFilter actionFilter;
+ private final Object guiPlugin;
+ private final Method method;
+
+ private ResolvedActionFilter(GuiActionFilter actionFilter, Object
guiPlugin, Method method) {
+ this.actionFilter = actionFilter;
+ this.guiPlugin = guiPlugin;
+ this.method = method;
+ }
+ }
}
diff --git
a/ui/src/test/java/org/apache/hop/ui/hopgui/context/BaseGuiContextHandlerFilterTest.java
b/ui/src/test/java/org/apache/hop/ui/hopgui/context/BaseGuiContextHandlerFilterTest.java
new file mode 100644
index 0000000000..1d2e473aa6
--- /dev/null
+++
b/ui/src/test/java/org/apache/hop/ui/hopgui/context/BaseGuiContextHandlerFilterTest.java
@@ -0,0 +1,126 @@
+/*
+ * 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.ui.hopgui.context;
+
+import static org.junit.jupiter.api.Assertions.assertEquals;
+
+import java.lang.reflect.Method;
+import java.util.ArrayList;
+import java.util.List;
+import org.apache.hop.core.action.GuiContextAction;
+import org.apache.hop.core.action.GuiContextActionFilter;
+import org.apache.hop.core.gui.plugin.GuiRegistry;
+import org.apache.hop.core.gui.plugin.action.GuiAction;
+import org.apache.hop.core.gui.plugin.action.GuiActionType;
+import org.apache.hop.core.logging.HopLogStore;
+import org.junit.jupiter.api.Test;
+
+/**
+ * A filter that cannot be created used to throw once per action. Resolving it
once keeps every
+ * action visible and logs the failure a single time.
+ */
+class BaseGuiContextHandlerFilterTest {
+
+ private static final String PREPARE_ONCE =
"BaseGuiContextHandlerFilterTest-prepare-once";
+
+ @Test
+ void brokenFilterIsPreparedOnceForEveryAction() throws Exception {
+ HopLogStore.init();
+ registerPrepareOnceActions();
+ CountingPlugin.lookups = 0;
+
+ List<String> actionIds = new ArrayList<>();
+ for (Object action : new PrepareOnceContext().getPluginActions(true)) {
+ actionIds.add(((GuiAction) action).getId());
+ }
+
+ assertEquals(1, CountingPlugin.lookups, "a broken filter is created once,
not once per action");
+ assertEquals(List.of("prepare-once-a", "prepare-once-b"), actionIds);
+ }
+
+ private static void registerPrepareOnceActions() throws Exception {
+ GuiRegistry registry = GuiRegistry.getInstance();
+ if (registry.getGuiContextActions(PREPARE_ONCE) != null) {
+ return;
+ }
+ Class<?> pluginClass = CountingPlugin.class;
+ ClassLoader classLoader = pluginClass.getClassLoader();
+ for (String methodName : List.of("actionA", "actionB")) {
+ Method method = pluginClass.getDeclaredMethod(methodName,
PrepareOnceContext.class);
+ registry.addGuiContextAction(
+ pluginClass.getName(), method,
method.getAnnotation(GuiContextAction.class), classLoader);
+ }
+ Method filterMethod =
+ pluginClass.getDeclaredMethod("filterActions", String.class,
PrepareOnceContext.class);
+ registry.addGuiActionFilter(
+ pluginClass.getName(),
+ filterMethod,
+ filterMethod.getAnnotation(GuiContextActionFilter.class),
+ classLoader);
+ }
+
+ /**
+ * {@code getInstance()} returns null and there is no zero-arg constructor.
Preparing the menu
+ * calls that lookup once.
+ */
+ public static final class CountingPlugin {
+ static int lookups;
+
+ @SuppressWarnings("unused")
+ private final int marker;
+
+ private CountingPlugin(int marker) {
+ this.marker = marker;
+ }
+
+ public static CountingPlugin getInstance() {
+ lookups++;
+ return null;
+ }
+
+ @GuiContextAction(
+ id = "prepare-once-b",
+ parentId = PREPARE_ONCE,
+ type = GuiActionType.Modify,
+ name = "B",
+ tooltip = "B",
+ image = "b.svg")
+ public void actionB(PrepareOnceContext context) {}
+
+ @GuiContextAction(
+ id = "prepare-once-a",
+ parentId = PREPARE_ONCE,
+ type = GuiActionType.Modify,
+ name = "A",
+ tooltip = "A",
+ image = "a.svg")
+ public void actionA(PrepareOnceContext context) {}
+
+ @GuiContextActionFilter(parentId = PREPARE_ONCE)
+ public boolean filterActions(String contextActionId, PrepareOnceContext
context) {
+ return false;
+ }
+ }
+
+ public static final class PrepareOnceContext extends BaseGuiContextHandler {
+ @Override
+ public String getContextId() {
+ return PREPARE_ONCE;
+ }
+ }
+}
