mirror of
https://github.com/apache/struts.git
synced 2026-08-05 22:56:59 +00:00
fix(convention): WW-4421 detect duplicate @Action names when execute() is annotated (#1579)
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. 🤖 Generated with [Claude Code](https://claude.com/claude-code) Co-authored-by: Claude <noreply@anthropic.com>
This commit is contained in:
+48
-30
@@ -738,42 +738,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) {
|
||||
@@ -789,7 +775,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);
|
||||
}
|
||||
|
||||
@@ -840,6 +826,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
|
||||
|
||||
+101
-2
@@ -30,6 +30,7 @@ import org.apache.struts2.FileManager;
|
||||
import org.apache.struts2.FileManagerFactory;
|
||||
import org.apache.struts2.ObjectFactory;
|
||||
import org.apache.struts2.config.Configuration;
|
||||
import org.apache.struts2.config.ConfigurationException;
|
||||
import org.apache.struts2.config.entities.ActionConfig;
|
||||
import org.apache.struts2.config.entities.ExceptionMappingConfig;
|
||||
import org.apache.struts2.config.entities.InterceptorConfig;
|
||||
@@ -249,6 +250,103 @@ public class PackageBasedActionConfigBuilderTest extends TestCase {
|
||||
assertFalse("ClassNotFoundException should be caught and class should be excluded", result);
|
||||
}
|
||||
|
||||
/**
|
||||
* 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() throws Exception {
|
||||
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);
|
||||
}
|
||||
@@ -544,7 +642,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);
|
||||
@@ -854,11 +952,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 */
|
||||
|
||||
+5
-5
@@ -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;
|
||||
|
||||
+38
@@ -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";
|
||||
}
|
||||
}
|
||||
+42
@@ -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";
|
||||
}
|
||||
}
|
||||
+141
@@ -0,0 +1,141 @@
|
||||
---
|
||||
date: 2026-02-15T12:00:00+01:00
|
||||
topic: "WW-4421: Duplicate @Action value annotation check skipped"
|
||||
tags: [research, codebase, convention-plugin, annotations, duplicate-detection]
|
||||
status: complete
|
||||
git_commit: fd874258631e999f5bd5ffc3fea8c0e416c61962
|
||||
---
|
||||
|
||||
# Research: WW-4421 - Duplicate @Action value annotation check skipped
|
||||
|
||||
**Date**: 2026-02-15
|
||||
**JIRA**: [WW-4421](https://issues.apache.org/jira/browse/WW-4421)
|
||||
|
||||
## Research Question
|
||||
Investigate the core issue behind duplicate @Action annotation detection being bypassed in the Convention plugin.
|
||||
|
||||
## Summary
|
||||
|
||||
The duplicate @Action name detection logic in `PackageBasedActionConfigBuilder.buildConfiguration` is **guarded by a conditional that excludes the most common case**: when the `execute()` method is annotated with `@Action`. The duplicate check only runs when `!map.containsKey(DEFAULT_METHOD)` — meaning if the class overrides `execute()` and decorates it with `@Action`, the entire duplicate detection block is skipped.
|
||||
|
||||
Additionally, the code uses `Class.getMethods()` and `HashMap` — both with **non-deterministic ordering** — making the behavior inconsistent across JVM versions.
|
||||
|
||||
## Detailed Findings
|
||||
|
||||
### The Bug: Conditional Guard Excludes Common Case
|
||||
|
||||
**File**: [`PackageBasedActionConfigBuilder.java:744-747`](https://github.com/apache/struts/blob/fd874258631e999f5bd5ffc3fea8c0e416c61962/plugins/convention/src/main/java/org/apache/struts2/convention/PackageBasedActionConfigBuilder.java#L744-L747)
|
||||
|
||||
```java
|
||||
if (!map.containsKey(DEFAULT_METHOD) // <-- THE BUG
|
||||
&& hasDefaultMethod
|
||||
&& actionAnnotation == null && actionsAnnotation == null
|
||||
&& (alwaysMapExecute || map.isEmpty())) {
|
||||
// ... duplicate detection code is INSIDE this block ...
|
||||
Set<String> actionNames = new HashSet<>();
|
||||
for (List<Action> actions : map.values()) {
|
||||
for (Action action : actions) {
|
||||
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);
|
||||
}
|
||||
}
|
||||
}
|
||||
}
|
||||
```
|
||||
|
||||
The condition `!map.containsKey(DEFAULT_METHOD)` means:
|
||||
- **If `execute()` has an `@Action` annotation** → `map` contains key `"execute"` → condition is `false` → **duplicate check is SKIPPED entirely**
|
||||
- **If `execute()` does NOT have an `@Action` annotation** → condition can be `true` → duplicate check runs
|
||||
|
||||
This is backwards from what you'd expect. The most common pattern — overriding `execute()` with `@Action` — is exactly the case that bypasses validation.
|
||||
|
||||
### Scenario: When Duplicate Detection Fails
|
||||
|
||||
```java
|
||||
// This class will NOT trigger the duplicate detection error:
|
||||
public class MethodCheckSkippedOnStartup extends ActionSupport {
|
||||
@Action("myAction") // <-- annotated execute() puts "execute" in the map
|
||||
public String execute() {
|
||||
return SUCCESS;
|
||||
}
|
||||
|
||||
@Action("myAction") // <-- duplicate name, but no error thrown!
|
||||
public String anotherMethod() {
|
||||
return SUCCESS;
|
||||
}
|
||||
}
|
||||
```
|
||||
|
||||
```java
|
||||
// This class WILL trigger the duplicate detection error:
|
||||
public class MethodCheckFailsOnStartup extends ActionSupport {
|
||||
// execute() is NOT annotated — not in the map
|
||||
public String execute() {
|
||||
return SUCCESS;
|
||||
}
|
||||
|
||||
@Action("myAction")
|
||||
public String method1() {
|
||||
return SUCCESS;
|
||||
}
|
||||
|
||||
@Action("myAction") // <-- duplicate detected, throws ConfigurationException
|
||||
public String method2() {
|
||||
return SUCCESS;
|
||||
}
|
||||
}
|
||||
```
|
||||
|
||||
### Secondary Issue: Non-Deterministic Method Ordering
|
||||
|
||||
**File**: [`PackageBasedActionConfigBuilder.java:943-944`](https://github.com/apache/struts/blob/fd874258631e999f5bd5ffc3fea8c0e416c61962/plugins/convention/src/main/java/org/apache/struts2/convention/PackageBasedActionConfigBuilder.java#L943-L944)
|
||||
|
||||
```java
|
||||
Method[] methods = actionClass.getMethods(); // no guaranteed order
|
||||
Map<String, List<Action>> map = new HashMap<>(); // no guaranteed iteration order
|
||||
```
|
||||
|
||||
- `Class.getMethods()` returns methods in **no particular order** (JVM spec does not guarantee ordering)
|
||||
- Results are stored in a `HashMap`, which also has **no guaranteed iteration order**
|
||||
- This means the second `@Action` annotation that "wins" (overwrites the mapping) is non-deterministic
|
||||
- Java 7 and Java 8 had different internal `HashMap` implementations, causing different behavior across versions
|
||||
|
||||
### The Duplicate Check Should Be Independent
|
||||
|
||||
The duplicate detection logic (lines 748-760) is embedded inside a block that also handles default `execute()` method mapping. These are **two separate concerns**:
|
||||
|
||||
1. **Should we auto-create an action config for `execute()`?** — This depends on `!map.containsKey(DEFAULT_METHOD)`
|
||||
2. **Are there duplicate action names across annotated methods?** — This should ALWAYS be checked
|
||||
|
||||
The fix should extract the duplicate detection loop so it runs unconditionally (or at least independently of whether `execute()` is in the annotation map).
|
||||
|
||||
## Code References
|
||||
|
||||
- [`PackageBasedActionConfigBuilder.java:87`](https://github.com/apache/struts/blob/fd874258631e999f5bd5ffc3fea8c0e416c61962/plugins/convention/src/main/java/org/apache/struts2/convention/PackageBasedActionConfigBuilder.java#L87) — `DEFAULT_METHOD = "execute"`
|
||||
- [`PackageBasedActionConfigBuilder.java:741-773`](https://github.com/apache/struts/blob/fd874258631e999f5bd5ffc3fea8c0e416c61962/plugins/convention/src/main/java/org/apache/struts2/convention/PackageBasedActionConfigBuilder.java#L741-L773) — The buggy block with embedded duplicate check
|
||||
- [`PackageBasedActionConfigBuilder.java:776-788`](https://github.com/apache/struts/blob/fd874258631e999f5bd5ffc3fea8c0e416c61962/plugins/convention/src/main/java/org/apache/struts2/convention/PackageBasedActionConfigBuilder.java#L776-L788) — Action config creation loop (no duplicate check here)
|
||||
- [`PackageBasedActionConfigBuilder.java:942-959`](https://github.com/apache/struts/blob/fd874258631e999f5bd5ffc3fea8c0e416c61962/plugins/convention/src/main/java/org/apache/struts2/convention/PackageBasedActionConfigBuilder.java#L942-L959) — `getActionAnnotations()` with non-deterministic ordering
|
||||
- [`ReflectionTools.java:38-45`](https://github.com/apache/struts/blob/fd874258631e999f5bd5ffc3fea8c0e416c61962/plugins/convention/src/main/java/org/apache/struts2/convention/ReflectionTools.java#L38-L45) — `containsMethod()` helper
|
||||
|
||||
## Architecture Insights
|
||||
|
||||
1. **Separation of concerns violation**: Duplicate detection is tangled with default-execute-mapping logic inside the same conditional block
|
||||
2. **Non-deterministic reflection**: Using `getMethods()` + `HashMap` means behavior varies across JVMs
|
||||
3. **Silent failure mode**: When the check is skipped, one action config silently overwrites another — no error, no warning, just undefined behavior at runtime
|
||||
|
||||
## Suggested Fix Direction
|
||||
|
||||
1. **Extract duplicate detection** out of the `!map.containsKey(DEFAULT_METHOD)` conditional so it runs for ALL annotated action classes
|
||||
2. **Also check class-level annotations** (`actionAnnotation`/`actionsAnnotation`) against method-level annotations for name conflicts
|
||||
3. **Consider using `LinkedHashMap`** or sorting methods for deterministic processing order
|
||||
|
||||
## Open Questions
|
||||
|
||||
1. Should duplicate detection also consider action names across different classes in the same namespace?
|
||||
2. Should a warning be issued instead of a `ConfigurationException` to allow intentional overrides?
|
||||
3. Are there backward-compatibility concerns if we start detecting duplicates that were previously silently ignored?
|
||||
Reference in New Issue
Block a user