re #2960 SCP BVP replace reflection with proper LazyBindings types

Author: adamcin

bundle/pom.xml

@@ -41,37 +41,6 @@
                     </archive>
                 </configuration>
             </plugin>
-            <plugin>
-                <groupId>org.apache.maven.plugins</groupId>
-                <artifactId>maven-dependency-plugin</artifactId>
-                <executions>
-                    <execution>
-                        <!--
-                        this execution downloads org.apache.sling.api-2.22.0.jar for
-                        *TESTING* pre-6.5.7 LazyBindings support in our BindingsValuesProvider services,
-                        and specifically, SharedComponentPropertiesBindingsValuesProvider.
-                        -->
-                        <id>download-sling-api-2-22-0-jar</id>
-                        <phase>process-test-resources</phase>
-                        <goals>
-                            <goal>copy</goal>
-                        </goals>
-                        <configuration>
-                            <artifactItems>
-                                <item>
-                                    <groupId>org.apache.sling</groupId>
-                                    <artifactId>org.apache.sling.api</artifactId>
-                                    <version>2.22.0</version>
-                                    <type>jar</type>
-                                </item>
-                            </artifactItems>
-                            <!-- drop it into the target/test-classes directory -->
-                            <!-- required by SharedComponentPropertiesBindingsValuesProviderTest -->
-                            <outputDirectory>${project.build.testOutputDirectory}</outputDirectory>
-                        </configuration>
-                    </execution>
-                </executions>
-            </plugin>
             <plugin>
                 <groupId>biz.aQute.bnd</groupId>
                 <artifactId>bnd-baseline-maven-plugin</artifactId>
@@ -356,6 +325,8 @@
         <dependency>
             <groupId>org.osgi</groupId>
             <artifactId>org.osgi.dto</artifactId>
+            <!-- defer to io.wcm.maven:io.wcm.maven.aem-dependencies managed version if possible -->
+            <version>1.1.0</version>
             <scope>provided</scope>
         </dependency>
         <dependency>
@@ -383,11 +354,6 @@
             <artifactId>slf4j-api</artifactId>
             <scope>provided</scope>
         </dependency>
-        <dependency>
-            <groupId>javax.mail</groupId>
-            <artifactId>mail</artifactId>
-            <scope>provided</scope>
-        </dependency>
         <!-- for com.adobe.acs.commons.logging.impl.SyslogAppender  -->
         <dependency>
             <groupId>ch.qos.logback</groupId>
@@ -423,6 +389,8 @@
         <dependency>
             <groupId>com.github.jknack</groupId>
             <artifactId>handlebars</artifactId>
+            <!-- defer to io.wcm.maven:io.wcm.maven.aem-dependencies managed version if possible -->
+            <version>4.0.5</version>
             <scope>provided</scope>
         </dependency>
         <!-- END runtime dependencies not contained in uber-jar -->
@@ -745,7 +713,7 @@
         <dependency>
             <groupId>com.adobe.aem</groupId>
             <artifactId>uber-jar</artifactId>
-            <classifier>apis</classifier>
+            <classifier>apis-with-deprecations</classifier>
             <scope>provided</scope>
         </dependency>
     </dependencies>

bundle/src/main/java/com/adobe/acs/commons/wcm/properties/shared/impl/SharedComponentPropertiesBindingsValuesProvider.java

@@ -20,7 +20,6 @@
 package com.adobe.acs.commons.wcm.properties.shared.impl;
 
 import com.adobe.acs.commons.wcm.properties.shared.SharedComponentProperties;
-import org.apache.felix.scr.annotations.Activate;
 import org.apache.felix.scr.annotations.Component;
 import org.apache.felix.scr.annotations.Reference;
 import org.apache.felix.scr.annotations.ReferenceCardinality;
@@ -29,18 +28,15 @@
 import org.apache.sling.api.SlingHttpServletRequest;
 import org.apache.sling.api.resource.Resource;
 import org.apache.sling.api.resource.ValueMap;
+import org.apache.sling.api.scripting.LazyBindings;
 import org.apache.sling.api.scripting.SlingBindings;
 import org.apache.sling.scripting.api.BindingsValuesProvider;
 import org.slf4j.Logger;
 import org.slf4j.LoggerFactory;
 
 import javax.script.Bindings;
-import java.lang.reflect.InvocationHandler;
-import java.lang.reflect.Method;
-import java.lang.reflect.Proxy;
 import java.util.Optional;
 import java.util.function.Supplier;
-import java.util.stream.Stream;
 
 /**
  * Bindings Values Provider that adds bindings for globalProperties,
@@ -62,170 +58,32 @@
 public class SharedComponentPropertiesBindingsValuesProvider implements BindingsValuesProvider {
     private static final Logger log = LoggerFactory.getLogger(SharedComponentPropertiesBindingsValuesProvider.class);
 
-    /**
-     * The LazyBindings class, and its Supplier child interface, are introduced in org.apache.sling.api version 2.22.0,
-     * which is first included in AEM 6.5 SP7.
-     */
-    protected static final String FQDN_LAZY_BINDINGS = "org.apache.sling.api.scripting.LazyBindings";
-    protected static final String SUPPLIER_PROXY_LABEL = "ACS AEM Commons SCP BVP reflective Proxy for LazyBindings.Supplier";
-
     /**
      * Bind if available, check for null when reading.
      */
     @Reference(policyOption = ReferencePolicyOption.GREEDY, cardinality = ReferenceCardinality.OPTIONAL_UNARY)
     SharedComponentProperties sharedComponentProperties;
 
-    /**
-     * Added for pre-6.5.7 support for LazyBindings. This holds the LazyBindings interface
-     * if it is discovered on activation, and is used to check if the {@link #addBindings(Bindings)} param
-     * is an instance of LazyBindings. This hack is necessary until this bundle can drop support for
-     * AEM versions prior to 6.5.7, at which point this variable can be removed, and the {@link #isLazy(Bindings)}
-     * method can be simplified to return {@code bindings instanceof LazyBindings}.
-     */
-    private Class<? extends Bindings> lazyBindingsType;
-
-    /**
-     * Added for pre-6.5.7 support for LazyBindings. This holds the LazyBindings.Supplier interface
-     * if it is discovered on activation, and is used to create reflection Proxy instances as a hack
-     * until this bundle can drop support for AEM versions prior to 6.5.7, at which point this variable
-     * can be removed, and the {@link #wrapSupplier(Supplier)} method can be simplified to accept a
-     * LazyBindings.Supplier instead of a java.util.function.Supplier and return it (for matching a
-     * lambda expression passed at the call site), or to simply return a lambda that calls the get()
-     * method on the java.util.function.Supplier argument.
-     */
-    private Class<? extends Supplier> supplierType;
-
-    /**
-     * This variable only exists to facilitate testing for pre-6.5.7 LazyBindings support, so that a non-classpath
-     * class loader can be injected, to provide the LazyBindings class.
-     */
-    private ClassLoader lazyBindingsClassLoader = SlingBindings.class.getClassLoader();
-
-    /**
-     * Called by the unit test to inject a URL class loader that provides a LazyBindings instance
-     * at {@link #FQDN_LAZY_BINDINGS}.
-     *
-     * @param classLoader a new class loader
-     * @return the old class loader
-     */
-    protected ClassLoader swapLazyBindingsClassLoaderForTesting(ClassLoader classLoader) {
-        if (classLoader != null) {
-            ClassLoader oldClassLoader = this.lazyBindingsClassLoader;
-            this.lazyBindingsClassLoader = classLoader;
-            return oldClassLoader;
-        }
-        return null;
-    }
-
-    /**
-     * Return the resolved lazyBindingsType for testing.
-     *
-     * @return the lazyBindingsType
-     */
-    protected Class<? extends Bindings> getLazyBindingsType() {
-        return this.lazyBindingsType;
-    }
-
-    /**
-     * Return the resolved supplierType for testing.
-     *
-     * @return the supplierType
-     */
-    protected Class<? extends Supplier> getSupplierType() {
-        return this.supplierType;
-    }
-
     /**
      * This method ensures that the provided supplier is appropriately typed for insertion into a SlingBindings
      * object. It primarily facilitates lambda type inference (i.e., {@code wrapSupplier(() -> something)} forces
-     * inference to the functional interface type of the method parameter). And so long as pre-6.5.7 AEMs are supported,
-     * this method is also responsible for constructing the {@link Proxy} instance when LazyBindings is present at
-     * runtime, and for immediately returning {@code Supplier.get()} when it is not present.
-     * After support for pre-6.5.7 AEMs is dropped, the method return type can be changed from {@code Object} to
-     * {@code <T> LazyBindings.Supplier<T>} to fully support lazy injection.
+     * inference to the functional interface type of the method parameter).
      *
      * @param supplier the provided supplier
-     * @return the Supplier as a LazyBindings.Supplier if supported, or the value of the provided supplier if not
-     */
-    protected Object wrapSupplier(final Supplier<?> supplier) {
-        if (this.supplierType != null) {
-            return Proxy.newProxyInstance(lazyBindingsClassLoader, new Class[]{this.supplierType},
-                    new SupplierWrapper(supplier));
-        }
-        return supplier.get();
-    }
-
-    /**
-     * The only purpose of this class is to drive the pre-6.5.7 reflection-based Proxy instance returned
-     * by {@link #wrapSupplier(Supplier)}.
-     */
-    protected static class SupplierWrapper implements InvocationHandler {
-        private final Supplier<?> wrapped;
-
-        public SupplierWrapper(final Supplier<?> supplier) {
-            this.wrapped = supplier;
-        }
-
-        @Override
-        public Object invoke(Object proxy, Method method, Object[] args) throws Throwable {
-            // we are implementing a @FunctionalInterface, so don't get carried away with implementing
-            // Object methods.
-            if ("get".equals(method.getName())) {
-                return wrapped.get();
-            } else if ("toString".equals(method.getName())) {
-                // return this marker string for visibility in debugging tools. Otherwise,
-                // the default toString is "\"null\"", which is confusing
-                return SUPPLIER_PROXY_LABEL;
-            }
-            return method.getDefaultValue();
-        }
-    }
-
-    /**
-     * The purpose of this activate method is to determine if we are running in a 6.5.7+ AEM environment
-     * without having to explicitly require {@code org.apache.sling.api.scripting} package version 2.5.0.
-     */
-    @Activate
-    protected void activate() {
-        // use SlingBindings class loader to check for LazyBindings class,
-        // to minimize risk involved with using reflection.
-        try {
-            this.checkAndSetLazyBindingsType(lazyBindingsClassLoader.loadClass(FQDN_LAZY_BINDINGS));
-        } catch (ReflectiveOperationException cnfe) {
-            log.info("LazyBindings not found, will resort to injecting immediate Bindings values", cnfe);
-        }
-    }
-
-    /**
-     * Check that the provided {@code lazyBindingsType} implements {@link Bindings} and defines an enclosed marker
-     * interface named {@code Supplier} that extends {@link Supplier}, and if so, set {@code this.lazyBindingsType} and
-     * {@code this.supplierType}. Otherwise, set both to {@code null}.
+     * @return the Supplier as a LazyBindings.Supplier
      */
-    @SuppressWarnings({"squid:S1872", "unchecked"})
-    protected void checkAndSetLazyBindingsType(final Class<?> lazyBindingsType) {
-        if (lazyBindingsType != null && Bindings.class.isAssignableFrom(lazyBindingsType)) {
-            this.supplierType = (Class<? extends Supplier>) Stream.of(lazyBindingsType.getDeclaredClasses())
-                    .filter(clazz -> Supplier.class.getSimpleName().equals(clazz.getSimpleName())
-                            && Supplier.class.isAssignableFrom(clazz)).findFirst().orElse(null);
-            this.lazyBindingsType = (Class<? extends Bindings>) lazyBindingsType;
-        } else {
-            log.info("Supplier interface not declared by lazyBindingsType: {}, will resort to immediate Bindings values",
-                    lazyBindingsType);
-            this.supplierType = null;
-            this.lazyBindingsType = null;
-        }
+    protected LazyBindings.Supplier wrapSupplier(final Supplier<?> supplier) {
+        return () -> supplier != null ? supplier.get() : null;
     }
 
     /**
-     * Check if provided {@code bindings} implements LazyBindings.
+     * Check if provided {@code bindings} is an instance of {@link LazyBindings}.
      *
      * @param bindings the parameter from {@link #addBindings(Bindings)}
      * @return true if bindings implements LazyBindings
      */
     private boolean isLazy(Bindings bindings) {
-        return Optional.ofNullable(this.lazyBindingsType)
-                .map(clazz -> clazz.isInstance(bindings))
-                .orElse(false);
+        return bindings instanceof LazyBindings;
     }
 
     /**

bundle/src/test/java/com/adobe/acs/commons/images/transformers/impl/AdjustImageTransformerImplTest.java

@@ -20,7 +20,6 @@
 
 package com.adobe.acs.commons.images.transformers.impl;
 
-import com.adobe.acs.commons.images.transformers.impl.AdjustImageTransformerImpl;
 import com.day.image.Layer;
 
 import org.apache.sling.api.resource.ValueMap;
@@ -49,25 +48,22 @@ public class AdjustImageTransformerImplTest {
     @Mock
     Layer layer;
 
-    Map<String, Object> map = null;
 
     @Before
     public void setUp() throws Exception {
-        map = new HashMap<String, Object>();
         transformer = new AdjustImageTransformerImpl();
     }
 
     @After
     public void tearDown() throws Exception {
         reset(layer);
-        map = null;
     }
 
     @Test
     public void testTransform() throws Exception {
         final Integer brightness = 100;
         final Float contrast = 0.05F;
-
+        Map<String, Object> map = new HashMap<>();
         map.put("brightness", brightness.toString());
         map.put("contrast", contrast.toString());
         ValueMap properties = new ValueMapDecorator(map);
@@ -80,6 +76,7 @@ public void testTransform() throws Exception {
 
     @Test
     public void testTransform_noParams() throws Exception {
+        Map<String, Object> map = new HashMap<>();
         ValueMap properties = new ValueMapDecorator(map);
 
         transformer.transform(layer, properties);
@@ -90,7 +87,7 @@ public void testTransform_noParams() throws Exception {
     @Test
     public void testTransform_onlyBrightness() throws Exception {
         final Integer brightness = 100;
-
+        Map<String, Object> map = new HashMap<>();
         map.put("brightness", brightness.toString());
         ValueMap properties = new ValueMapDecorator(map);
 
@@ -103,7 +100,7 @@ public void testTransform_onlyBrightness() throws Exception {
     @Test
     public void testTransform_onlyContrast() throws Exception {
         final Float contrast = 0.05F;
-
+        Map<String, Object> map = new HashMap<>();
         map.put("contrast", contrast.toString());
         ValueMap properties = new ValueMapDecorator(map);