This is an automated email from the ASF dual-hosted git repository. lukaszlenart pushed a commit to branch fix/WW-4421-duplicate-action-name-detection in repository https://gitbox.apache.org/repos/asf/struts.git
commit c2674aad19c337c7d106695bebc9054f66f78f7f Author: Lukasz Lenart <[email protected]> AuthorDate: Sat Feb 21 14:54:20 2026 +0100 fix(convention): WW-4421 detect duplicate @Action names when execute() is annotated The duplicate @Action name detection in PackageBasedActionConfigBuilder was embedded inside a conditional block that only ran when execute() was NOT annotated with @Action. This meant two methods could map to the same action name silently when execute() had an @Action annotation, with one overwriting the other non-deterministically. Extract the duplicate check to run unconditionally before the conditional block, so it applies to all annotated methods regardless of whether execute() is annotated. Backport of apache/struts#1579 from Struts 7.x to 6.x. 🤖 Generated with [Claude Code](https://claude.com/claude-code) Co-Authored-By: Claude <[email protected]> --- .../PackageBasedActionConfigBuilder.java | 78 ++++++++++------ .../PackageBasedActionConfigBuilderTest.java | 103 ++++++++++++++++++++- .../result/ActionLevelResultsNamesAction.java | 10 +- ...cateActionNameWithExecuteAnnotationAction.java} | 27 ++---- ...eActionNameWithoutExecuteAnnotationAction.java} | 31 +++---- 5 files changed, 178 insertions(+), 71 deletions(-) diff --git a/plugins/convention/src/main/java/org/apache/struts2/convention/PackageBasedActionConfigBuilder.java b/plugins/convention/src/main/java/org/apache/struts2/convention/PackageBasedActionConfigBuilder.java index 71331d4ef..c4b94376d 100644 --- a/plugins/convention/src/main/java/org/apache/struts2/convention/PackageBasedActionConfigBuilder.java +++ b/plugins/convention/src/main/java/org/apache/struts2/convention/PackageBasedActionConfigBuilder.java @@ -716,42 +716,28 @@ public class PackageBasedActionConfigBuilder implements ActionConfigBuilder { // Verify that the annotations have no errors and also determine if the default action // configuration should still be built or not. - Map<String, List<Action>> map = getActionAnnotations(actionClass); - Set<String> actionNames = new HashSet<>(); + Map<String, List<Action>> actionAnnotationsByMethod = getActionAnnotations(actionClass); boolean hasDefaultMethod = ReflectionTools.containsMethod(actionClass, DEFAULT_METHOD); - if (!map.containsKey(DEFAULT_METHOD) - && hasDefaultMethod - && actionAnnotation == null && actionsAnnotation == null - && (alwaysMapExecute || map.isEmpty())) { - boolean found = false; - for (List<Action> actions : map.values()) { - for (Action action : actions) { - - // Check if there are duplicate action names in the annotations. - String actionName = action.value().equals(Action.DEFAULT_VALUE) ? defaultActionName : action.value(); - if (actionNames.contains(actionName)) { - throw new ConfigurationException("The action class [" + actionClass + - "] contains two methods with an action name annotation whose value " + - "is the same (they both might be empty as well)."); - } else { - actionNames.add(actionName); - } - - // Check this annotation is the default action - if (action.value().equals(Action.DEFAULT_VALUE)) { - found = true; - } + + // Check for duplicate action names across all annotated methods + Set<String> actionNames = new HashSet<>(); + for (List<Action> actions : actionAnnotationsByMethod.values()) { + for (Action action : actions) { + String actionName = action.value().equals(Action.DEFAULT_VALUE) ? defaultActionName : action.value(); + if (!actionNames.add(actionName)) { + throw new ConfigurationException("The action class [" + actionClass + + "] contains two methods with an action name annotation whose value " + + "is the same (they both might be empty as well)."); } } + } - // Build the default - if (!found) { - createActionConfig(defaultPackageConfig, actionClass, defaultActionName, DEFAULT_METHOD, null, allowedMethods); - } + if (shouldMapDefaultExecuteMethod(actionAnnotationsByMethod, hasDefaultMethod, actionAnnotation, actionsAnnotation)) { + createActionConfig(defaultPackageConfig, actionClass, defaultActionName, DEFAULT_METHOD, null, allowedMethods); } // Build the actions for the annotations - for (Map.Entry<String, List<Action>> entry : map.entrySet()) { + for (Map.Entry<String, List<Action>> entry : actionAnnotationsByMethod.entrySet()) { String method = entry.getKey(); List<Action> actions = entry.getValue(); for (Action action : actions) { @@ -767,7 +753,7 @@ public class PackageBasedActionConfigBuilder implements ActionConfigBuilder { // some actions will not have any @Action or a default method, like the rest actions // where the action mapper is the one that finds the right method at runtime - if (map.isEmpty() && mapAllMatches && actionAnnotation == null && actionsAnnotation == null) { + if (actionAnnotationsByMethod.isEmpty() && mapAllMatches && actionAnnotation == null && actionsAnnotation == null) { createActionConfig(defaultPackageConfig, actionClass, defaultActionName, null, actionAnnotation, allowedMethods); } @@ -818,6 +804,38 @@ public class PackageBasedActionConfigBuilder implements ActionConfigBuilder { (actionClass.getModifiers() & Modifier.ABSTRACT) != 0 || actionClass.isAnonymousClass(); } + /** + * Determines whether the default {@code execute()} method should be automatically mapped as an action. + * This is the case when no explicit mapping exists for the default method, the class has an execute method, + * there are no class-level @Action/@Actions annotations, and no other method already maps to the default action name. + * + * @param actionAnnotationsByMethod the map of method names to their @Action annotations + * @param hasDefaultMethod whether the action class has an execute() method + * @param classAction the class-level @Action annotation, or null + * @param classActions the class-level @Actions annotation, or null + * @return true if the default execute method should be mapped + */ + protected boolean shouldMapDefaultExecuteMethod(Map<String, List<Action>> actionAnnotationsByMethod, boolean hasDefaultMethod, + Action classAction, Actions classActions) { + if (actionAnnotationsByMethod.containsKey(DEFAULT_METHOD) || !hasDefaultMethod) { + return false; + } + if (classAction != null || classActions != null) { + return false; + } + if (!alwaysMapExecute && !actionAnnotationsByMethod.isEmpty()) { + return false; + } + for (List<Action> actions : actionAnnotationsByMethod.values()) { + for (Action action : actions) { + if (action.value().equals(Action.DEFAULT_VALUE)) { + return false; + } + } + } + return true; + } + /** * Determines the namespace(s) for the action based on the action class. If there is a {@link Namespace} * annotation on the class (including parent classes) or on the package that the class is in, than diff --git a/plugins/convention/src/test/java/org/apache/struts2/convention/PackageBasedActionConfigBuilderTest.java b/plugins/convention/src/test/java/org/apache/struts2/convention/PackageBasedActionConfigBuilderTest.java index b6a9de4be..69d3324cd 100644 --- a/plugins/convention/src/test/java/org/apache/struts2/convention/PackageBasedActionConfigBuilderTest.java +++ b/plugins/convention/src/test/java/org/apache/struts2/convention/PackageBasedActionConfigBuilderTest.java @@ -25,6 +25,7 @@ import com.opensymphony.xwork2.FileManagerFactory; import com.opensymphony.xwork2.ObjectFactory; import com.opensymphony.xwork2.Result; import com.opensymphony.xwork2.config.Configuration; +import com.opensymphony.xwork2.config.ConfigurationException; import com.opensymphony.xwork2.config.entities.ActionConfig; import com.opensymphony.xwork2.config.entities.ExceptionMappingConfig; import com.opensymphony.xwork2.config.entities.InterceptorConfig; @@ -131,6 +132,103 @@ public class PackageBasedActionConfigBuilderTest extends TestCase { .bind(); } + /** + * Tests that duplicate @Action name detection works when execute() is annotated with @Action. + * Before the fix for WW-4421, the duplicate check was inside a conditional block that was + * skipped when execute() had an @Action annotation, allowing duplicate action names silently. + * + * @see <a href="https://issues.apache.org/jira/browse/WW-4421";>WW-4421</a> + */ + public void testDuplicateActionNameWithAnnotatedExecute() { + ResultTypeConfig defaultResult = new ResultTypeConfig.Builder("dispatcher", + ServletDispatcherResult.class.getName()).defaultResultParam("location").build(); + PackageConfig strutsDefault = makePackageConfig("struts-default", null, null, "dispatcher", + new ResultTypeConfig[]{defaultResult}, null, null, null, true); + + final DummyContainer mockContainer = new DummyContainer(); + Configuration configuration = new DefaultConfiguration() { + @Override + public Container getContainer() { + return mockContainer; + } + }; + configuration.addPackageConfig("struts-default", strutsDefault); + + ActionNameBuilder actionNameBuilder = new SEOActionNameBuilder("true", "-"); + ObjectFactory of = new ObjectFactory(); + of.setContainer(mockContainer); + + mockContainer.setActionNameBuilder(actionNameBuilder); + mockContainer.setConventionsService(new ConventionsServiceImpl("")); + + PackageBasedActionConfigBuilder builder = new PackageBasedActionConfigBuilder( + configuration, mockContainer, of, "false", "struts-default", "false"); + builder.setActionPackages("org.apache.struts2.convention.duplicate.annotatedexecute"); + builder.setActionSuffix("Action"); + + DefaultFileManagerFactory fileManagerFactory = new DefaultFileManagerFactory(); + fileManagerFactory.setContainer(ActionContext.getContext().getContainer()); + fileManagerFactory.setFileManager(new DefaultFileManager()); + builder.setFileManagerFactory(fileManagerFactory); + builder.setProviderAllowlist(new ProviderAllowlist()); + + try { + builder.buildActionConfigs(); + fail("Expected ConfigurationException for duplicate action names"); + } catch (ConfigurationException e) { + assertTrue("Exception message should mention duplicate action names", + e.getMessage().contains("two methods with an action name annotation whose value is the same")); + } + } + + /** + * Tests that duplicate @Action name detection works when execute() is NOT annotated with @Action. + * This is a regression guard for WW-4421 — this case was already detected before the fix. + * + * @see <a href="https://issues.apache.org/jira/browse/WW-4421";>WW-4421</a> + */ + public void testDuplicateActionNameWithoutAnnotatedExecute() { + ResultTypeConfig defaultResult = new ResultTypeConfig.Builder("dispatcher", + ServletDispatcherResult.class.getName()).defaultResultParam("location").build(); + PackageConfig strutsDefault = makePackageConfig("struts-default", null, null, "dispatcher", + new ResultTypeConfig[]{defaultResult}, null, null, null, true); + + final DummyContainer mockContainer = new DummyContainer(); + Configuration configuration = new DefaultConfiguration() { + @Override + public Container getContainer() { + return mockContainer; + } + }; + configuration.addPackageConfig("struts-default", strutsDefault); + + ActionNameBuilder actionNameBuilder = new SEOActionNameBuilder("true", "-"); + ObjectFactory of = new ObjectFactory(); + of.setContainer(mockContainer); + + mockContainer.setActionNameBuilder(actionNameBuilder); + mockContainer.setConventionsService(new ConventionsServiceImpl("")); + + PackageBasedActionConfigBuilder builder = new PackageBasedActionConfigBuilder( + configuration, mockContainer, of, "false", "struts-default", "false"); + builder.setActionPackages("org.apache.struts2.convention.duplicate.unannotatedexecute"); + builder.setActionSuffix("Action"); + + DefaultFileManagerFactory fileManagerFactory = new DefaultFileManagerFactory(); + fileManagerFactory.setContainer(ActionContext.getContext().getContainer()); + fileManagerFactory.setFileManager(new DefaultFileManager()); + builder.setFileManagerFactory(fileManagerFactory); + builder.setProviderAllowlist(new ProviderAllowlist()); + + try { + builder.buildActionConfigs(); + fail("Expected ConfigurationException for duplicate action names"); + } catch (ConfigurationException e) { + assertTrue("Exception message should mention duplicate action names", + e.getMessage().contains("two methods with an action name annotation whose value is the same")); + } + } + public void testActionPackages() throws MalformedURLException { run("org.apache.struts2.convention.actions", null, null); } @@ -352,7 +450,7 @@ public class PackageBasedActionConfigBuilderTest extends TestCase { expect(resultMapBuilder.build(GlobalResultAction.class, null, "global-result", globalResultPkg)).andReturn(results); expect(resultMapBuilder.build(GlobalResultOverrideAction.class, null, "global-result-override", globalResultPkg)).andReturn(results); expect(resultMapBuilder.build(ActionLevelResultsNamesAction.class, getAnnotation(ActionLevelResultsNamesAction.class, "execute", Action.class), "action-level-results-names", resultPkg)).andReturn(results); - expect(resultMapBuilder.build(ActionLevelResultsNamesAction.class, getAnnotation(ActionLevelResultsNamesAction.class, "noname", Action.class), "action-level-results-names", resultPkg)).andReturn(results); + expect(resultMapBuilder.build(ActionLevelResultsNamesAction.class, getAnnotation(ActionLevelResultsNamesAction.class, "noname", Action.class), "action-level-results-names-noname", resultPkg)).andReturn(results); /* org.apache.struts2.convention.actions.resultpath */ expect(resultMapBuilder.build(ClassLevelResultPathAction.class, null, "class-level-result-path", resultPathPkg)).andReturn(results); @@ -662,11 +760,12 @@ public class PackageBasedActionConfigBuilderTest extends TestCase { pkgConfig = configuration.getPackageConfig("org.apache.struts2.convention.actions.result#struts-default#/result"); assertNotNull(pkgConfig); checkSmiValue(pkgConfig, strutsDefault, isSmiInheritanceEnabled); - assertEquals(7, pkgConfig.getActionConfigs().size()); + assertEquals(8, pkgConfig.getActionConfigs().size()); verifyActionConfig(pkgConfig, "class-level-result", ClassLevelResultAction.class, "execute", pkgConfig.getName()); verifyActionConfig(pkgConfig, "class-level-results", ClassLevelResultsAction.class, "execute", pkgConfig.getName()); verifyActionConfig(pkgConfig, "action-level-result", ActionLevelResultAction.class, "execute", pkgConfig.getName()); verifyActionConfig(pkgConfig, "action-level-results", ActionLevelResultsAction.class, "execute", pkgConfig.getName()); + verifyActionConfig(pkgConfig, "action-level-results-names-noname", ActionLevelResultsNamesAction.class, "noname", pkgConfig.getName()); verifyActionConfig(pkgConfig, "inherited-result-extends", InheritedResultExtends.class, "execute", pkgConfig.getName()); /* org.apache.struts2.convention.actions.resultpath */ diff --git a/plugins/convention/src/test/java/org/apache/struts2/convention/actions/result/ActionLevelResultsNamesAction.java b/plugins/convention/src/test/java/org/apache/struts2/convention/actions/result/ActionLevelResultsNamesAction.java index 2136f8d0a..8a8aee64d 100644 --- a/plugins/convention/src/test/java/org/apache/struts2/convention/actions/result/ActionLevelResultsNamesAction.java +++ b/plugins/convention/src/test/java/org/apache/struts2/convention/actions/result/ActionLevelResultsNamesAction.java @@ -28,16 +28,16 @@ import org.apache.struts2.convention.annotation.Result; */ public class ActionLevelResultsNamesAction { @Action(results = { - @Result(name={"error", "input"}, location="error.jsp"), - @Result(name="success", location="/WEB-INF/location/namespace/action-success.jsp"), - @Result(name="failure", location="/WEB-INF/location/namespace/action-failure.jsp") + @Result(name = {"error", "input"}, location = "error.jsp"), + @Result(name = "success", location = "/WEB-INF/location/namespace/action-success.jsp"), + @Result(name = "failure", location = "/WEB-INF/location/namespace/action-failure.jsp") }) public String execute() { return null; } - @Action(results = { - @Result(location="/WEB-INF/location/namespace/action-success.jsp") + @Action(value = "action-level-results-names-noname", results = { + @Result(location = "/WEB-INF/location/namespace/action-success.jsp") }) public String noname() { return null; diff --git a/plugins/convention/src/test/java/org/apache/struts2/convention/actions/result/ActionLevelResultsNamesAction.java b/plugins/convention/src/test/java/org/apache/struts2/convention/duplicate/annotatedexecute/DuplicateActionNameWithExecuteAnnotationAction.java similarity index 57% copy from plugins/convention/src/test/java/org/apache/struts2/convention/actions/result/ActionLevelResultsNamesAction.java copy to plugins/convention/src/test/java/org/apache/struts2/convention/duplicate/annotatedexecute/DuplicateActionNameWithExecuteAnnotationAction.java index 2136f8d0a..89b3256d6 100644 --- a/plugins/convention/src/test/java/org/apache/struts2/convention/actions/result/ActionLevelResultsNamesAction.java +++ b/plugins/convention/src/test/java/org/apache/struts2/convention/duplicate/annotatedexecute/DuplicateActionNameWithExecuteAnnotationAction.java @@ -16,30 +16,23 @@ * specific language governing permissions and limitations * under the License. */ -package org.apache.struts2.convention.actions.result; +package org.apache.struts2.convention.duplicate.annotatedexecute; import org.apache.struts2.convention.annotation.Action; -import org.apache.struts2.convention.annotation.Result; /** - * <p> - * This is a test action with multiple results names. - * </p> + * Test action with duplicate @Action names where execute() is annotated. + * This is the case that was previously not detected (WW-4421). */ -public class ActionLevelResultsNamesAction { - @Action(results = { - @Result(name={"error", "input"}, location="error.jsp"), - @Result(name="success", location="/WEB-INF/location/namespace/action-success.jsp"), - @Result(name="failure", location="/WEB-INF/location/namespace/action-failure.jsp") - }) +public class DuplicateActionNameWithExecuteAnnotationAction { + + @Action("duplicate") public String execute() { - return null; + return "success"; } - @Action(results = { - @Result(location="/WEB-INF/location/namespace/action-success.jsp") - }) - public String noname() { - return null; + @Action("duplicate") + public String anotherMethod() { + return "success"; } } diff --git a/plugins/convention/src/test/java/org/apache/struts2/convention/actions/result/ActionLevelResultsNamesAction.java b/plugins/convention/src/test/java/org/apache/struts2/convention/duplicate/unannotatedexecute/DuplicateActionNameWithoutExecuteAnnotationAction.java similarity index 57% copy from plugins/convention/src/test/java/org/apache/struts2/convention/actions/result/ActionLevelResultsNamesAction.java copy to plugins/convention/src/test/java/org/apache/struts2/convention/duplicate/unannotatedexecute/DuplicateActionNameWithoutExecuteAnnotationAction.java index 2136f8d0a..8048fa2ff 100644 --- a/plugins/convention/src/test/java/org/apache/struts2/convention/actions/result/ActionLevelResultsNamesAction.java +++ b/plugins/convention/src/test/java/org/apache/struts2/convention/duplicate/unannotatedexecute/DuplicateActionNameWithoutExecuteAnnotationAction.java @@ -16,30 +16,27 @@ * specific language governing permissions and limitations * under the License. */ -package org.apache.struts2.convention.actions.result; +package org.apache.struts2.convention.duplicate.unannotatedexecute; import org.apache.struts2.convention.annotation.Action; -import org.apache.struts2.convention.annotation.Result; /** - * <p> - * This is a test action with multiple results names. - * </p> + * Test action with duplicate @Action names where execute() is NOT annotated. + * This is the case that was already detected before the fix (regression guard for WW-4421). */ -public class ActionLevelResultsNamesAction { - @Action(results = { - @Result(name={"error", "input"}, location="error.jsp"), - @Result(name="success", location="/WEB-INF/location/namespace/action-success.jsp"), - @Result(name="failure", location="/WEB-INF/location/namespace/action-failure.jsp") - }) +public class DuplicateActionNameWithoutExecuteAnnotationAction { + public String execute() { - return null; + return "success"; + } + + @Action("duplicate") + public String method1() { + return "success"; } - @Action(results = { - @Result(location="/WEB-INF/location/namespace/action-success.jsp") - }) - public String noname() { - return null; + @Action("duplicate") + public String method2() { + return "success"; } }
