From 5de416ae21a69ae80fba09e85deb0578cc7e5c19 Mon Sep 17 00:00:00 2001 From: Lukasz Lenart Date: Sun, 26 Jul 2026 05:29:33 +0200 Subject: [PATCH 1/9] WW-3784 docs: design for specificity-ordered wildcard matching in annotated actions Co-Authored-By: Claude Opus 4.8 --- ...ed-wildcard-specificity-ordering-design.md | 171 ++++++++++++++++++ 1 file changed, 171 insertions(+) create mode 100644 docs/superpowers/specs/2026-07-26-WW-3784-annotated-wildcard-specificity-ordering-design.md 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..4f385ae4fa --- /dev/null +++ b/docs/superpowers/specs/2026-07-26-WW-3784-annotated-wildcard-specificity-ordering-design.md @@ -0,0 +1,171 @@ +# 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 a general pattern such as `some/*` can be +evaluated before, and shadow, a more specific pattern such as `some/usefull/*`, 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` | + +With the default `WildcardHelper` matcher, `*` is greedy and crosses `/`, so `some/*` also matches +`/some/usefull/sleeping`. If `some/*` happens to be registered first, it wins and `some/usefull/*` +never matches. + +### Matchers in play + +- `WildcardHelper` (default bean `struts`): `*` and `**`, greedy, `*` crosses `/`. +- `NamedVariablePatternMatcher` (bean `namedVariable`): `{var}` → `([^/]+)`, 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. + +## 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`. From 9f70d466993958f437f4fbd8f2bbd3c10decfb30 Mon Sep 17 00:00:00 2001 From: Lukasz Lenart Date: Sun, 26 Jul 2026 05:34:25 +0200 Subject: [PATCH 2/9] WW-3784 docs: implementation plan for annotated wildcard specificity ordering Co-Authored-By: Claude Opus 4.8 --- ...annotated-wildcard-specificity-ordering.md | 484 ++++++++++++++++++ 1 file changed, 484 insertions(+) create mode 100644 docs/superpowers/plans/2026-07-26-WW-3784-annotated-wildcard-specificity-ordering.md 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. From 429d49509b70811c21214fbc6ee5af641ba9af73 Mon Sep 17 00:00:00 2001 From: Lukasz Lenart Date: Sun, 26 Jul 2026 05:42:00 +0200 Subject: [PATCH 3/9] WW-3784 feat(convention): add action-name specificity comparator Co-Authored-By: Claude Opus 4.8 --- .../ActionNameSpecificityComparator.java | 98 +++++++++++++++++++ .../ActionNameSpecificityComparatorTest.java | 53 ++++++++++ 2 files changed, 151 insertions(+) create mode 100644 plugins/convention/src/main/java/org/apache/struts2/convention/ActionNameSpecificityComparator.java create mode 100644 plugins/convention/src/test/java/org/apache/struts2/convention/ActionNameSpecificityComparatorTest.java 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/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..5722a4ae37 --- /dev/null +++ b/plugins/convention/src/test/java/org/apache/struts2/convention/ActionNameSpecificityComparatorTest.java @@ -0,0 +1,53 @@ +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); + } +} From 93cfe1ea01d2f0a1c13373b852278248cbd415bb Mon Sep 17 00:00:00 2001 From: Lukasz Lenart Date: Sun, 26 Jul 2026 05:46:00 +0200 Subject: [PATCH 4/9] WW-3784 fix(convention): add Apache License header to test file Co-Authored-By: Claude Opus 4.8 --- .../ActionNameSpecificityComparatorTest.java | 18 ++++++++++++++++++ 1 file changed, 18 insertions(+) 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 index 5722a4ae37..33f451bf4c 100644 --- a/plugins/convention/src/test/java/org/apache/struts2/convention/ActionNameSpecificityComparatorTest.java +++ b/plugins/convention/src/test/java/org/apache/struts2/convention/ActionNameSpecificityComparatorTest.java @@ -1,3 +1,21 @@ +/* + * 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; From da414ee25a87d918deec371c30d1dfd93e4acfa3 Mon Sep 17 00:00:00 2001 From: Lukasz Lenart Date: Sun, 26 Jul 2026 05:48:31 +0200 Subject: [PATCH 5/9] WW-3784 feat(core): add PackageConfig.Builder.reorderActionConfigs Co-Authored-By: Claude Opus 4.8 --- .../config/entities/PackageConfig.java | 12 +++++ .../PackageConfigBuilderReorderTest.java | 44 +++++++++++++++++++ 2 files changed, 56 insertions(+) create mode 100644 core/src/test/java/org/apache/struts2/config/entities/PackageConfigBuilderReorderTest.java 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..df9c7c8f19 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,17 @@ public Builder addActionConfig(String name, ActionConfig action) { return this; } + 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(); + } +} From 6402719bc6f955d5604a799c78ae2aa29afc346b Mon Sep 17 00:00:00 2001 From: Lukasz Lenart Date: Sun, 26 Jul 2026 05:51:12 +0200 Subject: [PATCH 6/9] WW-3784 docs: add javadoc for PackageConfig.Builder.reorderActionConfigs Co-Authored-By: Claude Opus 4.8 --- .../apache/struts2/config/entities/PackageConfig.java | 9 +++++++++ 1 file changed, 9 insertions(+) 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 df9c7c8f19..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 @@ -518,6 +518,15 @@ 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)); From caf474f53847a161fe40b8e61b2cded33284a5ef Mon Sep 17 00:00:00 2001 From: Lukasz Lenart Date: Sun, 26 Jul 2026 05:55:32 +0200 Subject: [PATCH 7/9] 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 --- .../PackageBasedActionConfigBuilder.java | 19 +++++++ ...geBasedActionConfigBuilderReorderTest.java | 53 +++++++++++++++++++ 2 files changed, 72 insertions(+) create mode 100644 plugins/convention/src/test/java/org/apache/struts2/convention/PackageBasedActionConfigBuilderReorderTest.java 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/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(); + } +} From 035199c33e2881b5351739df4a6b6893583bccea Mon Sep 17 00:00:00 2001 From: Lukasz Lenart Date: Sun, 26 Jul 2026 06:05:34 +0200 Subject: [PATCH 8/9] WW-3784 docs: correct wildcard cross-segment claims and note comparator limitations The spec incorrectly stated that WildcardHelper's single `*` is greedy and crosses `/`, and that `some/*` shadows `some/usefull/*`. Verified against WildcardHelper.java and NamedVariablePatternMatcher.java: only `**` crosses `/`, so those two patterns are actually disjoint (different segment counts) and never compete for the same request. Correct the Problem narrative, ticket example, and matcher bullets to state this accurately, and document two known limitations of the specificity comparator (raw wildcard-token-count key can misrank `**` ahead of narrower multi-token patterns; parent-package actions bypass sorting). Also add a test asserting the natural-order alphabetical tiebreak key. Co-Authored-By: Claude Opus 4.8 --- ...ed-wildcard-specificity-ordering-design.md | 42 +++++++++++++++---- .../ActionNameSpecificityComparatorTest.java | 6 +++ 2 files changed, 40 insertions(+), 8 deletions(-) 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 index 4f385ae4fa..efffbeb851 100644 --- 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 @@ -25,9 +25,11 @@ patterns are placed before general ones, so first-match-wins does the right thin 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 a general pattern such as `some/*` can be -evaluated before, and shadow, a more specific pattern such as `some/usefull/*`, leaving the specific -action unreachable. +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) @@ -36,14 +38,23 @@ action unreachable. | `some/usefull/*` | specific — should handle `/some/usefull/sleeping` | | `some/*` | general — should handle `/some/eating` | -With the default `WildcardHelper` matcher, `*` is greedy and crosses `/`, so `some/*` also matches -`/some/usefull/sleeping`. If `some/*` happens to be registered first, it wins and `some/usefull/*` -never matches. +**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`): `*` and `**`, greedy, `*` crosses `/`. -- `NamedVariablePatternMatcher` (bean `namedVariable`): `{var}` → `([^/]+)`, does **not** cross `/`. +- `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. @@ -151,6 +162,21 @@ builders. 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 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 index 33f451bf4c..f97a1d1d4c 100644 --- a/plugins/convention/src/test/java/org/apache/struts2/convention/ActionNameSpecificityComparatorTest.java +++ b/plugins/convention/src/test/java/org/apache/struts2/convention/ActionNameSpecificityComparatorTest.java @@ -68,4 +68,10 @@ public void sortIsDeterministicRegardlessOfInputOrder() { 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); + } } From 5472b65801698829e945f05224eb9214732a348b Mon Sep 17 00:00:00 2001 From: Lukasz Lenart Date: Sun, 26 Jul 2026 08:59:57 +0200 Subject: [PATCH 9/9] WW-3784 test(convention): prove specificity ordering fixes wildcard shadowing end-to-end Adds an end-to-end routing test driving the production reorder (PackageConfig.Builder.reorderActionConfigs + ActionNameSpecificityComparator) through the real ActionConfigMatcher/WildcardHelper. some/** and some/usefull/* genuinely overlap for some/usefull/sleeping (** crosses '/'), so the test asserts the general pattern shadows the specific one when registered first, and that specificity ordering makes the specific action reachable again. Co-Authored-By: Claude Opus 4.8 --- .../WildcardSpecificityRoutingTest.java | 91 +++++++++++++++++++ 1 file changed, 91 insertions(+) create mode 100644 plugins/convention/src/test/java/org/apache/struts2/convention/WildcardSpecificityRoutingTest.java 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(); + } +}