WW-3784 Order annotated wildcard actions most-specific-first (#1813)

* WW-3784 docs: design for specificity-ordered wildcard matching in annotated actions Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com> * WW-3784 docs: implementation plan for annotated wildcard specificity ordering Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com> * WW-3784 feat(convention): add action-name specificity comparator Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com> * WW-3784 fix(convention): add Apache License header to test file Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com> * WW-3784 feat(core): add PackageConfig.Builder.reorderActionConfigs Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com> * WW-3784 docs: add javadoc for PackageConfig.Builder.reorderActionConfigs Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com> * 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 <noreply@anthropic.com> * 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 <noreply@anthropic.com> * 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 <noreply@anthropic.com> --------- Co-authored-by: Claude Opus 4.8 <noreply@anthropic.com>
2026-08-06 23:27:07 +00:00 · 2026-07-29 07:56:58 +02:00
parent 6f802987f5
commit 94a8fcb26c
9 changed files with 1084 additions and 0 deletions
@@ -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/*}).
+ *
+ * <p>Ordering keys, applied in order:</p>
+ * <ol>
+ *   <li>fewer wildcard tokens first (a {@code *}/{@code **} run, or a <code>{var}</code> group);</li>
+ *   <li>more literal characters first;</li>
+ *   <li>fewer path-spanning {@code **} tokens first;</li>
+ *   <li>natural (alphabetical) order of the pattern, for deterministic tie-breaking.</li>
+ * </ol>
+ *
+ * <p>Matcher-agnostic: it recognises both {@code *}/{@code **} (WildcardHelper) and
+ * <code>{var}</code> (NamedVariablePatternMatcher) wildcards.</p>
+ *
+ * @since 7.3.0 (WW-3784)
+ */
+public class ActionNameSpecificityComparator implements Comparator<String> {
+
+    @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) {
+    }
+}
@@ -69,6 +69,7 @@ import java.util.ArrayList;
 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 @@ public class PackageBasedActionConfigBuilder implements ActionConfigBuilder {

        buildIndexActions(packageConfigs);

+        reorderActionConfigsBySpecificity(packageConfigs);
+
        // Add the new actions to the configuration
        Set<String> packageNames = packageConfigs.keySet();
        for (String packageName : packageNames) {
@@ -802,6 +805,22 @@ public class PackageBasedActionConfigBuilder implements ActionConfigBuilder {
        }
    }

+    /**
+     * 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<String, PackageConfig.Builder> packageConfigs) {
+        Comparator<String> bySpecificity = new ActionNameSpecificityComparator();
+        for (PackageConfig.Builder packageConfig : packageConfigs.values()) {
+            packageConfig.reorderActionConfigs(bySpecificity);
+        }
+    }
+
    private Set<String> getAllowedMethods(Class<?> actionClass) {
        List<AllowedMethods> annotations = AnnotationUtils.findAnnotations(actionClass, AllowedMethods.class);
        if (annotations.isEmpty()) {
@@ -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<String> expected = Arrays.asList("some/usefull/*", "some/*", "*");
+        List<String> 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);
+    }
+}
@@ -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<String, PackageConfig.Builder> packageConfigs = new HashMap<>();
+        packageConfigs.put("test", pkg);
+
+        PackageBasedActionConfigBuilder.reorderActionConfigsBySpecificity(packageConfigs);
+
+        List<String> 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();
+    }
+}
@@ -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.
+ *
+ * <p>{@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.</p>
+ */
+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<String, ActionConfig> 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<String, ActionConfig> 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();
+    }
+}