This is an automated email from the ASF dual-hosted git repository. lukaszlenart pushed a commit to branch fix/WW-4421-duplicate-action-annotation-check in repository https://gitbox.apache.org/repos/asf/struts.git
commit 99e4d496becdbabe0b58e2391ef812b32ee73558 Author: Lukasz Lenart <[email protected]> AuthorDate: Sun Feb 15 16:22:35 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. 🤖 Generated with [Claude Code](https://claude.com/claude-code) Co-Authored-By: Claude <[email protected]> --- .../PackageBasedActionConfigBuilder.java | 27 ++-- .../PackageBasedActionConfigBuilderTest.java | 98 ++++++++++++++ ...icateActionNameWithExecuteAnnotationAction.java | 38 ++++++ ...teActionNameWithoutExecuteAnnotationAction.java | 42 ++++++ ...21-duplicate-action-annotation-check-skipped.md | 141 +++++++++++++++++++++ 5 files changed, 334 insertions(+), 12 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 bbd718497..4211e0cdf 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 @@ -739,8 +739,21 @@ 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<>(); boolean hasDefaultMethod = ReflectionTools.containsMethod(actionClass, DEFAULT_METHOD); + + // Check for duplicate action names across all annotated methods + 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.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)."); + } + } + } + if (!map.containsKey(DEFAULT_METHOD) && hasDefaultMethod && actionAnnotation == null && actionsAnnotation == null @@ -748,20 +761,10 @@ public class PackageBasedActionConfigBuilder implements ActionConfigBuilder { 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; + break; } } } 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 975aaea35..c7c46b93b 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 @@ -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); } diff --git a/plugins/convention/src/test/java/org/apache/struts2/convention/duplicate/annotatedexecute/DuplicateActionNameWithExecuteAnnotationAction.java b/plugins/convention/src/test/java/org/apache/struts2/convention/duplicate/annotatedexecute/DuplicateActionNameWithExecuteAnnotationAction.java new file mode 100644 index 000000000..89b3256d6 --- /dev/null +++ b/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"; + } +} diff --git a/plugins/convention/src/test/java/org/apache/struts2/convention/duplicate/unannotatedexecute/DuplicateActionNameWithoutExecuteAnnotationAction.java b/plugins/convention/src/test/java/org/apache/struts2/convention/duplicate/unannotatedexecute/DuplicateActionNameWithoutExecuteAnnotationAction.java new file mode 100644 index 000000000..8048fa2ff --- /dev/null +++ b/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"; + } +} diff --git a/thoughts/shared/research/2026-02-15-WW-4421-duplicate-action-annotation-check-skipped.md b/thoughts/shared/research/2026-02-15-WW-4421-duplicate-action-annotation-check-skipped.md new file mode 100644 index 000000000..b3f6d51eb --- /dev/null +++ b/thoughts/shared/research/2026-02-15-WW-4421-duplicate-action-annotation-check-skipped.md @@ -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?
