fix(convention): WW-4421 detect duplicate @action names when execute() is annotated (#1590)

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 #1579 from Struts 7.x to 6.x.

🤖 Generated with [Claude Code](https://claude.com/claude-code)

Co-authored-by: Claude <noreply@anthropic.com>

Author: lukaszlenart

plugins/convention/src/main/java/org/apache/struts2/convention/PackageBasedActionConfigBuilder.java

@@ -716,42 +716,28 @@ protected void buildConfiguration(Set<Class<?>> classes) {
 
                 // 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 @@ protected void buildConfiguration(Set<Class<?>> classes) {
 
                 // 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 @@ protected boolean cannotInstantiate(Class<?> actionClass) {
                 (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

plugins/convention/src/test/java/org/apache/struts2/convention/PackageBasedActionConfigBuilderTest.java

@@ -25,6 +25,7 @@
 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 void setUp() throws Exception {
             .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 @@ private void run(String actionPackages, String packageLocators, String excludePa
         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 Container getContainer() {
         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 */

plugins/convention/src/test/java/org/apache/struts2/convention/actions/result/ActionLevelResultsNamesAction.java

@@ -28,16 +28,16 @@
  */
 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;

plugins/convention/src/test/java/org/apache/struts2/convention/duplicate/annotatedexecute/DuplicateActionNameWithExecuteAnnotationAction.java

@@ -0,0 +1,38 @@
+/*
+ * 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.struts2.convention.duplicate.annotatedexecute;
+
+import org.apache.struts2.convention.annotation.Action;
+
+/**
+ * Test action with duplicate @Action names where execute() is annotated.
+ * This is the case that was previously not detected (WW-4421).
+ */
+public class DuplicateActionNameWithExecuteAnnotationAction {
+
+    @Action("duplicate")
+    public String execute() {
+        return "success";
+    }
+
+    @Action("duplicate")
+    public String anotherMethod() {
+        return "success";
+    }
+}

plugins/convention/src/test/java/org/apache/struts2/convention/duplicate/unannotatedexecute/DuplicateActionNameWithoutExecuteAnnotationAction.java

@@ -0,0 +1,42 @@
+/*
+ * 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.struts2.convention.duplicate.unannotatedexecute;
+
+import org.apache.struts2.convention.annotation.Action;
+
+/**
+ * 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 DuplicateActionNameWithoutExecuteAnnotationAction {
+
+    public String execute() {
+        return "success";
+    }
+
+    @Action("duplicate")
+    public String method1() {
+        return "success";
+    }
+
+    @Action("duplicate")
+    public String method2() {
+        return "success";
+    }
+}