henrib commented on code in PR #400:
URL: https://github.com/apache/commons-jexl/pull/400#discussion_r3143602582


##########
src/main/java/org/apache/commons/jexl3/introspection/JexlPermissions.java:
##########
@@ -69,71 +89,121 @@ public interface JexlPermissions {
     /**
      * A permission delegation that augments the RESTRICTED permission with an 
explicit
      * set of classes.
-     * <p>Typical use case is to deny access to a package - and thus all its 
classes - but allow
+     * <p>A typical use case is to deny access to a package - and thus all its 
classes - but allow
      * a few specific classes.</p>
      * <p>Note that the newer positive restriction syntax is preferable as in:
      * <code>RESTRICTED.compose("java.lang { +Class {} }")</code>.</p>
      */
-     final class ClassPermissions extends JexlPermissions.Delegate {
-
-        /** The set of explicitly allowed classes, overriding the delegate 
permissions. */
-        private final Set<String> allowedClasses;
-
-        /**
-         * Creates permissions based on the RESTRICTED set but allowing an 
explicit set.
-         *
-         * @param allow the set of allowed classes
-         */
-        public ClassPermissions(final Class<?>... allow) {
-            this(JexlPermissions.RESTRICTED,
-                    Arrays.stream(Objects.requireNonNull(allow))
-                        .map(Class::getCanonicalName)
-                        .collect(Collectors.toList()));
+    final class ClassPermissions extends JexlPermissions.Delegate {
+      /**
+       * The set of explicitly allowed classes, overriding the delegate 
permissions.
+       */
+      private final Set<String> allowedClasses;
+
+      /**

Review Comment:
   I'm not seeing where it would be beneficial or which constructors you are 
referring to ? Are you suggesting we try and tie the 3 in ClassPermissions and 
compose ? We could add a static method to create an instance of 
ClassPermissions from another class , for instance:
   ```
    default JexlPermissions grantAccess(Class<?>... classes) {
         return new ClassPermissions(this, classes);
       }
   ) 
   ```
   but given the expected usage and the existence of compose() and parse(), 
this seems like a waste of syntactic sugar. :-)



-- 
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]

Reply via email to