From a6e31d2485f743db20459d414669015235b4a99e Mon Sep 17 00:00:00 2001 From: Mark Adamcin Date: Wed, 13 Oct 2021 14:01:17 -0700 Subject: [PATCH] resolves #2716 optimize SCP BVP execution --- CHANGELOG.md | 1 + ...onentPropertiesBindingsValuesProvider.java | 58 ++++++++++++++----- ...tPropertiesBindingsValuesProviderTest.java | 51 +++++++++++++--- 3 files changed, 90 insertions(+), 20 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index c6b8b3aad7..404ae143f3 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -12,6 +12,7 @@ The format is based on [Keep a Changelog](http://keepachangelog.com) ### Fixed - #2704 - Fixed issue with MCP report generation throwing an exception, and fixed some minor UI issues on AEM SDK (added BG color) +- #2716 - Fixed issue with Shared Component Properties Bindings Values Provider facing lock contention ## 5.0.12 - 2021-09-24 diff --git a/bundle/src/main/java/com/adobe/acs/commons/wcm/properties/shared/impl/SharedComponentPropertiesBindingsValuesProvider.java b/bundle/src/main/java/com/adobe/acs/commons/wcm/properties/shared/impl/SharedComponentPropertiesBindingsValuesProvider.java index 3a668a50cc..e92ba43c5c 100644 --- a/bundle/src/main/java/com/adobe/acs/commons/wcm/properties/shared/impl/SharedComponentPropertiesBindingsValuesProvider.java +++ b/bundle/src/main/java/com/adobe/acs/commons/wcm/properties/shared/impl/SharedComponentPropertiesBindingsValuesProvider.java @@ -22,11 +22,12 @@ import com.adobe.acs.commons.wcm.PageRootProvider; import com.adobe.acs.commons.wcm.properties.shared.SharedComponentProperties; import com.day.cq.wcm.api.Page; -import com.day.cq.wcm.api.components.Component; -import com.day.cq.wcm.commons.WCMUtils; +import org.apache.commons.lang3.StringUtils; +import org.apache.felix.scr.annotations.Component; import org.apache.felix.scr.annotations.Reference; import org.apache.felix.scr.annotations.Service; import org.apache.sling.api.resource.Resource; +import org.apache.sling.api.resource.ResourceUtil; import org.apache.sling.api.resource.ValueMap; import org.apache.sling.api.wrappers.ValueMapDecorator; import org.apache.sling.scripting.api.BindingsValuesProvider; @@ -52,7 +53,7 @@ * to instance-level values, then shared values, and finally global * values when properties exist at multiple levels with the same name. */ -@org.apache.felix.scr.annotations.Component +@Component @Service public class SharedComponentPropertiesBindingsValuesProvider implements BindingsValuesProvider { private static final Logger log = LoggerFactory.getLogger(SharedComponentPropertiesBindingsValuesProvider.class); @@ -66,10 +67,9 @@ public class SharedComponentPropertiesBindingsValuesProvider implements Bindings @Override public void addBindings(Bindings bindings) { Resource resource = (Resource) bindings.get("resource"); - Component component = WCMUtils.getComponent(resource); - if (component != null) { + if (resource != null) { if (pageRootProvider != null) { - setSharedProperties(bindings, resource, component); + setSharedProperties(bindings, resource); } else { log.debug("Page Root Provider must be configured for shared component properties to be supported"); } @@ -77,7 +77,35 @@ public void addBindings(Bindings bindings) { } } - private void setSharedProperties(Bindings bindings, Resource resource, Component component) { + /** + * Construct a canonical resource type relative path for the provided resource type, + * or null if the result is not acceptable. + * Step 1: discard empty and JCR node types / sling:nonexisting (contains ":") + * Step 2: return result if already relative (does not start with /) + * Step 3: relativize an absolute path using elements of {@code searchPaths} and return the first match found. + * + * @param resourceType the request resource resourceType + * @param searchPaths {@link org.apache.sling.api.resource.ResourceResolver#getSearchPath()} + * @return the canonical resource type or null + */ + static String getCanonicalResourceTypeRelativePath(final String resourceType, final String[] searchPaths) { + if (StringUtils.isEmpty(resourceType) || resourceType.contains(":")) { + return null; + } + + if (resourceType.charAt(0) != '/') { + return resourceType; + } else if (searchPaths != null) { + for (final String searchPath : searchPaths) { + if (resourceType.startsWith(searchPath)) { + return resourceType.substring(searchPath.length()); + } + } + } + return null; + } + + private void setSharedProperties(Bindings bindings, Resource resource) { Page pageRoot = pageRootProvider.getRootPage(resource); if (pageRoot != null) { String globalPropsPath = pageRoot.getPath() + "/jcr:content/" + SharedComponentProperties.NN_GLOBAL_COMPONENT_PROPERTIES; @@ -87,12 +115,16 @@ private void setSharedProperties(Bindings bindings, Resource resource, Component bindings.put(SharedComponentProperties.GLOBAL_PROPERTIES_RESOURCE, globalPropsResource); } - String sharedPropsPath = pageRoot.getPath() + "/jcr:content/" + SharedComponentProperties.NN_SHARED_COMPONENT_PROPERTIES + "/" - + component.getResourceType(); - Resource sharedPropsResource = resource.getResourceResolver().getResource(sharedPropsPath); - if (sharedPropsResource != null) { - bindings.put(SharedComponentProperties.SHARED_PROPERTIES, sharedPropsResource.getValueMap()); - bindings.put(SharedComponentProperties.SHARED_PROPERTIES_RESOURCE, sharedPropsResource); + final String resourceTypeRelativePath = getCanonicalResourceTypeRelativePath(resource.getResourceType(), + resource.getResourceResolver().getSearchPath()); + if (resourceTypeRelativePath != null) { + String sharedPropsPath = pageRoot.getPath() + "/jcr:content/" + SharedComponentProperties.NN_SHARED_COMPONENT_PROPERTIES + "/" + + resourceTypeRelativePath; + Resource sharedPropsResource = resource.getResourceResolver().getResource(sharedPropsPath); + if (sharedPropsResource != null) { + bindings.put(SharedComponentProperties.SHARED_PROPERTIES, sharedPropsResource.getValueMap()); + bindings.put(SharedComponentProperties.SHARED_PROPERTIES_RESOURCE, sharedPropsResource); + } } } else { log.debug("Could not determine shared properties root for resource {}", resource.getPath()); diff --git a/bundle/src/test/java/com/adobe/acs/commons/wcm/properties/shared/impl/SharedComponentPropertiesBindingsValuesProviderTest.java b/bundle/src/test/java/com/adobe/acs/commons/wcm/properties/shared/impl/SharedComponentPropertiesBindingsValuesProviderTest.java index 4b8bab94de..743c485e74 100644 --- a/bundle/src/test/java/com/adobe/acs/commons/wcm/properties/shared/impl/SharedComponentPropertiesBindingsValuesProviderTest.java +++ b/bundle/src/test/java/com/adobe/acs/commons/wcm/properties/shared/impl/SharedComponentPropertiesBindingsValuesProviderTest.java @@ -20,6 +20,7 @@ package com.adobe.acs.commons.wcm.properties.shared.impl; import static org.junit.Assert.assertEquals; +import static org.junit.Assert.assertNull; import static org.mockito.Mockito.mock; import static org.mockito.Mockito.when; @@ -38,7 +39,13 @@ import org.mockito.junit.MockitoJUnitRunner; import org.powermock.reflect.Whitebox; +import java.util.Arrays; +import java.util.Collections; import java.util.HashMap; +import java.util.List; +import java.util.Optional; +import java.util.function.BiFunction; +import java.util.function.Function; import javax.script.Bindings; import javax.script.SimpleBindings; @@ -54,9 +61,7 @@ public class SharedComponentPropertiesBindingsValuesProviderTest { private Resource globalPropsResource; private Page page; private Bindings bindings; - private Component component; private ResourceResolver resourceResolver; - private ComponentManager componentManager; private ValueMap sharedProps; private ValueMap globalProps; @@ -66,11 +71,9 @@ public void setUp() throws Exception { pageRootProvider = mock(PageRootProvider.class); page = mock(Page.class); bindings = new SimpleBindings(); - component = mock(Component.class); sharedPropsResource = mock(Resource.class); globalPropsResource = mock(Resource.class); resourceResolver = mock(ResourceResolver.class); - componentManager = mock(ComponentManager.class); final String globalPropsPath = SITE_ROOT + "/jcr:content/" + SharedComponentProperties.NN_GLOBAL_COMPONENT_PROPERTIES; final String sharedPropsPath = SITE_ROOT + "/jcr:content/" + SharedComponentProperties.NN_SHARED_COMPONENT_PROPERTIES + "/" @@ -79,14 +82,13 @@ public void setUp() throws Exception { bindings.put("resource", resource); when(resource.getResourceResolver()).thenReturn(resourceResolver); + when(resource.getResourceType()).thenReturn(RESOURCE_TYPE); + when(resourceResolver.getSearchPath()).thenReturn(new String[]{"/apps/", "/libs/"}); when(resourceResolver.getResource(sharedPropsPath)).thenReturn(sharedPropsResource); when(resourceResolver.getResource(globalPropsPath)).thenReturn(globalPropsResource); - when(resourceResolver.adaptTo(ComponentManager.class)).thenReturn(componentManager); - when(componentManager.getComponentOfResource(resource)).thenReturn(component); when(page.getPath()).thenReturn(SITE_ROOT); when(pageRootProvider.getRootPage(resource)).thenReturn(page); - when(component.getResourceType()).thenReturn(RESOURCE_TYPE); sharedProps = new ValueMapDecorator(new HashMap()); globalProps = new ValueMapDecorator(new HashMap()); @@ -97,6 +99,41 @@ public void setUp() throws Exception { when(sharedPropsResource.getValueMap()).thenReturn(sharedProps); } + @Test + public void testGetCanonicalResourceTypeRelativePath() { + // make this test readable by wrapping the long method name with a function + final BiFunction, String> asFunction = + (resourceType, searchPaths) -> SharedComponentPropertiesBindingsValuesProvider + .getCanonicalResourceTypeRelativePath(resourceType, + Optional.ofNullable(searchPaths) + .map(list -> list.toArray(new String[0])).orElse(null)); + + final List emptySearchPaths = Collections.emptyList(); + final List realSearchPaths = Arrays.asList("/apps/", "/libs/"); + assertNull("expect null for null rt", asFunction.apply(null, emptySearchPaths)); + assertNull("expect null for empty rt", asFunction.apply("", emptySearchPaths)); + assertNull("expect null for absolute rt and null search paths", + asFunction.apply("/fail/" + RESOURCE_TYPE, null)); + assertNull("expect null for cq:Page", + asFunction.apply("cq:Page", realSearchPaths)); + assertNull("expect null for nt:unstructured", + asFunction.apply("nt:unstructured", realSearchPaths)); + assertNull("expect null for absolute rt and empty search paths", + asFunction.apply("/fail/" + RESOURCE_TYPE, emptySearchPaths)); + assertNull("expect null for sling nonexisting rt", + asFunction.apply(Resource.RESOURCE_TYPE_NON_EXISTING, emptySearchPaths)); + assertEquals("expect same for relative rt", RESOURCE_TYPE, + asFunction.apply(RESOURCE_TYPE, emptySearchPaths)); + assertEquals("expect same for relative rt and real search paths", RESOURCE_TYPE, + asFunction.apply(RESOURCE_TYPE, realSearchPaths)); + assertEquals("expect relative for /apps/ + relative and real search paths", RESOURCE_TYPE, + asFunction.apply("/apps/" + RESOURCE_TYPE, realSearchPaths)); + assertEquals("expect relative for /libs/ + relative and real search paths", RESOURCE_TYPE, + asFunction.apply("/libs/" + RESOURCE_TYPE, realSearchPaths)); + assertNull("expect null for /fail/ + relative and real search paths", + asFunction.apply("/fail/" + RESOURCE_TYPE, realSearchPaths)); + } + @Test public void addBindings() { final SharedComponentPropertiesBindingsValuesProvider sharedComponentPropertiesBindingsValuesProvider