Copilot commented on code in PR #1830:
URL: https://github.com/apache/struts/pull/1830#discussion_r3703858249
##########
core/src/main/java/org/apache/struts2/ognl/SecurityMemberAccess.java:
##########
@@ -387,10 +390,47 @@ protected boolean isExcludedPackageNames(Class<?> clazz) {
}
public static boolean isClassBelongsToPackages(Class<?> clazz, Set<String>
matchingPackages) {
- List<String> packageParts = List.of(toPackageName(clazz).split("\\."));
- return IntStream.range(0, packageParts.size())
- .mapToObj(i -> String.join(".", packageParts.subList(0, i +
1)))
- .anyMatch(matchingPackages::contains);
+ return isClassBelongsToPackages(clazz, matchingPackages, emptySet());
+ }
+
+ /**
+ * Tests the class's package against two sets in a single walk. Equivalent
to calling
+ * {@link #isClassBelongsToPackages(Class, Set)} once per set and OR-ing
the results, but
+ * walks the package name only once.
+ *
+ * @param clazz the class whose package is tested
+ * @param first the first set of package names to match against
+ * @param second the second set of package names to match against
+ * @return {@code true} if the class's package or any parent package is in
either set
+ */
+ static boolean isClassBelongsToPackages(Class<?> clazz, Set<String> first,
Set<String> second) {
+ return isPackageBelongsToPackages(toPackageName(clazz), first, second);
+ }
+
+ /**
+ * Tests whether the given package name, or any of its parent packages, is
present in either
+ * set. Walks the name in place rather than building the full prefix list,
since this runs on
+ * the OGNL member-access path. Shortest prefix first, so broad entries
such as {@code java.io}
+ * short-circuit earliest.
+ *
+ * @param packageName the package name to test, empty for the default
package
+ * @param first the first set of package names to match against
+ * @param second the second set of package names to match against
+ * @return {@code true} if the package or any parent package is in either
set
+ */
+ static boolean isPackageBelongsToPackages(String packageName, Set<String>
first, Set<String> second) {
+ if (first.isEmpty() && second.isEmpty()) {
+ return false;
+ }
+ int idx = packageName.indexOf('.');
+ while (idx != -1) {
+ String prefix = packageName.substring(0, idx);
+ if (first.contains(prefix) || second.contains(prefix)) {
+ return true;
+ }
+ idx = packageName.indexOf('.', idx + 1);
+ }
+ return first.contains(packageName) || second.contains(packageName);
}
Review Comment:
Despite the PR title/wording calling this \"allocation-free\", the new
implementation still allocates a new String for each prefix via substring (Java
17 substring creates a new String). If the intent is truly allocation-free
matching, this needs a different approach (e.g., a specialized data structure
keyed by the original String/char[] with prefix hashing) or the PR/docs/title
should be updated to describe it as \"allocation-reduced\" rather than
\"allocation-free\".
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
