Copilot commented on code in PR #400:
URL: https://github.com/apache/commons-jexl/pull/400#discussion_r3143614119
##########
src/main/java/org/apache/commons/jexl3/introspection/JexlPermissions.java:
##########
@@ -361,27 +428,54 @@ static JexlPermissions parse(final String... src) {
boolean allow(Constructor<?> ctor);
/**
- * Checks whether a field explicitly disallows JEXL introspection.
- * <p>If a field is not allowed, it cannot resolved and accessed in
scripts or expressions.</p>
+ * Checks whether a field explicitly allows JEXL introspection.
+ * <p>If a field is not allowed, it cannot be resolved and accessed in
scripts or expressions.</p>
*
* @param field the field to check
* @return true if JEXL is allowed to introspect, false otherwise
* @since 3.3
*/
boolean allow(Field field);
+ /**
+ * Checks whether a field explicitly allows JEXL introspection.
+ * <p>If a field is not allowed, it cannot be resolved and accessed in
scripts or expressions.</p>
+ * @param clazz the class from which the field is accessed, used to check
that the field is allowed for this class
+ * @param field the field to check
+ * @return true if JEXL is allowed to introspect, false otherwise
+ * @since 3.7
+ */
+ default boolean allow(Class<?> clazz, Field field) {
+ return allow(field);
+ }
+
/**
* Checks whether a method allows JEXL introspection.
- * <p>If a method is not allowed, it cannot resolved and called in scripts
or expressions.</p>
+ * <p>If a method is not allowed, it cannot be resolved and called in
scripts or expressions.</p>
* <p>Since methods can be overridden and overloaded, this also checks
that no superclass or interface
- * explicitly disallows this methods.</p>
+ * explicitly disallows this method.</p>
*
* @param method the method to check
* @return true if JEXL is allowed to introspect, false otherwise
* @since 3.3
*/
boolean allow(Method method);
+ /**
+ * Checks whether a method allows JEXL introspection.
+ * <p>If a method is not allowed, it cannot be resolved and called in
scripts or expressions.</p>
+ * <p>Since methods can be overridden and overloaded, this checks that
this class explicitly allows
+ * this method - superseding any superclass or interface specified
permissions.</p>
+ *
+ * @param clazz the class from which the method is accessed, used to check
that the method is allowed for this class
+ * @param method the method to check
+ * @return true if JEXL is allowed to introspect, false otherwise
+ * @since 3.7
+ */
+ default boolean allow(Class<?> clazz, Method method) {
+ return allow(method);
Review Comment:
The newly added overloads `allow(Class<?>, Field)` and `allow(Class<?>,
Method)` are marked `@since 3.7`, but this change is being released under 3.6.3
(per changes.xml / release notes). The `@since` tags should match the release
version that first ships the API change.
##########
src/test/java/org/apache/commons/jexl3/Issues400Test.java:
##########
@@ -863,21 +869,27 @@ void test450a() {
}
@Test
- void test450b() {
+ void test450b() throws NoSuchMethodException {
// cannot load System with RESTRICTED
assertThrows(JexlException.Method.class, () ->
run450b(JexlPermissions.RESTRICTED),
"should not be able to load System with RESTRICTED");
- // can load System with UNRESTRICTED
+ // can load System.exit with UNRESTRICTED
+ Method exitMethod = java.lang.System.class.getMethod("exit",
int.class);
+ assertTrue(UNRESTRICTED.allow(exitMethod));
+ assertFalse(RESTRICTED.allow(exitMethod));
assertEquals(java.lang.System.class, run450b(UNRESTRICTED));
// need to explicitly allow Uberspect and the current class loader to
load System
+ Class<? extends ClassLoader> cloader =
getClass().getClassLoader().getClass();
JexlPermissions perm = new JexlPermissions.ClassPermissions(
getClass().getClassLoader().getClass(),
org.apache.commons.jexl3.internal.introspection.Uberspect.class);
+ //assertTrue(perm.allow(cloader), "should be able to access
ClassLoader");
Review Comment:
There is a commented-out assertion left in the test (`//assertTrue(...)`).
If this check is still relevant, it should be an active assertion; otherwise it
should be removed to avoid leaving dead code in the test suite.
```suggestion
assertTrue(perm.allow(cloader), "should be able to access
ClassLoader");
```
##########
src/test/java/org/apache/commons/jexl3/scripting/JexlScriptEngineTest.java:
##########
@@ -291,24 +292,28 @@ void testMain1() throws Exception {
@Test
void testMain2() throws Exception {
- final ByteArrayOutputStream outContent = new ByteArrayOutputStream();
final PrintStream originalOut = System.out;
Path file = null;
try {
- System.setOut(new PrintStream(outContent));
file = Files.createTempFile("test-jsr233", ".jexl");
- final BufferedWriter writer = new BufferedWriter(new
FileWriter(file.toFile()));
- writer.write("a=20;\nb=22;\na+b\n");
- writer.close();
- final String ctl = ">>: 42" + LF;
- Main.main(new String[]{file.toString()});
- assertEquals(ctl, outContent.toString());
+ try (BufferedWriter writer = new BufferedWriter(new
FileWriter(file.toFile()));
+ ByteArrayOutputStream outContent = new
ByteArrayOutputStream();
+ PrintStream printStream = new PrintStream(outContent)) {
+
+ writer.write("a=20;\nb=22;\na+b\n");
+ writer.close(); // Explicit close before using the file
Review Comment:
`writer` is managed by try-with-resources, so calling `writer.close()`
manually here is redundant and can make exception handling noisier
(double-close, suppressed exceptions). If the goal is to ensure data is written
before invoking Main.main(), prefer `writer.flush()` or rely on
try-with-resources scoping by moving the Main.main() call after the writer
resource scope ends.
```suggestion
writer.flush(); // Ensure content is written before using
the file
```
--
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]
