diff --git a/core/src/main/java/org/apache/struts2/config/entities/PackageConfig.java b/core/src/main/java/org/apache/struts2/config/entities/PackageConfig.java index ccf2e756fb..391ead116d 100644 --- a/core/src/main/java/org/apache/struts2/config/entities/PackageConfig.java +++ b/core/src/main/java/org/apache/struts2/config/entities/PackageConfig.java @@ -24,6 +24,7 @@ import java.io.Serializable; import java.util.ArrayList; import java.util.Collections; +import java.util.Comparator; import java.util.HashSet; import java.util.LinkedHashMap; import java.util.LinkedHashSet; @@ -517,6 +518,26 @@ public Builder addActionConfig(String name, ActionConfig action) { return this; } + /** + * Re-inserts this package's action configs into a new insertion-ordered map, + * ordered by the supplied comparator over the action-name keys. Must be called + * before {@link #build()}. + * + * @param byActionName comparator over action names determining match precedence + * @return this builder + * @since 7.3.0 (WW-3784) + */ + public Builder reorderActionConfigs(Comparator byActionName) { + List> entries = new ArrayList<>(target.actionConfigs.entrySet()); + entries.sort(Map.Entry.comparingByKey(byActionName)); + Map reordered = new LinkedHashMap<>(); + for (Map.Entry entry : entries) { + reordered.put(entry.getKey(), entry.getValue()); + } + target.actionConfigs = reordered; + return this; + } + public Builder addParents(List parents) { for (PackageConfig config : parents) { addParent(config); diff --git a/core/src/test/java/org/apache/struts2/config/entities/PackageConfigBuilderReorderTest.java b/core/src/test/java/org/apache/struts2/config/entities/PackageConfigBuilderReorderTest.java new file mode 100644 index 0000000000..71769df62a --- /dev/null +++ b/core/src/test/java/org/apache/struts2/config/entities/PackageConfigBuilderReorderTest.java @@ -0,0 +1,44 @@ +/* + * 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.config.entities; + +import junit.framework.TestCase; + +import java.util.ArrayList; +import java.util.Comparator; +import java.util.List; + +public class PackageConfigBuilderReorderTest extends TestCase { + + public void testReorderActionConfigsAppliesComparator() { + PackageConfig.Builder builder = new PackageConfig.Builder("test"); + builder.addActionConfig("some/*", action("some/*")); + builder.addActionConfig("some/usefull/*", action("some/usefull/*")); + + // reverse-alphabetical proves the map is genuinely reordered, not left as-inserted + builder.reorderActionConfigs(Comparator.reverseOrder()); + + List keys = new ArrayList<>(builder.build().getActionConfigs().keySet()); + assertEquals(List.of("some/usefull/*", "some/*"), keys); + } + + private ActionConfig action(String name) { + return new ActionConfig.Builder("test", name, "com.example.Action").build(); + } +} diff --git a/docs/superpowers/plans/2026-07-26-WW-3784-annotated-wildcard-specificity-ordering.md b/docs/superpowers/plans/2026-07-26-WW-3784-annotated-wildcard-specificity-ordering.md new file mode 100644 index 0000000000..2375161e54 --- /dev/null +++ b/docs/superpowers/plans/2026-07-26-WW-3784-annotated-wildcard-specificity-ordering.md @@ -0,0 +1,484 @@ +# WW-3784 Specificity-ordered wildcard matching for annotated actions — Implementation Plan + +> **For agentic workers:** REQUIRED SUB-SKILL: Use superpowers:subagent-driven-development (recommended) or superpowers:executing-plans to implement this plan task-by-task. Steps use checkbox (`- [ ]`) syntax for tracking. + +**Goal:** Make annotation-based (Convention plugin) wildcard action patterns match most-specific-first, so a specific pattern like `some/usefull/*` is never shadowed by a general one like `some/*`, regardless of class-scan order. + +**Architecture:** The fix lives entirely in the Convention plugin plus one neutral core helper. A new `Comparator` ranks action-name patterns by specificity; a new `PackageConfig.Builder.reorderActionConfigs(Comparator)` re-sorts a package's `LinkedHashMap` of action configs; and `PackageBasedActionConfigBuilder` applies the sort to every package after building it. Core matchers and XML configuration providers are untouched, so XML's explicit file-order semantics are preserved. + +**Tech Stack:** Java 17, Maven multi-module (`core`, `plugins/convention`), JUnit 4 (convention tests) and JUnit 3 / `junit.framework.TestCase` (core tests), AssertJ/Mockito available but not required here. + +## Global Constraints + +- **Commit prefix:** every commit message MUST start with `WW-3784` followed by a Conventional-Commits type (`feat`/`test`/`refactor`/`docs`). End each commit body with `Co-Authored-By: Claude Opus 4.8 `. +- **Branch:** work on `WW-3784-annotated-wildcard-specificity-ordering` (already created and checked out). Never commit to `main`. +- **Fix version target:** 7.3.0. Use `@since 7.3.0 (WW-3784)` on new public/protected API. +- **No XML/core-matcher behavior changes:** do not modify `AbstractMatcher`, `ActionConfigMatcher`, or any `config/providers/*Xml*` class. +- **Core cannot depend on Convention:** the specificity `Comparator` lives in the convention plugin; the core `reorderActionConfigs` helper must accept a generic `Comparator` and must not reference the comparator class. +- **Core test trap:** core tests that `extends XWorkTestCase` silently ignore JUnit 4 `@Test`. New core tests here use `extends junit.framework.TestCase` with `testXxx()` methods. +- **Build/test commands:** `mvn test -DskipAssembly -pl core -Dtest=...` and `mvn test -DskipAssembly -pl plugins/convention -Dtest=...` (add `-am` on the first plugin run so the freshly-built core is available). + +--- + +### Task 1: `ActionNameSpecificityComparator` (Convention plugin) + +A pure `Comparator` that orders wildcard action-name patterns most-specific-first. + +**Files:** +- Create: `plugins/convention/src/main/java/org/apache/struts2/convention/ActionNameSpecificityComparator.java` +- Test: `plugins/convention/src/test/java/org/apache/struts2/convention/ActionNameSpecificityComparatorTest.java` + +**Interfaces:** +- Consumes: nothing. +- Produces: `public class ActionNameSpecificityComparator implements java.util.Comparator` with `public int compare(String a, String b)`. Ordering keys, most-specific first: (1) fewer wildcard tokens; (2) more literal characters; (3) fewer path-spanning `**` tokens; (4) natural `String` order (deterministic tiebreak). A wildcard token is one `*`/`**`/`***…` run **or** one `{var}` group. + +- [ ] **Step 1: Write the failing test** + +```java +package org.apache.struts2.convention; + +import org.junit.Test; + +import java.util.ArrayList; +import java.util.Arrays; +import java.util.Collections; +import java.util.List; +import java.util.Random; + +import static org.junit.Assert.assertEquals; +import static org.junit.Assert.assertTrue; + +public class ActionNameSpecificityComparatorTest { + + private final ActionNameSpecificityComparator comparator = new ActionNameSpecificityComparator(); + + @Test + public void moreLiteralPrefixIsMoreSpecific_ticketCase() { + // equal wildcard count (1 each); "some/usefull/*" has more literal chars -> more specific + assertTrue(comparator.compare("some/usefull/*", "some/*") < 0); + } + + @Test + public void fewerWildcardsIsMoreSpecific() { + assertTrue(comparator.compare("a/*", "a/*/*") < 0); + } + + @Test + public void singleStarBeatsPathStarAtEqualLiterals() { + // both "a/" literal (2 chars), one wildcard each; "a/*" (file) beats "a/**" (path) + assertTrue(comparator.compare("a/*", "a/**") < 0); + } + + @Test + public void namedVariablesCountAsWildcards() { + assertTrue(comparator.compare("some/usefull/{id}", "some/{id}") < 0); + } + + @Test + public void literalRanksBeforeAnyWildcard() { + assertTrue(comparator.compare("some/list", "some/*") < 0); + } + + @Test + public void sortIsDeterministicRegardlessOfInputOrder() { + List expected = Arrays.asList("some/usefull/*", "some/*", "*"); + List shuffled = new ArrayList<>(expected); + Collections.shuffle(shuffled, new Random(42)); + shuffled.sort(comparator); + assertEquals(expected, shuffled); + } +} +``` + +- [ ] **Step 2: Run test to verify it fails** + +Run: `mvn test -DskipAssembly -pl plugins/convention -am -Dtest=ActionNameSpecificityComparatorTest` +Expected: FAIL to compile — `ActionNameSpecificityComparator` does not exist. + +- [ ] **Step 3: Write the implementation** + +```java +/* + * 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; + +import java.util.Comparator; + +/** + * Orders wildcard action-name patterns most-specific-first so that, under the framework's + * first-match-wins matching, a specific pattern (e.g. {@code some/usefull/*}) is evaluated + * before a general one (e.g. {@code some/*}). + * + *

Ordering keys, applied in order:

+ *
    + *
  1. fewer wildcard tokens first (a {@code *}/{@code **} run, or a {var} group);
    2. + *
  2. more literal characters first;
    3. + *
  3. fewer path-spanning {@code **} tokens first;
    4. + *
  4. natural (alphabetical) order of the pattern, for deterministic tie-breaking.
    5. + *
+ * + *

Matcher-agnostic: it recognises both {@code *}/{@code **} (WildcardHelper) and + * {var} (NamedVariablePatternMatcher) wildcards.

+ * + * @since 7.3.0 (WW-3784) + */ +public class ActionNameSpecificityComparator implements Comparator { + + @Override + public int compare(String a, String b) { + Counts ca = count(a); + Counts cb = count(b); + + int byWildcards = Integer.compare(ca.wildcards, cb.wildcards); + if (byWildcards != 0) { + return byWildcards; + } + int byLiterals = Integer.compare(cb.literals, ca.literals); // more literals first + if (byLiterals != 0) { + return byLiterals; + } + int byPathWildcards = Integer.compare(ca.pathWildcards, cb.pathWildcards); + if (byPathWildcards != 0) { + return byPathWildcards; + } + return a.compareTo(b); + } + + private Counts count(String pattern) { + int wildcards = 0; + int pathWildcards = 0; + int literals = 0; + int i = 0; + int len = pattern.length(); + while (i < len) { + char c = pattern.charAt(i); + if (c == '*') { + int start = i; + while (i < len && pattern.charAt(i) == '*') { + i++; + } + wildcards++; + if (i - start >= 2) { + pathWildcards++; + } + } else if (c == '{') { + int close = pattern.indexOf('}', i); + if (close < 0) { + literals += len - i; // malformed: treat the remainder as literal + break; + } + wildcards++; + i = close + 1; + } else { + literals++; + i++; + } + } + return new Counts(wildcards, pathWildcards, literals); + } + + private record Counts(int wildcards, int pathWildcards, int literals) { + } +} +``` + +- [ ] **Step 4: Run test to verify it passes** + +Run: `mvn test -DskipAssembly -pl plugins/convention -am -Dtest=ActionNameSpecificityComparatorTest` +Expected: PASS (6 tests). + +- [ ] **Step 5: Commit** + +```bash +git add plugins/convention/src/main/java/org/apache/struts2/convention/ActionNameSpecificityComparator.java \ + plugins/convention/src/test/java/org/apache/struts2/convention/ActionNameSpecificityComparatorTest.java +git commit -m "WW-3784 feat(convention): add action-name specificity comparator + +Co-Authored-By: Claude Opus 4.8 " +``` + +--- + +### Task 2: `PackageConfig.Builder.reorderActionConfigs` (core helper) + +A neutral core helper that re-sorts a package's action-config map by a caller-supplied comparator over action names. Generic on purpose — core never references the convention comparator. + +**Files:** +- Modify: `core/src/main/java/org/apache/struts2/config/entities/PackageConfig.java` (add `import java.util.Comparator;` near the other `java.util` imports at lines 25-33; add the `reorderActionConfigs` method inside the `Builder` class, next to `addActionConfig` at line 515) +- Test: `core/src/test/java/org/apache/struts2/config/entities/PackageConfigBuilderReorderTest.java` + +**Interfaces:** +- Consumes: nothing from other tasks. +- Produces: `public PackageConfig.Builder reorderActionConfigs(java.util.Comparator byActionName)` — re-inserts the builder's action configs into a fresh `LinkedHashMap` ordered by `byActionName` over the action-name keys; returns `this`. Must be called before `build()`. + +- [ ] **Step 1: Write the failing test** + +```java +/* + * 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.config.entities; + +import junit.framework.TestCase; + +import java.util.ArrayList; +import java.util.Comparator; +import java.util.List; + +public class PackageConfigBuilderReorderTest extends TestCase { + + public void testReorderActionConfigsAppliesComparator() { + PackageConfig.Builder builder = new PackageConfig.Builder("test"); + builder.addActionConfig("some/*", action("some/*")); + builder.addActionConfig("some/usefull/*", action("some/usefull/*")); + + // reverse-alphabetical proves the map is genuinely reordered, not left as-inserted + builder.reorderActionConfigs(Comparator.reverseOrder()); + + List keys = new ArrayList<>(builder.build().getActionConfigs().keySet()); + assertEquals(List.of("some/usefull/*", "some/*"), keys); + } + + private ActionConfig action(String name) { + return new ActionConfig.Builder("test", name, "com.example.Action").build(); + } +} +``` + +- [ ] **Step 2: Run test to verify it fails** + +Run: `mvn test -DskipAssembly -pl core -Dtest=PackageConfigBuilderReorderTest` +Expected: FAIL to compile — `reorderActionConfigs` does not exist. + +- [ ] **Step 3: Add the import and the method** + +Add near the other `java.util` imports (lines 25-33): + +```java +import java.util.Comparator; +``` + +Add inside the `Builder` class, immediately after `addActionConfig` (line 515-518): + +```java +public Builder reorderActionConfigs(Comparator byActionName) { + List> entries = new ArrayList<>(target.actionConfigs.entrySet()); + entries.sort(Map.Entry.comparingByKey(byActionName)); + Map reordered = new LinkedHashMap<>(); + for (Map.Entry entry : entries) { + reordered.put(entry.getKey(), entry.getValue()); + } + target.actionConfigs = reordered; + return this; +} +``` + +(`ArrayList`, `List`, `Map`, and `LinkedHashMap` are already imported.) + +- [ ] **Step 4: Run test to verify it passes** + +Run: `mvn test -DskipAssembly -pl core -Dtest=PackageConfigBuilderReorderTest` +Expected: PASS (1 test). + +- [ ] **Step 5: Commit** + +```bash +git add core/src/main/java/org/apache/struts2/config/entities/PackageConfig.java \ + core/src/test/java/org/apache/struts2/config/entities/PackageConfigBuilderReorderTest.java +git commit -m "WW-3784 feat(core): add PackageConfig.Builder.reorderActionConfigs + +Co-Authored-By: Claude Opus 4.8 " +``` + +--- + +### Task 3: Wire specificity ordering into `PackageBasedActionConfigBuilder` + +Apply the comparator to every package the Convention plugin builds, right after index actions are added and before packages are registered with the configuration. + +**Files:** +- Modify: `plugins/convention/src/main/java/org/apache/struts2/convention/PackageBasedActionConfigBuilder.java` (add `import java.util.Comparator;` near line 75; call the new method in `buildConfiguration` after `buildIndexActions(packageConfigs);` at line 796; add the `reorderActionConfigsBySpecificity` method) +- Test: `plugins/convention/src/test/java/org/apache/struts2/convention/PackageBasedActionConfigBuilderReorderTest.java` + +**Interfaces:** +- Consumes: `ActionNameSpecificityComparator` (Task 1); `PackageConfig.Builder.reorderActionConfigs(Comparator)` (Task 2). +- Produces: `static void reorderActionConfigsBySpecificity(Map packageConfigs)` — package-private, `static` so it is unit-testable without constructing a builder; applies `new ActionNameSpecificityComparator()` to every builder in the map. + +- [ ] **Step 1: Write the failing test** + +```java +/* + * 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; + +import org.apache.struts2.config.entities.ActionConfig; +import org.apache.struts2.config.entities.PackageConfig; +import org.junit.Test; + +import java.util.ArrayList; +import java.util.HashMap; +import java.util.List; +import java.util.Map; + +import static org.junit.Assert.assertEquals; + +public class PackageBasedActionConfigBuilderReorderTest { + + @Test + public void reordersEveryPackageSpecificFirst() { + // input insertion order is general-before-specific (the bug scenario) + PackageConfig.Builder pkg = new PackageConfig.Builder("test"); + pkg.addActionConfig("some/*", action("some/*")); + pkg.addActionConfig("some/usefull/*", action("some/usefull/*")); + + Map packageConfigs = new HashMap<>(); + packageConfigs.put("test", pkg); + + PackageBasedActionConfigBuilder.reorderActionConfigsBySpecificity(packageConfigs); + + List keys = new ArrayList<>(pkg.build().getActionConfigs().keySet()); + assertEquals(List.of("some/usefull/*", "some/*"), keys); + } + + private ActionConfig action(String name) { + return new ActionConfig.Builder("test", name, "com.example.Action").build(); + } +} +``` + +- [ ] **Step 2: Run test to verify it fails** + +Run: `mvn test -DskipAssembly -pl plugins/convention -am -Dtest=PackageBasedActionConfigBuilderReorderTest` +Expected: FAIL to compile — `reorderActionConfigsBySpecificity` does not exist. + +- [ ] **Step 3: Add the import, the call site, and the method** + +Add near line 75 (with the other `java.util` imports): + +```java +import java.util.Comparator; +``` + +In `buildConfiguration`, change the block at lines 796-802 so the reorder runs after index actions and before registration: + +```java + buildIndexActions(packageConfigs); + + reorderActionConfigsBySpecificity(packageConfigs); + + // Add the new actions to the configuration + Set packageNames = packageConfigs.keySet(); + for (String packageName : packageNames) { + configuration.addPackageConfig(packageName, packageConfigs.get(packageName).build()); + } +``` + +Add the method (e.g. immediately after `buildConfiguration`, before `getAllowedMethods` at line 805): + +```java + /** + * Reorders each package's action configs most-specific-first so that annotated wildcard + * patterns follow specific-before-general precedence under first-match-wins matching. + * XML-defined packages are untouched: only packages built by this convention builder pass + * through here. + * + * @param packageConfigs the packages built during {@link #buildConfiguration(Set)} + * @since 7.3.0 (WW-3784) + */ + static void reorderActionConfigsBySpecificity(Map packageConfigs) { + Comparator bySpecificity = new ActionNameSpecificityComparator(); + for (PackageConfig.Builder packageConfig : packageConfigs.values()) { + packageConfig.reorderActionConfigs(bySpecificity); + } + } +``` + +- [ ] **Step 4: Run test to verify it passes** + +Run: `mvn test -DskipAssembly -pl plugins/convention -am -Dtest=PackageBasedActionConfigBuilderReorderTest` +Expected: PASS (1 test). + +- [ ] **Step 5: Run the full convention + core suites for regressions** + +Run: `mvn test -DskipAssembly -pl core,plugins/convention -am` +Expected: PASS. Pay attention to `PackageBasedActionConfigBuilderTest` — if any assertion depended on the old (arbitrary) action-config ordering, update that expectation to the new specificity order and note it in the commit body. + +- [ ] **Step 6: Commit** + +```bash +git add plugins/convention/src/main/java/org/apache/struts2/convention/PackageBasedActionConfigBuilder.java \ + plugins/convention/src/test/java/org/apache/struts2/convention/PackageBasedActionConfigBuilderReorderTest.java +git commit -m "WW-3784 feat(convention): order annotated wildcard actions most-specific-first + +Sorts each convention-built package's action configs by pattern specificity so a +specific pattern (some/usefull/*) is matched before a general one (some/*), +regardless of class-scan order. Also makes convention action ordering deterministic. + +Co-Authored-By: Claude Opus 4.8 " +``` + +--- + +## Notes for the implementer + +- **Known limitation (documented in the spec):** ordering is per-package. Convention places all actions of one namespace into the same package, so the common case is covered; competing wildcards spread across *different* convention packages sharing a namespace remain in package-registration order. Do not try to expand scope to namespace-wide ordering — that would require touching core `DefaultConfiguration` and risk affecting XML. +- **Do not add a config flag.** The behavior is always on for Convention by design. +- **If `PackageBasedActionConfigBuilderTest` fails only on ordering:** that is expected fallout of making order deterministic; re-baseline the affected assertions to specificity order. Any *non-ordering* failure is a real regression — stop and investigate. + +## Self-Review + +- **Spec coverage:** + - Scalar multi-key comparator (spec §"The specificity comparator") → Task 1. + - `PackageConfig.Builder` reorder helper (spec §"Architecture & placement") → Task 2. + - Convention integration point after `buildIndexActions` (spec §"Integration point") → Task 3. + - Testing — comparator unit tests incl. shuffle-invariance (spec §Testing) → Task 1; wiring/order test → Task 3; core helper test → Task 2. + - XML/core-matcher untouched (spec §Scope, §Out of scope) → enforced by Global Constraints; no such files modified. +- **Placeholder scan:** none — all steps contain concrete code and exact commands. +- **Type consistency:** `reorderActionConfigs(Comparator)` is defined in Task 2 and consumed with the identical signature in Task 3; `ActionNameSpecificityComparator` is defined in Task 1 and instantiated in Task 3; `reorderActionConfigsBySpecificity(Map)` is defined and tested with the same signature in Task 3. diff --git a/docs/superpowers/specs/2026-07-26-WW-3784-annotated-wildcard-specificity-ordering-design.md b/docs/superpowers/specs/2026-07-26-WW-3784-annotated-wildcard-specificity-ordering-design.md new file mode 100644 index 0000000000..efffbeb851 --- /dev/null +++ b/docs/superpowers/specs/2026-07-26-WW-3784-annotated-wildcard-specificity-ordering-design.md @@ -0,0 +1,197 @@ +# WW-3784 — Specificity-ordered wildcard matching for annotated actions + +- **Jira:** [WW-3784](https://issues.apache.org/jira/browse/WW-3784) — *Greedy and non-greedy matching behaviour should work in action methods using annotated wildcards* +- **Type:** Bug (Core Actions) +- **Fix version:** 7.3.0 +- **Date:** 2026-07-26 + +## Problem + +Struts matches wildcard action patterns on a **first-match-wins, insertion-order** basis. +`AbstractMatcher.match()` iterates its `compiledPatterns` list and `break`s on the first hit: + +```java +for (Mapping m : compiledPatterns) { + if (wildcard.match(vars, potentialMatch, m.pattern())) { + config = convert(potentialMatch, m.target(), vars); + break; + } +} +``` + +In **XML** configuration, precedence is controlled by *physically ordering* mappings — specific +patterns are placed before general ones, so first-match-wins does the right thing. + +In **annotation-based** configuration (Convention plugin `@Action` wildcards), there is **no ordering +guarantee**. The registration order is derived from `Set>` class-scan order in +`PackageBasedActionConfigBuilder.buildConfiguration(...)`, which is effectively arbitrary and +non-deterministic across JVMs/classloaders. Consequently, when two annotated patterns genuinely +overlap — most clearly when a broad `**` pattern (the only token that crosses `/`) is registered +before a narrower, more specific pattern it can also match, or when two patterns share the same +matchable shape — the general pattern can win first-match-wins and shadow the specific one, leaving +the specific action unreachable. + +### Concrete example (from the ticket) + +| Pattern (annotated) | Intent | +|---|---| +| `some/usefull/*` | specific — should handle `/some/usefull/sleeping` | +| `some/*` | general — should handle `/some/eating` | + +**Clarification (post-ticket):** with the default `WildcardHelper` matcher, single `*` (`MATCH_FILE`) +matches only within one path segment and does **not** cross `/` — only `**` (`MATCH_PATH`) does. +Under that rule, `some/*` matches exactly one segment after `some/` and `some/usefull/*` matches +exactly one segment after `some/usefull/` — different segment counts, so the two patterns are +**disjoint** and never compete for the same incoming URL; `some/*` alone cannot shadow +`some/usefull/*`. The ticket's original example predates this clarification. The comparator still +orders this pair deterministically (see the worked ranking below), which is a harmless improvement +over non-deterministic scan order, but the *shadowing* failure mode described above requires +genuinely overlapping patterns — which arises most clearly via `**` (e.g. a `**` catch-all registered +ahead of a narrower sibling it can also match) or via patterns that share the same matchable shape. + +### Matchers in play + +- `WildcardHelper` (default bean `struts`): `*` (`MATCH_FILE`) matches within a single path segment + and does **not** cross `/`; `**` (`MATCH_PATH`) is the only token that crosses `/`. +- `NamedVariablePatternMatcher` (bean `namedVariable`): `{var}` → `([^/]+)`, also does **not** cross + `/`. + +The defect is fundamentally about **match precedence / ordering**, not the regex semantics +themselves. + +## Scope decision + +**Automatic specificity-ordering, annotation-sourced configs only.** XML keeps its explicit +file-order semantics untouched. No configuration flag — always on for Convention. This is safe +because the prior Convention order was non-deterministic, so no application could reliably depend on +it. + +Rejected alternatives: + +- *Specificity-ordering for all configs incl. XML* — would change long-standing XML first-match-wins + behavior and risk breaking configs that rely on order. +- *Opt-in flag* — pushes the burden onto users who would need to discover it; the bug should just be + fixed for annotations. + +## Architecture & placement + +The entire change lives in the **Convention plugin**. Core matchers +(`AbstractMatcher` / `ActionConfigMatcher`) and the XML configuration providers are **not modified**, +so XML behavior is fully preserved. + +We change *the order in which Convention registers wildcard action patterns into each +`PackageConfig`*. That `LinkedHashMap` insertion order is exactly what flows through +`DefaultConfiguration` into the per-namespace `ActionConfigMatcher` and drives the runtime +first-match-wins loop. + +``` +@Action wildcards + │ (Set scan order — arbitrary) + ▼ +PackageBasedActionConfigBuilder.buildConfiguration() + │ ── NEW: reorder each PackageConfig.Builder's actions by specificity + ▼ +PackageConfig.actionConfigs (LinkedHashMap, now specific-first) + │ + ▼ +DefaultConfiguration → ActionConfigMatcher (first-match-wins over specific-first list) +``` + +Two pieces: + +1. **`ActionNameSpecificityComparator`** — new pure `Comparator` over action-name patterns, + in `org.apache.struts2.convention`. +2. **A sort pass** in `PackageBasedActionConfigBuilder`, applied to each `PackageConfig.Builder` + after all actions are collected and before packages are handed to the configuration. + +One small, generic helper is added to core so the plugin can reorder a builder's map: +`PackageConfig.Builder.reorderActionConfigs(Comparator byActionName)`, which clears and +re-inserts `actionConfigs` in sorted key order. It is a neutral utility that XML code never calls. + +## The specificity comparator + +`ActionNameSpecificityComparator implements Comparator` orders action-name patterns +**most-specific first** using these keys, in order: + +1. **Fewer wildcard tokens** first. A token is a `*` / `**` run (WildcardHelper) or a `{var}` group + (NamedVariable). +2. **More literal characters** first — total pattern length minus the characters consumed by wildcard + tokens. +3. **`*` before `**`** — fewer path-spanning (`**`) tokens is more specific. +4. **Alphabetical** on the raw pattern string — deterministic tiebreak. + +Notes: + +- The comparator is **matcher-agnostic**: it counts both `*`/`**` runs and `{var}` groups as + wildcards, so it behaves correctly whether the application uses `WildcardHelper` or + `NamedVariablePatternMatcher`. +- Literal (wildcard-free) names naturally sort first (0 wildcards, all-literal). This is harmless: + literal action names are resolved by exact-map lookup (`actions.get(name)`) **before** the wildcard + loop runs, so their relative order never affects matching. +- **Secondary benefit:** ordering becomes **deterministic** across JVMs/classloaders, which it is not + today. + +### Worked ranking — ticket case + +| Pattern | wildcards | literal chars | `**` count | order | +|---|---|---|---|---| +| `some/usefull/*` | 1 | 13 | 0 | **1st (specific)** | +| `some/*` | 1 | 5 | 0 | 2nd (general) | + +Tie on key 1 (both 1 wildcard) → key 2 decides: `some/usefull/*` has more literal characters, so it +is tried first. Result: `/some/usefull/sleeping` → specific action; `/some/eating` → general action — +regardless of scan order. + +## Integration point + +In `PackageBasedActionConfigBuilder.buildConfiguration(Set> classes)`, after the `classes` +loop finishes populating the `packageConfigs` map, iterate each `PackageConfig.Builder` and reorder +its action configs via `reorderActionConfigs(new ActionNameSpecificityComparator())`. Existing +downstream steps (index actions, adding packages to the configuration) run unchanged on the reordered +builders. + +## Edge cases & limitations + +- **Per-package scope.** Sorting is applied within each `PackageConfig`. Convention places all actions + of a given namespace into the same package builder, so the common "several action classes, one + namespace" case is fully covered. Competing wildcards spread across *different* convention packages + that share a namespace remain in package-registration order. This is documented as a known limit and + is out of scope; addressing it would require moving ordering into core `DefaultConfiguration`, which + would risk affecting XML. +- **Genuine ties.** Patterns of identical specificity that truly overlap resolve alphabetically — + deterministic, if arbitrary. Documented behavior. +- **No config flag.** Always on for Convention (per scope decision). The prior order was + non-deterministic, so nothing could reliably depend on it. +- **Known limitation — primary key can misrank `**` vs. multi-token patterns.** The comparator's + primary key is *raw* wildcard-token count, not `**`-awareness. A single-token `**` catch-all (1 + wildcard) therefore ranks ahead of a narrower two-token pattern such as `*/*` (2 wildcards) even + though `*/*` matches a strictly smaller set of paths, so a `**` catch-all can shadow a more-specific + sibling within the same package. This is an accepted known limitation — it is still a net + improvement over the prior non-deterministic order, since the outcome is at least stable across + JVMs/classloaders. A future refinement would lift the `**`-count key above the raw + wildcard-token-count key so path-spanning patterns are always penalized first; that is out of scope + for this fix. +- **Known limitation — parent-package actions are not resorted.** + `PackageConfig.getAllActionConfigs()` inserts parent-package actions into the result map *before* + the (sorted) own-package actions, so a parent package's wildcard action would still be matched ahead + of the sorted actions of a child package regardless of specificity. In practice this has no + observable impact: convention parent packages (e.g. `struts-default`) declare no wildcard action + mappings, so there is nothing there to shadow child-package actions. + +## Testing + +- **Unit — `ActionNameSpecificityComparator`:** + - Ticket case: `some/usefull/*` ranks before `some/*`. + - `*` vs `**`: single-star ranks before double-star at equal literal length. + - `{var}` patterns ranked consistently with `*` patterns. + - Literals rank before any wildcard pattern. + - **Shuffle-invariance:** a randomized input list produces an identical sorted output. +- **Integration — Convention plugin:** register competing annotated wildcards on action classes and + assert that `/some/usefull/sleeping` resolves to the specific action and `/some/eating` to the + general action, independent of class-registration order. + +## Out of scope + +- Changes to XML wildcard precedence or core matcher semantics. +- Namespace-wide (cross-package) ordering. +- Any change to greedy vs. non-greedy regex behavior of `WildcardHelper` / `NamedVariablePatternMatcher`. diff --git a/plugins/convention/src/main/java/org/apache/struts2/convention/ActionNameSpecificityComparator.java b/plugins/convention/src/main/java/org/apache/struts2/convention/ActionNameSpecificityComparator.java new file mode 100644 index 0000000000..a0455dcedc --- /dev/null +++ b/plugins/convention/src/main/java/org/apache/struts2/convention/ActionNameSpecificityComparator.java @@ -0,0 +1,98 @@ +/* + * 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; + +import java.util.Comparator; + +/** + * Orders wildcard action-name patterns most-specific-first so that, under the framework's + * first-match-wins matching, a specific pattern (e.g. {@code some/usefull/*}) is evaluated + * before a general one (e.g. {@code some/*}). + * + *

Ordering keys, applied in order:

+ *
    + *
  1. fewer wildcard tokens first (a {@code *}/{@code **} run, or a {var} group);
    2. + *
  2. more literal characters first;
    3. + *
  3. fewer path-spanning {@code **} tokens first;
    4. + *
  4. natural (alphabetical) order of the pattern, for deterministic tie-breaking.
    5. + *
+ * + *

Matcher-agnostic: it recognises both {@code *}/{@code **} (WildcardHelper) and + * {var} (NamedVariablePatternMatcher) wildcards.

+ * + * @since 7.3.0 (WW-3784) + */ +public class ActionNameSpecificityComparator implements Comparator { + + @Override + public int compare(String a, String b) { + Counts ca = count(a); + Counts cb = count(b); + + int byWildcards = Integer.compare(ca.wildcards, cb.wildcards); + if (byWildcards != 0) { + return byWildcards; + } + int byLiterals = Integer.compare(cb.literals, ca.literals); // more literals first + if (byLiterals != 0) { + return byLiterals; + } + int byPathWildcards = Integer.compare(ca.pathWildcards, cb.pathWildcards); + if (byPathWildcards != 0) { + return byPathWildcards; + } + return a.compareTo(b); + } + + private Counts count(String pattern) { + int wildcards = 0; + int pathWildcards = 0; + int literals = 0; + int i = 0; + int len = pattern.length(); + while (i < len) { + char c = pattern.charAt(i); + if (c == '*') { + int start = i; + while (i < len && pattern.charAt(i) == '*') { + i++; + } + wildcards++; + if (i - start >= 2) { + pathWildcards++; + } + } else if (c == '{') { + int close = pattern.indexOf('}', i); + if (close < 0) { + literals += len - i; // malformed: treat the remainder as literal + break; + } + wildcards++; + i = close + 1; + } else { + literals++; + i++; + } + } + return new Counts(wildcards, pathWildcards, literals); + } + + private record Counts(int wildcards, int pathWildcards, int literals) { + } +} 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 711b25b725..9054fdae95 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 @@ -69,6 +69,7 @@ import java.util.Arrays; import java.util.Collection; import java.util.Collections; +import java.util.Comparator; import java.util.HashMap; import java.util.HashSet; import java.util.List; @@ -795,6 +796,8 @@ protected void buildConfiguration(Set> classes) { buildIndexActions(packageConfigs); + reorderActionConfigsBySpecificity(packageConfigs); + // Add the new actions to the configuration Set packageNames = packageConfigs.keySet(); for (String packageName : packageNames) { @@ -802,6 +805,22 @@ protected void buildConfiguration(Set> classes) { } } + /** + * Reorders each package's action configs most-specific-first so that annotated wildcard + * patterns follow specific-before-general precedence under first-match-wins matching. + * XML-defined packages are untouched: only packages built by this convention builder pass + * through here. + * + * @param packageConfigs the packages built during {@link #buildConfiguration(Set)} + * @since 7.3.0 (WW-3784) + */ + static void reorderActionConfigsBySpecificity(Map packageConfigs) { + Comparator bySpecificity = new ActionNameSpecificityComparator(); + for (PackageConfig.Builder packageConfig : packageConfigs.values()) { + packageConfig.reorderActionConfigs(bySpecificity); + } + } + private Set getAllowedMethods(Class actionClass) { List annotations = AnnotationUtils.findAnnotations(actionClass, AllowedMethods.class); if (annotations.isEmpty()) { diff --git a/plugins/convention/src/test/java/org/apache/struts2/convention/ActionNameSpecificityComparatorTest.java b/plugins/convention/src/test/java/org/apache/struts2/convention/ActionNameSpecificityComparatorTest.java new file mode 100644 index 0000000000..f97a1d1d4c --- /dev/null +++ b/plugins/convention/src/test/java/org/apache/struts2/convention/ActionNameSpecificityComparatorTest.java @@ -0,0 +1,77 @@ +/* + * 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; + +import org.junit.Test; + +import java.util.ArrayList; +import java.util.Arrays; +import java.util.Collections; +import java.util.List; +import java.util.Random; + +import static org.junit.Assert.assertEquals; +import static org.junit.Assert.assertTrue; + +public class ActionNameSpecificityComparatorTest { + + private final ActionNameSpecificityComparator comparator = new ActionNameSpecificityComparator(); + + @Test + public void moreLiteralPrefixIsMoreSpecific_ticketCase() { + // equal wildcard count (1 each); "some/usefull/*" has more literal chars -> more specific + assertTrue(comparator.compare("some/usefull/*", "some/*") < 0); + } + + @Test + public void fewerWildcardsIsMoreSpecific() { + assertTrue(comparator.compare("a/*", "a/*/*") < 0); + } + + @Test + public void singleStarBeatsPathStarAtEqualLiterals() { + // both "a/" literal (2 chars), one wildcard each; "a/*" (file) beats "a/**" (path) + assertTrue(comparator.compare("a/*", "a/**") < 0); + } + + @Test + public void namedVariablesCountAsWildcards() { + assertTrue(comparator.compare("some/usefull/{id}", "some/{id}") < 0); + } + + @Test + public void literalRanksBeforeAnyWildcard() { + assertTrue(comparator.compare("some/list", "some/*") < 0); + } + + @Test + public void sortIsDeterministicRegardlessOfInputOrder() { + List expected = Arrays.asList("some/usefull/*", "some/*", "*"); + List shuffled = new ArrayList<>(expected); + Collections.shuffle(shuffled, new Random(42)); + shuffled.sort(comparator); + assertEquals(expected, shuffled); + } + + @Test + public void naturalOrderBreaksTiesForEquallySpecificPatterns() { + // equal on wildcard count, literal chars, and ** count -> alphabetical tiebreak + assertTrue(comparator.compare("a/*", "b/*") < 0); + } +} diff --git a/plugins/convention/src/test/java/org/apache/struts2/convention/PackageBasedActionConfigBuilderReorderTest.java b/plugins/convention/src/test/java/org/apache/struts2/convention/PackageBasedActionConfigBuilderReorderTest.java new file mode 100644 index 0000000000..110e2614a9 --- /dev/null +++ b/plugins/convention/src/test/java/org/apache/struts2/convention/PackageBasedActionConfigBuilderReorderTest.java @@ -0,0 +1,53 @@ +/* + * 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; + +import org.apache.struts2.config.entities.ActionConfig; +import org.apache.struts2.config.entities.PackageConfig; +import org.junit.Test; + +import java.util.ArrayList; +import java.util.HashMap; +import java.util.List; +import java.util.Map; + +import static org.junit.Assert.assertEquals; + +public class PackageBasedActionConfigBuilderReorderTest { + + @Test + public void reordersEveryPackageSpecificFirst() { + // input insertion order is general-before-specific (the bug scenario) + PackageConfig.Builder pkg = new PackageConfig.Builder("test"); + pkg.addActionConfig("some/*", action("some/*")); + pkg.addActionConfig("some/usefull/*", action("some/usefull/*")); + + Map packageConfigs = new HashMap<>(); + packageConfigs.put("test", pkg); + + PackageBasedActionConfigBuilder.reorderActionConfigsBySpecificity(packageConfigs); + + List keys = new ArrayList<>(pkg.build().getActionConfigs().keySet()); + assertEquals(List.of("some/usefull/*", "some/*"), keys); + } + + private ActionConfig action(String name) { + return new ActionConfig.Builder("test", name, "com.example.Action").build(); + } +} diff --git a/plugins/convention/src/test/java/org/apache/struts2/convention/WildcardSpecificityRoutingTest.java b/plugins/convention/src/test/java/org/apache/struts2/convention/WildcardSpecificityRoutingTest.java new file mode 100644 index 0000000000..c61105f8e6 --- /dev/null +++ b/plugins/convention/src/test/java/org/apache/struts2/convention/WildcardSpecificityRoutingTest.java @@ -0,0 +1,91 @@ +/* + * 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; + +import org.apache.struts2.config.entities.ActionConfig; +import org.apache.struts2.config.entities.PackageConfig; +import org.apache.struts2.config.impl.ActionConfigMatcher; +import org.apache.struts2.util.WildcardHelper; +import org.junit.Test; + +import java.util.Map; + +import static org.junit.Assert.assertEquals; + +/** + * End-to-end proof that specificity ordering fixes the WW-3784 shadowing bug: it drives the + * production reorder ({@link PackageConfig.Builder#reorderActionConfigs} with + * {@link ActionNameSpecificityComparator}) through the real runtime matcher + * ({@link ActionConfigMatcher} over {@link WildcardHelper}) and asserts that a request which two + * genuinely-overlapping wildcard patterns can both match is routed to the more specific action. + * + *

{@code some/**} and {@code some/usefull/*} genuinely overlap for {@code some/usefull/sleeping} + * because {@code **} ({@code MATCH_PATH}) crosses '/', so first-match-wins order decides the winner. + * With the general {@code some/**} registered first, it shadows {@code some/usefull/*}; after + * specificity ordering, the specific pattern wins.

+ */ +public class WildcardSpecificityRoutingTest { + + private static final String GENERAL_PATTERN = "some/**"; + private static final String SPECIFIC_PATTERN = "some/usefull/*"; + private static final String GENERAL_CLASS = "com.example.GeneralAction"; + private static final String SPECIFIC_CLASS = "com.example.SpecificAction"; + private static final String OVERLAPPING_REQUEST = "some/usefull/sleeping"; + private static final String GENERAL_ONLY_REQUEST = "some/eating"; + + @Test + public void generalPatternShadowsSpecificWhenRegisteredFirst() { + // Reproduces the bug: general-before-specific insertion order, no reordering applied. + Map configs = generalFirstBuilder().build().getActionConfigs(); + ActionConfigMatcher matcher = new ActionConfigMatcher(new WildcardHelper(), configs, false); + + ActionConfig matched = matcher.match(OVERLAPPING_REQUEST); + + assertEquals("general some/** shadows the specific action when registered first", + GENERAL_CLASS, matched.getClassName()); + } + + @Test + public void specificPatternWinsAfterSpecificityOrdering() { + // Same actions, same insertion order, but reordered most-specific-first before matching. + PackageConfig.Builder builder = generalFirstBuilder(); + builder.reorderActionConfigs(new ActionNameSpecificityComparator()); + Map configs = builder.build().getActionConfigs(); + ActionConfigMatcher matcher = new ActionConfigMatcher(new WildcardHelper(), configs, false); + + assertEquals("specific some/usefull/* is now reachable for the overlapping request", + SPECIFIC_CLASS, matcher.match(OVERLAPPING_REQUEST).getClassName()); + assertEquals("general some/** still handles requests only it can match", + GENERAL_CLASS, matcher.match(GENERAL_ONLY_REQUEST).getClassName()); + } + + private PackageConfig.Builder generalFirstBuilder() { + PackageConfig.Builder builder = new PackageConfig.Builder("test"); + builder.addActionConfig(GENERAL_PATTERN, action(GENERAL_PATTERN, GENERAL_CLASS)); + builder.addActionConfig(SPECIFIC_PATTERN, action(SPECIFIC_PATTERN, SPECIFIC_CLASS)); + return builder; + } + + private ActionConfig action(String name, String className) { + return new ActionConfig.Builder("test", name, className) + .methodName("execute") + .setStrictMethodInvocation(false) + .build(); + } +}