mirror of
https://github.com/apache/struts.git
synced 2026-08-05 22:56:59 +00:00
fix(convention): WW-5594 exclude root package classes with wildcard patterns (#1468)
The exclusion pattern "org.apache.struts2.*" was not properly excluding classes directly in the root package (like XWorkTestCase) because: 1. PackageBasedActionConfigBuilder extracts package names using substringBeforeLast(className, ".") which produces "org.apache.struts2" (no trailing dot) 2. The wildcard pattern requires a literal "." before "*" 3. Result: Pattern doesn't match root package classes Fix: Enhanced checkExcludePackages() to automatically handle patterns ending with ".*" by also checking if the package name equals the base pattern (without ".*"). Now "org.apache.struts2.*" properly excludes both: - Classes in root package: org.apache.struts2.XWorkTestCase - Classes in subpackages: org.apache.struts2.dispatcher.SomeClass Closes [WW-5594](https://issues.apache.org/jira/browse/WW-5594) 🤖 Generated with [Claude Code](https://claude.com/claude-code) Co-authored-by: Claude <noreply@anthropic.com>
This commit is contained in:
Lukasz Lenart
@@ -80,4 +80,57 @@ public class WildcardHelperTest extends XWorkTestCase {
|
||||
assertEquals("", matchedPatterns.get("1"));
|
||||
}
|
||||
|
||||
/**
|
||||
* WW-5594: Tests that pattern "org.apache.struts2.*" does NOT match "org.apache.struts2"
|
||||
* (package name without trailing dot).
|
||||
* <p>
|
||||
* This is important because when checking exclusion patterns, the convention plugin
|
||||
* extracts package names from class names using StringUtils.substringBeforeLast(className, "."),
|
||||
* which produces package names without trailing dots.
|
||||
* <p>
|
||||
* For example:
|
||||
* - Class "org.apache.struts2.XWorkTestCase" -> package "org.apache.struts2" (no trailing dot)
|
||||
* - Pattern "org.apache.struts2.*" requires a literal "." before the wildcard
|
||||
* - Result: Pattern doesn't match, class is not excluded
|
||||
* <p>
|
||||
* The fix is to include both "org.apache.struts2" and "org.apache.struts2.*" in exclusion patterns.
|
||||
*/
|
||||
public void testWW5594_WildcardPatternRequiresTrailingDot() {
|
||||
// given
|
||||
HashMap<String, String> matchedPatterns = new HashMap<>();
|
||||
int[] wildcardPattern = wildcardHelper.compilePattern("org.apache.struts2.*");
|
||||
|
||||
// when & then - Pattern with wildcard does NOT match package name without trailing dot
|
||||
// This is the root cause of WW-5594
|
||||
assertFalse("Pattern 'org.apache.struts2.*' should NOT match 'org.apache.struts2' (no trailing dot)",
|
||||
wildcardHelper.match(matchedPatterns, "org.apache.struts2", wildcardPattern));
|
||||
|
||||
// But it DOES match with trailing dot
|
||||
assertTrue("Pattern 'org.apache.struts2.*' should match 'org.apache.struts2.' (with trailing dot)",
|
||||
wildcardHelper.match(matchedPatterns, "org.apache.struts2.", wildcardPattern));
|
||||
|
||||
// And it DOES match full class names
|
||||
assertTrue("Pattern 'org.apache.struts2.*' should match 'org.apache.struts2.SomeClass'",
|
||||
wildcardHelper.match(matchedPatterns, "org.apache.struts2.SomeClass", wildcardPattern));
|
||||
}
|
||||
|
||||
/**
|
||||
* WW-5594: Tests that exact package pattern matches package names correctly.
|
||||
* To properly exclude classes in a root package, use both the exact package name
|
||||
* and the wildcard pattern.
|
||||
*/
|
||||
public void testWW5594_ExactPackagePatternMatchesPackageName() {
|
||||
// given
|
||||
HashMap<String, String> matchedPatterns = new HashMap<>();
|
||||
int[] exactPattern = wildcardHelper.compilePattern("org.apache.struts2");
|
||||
|
||||
// when & then - Exact pattern matches exactly
|
||||
assertTrue("Exact pattern 'org.apache.struts2' should match 'org.apache.struts2'",
|
||||
wildcardHelper.match(matchedPatterns, "org.apache.struts2", exactPattern));
|
||||
|
||||
// But exact pattern does NOT match subpackages
|
||||
assertFalse("Exact pattern 'org.apache.struts2' should NOT match 'org.apache.struts2.core'",
|
||||
wildcardHelper.match(matchedPatterns, "org.apache.struts2.core", exactPattern));
|
||||
}
|
||||
|
||||
}
|
||||
|
||||
+26
-5
@@ -18,6 +18,7 @@
|
||||
*/
|
||||
package org.apache.struts2.convention;
|
||||
|
||||
import org.apache.commons.lang3.Strings;
|
||||
import org.apache.struts2.ActionContext;
|
||||
import org.apache.struts2.FileManager;
|
||||
import org.apache.struts2.FileManagerFactory;
|
||||
@@ -406,7 +407,7 @@ public class PackageBasedActionConfigBuilder implements ActionConfigBuilder {
|
||||
if (ctx != null) {
|
||||
classLoaderInterface = (ClassLoaderInterface) ctx.get(ClassLoaderInterface.CLASS_LOADER_INTERFACE);
|
||||
}
|
||||
return ObjectUtils.defaultIfNull(classLoaderInterface, new ClassLoaderInterfaceDelegate(getClassLoader()));
|
||||
return ObjectUtils.getIfNull(classLoaderInterface, new ClassLoaderInterfaceDelegate(getClassLoader()));
|
||||
}
|
||||
}
|
||||
|
||||
@@ -561,7 +562,17 @@ public class PackageBasedActionConfigBuilder implements ActionConfigBuilder {
|
||||
}
|
||||
|
||||
/**
|
||||
* Checks if provided class package is on the exclude list
|
||||
* Checks if provided class package is on the exclude list.
|
||||
* <p>
|
||||
* WW-5594: For patterns ending with ".*", this method also checks if the package name
|
||||
* equals the base pattern (without ".*"). This ensures that classes directly in the
|
||||
* root package are excluded, not just classes in subpackages.
|
||||
* <p>
|
||||
* For example, pattern "org.apache.struts2.*" will exclude both:
|
||||
* <ul>
|
||||
* <li>Classes in subpackages like "org.apache.struts2.dispatcher.SomeClass"</li>
|
||||
* <li>Classes directly in the root package like "org.apache.struts2.XWorkTestCase"</li>
|
||||
* </ul>
|
||||
*
|
||||
* @param classPackageName name of class package
|
||||
* @return false if class package is on the {@link #excludePackages} list
|
||||
@@ -574,6 +585,16 @@ public class PackageBasedActionConfigBuilder implements ActionConfigBuilder {
|
||||
Map<String, String> matchMap = new HashMap<>();
|
||||
|
||||
for (String packageExclude : excludePackages) {
|
||||
// WW-5594: For patterns ending with ".*", also check if package equals the base
|
||||
// This handles root package exclusion (e.g., pattern "org.apache.struts2.*"
|
||||
// should also exclude classes in package "org.apache.struts2")
|
||||
if (packageExclude.endsWith(".*")) {
|
||||
String basePackage = packageExclude.substring(0, packageExclude.length() - 2);
|
||||
if (classPackageName.equals(basePackage)) {
|
||||
return false;
|
||||
}
|
||||
}
|
||||
|
||||
int[] packagePattern = wildcardHelper.compilePattern(packageExclude);
|
||||
if (wildcardHelper.match(matchMap, classPackageName, packagePattern)) {
|
||||
return false;
|
||||
@@ -647,7 +668,7 @@ public class PackageBasedActionConfigBuilder implements ActionConfigBuilder {
|
||||
* should be included in the package scan
|
||||
*/
|
||||
protected Test<ClassFinder.ClassInfo> getActionClassTest() {
|
||||
return new Test<ClassFinder.ClassInfo>() {
|
||||
return new Test<>() {
|
||||
public boolean test(ClassFinder.ClassInfo classInfo) {
|
||||
|
||||
// Why do we call includeClassNameInActionScan here, when it's
|
||||
@@ -973,7 +994,7 @@ public class PackageBasedActionConfigBuilder implements ActionConfigBuilder {
|
||||
String className = actionClass.getName();
|
||||
if (annotation != null) {
|
||||
actionName = annotation.value().equals(Action.DEFAULT_VALUE) ? actionName : annotation.value();
|
||||
actionName = StringUtils.contains(actionName, "/") && !slashesInActionNames ? StringUtils.substringAfterLast(actionName, "/") : actionName;
|
||||
actionName = Strings.CI.contains(actionName, "/") && !slashesInActionNames ? StringUtils.substringAfterLast(actionName, "/") : actionName;
|
||||
if (!Action.DEFAULT_VALUE.equals(annotation.className())) {
|
||||
className = annotation.className();
|
||||
}
|
||||
@@ -1060,7 +1081,7 @@ public class PackageBasedActionConfigBuilder implements ActionConfigBuilder {
|
||||
if (action != null && !action.value().equals(Action.DEFAULT_VALUE)) {
|
||||
LOG.trace("Using non-default action namespace from the Action annotation of [{}]", action.value());
|
||||
String actionName = action.value();
|
||||
actionNamespace = StringUtils.contains(actionName, "/") ? StringUtils.substringBeforeLast(actionName, "/") : StringUtils.EMPTY;
|
||||
actionNamespace = Strings.CI.contains(actionName, "/") ? StringUtils.substringBeforeLast(actionName, "/") : StringUtils.EMPTY;
|
||||
}
|
||||
|
||||
// Next grab the parent annotation from the class
|
||||
|
||||
@@ -21,53 +21,61 @@
|
||||
-->
|
||||
|
||||
<!DOCTYPE struts PUBLIC
|
||||
"-//Apache Software Foundation//DTD Struts Configuration 6.0//EN"
|
||||
"https://struts.apache.org/dtds/struts-6.0.dtd">
|
||||
"-//Apache Software Foundation//DTD Struts Configuration 6.0//EN"
|
||||
"https://struts.apache.org/dtds/struts-6.0.dtd">
|
||||
|
||||
<struts order="20">
|
||||
<bean type="org.apache.struts2.UnknownHandler" name="convention" class="org.apache.struts2.convention.ConventionUnknownHandler"/>
|
||||
<bean type="org.apache.struts2.UnknownHandler" name="convention"
|
||||
class="org.apache.struts2.convention.ConventionUnknownHandler"/>
|
||||
|
||||
<bean type="org.apache.struts2.convention.ActionConfigBuilder" name="convention" class="org.apache.struts2.convention.PackageBasedActionConfigBuilder"/>
|
||||
<bean type="org.apache.struts2.convention.ActionNameBuilder" name="convention" class="org.apache.struts2.convention.SEOActionNameBuilder"/>
|
||||
<bean type="org.apache.struts2.convention.ResultMapBuilder" name="convention" class="org.apache.struts2.convention.DefaultResultMapBuilder"/>
|
||||
<bean type="org.apache.struts2.convention.InterceptorMapBuilder" name="convention" class="org.apache.struts2.convention.DefaultInterceptorMapBuilder"/>
|
||||
<bean type="org.apache.struts2.convention.ConventionsService" name="convention" class="org.apache.struts2.convention.ConventionsServiceImpl"/>
|
||||
<bean type="org.apache.struts2.convention.ActionConfigBuilder" name="convention"
|
||||
class="org.apache.struts2.convention.PackageBasedActionConfigBuilder"/>
|
||||
<bean type="org.apache.struts2.convention.ActionNameBuilder" name="convention"
|
||||
class="org.apache.struts2.convention.SEOActionNameBuilder"/>
|
||||
<bean type="org.apache.struts2.convention.ResultMapBuilder" name="convention"
|
||||
class="org.apache.struts2.convention.DefaultResultMapBuilder"/>
|
||||
<bean type="org.apache.struts2.convention.InterceptorMapBuilder" name="convention"
|
||||
class="org.apache.struts2.convention.DefaultInterceptorMapBuilder"/>
|
||||
<bean type="org.apache.struts2.convention.ConventionsService" name="convention"
|
||||
class="org.apache.struts2.convention.ConventionsServiceImpl"/>
|
||||
|
||||
<bean type="org.apache.struts2.config.PackageProvider" name="convention.packageProvider" class="org.apache.struts2.convention.ClasspathPackageProvider"/>
|
||||
<bean type="org.apache.struts2.config.PackageProvider" name="convention.containerProvider" class="org.apache.struts2.convention.ClasspathConfigurationProvider"/>
|
||||
<bean type="org.apache.struts2.config.PackageProvider" name="convention.packageProvider"
|
||||
class="org.apache.struts2.convention.ClasspathPackageProvider"/>
|
||||
<bean type="org.apache.struts2.config.PackageProvider" name="convention.containerProvider"
|
||||
class="org.apache.struts2.convention.ClasspathConfigurationProvider"/>
|
||||
|
||||
<constant name="struts.convention.actionConfigBuilder" value="convention"/>
|
||||
<constant name="struts.convention.actionNameBuilder" value="convention"/>
|
||||
<constant name="struts.convention.resultMapBuilder" value="convention"/>
|
||||
<constant name="struts.convention.interceptorMapBuilder" value="convention"/>
|
||||
<constant name="struts.convention.conventionsService" value="convention"/>
|
||||
<constant name="struts.convention.actionConfigBuilder" value="convention"/>
|
||||
<constant name="struts.convention.actionNameBuilder" value="convention"/>
|
||||
<constant name="struts.convention.resultMapBuilder" value="convention"/>
|
||||
<constant name="struts.convention.interceptorMapBuilder" value="convention"/>
|
||||
<constant name="struts.convention.conventionsService" value="convention"/>
|
||||
|
||||
<constant name="struts.convention.result.path" value="/WEB-INF/content/"/>
|
||||
<constant name="struts.convention.result.flatLayout" value="true"/>
|
||||
<constant name="struts.convention.action.suffix" value="Action"/>
|
||||
<constant name="struts.convention.action.disableScanning" value="false"/>
|
||||
<constant name="struts.convention.action.mapAllMatches" value="false"/>
|
||||
<constant name="struts.convention.action.checkImplementsAction" value="true"/>
|
||||
<constant name="struts.convention.default.parent.package" value="convention-default"/>
|
||||
<constant name="struts.convention.action.name.lowercase" value="true"/>
|
||||
<constant name="struts.convention.action.name.separator" value="-"/>
|
||||
<constant name="struts.convention.package.locators" value="action,actions,struts,struts2"/>
|
||||
<constant name="struts.convention.package.locators.disable" value="false"/>
|
||||
<constant name="struts.convention.package.locators.basePackage" value=""/>
|
||||
<constant name="struts.convention.exclude.packages" value="org.apache.struts.*,org.apache.struts2.*,org.springframework.web.struts.*,org.springframework.web.struts2.*,org.hibernate.*"/>
|
||||
<constant name="struts.convention.relative.result.types" value="dispatcher,velocity,freemarker"/>
|
||||
<constant name="struts.convention.redirect.to.slash" value="true"/>
|
||||
<constant name="struts.convention.action.alwaysMapExecute" value="true"/>
|
||||
<constant name="struts.mapper.alwaysSelectFullNamespace" value="true"/>
|
||||
<!-- <constant name="struts.convention.action.includeJars" /> -->
|
||||
<constant name="struts.convention.action.fileProtocols" value="jar" />
|
||||
<constant name="struts.convention.result.path" value="/WEB-INF/content/"/>
|
||||
<constant name="struts.convention.result.flatLayout" value="true"/>
|
||||
<constant name="struts.convention.action.suffix" value="Action"/>
|
||||
<constant name="struts.convention.action.disableScanning" value="false"/>
|
||||
<constant name="struts.convention.action.mapAllMatches" value="false"/>
|
||||
<constant name="struts.convention.action.checkImplementsAction" value="true"/>
|
||||
<constant name="struts.convention.default.parent.package" value="convention-default"/>
|
||||
<constant name="struts.convention.action.name.lowercase" value="true"/>
|
||||
<constant name="struts.convention.action.name.separator" value="-"/>
|
||||
<constant name="struts.convention.package.locators" value="action,actions,struts,struts2"/>
|
||||
<constant name="struts.convention.package.locators.disable" value="false"/>
|
||||
<constant name="struts.convention.package.locators.basePackage" value=""/>
|
||||
<constant name="struts.convention.exclude.packages" value="org.apache.struts.*,org.apache.struts2.*,org.springframework.web.struts.*,org.springframework.web.struts2.*,org.hibernate.*"/>
|
||||
<constant name="struts.convention.relative.result.types" value="dispatcher,velocity,freemarker"/>
|
||||
<constant name="struts.convention.redirect.to.slash" value="true"/>
|
||||
<constant name="struts.convention.action.alwaysMapExecute" value="true"/>
|
||||
<constant name="struts.mapper.alwaysSelectFullNamespace" value="true"/>
|
||||
<!-- <constant name="struts.convention.action.includeJars" /> -->
|
||||
<constant name="struts.convention.action.fileProtocols" value="jar"/>
|
||||
|
||||
<constant name="struts.convention.classes.reload" value="false" />
|
||||
<constant name="struts.convention.classes.reload" value="false"/>
|
||||
|
||||
<constant name="struts.convention.exclude.parentClassLoader" value="true" />
|
||||
<constant name="struts.convention.exclude.parentClassLoader" value="true"/>
|
||||
|
||||
<constant name="struts.convention.enable.smi.inheritance" value="false" />
|
||||
<constant name="struts.convention.enable.smi.inheritance" value="false"/>
|
||||
|
||||
<package name="convention-default" extends="struts-default">
|
||||
</package>
|
||||
<package name="convention-default" extends="struts-default">
|
||||
</package>
|
||||
</struts>
|
||||
|
||||
+107
-33
@@ -18,7 +18,6 @@
|
||||
*/
|
||||
package org.apache.struts2.convention;
|
||||
|
||||
import org.apache.struts2.result.ActionChainResult;
|
||||
import jakarta.servlet.ServletContext;
|
||||
import junit.framework.TestCase;
|
||||
import org.apache.commons.lang3.StringUtils;
|
||||
@@ -94,6 +93,7 @@ import org.apache.struts2.inject.Container;
|
||||
import org.apache.struts2.inject.Scope.Strategy;
|
||||
import org.apache.struts2.ognl.OgnlReflectionProvider;
|
||||
import org.apache.struts2.ognl.ProviderAllowlist;
|
||||
import org.apache.struts2.result.ActionChainResult;
|
||||
import org.apache.struts2.result.Result;
|
||||
import org.apache.struts2.result.ServletDispatcherResult;
|
||||
import org.apache.struts2.util.TextParseUtil;
|
||||
@@ -127,8 +127,8 @@ public class PackageBasedActionConfigBuilderTest extends TestCase {
|
||||
public void setUp() throws Exception {
|
||||
super.setUp();
|
||||
ActionContext.of()
|
||||
.withContainer(new DummyContainer())
|
||||
.bind();
|
||||
.withContainer(new DummyContainer())
|
||||
.bind();
|
||||
}
|
||||
|
||||
public void testActionPackages() throws MalformedURLException {
|
||||
@@ -155,9 +155,83 @@ public class PackageBasedActionConfigBuilderTest extends TestCase {
|
||||
run("org.apache.struts2.convention.actions", null, null, "false");
|
||||
}
|
||||
|
||||
/**
|
||||
* WW-5594: Tests that exclusion patterns properly exclude classes in root packages.
|
||||
* <p>
|
||||
* The issue was that pattern "org.apache.struts2.*" did NOT match package name
|
||||
* "org.apache.struts2" (without trailing dot) because:
|
||||
* 1. PackageBasedActionConfigBuilder extracts package name using substringBeforeLast(className, ".")
|
||||
* 2. For class "org.apache.struts2.XWorkTestCase", this produces "org.apache.struts2" (no trailing dot)
|
||||
* 3. The wildcard pattern requires a literal "." before the "*"
|
||||
* <p>
|
||||
* The fix is in checkExcludePackages() which now handles patterns ending with ".*" by also
|
||||
* checking if the package name equals the base pattern (without ".*").
|
||||
*/
|
||||
public void testWW5594_RootPackageExclusion() {
|
||||
// Setup minimal configuration
|
||||
final DummyContainer mockContainer = new DummyContainer();
|
||||
Configuration configuration = new DefaultConfiguration() {
|
||||
@Override
|
||||
public Container getContainer() {
|
||||
return mockContainer;
|
||||
}
|
||||
};
|
||||
|
||||
ResultTypeConfig[] defaultResults = new ResultTypeConfig[]{
|
||||
new ResultTypeConfig.Builder("dispatcher", ServletDispatcherResult.class.getName())
|
||||
.defaultResultParam("location").build()
|
||||
};
|
||||
PackageConfig strutsDefault = makePackageConfig("struts-default", null, null, "dispatcher", defaultResults);
|
||||
configuration.addPackageConfig("struts-default", strutsDefault);
|
||||
|
||||
ActionNameBuilder actionNameBuilder = new SEOActionNameBuilder("true", "-");
|
||||
ObjectFactory of = new ObjectFactory();
|
||||
of.setContainer(mockContainer);
|
||||
|
||||
mockContainer.setActionNameBuilder(actionNameBuilder);
|
||||
mockContainer.setConventionsService(new ConventionsServiceImpl(""));
|
||||
|
||||
PackageBasedActionConfigBuilder builder = new PackageBasedActionConfigBuilder(
|
||||
configuration, mockContainer, of, "false", "struts-default", "false");
|
||||
|
||||
// Test 1: Wildcard pattern now properly excludes root package classes (WW-5594 fix)
|
||||
builder.setActionPackages("org.apache.struts2");
|
||||
builder.setExcludePackages("org.apache.struts2.*");
|
||||
|
||||
// Class in root package - package name is "org.apache.struts2" (no trailing dot)
|
||||
// With the fix, pattern "org.apache.struts2.*" now also matches the base package
|
||||
boolean includeRootPackageClass = builder.includeClassNameInActionScan("org.apache.struts2.XWorkTestCase");
|
||||
assertFalse("With wildcard pattern, root package class should be excluded (WW-5594 fix)",
|
||||
includeRootPackageClass);
|
||||
|
||||
// Test 2: Subpackage classes should also be excluded by wildcard pattern
|
||||
boolean includeSubpackageClass = builder.includeClassNameInActionScan("org.apache.struts2.core.ActionSupport");
|
||||
assertFalse("Subpackage classes should be excluded by wildcard pattern",
|
||||
includeSubpackageClass);
|
||||
|
||||
// Test 3: Exact pattern still works for specific package exclusion
|
||||
builder.setExcludePackages("org.apache.struts2");
|
||||
boolean includeWithExactPattern = builder.includeClassNameInActionScan("org.apache.struts2.XWorkTestCase");
|
||||
assertFalse("Exact pattern should exclude root package class",
|
||||
includeWithExactPattern);
|
||||
|
||||
// Test 4: Exact pattern should NOT exclude subpackage classes
|
||||
boolean includeSubpackageWithExact = builder.includeClassNameInActionScan("org.apache.struts2.core.ActionSupport");
|
||||
assertTrue("Exact pattern should NOT exclude subpackage classes",
|
||||
includeSubpackageWithExact);
|
||||
|
||||
// Test 5: Classes in unrelated packages should NOT be excluded
|
||||
builder.setActionPackages("com.example.actions");
|
||||
builder.setExcludePackages("org.apache.struts2.*");
|
||||
boolean includeUnrelated = builder.includeClassNameInActionScan("com.example.actions.MyAction");
|
||||
assertTrue("Classes in unrelated packages should not be excluded",
|
||||
includeUnrelated);
|
||||
}
|
||||
|
||||
private void run(String actionPackages, String packageLocators, String excludePackages) throws MalformedURLException {
|
||||
run(actionPackages, packageLocators, excludePackages, "");
|
||||
}
|
||||
|
||||
private void run(String actionPackages, String packageLocators, String excludePackages, String enableSmiInheritance) throws MalformedURLException {
|
||||
//setup interceptors
|
||||
List<InterceptorConfig> defaultInterceptors = new ArrayList<>();
|
||||
@@ -198,7 +272,7 @@ public class PackageBasedActionConfigBuilderTest extends TestCase {
|
||||
PackageConfig classLevelParentPkg = makePackageConfig("class-level", null, null, null);
|
||||
|
||||
PackageConfig rootPkg = makePackageConfig("org.apache.struts2.convention.actions#struts-default#",
|
||||
"", strutsDefault, null);
|
||||
"", strutsDefault, null);
|
||||
PackageConfig paramsPkg = makePackageConfig("org.apache.struts2.convention.actions.params#struts-default#/params",
|
||||
"/params", strutsDefault, null);
|
||||
PackageConfig defaultInterceptorPkg = makePackageConfig("org.apache.struts2.convention.actions.defaultinterceptor#struts-default#/defaultinterceptor",
|
||||
@@ -206,17 +280,17 @@ public class PackageBasedActionConfigBuilderTest extends TestCase {
|
||||
PackageConfig exceptionPkg = makePackageConfig("org.apache.struts2.convention.actions.exception#struts-default#/exception",
|
||||
"/exception", strutsDefault, null);
|
||||
PackageConfig actionPkg = makePackageConfig("org.apache.struts2.convention.actions.action#struts-default#/action",
|
||||
"/action", strutsDefault, null);
|
||||
"/action", strutsDefault, null);
|
||||
PackageConfig idxPkg = makePackageConfig("org.apache.struts2.convention.actions.idx#struts-default#/idx",
|
||||
"/idx", strutsDefault, null);
|
||||
"/idx", strutsDefault, null);
|
||||
PackageConfig idx2Pkg = makePackageConfig("org.apache.struts2.convention.actions.idx.idx2#struts-default#/idx/idx2",
|
||||
"/idx/idx2", strutsDefault, null);
|
||||
"/idx/idx2", strutsDefault, null);
|
||||
PackageConfig interceptorRefsPkg = makePackageConfig("org.apache.struts2.convention.actions.interceptor#struts-default#/interceptor",
|
||||
"/interceptor", strutsDefault, null);
|
||||
PackageConfig packageLevelPkg = makePackageConfig("org.apache.struts2.convention.actions.parentpackage#package-level#/parentpackage",
|
||||
"/parentpackage", packageLevelParentPkg, null);
|
||||
"/parentpackage", packageLevelParentPkg, null);
|
||||
PackageConfig packageLevelSubPkg = makePackageConfig("org.apache.struts2.convention.actions.parentpackage.sub#package-level#/parentpackage/sub",
|
||||
"/parentpackage/sub", packageLevelParentPkg, null);
|
||||
"/parentpackage/sub", packageLevelParentPkg, null);
|
||||
|
||||
// Unexpected method call build(class org.apache.struts2.convention.actions.allowedmethods.PackageLevelAllowedMethodsAction, null, "package-level-allowed-methods", PackageConfig: [org.apache.struts2.convention.actions.allowedmethods#struts-default#/allowedmethods] for namespace [/allowedmethods] with parents [[PackageConfig: [struts-default] for namespace [] with parents [[]]]]):
|
||||
PackageConfig packageLevelAllowedMethodsPkg = makePackageConfig("org.apache.struts2.convention.actions.allowedmethods#struts-default#/allowedmethods",
|
||||
@@ -228,17 +302,17 @@ public class PackageBasedActionConfigBuilderTest extends TestCase {
|
||||
"/allowedmethods", strutsDefault, null);
|
||||
|
||||
PackageConfig differentPkg = makePackageConfig("org.apache.struts2.convention.actions.parentpackage#class-level#/parentpackage",
|
||||
"/parentpackage", classLevelParentPkg, null);
|
||||
"/parentpackage", classLevelParentPkg, null);
|
||||
PackageConfig differentSubPkg = makePackageConfig("org.apache.struts2.convention.actions.parentpackage.sub#class-level#/parentpackage/sub",
|
||||
"/parentpackage/sub", classLevelParentPkg, null);
|
||||
"/parentpackage/sub", classLevelParentPkg, null);
|
||||
PackageConfig pkgLevelNamespacePkg = makePackageConfig("org.apache.struts2.convention.actions.namespace#struts-default#/package-level",
|
||||
"/package-level", strutsDefault, null);
|
||||
"/package-level", strutsDefault, null);
|
||||
PackageConfig classLevelNamespacePkg = makePackageConfig("org.apache.struts2.convention.actions.namespace#struts-default#/class-level",
|
||||
"/class-level", strutsDefault, null);
|
||||
"/class-level", strutsDefault, null);
|
||||
PackageConfig actionLevelNamespacePkg = makePackageConfig("org.apache.struts2.convention.actions.namespace#struts-default#/action-level",
|
||||
"/action-level", strutsDefault, null);
|
||||
"/action-level", strutsDefault, null);
|
||||
PackageConfig defaultNamespacePkg = makePackageConfig("org.apache.struts2.convention.actions.namespace2#struts-default#/namespace2",
|
||||
"/namespace2", strutsDefault, null);
|
||||
"/namespace2", strutsDefault, null);
|
||||
PackageConfig namespaces1Pkg = makePackageConfig("org.apache.struts2.convention.actions.namespace3#struts-default#/namespaces1",
|
||||
"/namespaces1", strutsDefault, null);
|
||||
PackageConfig namespaces2Pkg = makePackageConfig("org.apache.struts2.convention.actions.namespace3#struts-default#/namespaces2",
|
||||
@@ -248,19 +322,19 @@ public class PackageBasedActionConfigBuilderTest extends TestCase {
|
||||
PackageConfig namespaces4Pkg = makePackageConfig("org.apache.struts2.convention.actions.namespace4#struts-default#/namespaces4",
|
||||
"/namespaces4", strutsDefault, null);
|
||||
PackageConfig resultPkg = makePackageConfig("org.apache.struts2.convention.actions.result#struts-default#/result",
|
||||
"/result", strutsDefault, null);
|
||||
"/result", strutsDefault, null);
|
||||
PackageConfig globalResultPkg = makePackageConfig("org.apache.struts2.convention.actions.result#class-level#/result",
|
||||
"/result", classLevelParentPkg, null);
|
||||
PackageConfig resultPathPkg = makePackageConfig("org.apache.struts2.convention.actions.resultpath#struts-default#/resultpath",
|
||||
"/resultpath", strutsDefault, null);
|
||||
"/resultpath", strutsDefault, null);
|
||||
PackageConfig skipPkg = makePackageConfig("org.apache.struts2.convention.actions.skip#struts-default#/skip",
|
||||
"/skip", strutsDefault, null);
|
||||
"/skip", strutsDefault, null);
|
||||
PackageConfig chainPkg = makePackageConfig("org.apache.struts2.convention.actions.chain#struts-default#/chain",
|
||||
"/chain", strutsDefault, null);
|
||||
"/chain", strutsDefault, null);
|
||||
PackageConfig transPkg = makePackageConfig("org.apache.struts2.convention.actions.transactions#struts-default#/transactions",
|
||||
"/transactions", strutsDefault, null);
|
||||
"/transactions", strutsDefault, null);
|
||||
PackageConfig excludePkg = makePackageConfig("org.apache.struts2.convention.actions.exclude#struts-default#/exclude",
|
||||
"/exclude", strutsDefault, null);
|
||||
"/exclude", strutsDefault, null);
|
||||
|
||||
ResultMapBuilder resultMapBuilder = createStrictMock(ResultMapBuilder.class);
|
||||
checkOrder(resultMapBuilder, false);
|
||||
@@ -409,7 +483,7 @@ public class PackageBasedActionConfigBuilderTest extends TestCase {
|
||||
mockContainer.setResultMapBuilder(resultMapBuilder);
|
||||
mockContainer.setConventionsService(new ConventionsServiceImpl(""));
|
||||
|
||||
PackageBasedActionConfigBuilder builder = new PackageBasedActionConfigBuilder(configuration, mockContainer , of, "false", "struts-default", enableSmiInheritance);
|
||||
PackageBasedActionConfigBuilder builder = new PackageBasedActionConfigBuilder(configuration, mockContainer, of, "false", "struts-default", enableSmiInheritance);
|
||||
builder.setFileProtocols("jar");
|
||||
if (actionPackages != null) {
|
||||
builder.setActionPackages(actionPackages);
|
||||
@@ -706,7 +780,7 @@ public class PackageBasedActionConfigBuilderTest extends TestCase {
|
||||
verifyActionConfig(pkgConfig, "default-result-path", DefaultResultPathAction.class, "execute", pkgConfig.getName());
|
||||
verifyActionConfig(pkgConfig, "skip", Skip.class, "execute", pkgConfig.getName());
|
||||
verifyActionConfig(pkgConfig, "idx", org.apache.struts2.convention.actions.idx.Index.class, "execute",
|
||||
"org.apache.struts2.convention.actions.idx#struts-default#/idx");
|
||||
"org.apache.struts2.convention.actions.idx#struts-default#/idx");
|
||||
|
||||
/* org.apache.struts2.convention.actions.transactions */
|
||||
pkgConfig = configuration.getPackageConfig("org.apache.struts2.convention.actions.transactions#struts-default#/transactions");
|
||||
@@ -741,7 +815,7 @@ public class PackageBasedActionConfigBuilderTest extends TestCase {
|
||||
}
|
||||
|
||||
private void verifyActionConfig(PackageConfig pkgConfig, String actionName, Class<?> actionClass,
|
||||
String methodName, String packageName) {
|
||||
String methodName, String packageName) {
|
||||
ActionConfig ac = pkgConfig.getAllActionConfigs().get(actionName);
|
||||
assertNotNull(ac);
|
||||
assertEquals(actionClass.getName(), ac.getClassName());
|
||||
@@ -756,7 +830,7 @@ public class PackageBasedActionConfigBuilderTest extends TestCase {
|
||||
}
|
||||
|
||||
private void verifyMissingActionConfig(PackageConfig pkgConfig, String actionName, Class<?> actionClass,
|
||||
String methodName, String packageName) {
|
||||
String methodName, String packageName) {
|
||||
ActionConfig ac = pkgConfig.getAllActionConfigs().get(actionName);
|
||||
assertNull(ac);
|
||||
}
|
||||
@@ -769,10 +843,10 @@ public class PackageBasedActionConfigBuilderTest extends TestCase {
|
||||
assertEquals(packageName, ac.getPackageName());
|
||||
}
|
||||
|
||||
private void checkSmiValue(PackageConfig pkgConfig, PackageConfig parentConfig, boolean isSmiInheritanceEnabled) {
|
||||
private void checkSmiValue(PackageConfig pkgConfig, PackageConfig parentConfig, boolean isSmiInheritanceEnabled) {
|
||||
if (isSmiInheritanceEnabled) {
|
||||
assertEquals(parentConfig.isStrictMethodInvocation(), pkgConfig.isStrictMethodInvocation());
|
||||
} else if (!isSmiInheritanceEnabled && !parentConfig.isStrictMethodInvocation()){
|
||||
} else if (!isSmiInheritanceEnabled && !parentConfig.isStrictMethodInvocation()) {
|
||||
assertTrue(pkgConfig.isStrictMethodInvocation());
|
||||
}
|
||||
}
|
||||
@@ -788,13 +862,13 @@ public class PackageBasedActionConfigBuilderTest extends TestCase {
|
||||
}
|
||||
|
||||
private PackageConfig makePackageConfig(String name, String namespace, PackageConfig parent,
|
||||
String defaultResultType, ResultTypeConfig... results) {
|
||||
String defaultResultType, ResultTypeConfig... results) {
|
||||
return makePackageConfig(name, namespace, parent, defaultResultType, results, null, null, null, true);
|
||||
}
|
||||
|
||||
private PackageConfig makePackageConfig(String name, String namespace, PackageConfig parent,
|
||||
String defaultResultType, ResultTypeConfig[] results, List<InterceptorConfig> interceptors,
|
||||
List<InterceptorStackConfig> interceptorStacks, Set<String> globalAllowedMethods, boolean strictMethodInvocation) {
|
||||
String defaultResultType, ResultTypeConfig[] results, List<InterceptorConfig> interceptors,
|
||||
List<InterceptorStackConfig> interceptorStacks, Set<String> globalAllowedMethods, boolean strictMethodInvocation) {
|
||||
PackageConfig.Builder builder = new PackageConfig.Builder(name);
|
||||
if (namespace != null) {
|
||||
builder.namespace(namespace);
|
||||
@@ -852,7 +926,7 @@ public class PackageBasedActionConfigBuilderTest extends TestCase {
|
||||
public boolean equals(Object obj) {
|
||||
PackageConfig other = (PackageConfig) obj;
|
||||
return getName().equals(other.getName()) && getNamespace().equals(other.getNamespace()) &&
|
||||
getParents().get(0) == other.getParents().get(0) && getParents().size() == other.getParents().size();
|
||||
getParents().get(0) == other.getParents().get(0) && getParents().size() == other.getParents().size();
|
||||
}
|
||||
}
|
||||
|
||||
@@ -893,7 +967,7 @@ public class PackageBasedActionConfigBuilderTest extends TestCase {
|
||||
T obj;
|
||||
if (type == ObjectFactory.class) {
|
||||
obj = type.getConstructor().newInstance();
|
||||
((ObjectFactory)obj).setContainer(this);
|
||||
((ObjectFactory) obj).setContainer(this);
|
||||
|
||||
OgnlReflectionProvider rp = new OgnlReflectionProvider() {
|
||||
|
||||
@@ -929,7 +1003,7 @@ public class PackageBasedActionConfigBuilderTest extends TestCase {
|
||||
}
|
||||
return obj;
|
||||
} catch (Exception e) {
|
||||
throw new RuntimeException(e);
|
||||
throw new RuntimeException(e);
|
||||
}
|
||||
}
|
||||
|
||||
|
||||
@@ -0,0 +1,460 @@
|
||||
---
|
||||
date: 2025-12-02T18:33:08+01:00
|
||||
topic: "WW-5594: Convention plugin exclusion pattern wildcard matching issue"
|
||||
tags: [research, codebase, convention-plugin, wildcard-matching, configuration, bug-analysis, WW-5594]
|
||||
status: complete
|
||||
jira_ticket: WW-5594
|
||||
related_tickets: [WW-5593]
|
||||
---
|
||||
|
||||
# Research: WW-5594 - Convention Plugin Wildcard Exclusion Pattern Issue
|
||||
|
||||
**Date**: 2025-12-02T18:33:08+01:00
|
||||
**JIRA**: [WW-5594](https://issues.apache.org/jira/browse/WW-5594)
|
||||
**Related**: [WW-5593](https://issues.apache.org/jira/browse/WW-5593)
|
||||
|
||||
## Research Question
|
||||
|
||||
Why doesn't the default exclusion pattern `org.apache.struts2.*` properly exclude classes directly in the `org.apache.struts2` package (like `XWorkTestCase`)?
|
||||
|
||||
## Summary
|
||||
|
||||
The convention plugin's `PackageBasedActionConfigBuilder` extracts package names from class names using `StringUtils.substringBeforeLast(className, ".")`, then matches these package names against exclusion patterns. For class `org.apache.struts2.XWorkTestCase`, this produces package name `org.apache.struts2` (no trailing dot). The pattern `org.apache.struts2.*` expects something after the dot for the `*` to match, causing a mismatch for classes directly in the root package.
|
||||
|
||||
This is a conceptual mismatch between patterns designed for full class names vs. matching against extracted package names.
|
||||
|
||||
## Detailed Findings
|
||||
|
||||
### 1. Package Exclusion Logic
|
||||
|
||||
**Location**: `plugins/convention/src/main/java/org/apache/struts2/convention/PackageBasedActionConfigBuilder.java:558-584`
|
||||
|
||||
#### Package Name Extraction (Line 558-560)
|
||||
|
||||
```java
|
||||
protected boolean includeClassNameInActionScan(String className) {
|
||||
String classPackageName = StringUtils.substringBeforeLast(className, ".");
|
||||
return (checkActionPackages(classPackageName) || checkPackageLocators(classPackageName)) && checkExcludePackages(classPackageName);
|
||||
}
|
||||
```
|
||||
|
||||
**Examples**:
|
||||
- `"org.apache.struts2.core.ActionSupport"` → `"org.apache.struts2.core"`
|
||||
- `"com.example.actions.UserAction"` → `"com.example.actions"`
|
||||
- `"org.apache.struts2.XWorkTestCase"` → `"org.apache.struts2"` ⚠️
|
||||
|
||||
#### Exclusion Pattern Matching (Lines 569-584)
|
||||
|
||||
```java
|
||||
protected boolean checkExcludePackages(String classPackageName) {
|
||||
if (excludePackages != null && excludePackages.length > 0) {
|
||||
WildcardHelper wildcardHelper = new WildcardHelper();
|
||||
Map<String, String> matchMap = new HashMap<>();
|
||||
|
||||
for (String packageExclude : excludePackages) {
|
||||
int[] packagePattern = wildcardHelper.compilePattern(packageExclude);
|
||||
if (wildcardHelper.match(matchMap, classPackageName, packagePattern)) {
|
||||
return false;
|
||||
}
|
||||
}
|
||||
}
|
||||
return true;
|
||||
}
|
||||
```
|
||||
|
||||
### 2. The Conceptual Mismatch
|
||||
|
||||
**The Problem**:
|
||||
|
||||
When checking if `org.apache.struts2.XWorkTestCase` should be excluded:
|
||||
|
||||
1. **Step 1**: Extract package name
|
||||
- Input: `"org.apache.struts2.XWorkTestCase"`
|
||||
- `substringBeforeLast(className, ".")` → `"org.apache.struts2"`
|
||||
- No trailing dot
|
||||
|
||||
2. **Step 2**: Match against pattern `org.apache.struts2.*`
|
||||
- Pattern expects: `org.apache.struts2.` + `*` (something)
|
||||
- Actual input: `org.apache.struts2` (nothing after last segment)
|
||||
- **Mismatch**: Pattern requires a literal `.` that doesn't exist in the package name
|
||||
|
||||
**Root Cause**:
|
||||
- Exclusion patterns are written for **fully qualified class names** (e.g., `org.apache.struts2.*` should match `org.apache.struts2.XWorkTestCase`)
|
||||
- But matching is performed against **package names only** (e.g., `org.apache.struts2` after stripping class name)
|
||||
- This creates edge cases for classes directly in a package
|
||||
|
||||
### 3. WildcardHelper Pattern Matching
|
||||
|
||||
**Location**: `core/src/main/java/org/apache/struts2/util/WildcardHelper.java`
|
||||
|
||||
#### Pattern Compilation Constants
|
||||
|
||||
From `WildcardHelper.java:42-48`:
|
||||
```java
|
||||
public static final int MATCH_FILE = -1; // Single * - matches zero or more chars (excluding /)
|
||||
public static final int MATCH_PATH = -2; // Double ** - matches zero or more chars (including /)
|
||||
public static final int MATCH_BEGIN = -4; // Pattern must match from beginning
|
||||
public static final int MATCH_THEEND = -5; // Pattern must match to the end
|
||||
```
|
||||
|
||||
#### How Pattern `org.apache.struts2.*` Compiles
|
||||
|
||||
The pattern compiles to:
|
||||
```
|
||||
[MATCH_BEGIN, 'o', 'r', 'g', '.', 'a', 'p', 'a', 'c', 'h', 'e', '.', 's', 't', 'r', 'u', 't', 's', '2', '.', MATCH_FILE, MATCH_THEEND]
|
||||
```
|
||||
|
||||
**Matching Requirements**:
|
||||
1. Must start with `org.apache.struts2.` (literal characters)
|
||||
2. Must have `MATCH_FILE` (zero or more non-slash characters)
|
||||
3. Must end exactly (`MATCH_THEEND`)
|
||||
|
||||
#### Test Evidence
|
||||
|
||||
From `core/src/test/java/org/apache/struts2/util/WildcardHelperTest.java:54-76`:
|
||||
|
||||
```java
|
||||
public void testMatchStrutsPackages() {
|
||||
// given
|
||||
HashMap<String, String> matchedPatterns = new HashMap<>();
|
||||
int[] pattern = wildcardHelper.compilePattern("org.apache.struts2.*");
|
||||
|
||||
// when & then
|
||||
assertTrue(wildcardHelper.match(matchedPatterns, "org.apache.struts2.XWorkTestCase", pattern));
|
||||
assertEquals("org.apache.struts2.XWorkTestCase", matchedPatterns.get("0"));
|
||||
assertEquals("XWorkTestCase", matchedPatterns.get("1"));
|
||||
|
||||
assertTrue(wildcardHelper.match(matchedPatterns, "org.apache.struts2.core.SomeClass", pattern));
|
||||
assertEquals("org.apache.struts2.core.SomeClass", matchedPatterns.get("0"));
|
||||
assertEquals("core.SomeClass", matchedPatterns.get("1"));
|
||||
|
||||
// IMPORTANT: Pattern matches even with nothing after dot
|
||||
assertTrue(wildcardHelper.match(matchedPatterns, "org.apache.struts2.", pattern));
|
||||
assertEquals("org.apache.struts2.", matchedPatterns.get("0"));
|
||||
assertEquals("", matchedPatterns.get("1"));
|
||||
}
|
||||
```
|
||||
|
||||
**Key Insight**: The pattern `org.apache.struts2.*` **WILL match** `org.apache.struts2.` (with trailing dot) because `MATCH_FILE` can match zero characters. However, it **WON'T match** `org.apache.struts2` (without trailing dot) because the literal `.` character before `MATCH_FILE` is required.
|
||||
|
||||
### 4. Why This Matters
|
||||
|
||||
**Scenario**: Class `org.apache.struts2.XWorkTestCase` should be excluded
|
||||
|
||||
**What Happens**:
|
||||
1. Pattern in config: `org.apache.struts2.*`
|
||||
2. Class name: `org.apache.struts2.XWorkTestCase`
|
||||
3. Extracted package: `org.apache.struts2` (no trailing dot)
|
||||
4. Pattern match: `"org.apache.struts2"` vs pattern `"org.apache.struts2.*"`
|
||||
5. **Result**: NO MATCH (pattern expects literal `.` before the `*`)
|
||||
6. Class is NOT excluded
|
||||
7. Convention plugin tries to scan it
|
||||
8. Triggers WW-5593 (NoClassDefFoundError)
|
||||
|
||||
**Contrast with subpackage classes**:
|
||||
1. Class name: `org.apache.struts2.core.ActionSupport`
|
||||
2. Extracted package: `org.apache.struts2.core`
|
||||
3. Pattern match: `"org.apache.struts2.core"` vs pattern `"org.apache.struts2.*"`
|
||||
4. **Result**: NO MATCH (pattern expects `org.apache.struts2.` + something, but we have `org.apache.struts2.core` which is a different string)
|
||||
|
||||
**Wait, that's also wrong!** Let me reconsider...
|
||||
|
||||
Actually, looking at the test more carefully:
|
||||
|
||||
The test shows that `"org.apache.struts2.*"` matches:
|
||||
- `"org.apache.struts2.XWorkTestCase"` (full class name)
|
||||
- `"org.apache.struts2.core.SomeClass"` (subpackage class name)
|
||||
|
||||
But in the actual code, we're matching:
|
||||
- `"org.apache.struts2"` (extracted package, no class name)
|
||||
|
||||
So the pattern `"org.apache.struts2.*"` is designed to match **class names** like `"org.apache.struts2.XWorkTestCase"`, but it's being used to match **package names** like `"org.apache.struts2"`.
|
||||
|
||||
### 5. Default Configuration
|
||||
|
||||
**Location**: `plugins/convention/src/main/resources/struts-plugin.xml:57`
|
||||
|
||||
```xml
|
||||
<constant name="struts.convention.exclude.packages"
|
||||
value="org.apache.struts.*,org.apache.struts2.*,org.springframework.web.struts.*,org.springframework.web.struts2.*,org.hibernate.*"/>
|
||||
```
|
||||
|
||||
**Packages That Should Be Excluded**:
|
||||
- `org.apache.struts.*` - Struts 1.x packages
|
||||
- `org.apache.struts2.*` - Struts 2.x packages
|
||||
- `org.springframework.web.struts.*` - Spring Struts integration
|
||||
- `org.springframework.web.struts2.*` - Spring Struts 2 integration
|
||||
- `org.hibernate.*` - Hibernate packages
|
||||
|
||||
**Problem**: Classes directly in these root packages (without subpackages) may not be properly excluded:
|
||||
- `org.apache.struts2.XWorkTestCase` ❌ Not excluded
|
||||
- `org.apache.struts2.StrutsConstants` ❌ Not excluded
|
||||
- But: `org.apache.struts2.core.ActionSupport` - needs investigation
|
||||
|
||||
### 6. Email Thread Evidence
|
||||
|
||||
From Florian Schlittgen's debugging (2024-12-19 21:41):
|
||||
|
||||
```java
|
||||
public static void main(String[] args) {
|
||||
String packageExclude = "org.apache.struts2.*";
|
||||
String classPackageName = "org.apache.struts2";
|
||||
WildcardHelper wildcardHelper = new WildcardHelper();
|
||||
int[] packagePattern = wildcardHelper.compilePattern(packageExclude);
|
||||
System.out.println(wildcardHelper.match(new HashMap<>(), classPackageName, packagePattern));
|
||||
}
|
||||
```
|
||||
|
||||
**Output**: `false`
|
||||
|
||||
The pattern `"org.apache.struts2.*"` does NOT match `"org.apache.struts2"` (without trailing dot).
|
||||
|
||||
From Lukasz Lenart's response (2024-12-23 19:12):
|
||||
|
||||
```java
|
||||
// This WORKS (with trailing dot)
|
||||
String classPackageName = "org.apache.struts2.";
|
||||
// Output: true
|
||||
```
|
||||
|
||||
So the pattern requires a trailing dot to match.
|
||||
|
||||
## Code References
|
||||
|
||||
- `plugins/convention/src/main/java/org/apache/struts2/convention/PackageBasedActionConfigBuilder.java:558-584` - Package exclusion logic
|
||||
- `core/src/main/java/org/apache/struts2/util/WildcardHelper.java:71-254` - Wildcard pattern matching implementation
|
||||
- `core/src/test/java/org/apache/struts2/util/WildcardHelperTest.java:54-76` - Pattern matching tests
|
||||
- `plugins/convention/src/main/resources/struts-plugin.xml:57` - Default exclusion configuration
|
||||
|
||||
## Recommended Fixes
|
||||
|
||||
### Option A: Update Default Configuration (Simplest)
|
||||
|
||||
**File**: `plugins/convention/src/main/resources/struts-plugin.xml:57`
|
||||
|
||||
Add both root packages and wildcard patterns to the configuration.
|
||||
|
||||
**Disadvantages**:
|
||||
- ❌ Users with custom exclusion patterns still need to know about this
|
||||
- ❌ Makes the configuration string longer
|
||||
- ❌ Requires pattern duplication
|
||||
|
||||
### Option B: Match Against Full Class Name
|
||||
|
||||
Match exclusion patterns against the full class name instead of just the package name.
|
||||
|
||||
**Disadvantages**:
|
||||
- ❌ Changes method logic significantly
|
||||
- ❌ May affect performance (two wildcard checks instead of one)
|
||||
- ❌ Could have unintended side effects with existing configurations
|
||||
|
||||
### Option C: Enhance WildcardHelper for Package Matching
|
||||
|
||||
Create a new method `matchPackagePattern()` that understands package-style matching.
|
||||
|
||||
**Disadvantages**:
|
||||
- ❌ Most complex solution
|
||||
- ❌ Changes core utility class
|
||||
- ❌ Requires extensive testing
|
||||
- ❌ Overkill for the problem
|
||||
|
||||
### Option D: Enhance checkExcludePackages to Handle Root Packages (IMPLEMENTED ✅)
|
||||
|
||||
**File**: `plugins/convention/src/main/java/org/apache/struts2/convention/PackageBasedActionConfigBuilder.java`
|
||||
|
||||
Modify `checkExcludePackages()` to automatically handle patterns ending with `.*` by also checking if the package name equals the base pattern (without `.*`).
|
||||
|
||||
**Implementation**:
|
||||
```java
|
||||
protected boolean checkExcludePackages(String classPackageName) {
|
||||
if (excludePackages != null && excludePackages.length > 0) {
|
||||
WildcardHelper wildcardHelper = new WildcardHelper();
|
||||
Map<String, String> matchMap = new HashMap<>();
|
||||
|
||||
for (String packageExclude : excludePackages) {
|
||||
// WW-5594: For patterns ending with ".*", also check if package equals the base
|
||||
if (packageExclude.endsWith(".*")) {
|
||||
String basePackage = packageExclude.substring(0, packageExclude.length() - 2);
|
||||
if (classPackageName.equals(basePackage)) {
|
||||
return false;
|
||||
}
|
||||
}
|
||||
|
||||
int[] packagePattern = wildcardHelper.compilePattern(packageExclude);
|
||||
if (wildcardHelper.match(matchMap, classPackageName, packagePattern)) {
|
||||
return false;
|
||||
}
|
||||
}
|
||||
}
|
||||
return true;
|
||||
}
|
||||
```
|
||||
|
||||
**Advantages**:
|
||||
- ✅ No configuration duplication needed
|
||||
- ✅ Backward compatible - existing configurations with `.*` patterns now work correctly
|
||||
- ✅ Single point of fix in `checkExcludePackages()` method
|
||||
- ✅ Intuitive behavior - `org.apache.struts2.*` now excludes both root and subpackages
|
||||
- ✅ Minimal code change
|
||||
- ✅ Users don't need to update their configurations
|
||||
|
||||
**Disadvantages**:
|
||||
- ❌ Slightly more processing per pattern (trivial performance impact)
|
||||
|
||||
### Recommendation
|
||||
|
||||
**Option D** was chosen and implemented because:
|
||||
1. Most elegant solution - no configuration duplication
|
||||
2. Backward compatible - existing configurations work better
|
||||
3. Single point of fix
|
||||
4. Intuitive behavior for users
|
||||
5. Minimal code change with maximum benefit
|
||||
|
||||
## Impact Assessment
|
||||
|
||||
### Current Risk
|
||||
|
||||
**Severity**: MEDIUM
|
||||
|
||||
**Scenarios Affected**:
|
||||
1. Classes directly in root Struts packages (like `XWorkTestCase`)
|
||||
2. Users setting `struts.convention.action.includeJars`
|
||||
3. Custom exclusion patterns not including root packages
|
||||
|
||||
**Current Behavior**:
|
||||
- ❌ Test classes in root packages not excluded
|
||||
- ❌ Can trigger WW-5593 (NoClassDefFoundError)
|
||||
- ❌ Users must manually add root packages to exclusions
|
||||
- ❌ Non-intuitive pattern matching behavior
|
||||
|
||||
### After Fix
|
||||
|
||||
**Expected Behavior**:
|
||||
- ✅ Root package classes properly excluded by default
|
||||
- ✅ Reduces occurrence of WW-5593
|
||||
- ✅ More intuitive default configuration
|
||||
- ✅ Better documentation for custom patterns
|
||||
|
||||
## Testing Strategy
|
||||
|
||||
### Unit Tests to Add
|
||||
|
||||
1. **Test Root Package Exclusion**:
|
||||
```java
|
||||
@Test
|
||||
public void testExcludeRootPackageClasses() {
|
||||
String[] excludePackages = {"org.apache.struts2", "org.apache.struts2.*"};
|
||||
builder.setExcludePackages(excludePackages);
|
||||
|
||||
// Should exclude classes directly in org.apache.struts2
|
||||
assertFalse(builder.includeClassNameInActionScan("org.apache.struts2.XWorkTestCase"));
|
||||
assertFalse(builder.includeClassNameInActionScan("org.apache.struts2.StrutsConstants"));
|
||||
|
||||
// Should also exclude subpackages
|
||||
assertFalse(builder.includeClassNameInActionScan("org.apache.struts2.core.ActionSupport"));
|
||||
assertFalse(builder.includeClassNameInActionScan("org.apache.struts2.dispatcher.Dispatcher"));
|
||||
}
|
||||
```
|
||||
|
||||
2. **Test Wildcard Pattern Behavior**:
|
||||
```java
|
||||
@Test
|
||||
public void testWildcardPatternMatchingWithPackageNames() {
|
||||
WildcardHelper helper = new WildcardHelper();
|
||||
int[] pattern = helper.compilePattern("org.apache.struts2.*");
|
||||
|
||||
// Package name without trailing dot should NOT match
|
||||
assertFalse(helper.match(new HashMap<>(), "org.apache.struts2", pattern));
|
||||
|
||||
// Package name with trailing dot SHOULD match
|
||||
assertTrue(helper.match(new HashMap<>(), "org.apache.struts2.", pattern));
|
||||
|
||||
// Full class name SHOULD match
|
||||
assertTrue(helper.match(new HashMap<>(), "org.apache.struts2.XWorkTestCase", pattern));
|
||||
}
|
||||
```
|
||||
|
||||
3. **Test Default Configuration**:
|
||||
```java
|
||||
@Test
|
||||
public void testDefaultExclusionPatterns() {
|
||||
// Load default patterns from struts-plugin.xml
|
||||
builder.setExcludePackages(getDefaultExclusionPatterns());
|
||||
|
||||
// Should exclude Struts 2 root package classes
|
||||
assertFalse(builder.includeClassNameInActionScan("org.apache.struts2.XWorkTestCase"));
|
||||
|
||||
// Should exclude Struts 1 root package classes
|
||||
assertFalse(builder.includeClassNameInActionScan("org.apache.struts.Action"));
|
||||
|
||||
// Should exclude Hibernate root package classes
|
||||
assertFalse(builder.includeClassNameInActionScan("org.hibernate.Session"));
|
||||
}
|
||||
```
|
||||
|
||||
### Integration Tests
|
||||
|
||||
1. Test scanning with `struts.convention.action.includeJars` configured
|
||||
2. Verify classes in root excluded packages are not scanned
|
||||
3. Verify classes in subpackages are properly excluded
|
||||
4. Verify user action classes are still included
|
||||
|
||||
## Workaround for Users
|
||||
|
||||
Until the fix is released, users should update their exclusion patterns:
|
||||
|
||||
```xml
|
||||
<constant name="struts.convention.exclude.packages"
|
||||
value="org.apache.struts2,org.apache.struts2.*,com.opensymphony.xwork2,com.opensymphony.xwork2.*" />
|
||||
```
|
||||
|
||||
**Pattern**: For each package to exclude, include both:
|
||||
- Root package without wildcard: `org.apache.struts2`
|
||||
- Subpackages with wildcard: `org.apache.struts2.*`
|
||||
|
||||
## Documentation Updates Needed
|
||||
|
||||
1. **Convention Plugin Documentation**:
|
||||
- Explain wildcard pattern matching behavior
|
||||
- Provide examples of correct exclusion patterns
|
||||
- Document the difference between `org.example` and `org.example.*`
|
||||
|
||||
2. **Migration Guide**:
|
||||
- Note the improved default exclusions in release notes
|
||||
- Explain that custom exclusion patterns should include root packages
|
||||
|
||||
3. **Configuration Reference**:
|
||||
- Update `struts.convention.exclude.packages` documentation
|
||||
- Add examples showing both patterns
|
||||
|
||||
## Related Issues
|
||||
|
||||
- **WW-5593**: NoClassDefFoundError bug that this exclusion issue can trigger
|
||||
- Related research: `thoughts/shared/research/2025-12-02-WW-5593-convention-plugin-noclass-exception.md`
|
||||
|
||||
## Email Thread Reference
|
||||
|
||||
**From**: Florian Schlittgen (sc...@liwa.de)
|
||||
**Date**: December 19, 2024 - January 19, 2025
|
||||
**Subject**: Struts 7: action class finder
|
||||
**List**: dev@struts.apache.org
|
||||
|
||||
**Key Insight from Thread** (Florian, 2024-12-23 19:41):
|
||||
> "Thanks for looking into it. Your examples/tests are alright, but I think this is not the way it is being called. Please take a look at org.apache.struts2.convention.PackageBasedActionConfigBuilder.includeClassNameInActionScan(String). While debugging I can see that this method is being called with parameter "className" = "org.apache.struts2.XWorkTestCase". In the method's first line the package name is derived from the className by using "StringUtils.substringBeforeLast(className, ".")" which gives you "org.apache.struts2", no trailing dot. And without trailing dot it is not working."
|
||||
|
||||
## Implementation Status
|
||||
|
||||
1. ✅ Create JIRA ticket WW-5594
|
||||
2. ✅ Modify `checkExcludePackages()` to handle `.*` patterns for root packages (Option D)
|
||||
3. ✅ Default exclusion patterns in `struts-plugin.xml` unchanged (no duplication needed)
|
||||
4. ✅ Add unit tests for root package exclusion (`PackageBasedActionConfigBuilderTest.testWW5594_RootPackageExclusion`)
|
||||
5. ✅ Add unit tests documenting WildcardHelper behavior (`WildcardHelperTest.testWW5594_*`)
|
||||
6. ⏳ Add integration tests with includeJars configured
|
||||
7. ⏳ Update documentation for pattern matching behavior
|
||||
8. ⏳ Update migration guide with new default patterns
|
||||
|
||||
## Files Changed
|
||||
|
||||
- `plugins/convention/src/main/java/org/apache/struts2/convention/PackageBasedActionConfigBuilder.java` - Enhanced `checkExcludePackages()` method
|
||||
- `plugins/convention/src/test/java/org/apache/struts2/convention/PackageBasedActionConfigBuilderTest.java` - Added `testWW5594_RootPackageExclusion()`
|
||||
- `core/src/test/java/org/apache/struts2/util/WildcardHelperTest.java` - Added `testWW5594_WildcardPatternRequiresTrailingDot()` and `testWW5594_ExactPackagePatternMatchesPackageName()`
|
||||
Reference in New Issue
Block a user