Copilot commented on code in PR #402:
URL: https://github.com/apache/commons-jexl/pull/402#discussion_r3311219005
##########
src/main/java/org/apache/commons/jexl3/JexlException.java:
##########
@@ -728,27 +730,66 @@ public static String annotationError(final JexlNode node,
final String annotatio
}
/**
- * Cleans a Throwable from any org.apache.commons.jexl3.internal stack
trace element.
+ * Cleans a Throwable from cluttering stack trace elements.
*
* @param <X> the throwable type
- * @param xthrow the thowable
+ * @param xthrow the throwable
* @return the throwable
*/
static <X extends Throwable> X clean(final X xthrow) {
if (xthrow != null) {
final List<StackTraceElement> stackJexl = new ArrayList<>();
for (final StackTraceElement se : xthrow.getStackTrace()) {
final String className = se.getClassName();
- if (!className.startsWith("org.apache.commons.jexl3.internal")
- &&
!className.startsWith("org.apache.commons.jexl3.parser")) {
- stackJexl.add(se);
+ if (startsWithAny(CLEAN_STOP, className)) {
+ break;
+ }
+ if (startsWithAny(CLEAN_SKIP, className)) {
+ continue;
}
+ stackJexl.add(se);
}
xthrow.setStackTrace(stackJexl.toArray(EMPTY_STACK_TRACE_ELEMENT_ARRAY));
}
return xthrow;
}
+ /**
+ * Checks whether a given name starts with any of the given prefixes.
+ * @param prefixes the prefixes to check
+ * @param name the name to check
+ * @return true if the name starts with any of the prefixes, false
otherwise
+ */
+ private static boolean startsWithAny(final List<String> prefixes, final
String name) {
+ for (final String prefix : prefixes) {
+ if (name.startsWith(prefix)) {
+ return true;
+ }
+ }
+ return false;
+ }
+
+ /**
+ * A static, immutable list of package names that will be skipped during
+ * exception stack-trace cleansing.
+ */
+ private static final List<String> CLEAN_SKIP = Arrays.asList(
+ "org.apache.commons.jexl3.internal",
+ "org.apache.commons.jexl3.parser",
+ "java.lang.reflect",
+ "sun.reflect"
+ );
Review Comment:
The Javadoc says this is an "immutable" list, but `Arrays.asList(...)`
returns a fixed-size list that still allows element replacement via `set(...)`.
Either wrap it with `Collections.unmodifiableList(...)` (or similar) to make it
truly immutable, or adjust the wording to avoid claiming immutability.
##########
src/main/java/org/apache/commons/jexl3/JexlArithmetic.java:
##########
@@ -1005,6 +1007,37 @@ protected Object increment(final Object val, final int
incr) {
throw new ArithmeticException("Object "+(incr < 0?
"decrement":"increment")+":(" + val + ")");
}
+ /**
+ * Evaluates a supplier argument, eventually catching and logging any
JexlException.
+ *
+ * <p>Used primarily by {@link #empty(Object)} and {@link #size(Object)}
to guard argument evaluation.
+ * If evaluation succeeds, returns the supplier's result. If a {@link
JexlException} occurs, logs a
+ * warning and returns the supplier itself.</p>
+ *
+ * <p>This method is public to allow overriding. Implementations that
change the exception handling
+ * behavior must still return the supplier on failure to maintain the
contract.</p>
+ *
+ * @param logger the logger for warning messages; may be null
+ * @param arg the supplier providing the argument to evaluate
+ * @return the evaluated result on success, or the supplier itself on
JexlException
+ * @since 3.6.3
+ */
+ public Object evaluate(Log logger, Supplier<Object> arg) {
+ try {
+ return arg.get();
+ } catch (JexlException e) {
Review Comment:
To match the conventions used throughout `JexlArithmetic` (and to avoid
accidental reassignment), consider marking the method parameters as `final` and
using `catch (final JexlException e)` as is done elsewhere in the codebase.
##########
src/test/java/org/apache/commons/jexl3/Issues400Test.java:
##########
@@ -1183,5 +1183,20 @@ void testIssue36x_456_let() {
assertEquals("42", writer.toString());
}
+
+ @Test
+ void test459() {
+ CaptureLog capture = new CaptureLog("test459");
+ final JexlBuilder builder = new
JexlBuilder().logger(capture).safe(true).strict(false).silent(true);
+ final JexlEngine jexl = builder.create();
+ String expr = "empty('aaa'.substring(400,500))";
+ JexlScript script = jexl.createScript(expr);
+ Object result = script.execute(null);
+ assertEquals(true, result);
+ List<String> errors = capture.getCapturedMessages();
+ assertEquals(1, capture.count("warn"));
+ assertTrue(errors.stream().anyMatch(error ->
error.contains("substring")));
Review Comment:
This test exercises the new warning-on-exception behavior for `empty(...)`,
but the PR also changes `size(...)` handling similarly. Adding a companion
assertion for `size('aaa'.substring(...))` (expecting 0 and a warning) would
ensure both code paths are covered.
--
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]
