[
https://issues.apache.org/jira/browse/WW-5337?focusedWorklogId=877305&page=com.atlassian.jira.plugin.system.issuetabpanels:worklog-tabpanel#worklog-877305
]
ASF GitHub Bot logged work on WW-5337:
--------------------------------------
Author: ASF GitHub Bot
Created on: 21/Aug/23 14:39
Start Date: 21/Aug/23 14:39
Worklog Time Spent: 10m
Work Description: kusalk commented on code in PR #736:
URL: https://github.com/apache/struts/pull/736#discussion_r1300194447
##########
core/src/main/java/com/opensymphony/xwork2/ognl/OgnlUtil.java:
##########
@@ -261,7 +262,8 @@ public void setDevModeExcludedPackageExemptClasses(String
commaDelimitedClasses)
}
private Set<String> parseExcludedPackageNames(String
commaDelimitedPackageNames) {
- Set<String> parsedSet =
TextParseUtil.commaDelimitedStringToSet(commaDelimitedPackageNames);
+ Set<String> parsedSet =
commaDelimitedStringToSet(commaDelimitedPackageNames)
+ .stream().map(s -> strip(s, ".")).collect(toSet());
Review Comment:
I think it's unintuitive to require a period at the of each excluded
package. Currently if the period is missed, the exclusion will act like the
regex `com\.package.*` which likely isn't what users configuring their
application intend.
By stripping the period here, we retain backwards compatibility for excluded
package lists that still have trailing periods.
However, we do drop support for wildcard package exclusions where
applications genuinely desired the previous behaviour when a trailing period
wasn't included. They will need to move such exclusions to the regex exclusion
list instead.
##########
core/src/main/java/com/opensymphony/xwork2/ognl/OgnlUtil.java:
##########
@@ -218,14 +220,13 @@ protected void
setDevModeExcludedPackageNamePatterns(String commaDelimitedPackag
}
private Set<Pattern> parseExcludedPackageNamePatterns(String
commaDelimitedPackagePatterns) {
- Set<String> packagePatterns =
TextParseUtil.commaDelimitedStringToSet(commaDelimitedPackagePatterns);
- Set<Pattern> packageNamePatterns = new HashSet<>();
-
- for (String pattern : packagePatterns) {
- packageNamePatterns.add(Pattern.compile(pattern));
+ try {
+ return commaDelimitedStringToSet(commaDelimitedPackagePatterns)
+ .stream().map(Pattern::compile).collect(toSet());
+ } catch (PatternSyntaxException e) {
+ throw new ConfigurationException(
Review Comment:
Ensure we throw ConfigurationException when RegEx patterns provided are
invalid
##########
core/src/main/java/com/opensymphony/xwork2/ognl/SecurityMemberAccess.java:
##########
@@ -237,33 +242,27 @@ protected String toPackageName(Class<?> clazz) {
protected boolean isExcludedPackageNamePatterns(Class<?> clazz) {
String packageName = toPackageName(clazz);
- for (Pattern pattern : excludedPackageNamePatterns) {
- if (pattern.matcher(packageName).matches()) {
- return true;
- }
- }
- return false;
+ return excludedPackageNamePatterns.stream().anyMatch(pattern ->
pattern.matcher(packageName).matches());
}
protected boolean isExcludedPackageNames(Class<?> clazz) {
- String suffixedPackageName = toPackageName(clazz) + ".";
- for (String excludedPackageName : excludedPackageNames) {
- if (suffixedPackageName.startsWith(excludedPackageName)) {
+ String packageName = toPackageName(clazz);
+ List<String> packageParts = Arrays.asList(packageName.split("\\."));
+ for (int i = 0; i < packageParts.size(); i++) {
+ String parentPackage = String.join(".", packageParts.subList(0, i
+ 1));
+ if (excludedPackageNames.contains(parentPackage)) {
return true;
}
}
return false;
}
protected boolean isClassExcluded(Class<?> clazz) {
- if (clazz == Object.class || (clazz == Class.class &&
!allowStaticFieldAccess)) {
- return true;
- }
- return excludedClasses.stream().anyMatch(clazz::isAssignableFrom);
+ return excludedClasses.contains(clazz);
}
protected boolean isExcludedPackageExempt(Class<?> clazz) {
- return excludedPackageExemptClasses.stream().anyMatch(clazz::equals);
+ return excludedPackageExemptClasses.contains(clazz);
Review Comment:
Constant time operation
##########
core/src/main/java/com/opensymphony/xwork2/ognl/SecurityMemberAccess.java:
##########
@@ -237,33 +242,27 @@ protected String toPackageName(Class<?> clazz) {
protected boolean isExcludedPackageNamePatterns(Class<?> clazz) {
String packageName = toPackageName(clazz);
- for (Pattern pattern : excludedPackageNamePatterns) {
- if (pattern.matcher(packageName).matches()) {
- return true;
- }
- }
- return false;
+ return excludedPackageNamePatterns.stream().anyMatch(pattern ->
pattern.matcher(packageName).matches());
}
protected boolean isExcludedPackageNames(Class<?> clazz) {
- String suffixedPackageName = toPackageName(clazz) + ".";
- for (String excludedPackageName : excludedPackageNames) {
- if (suffixedPackageName.startsWith(excludedPackageName)) {
+ String packageName = toPackageName(clazz);
+ List<String> packageParts = Arrays.asList(packageName.split("\\."));
+ for (int i = 0; i < packageParts.size(); i++) {
Review Comment:
Operations proportional to number of package parts of class being checked.
Previously it was proportional to number of excluded packages in configuration.
Note that we still retain the behaviour of sub-packages also being excluded.
##########
core/src/main/java/com/opensymphony/xwork2/ognl/SecurityMemberAccess.java:
##########
@@ -237,33 +242,27 @@ protected String toPackageName(Class<?> clazz) {
protected boolean isExcludedPackageNamePatterns(Class<?> clazz) {
String packageName = toPackageName(clazz);
- for (Pattern pattern : excludedPackageNamePatterns) {
- if (pattern.matcher(packageName).matches()) {
- return true;
- }
- }
- return false;
+ return excludedPackageNamePatterns.stream().anyMatch(pattern ->
pattern.matcher(packageName).matches());
}
protected boolean isExcludedPackageNames(Class<?> clazz) {
- String suffixedPackageName = toPackageName(clazz) + ".";
- for (String excludedPackageName : excludedPackageNames) {
- if (suffixedPackageName.startsWith(excludedPackageName)) {
+ String packageName = toPackageName(clazz);
+ List<String> packageParts = Arrays.asList(packageName.split("\\."));
+ for (int i = 0; i < packageParts.size(); i++) {
+ String parentPackage = String.join(".", packageParts.subList(0, i
+ 1));
+ if (excludedPackageNames.contains(parentPackage)) {
return true;
}
}
return false;
}
protected boolean isClassExcluded(Class<?> clazz) {
- if (clazz == Object.class || (clazz == Class.class &&
!allowStaticFieldAccess)) {
Review Comment:
Moved this logic to #useExcludedClasses to make this method more performant
##########
core/src/test/java/com/opensymphony/xwork2/ognl/SecurityMemberAccessTest.java:
##########
@@ -178,23 +178,6 @@ public void testMiddleOfInheritanceExclusion3() throws
Exception {
assertTrue("barLogic() from BarInterface isn't accessible!!!",
accessible);
}
- @Test
- public void testMiddleOfInheritanceExclusion4() throws Exception {
Review Comment:
Superinterfaces and superclasses of excluded classes are no longer also
excluded. I'm not too sure of a real world example where this behaviour might
actually be useful/sensible. Or where you wouldn't just add to the exclusion
list the desired superinterfaces/superclasses instead?
If this behaviour is genuinely useful, it would be better if we moved such
interfaces/classes to a separate exclusion list to keep the regular excluded
class list performant.
##########
core/src/main/resources/struts-excluded-classes.xml:
##########
@@ -55,52 +55,52 @@
<!
Issue Time Tracking
-------------------
Worklog Id: (was: 877305)
Time Spent: 40m (was: 0.5h)
> Improve performance of excluded classes and packages
> ----------------------------------------------------
>
> Key: WW-5337
> URL: https://issues.apache.org/jira/browse/WW-5337
> Project: Struts 2
> Issue Type: Improvement
> Components: Core
> Reporter: Kusal Kithul-Godage
> Priority: Minor
> Fix For: 6.3.0
>
> Time Spent: 40m
> Remaining Estimate: 0h
>
> By using a HashMap we can significantly improve performance for applications
> with extensive exclusion lists.
--
This message was sent by Atlassian Jira
(v8.20.10#820010)
