This is an automated email from the ASF dual-hosted git repository. jsedding pushed a commit to branch master in repository https://gitbox.apache.org/repos/asf/sling-org-apache-sling-resourceresolver.git
The following commit(s) were added to refs/heads/master by this push: new 9d02522 SLING-12019 - Avoid duplicate ResourceResolverFactory registrations (#100) 9d02522 is described below commit 9d02522963d0709ba31063337ea0d684ebdc84d5 Author: Julian Sedding <jsedd...@apache.org> AuthorDate: Wed Sep 13 15:56:34 2023 +0200 SLING-12019 - Avoid duplicate ResourceResolverFactory registrations (#100) - extract factory registration/deregistration code into FactoryRegistrationHandler - add unit test for FactoryRegistrationHandler - fix unclosed RR and other exceptions in other tests --- .../impl/FactoryPreconditions.java | 194 +++++++----- .../impl/FactoryRegistrationHandler.java | 176 +++++++++++ .../impl/ResourceResolverFactoryActivator.java | 219 ++------------ .../stateful/AuthenticatedResourceProvider.java | 9 +- .../impl/EtcMappingResourceResolverTest.java | 6 + .../impl/FactoryPreconditionsTest.java | 30 +- .../impl/FactoryRegistrationHandlerTest.java | 181 +++++++++++ .../impl/MockedResourceResolverImplTest.java | 331 +++++++++++---------- .../mapping/AbstractMappingMapEntriesTest.java | 6 +- .../impl/mapping/InMemoryResourceProvider.java | 2 +- .../impl/mapping/ResourceMapperImplTest.java | 14 +- 11 files changed, 705 insertions(+), 463 deletions(-) diff --git a/src/main/java/org/apache/sling/resourceresolver/impl/FactoryPreconditions.java b/src/main/java/org/apache/sling/resourceresolver/impl/FactoryPreconditions.java index 4473142..702600a 100644 --- a/src/main/java/org/apache/sling/resourceresolver/impl/FactoryPreconditions.java +++ b/src/main/java/org/apache/sling/resourceresolver/impl/FactoryPreconditions.java @@ -19,6 +19,8 @@ package org.apache.sling.resourceresolver.impl; import java.util.ArrayList; +import java.util.Collections; +import java.util.HashSet; import java.util.List; import java.util.Set; @@ -26,9 +28,10 @@ import org.apache.sling.resourceresolver.impl.legacy.LegacyResourceProviderWhite import org.apache.sling.resourceresolver.impl.providers.ResourceProviderHandler; import org.apache.sling.resourceresolver.impl.providers.ResourceProviderInfo; import org.apache.sling.resourceresolver.impl.providers.ResourceProviderTracker; -import org.osgi.framework.BundleContext; +import org.jetbrains.annotations.NotNull; import org.osgi.framework.Constants; import org.osgi.framework.Filter; +import org.osgi.framework.FrameworkUtil; import org.osgi.framework.InvalidSyntaxException; import org.osgi.framework.ServiceReference; import org.slf4j.Logger; @@ -40,107 +43,134 @@ import org.slf4j.LoggerFactory; */ public class FactoryPreconditions { + private static final Logger LOG = LoggerFactory.getLogger(FactoryPreconditions.class); + private static final class RequiredProvider { public String name; public String pid; public Filter filter; }; - private volatile ResourceProviderTracker tracker; + private final ResourceProviderTracker tracker; - private volatile List<RequiredProvider> requiredProviders; + private final List<RequiredProvider> requiredProviders; - public void activate(final BundleContext bc, - final Set<String> legycyConfiguration, - final Set<String> namesConfiguration, - final ResourceProviderTracker tracker) { - synchronized ( this ) { - this.tracker = tracker; + public FactoryPreconditions( + final ResourceProviderTracker tracker, + final Set<String> requiredResourceProviderNames, + final Set<String> requiredResourceProvidersLegacy) { + this.tracker = tracker; + this.requiredProviders = initRequiredProviders(requiredResourceProviderNames, requiredResourceProvidersLegacy); + } - final List<RequiredProvider> rps = new ArrayList<>(); - if ( legycyConfiguration != null ) { - final Logger logger = LoggerFactory.getLogger(getClass()); - for(final String value : legycyConfiguration) { - RequiredProvider rp = new RequiredProvider(); - if ( value.startsWith("(") ) { - try { - rp.filter = bc.createFilter(value); - } catch (final InvalidSyntaxException e) { - logger.warn("Ignoring invalid filter syntax for required provider: " + value, e); - rp = null; - } - } else { - rp.pid = value; - } - if ( rp != null ) { - rps.add(rp); + @NotNull + private List<RequiredProvider> initRequiredProviders( + Set<String> requiredResourceProviderNames, Set<String> requiredResourceProvidersLegacy) { + + boolean hasLegacyRequiredProvider = false; + if ( requiredResourceProvidersLegacy != null ) { + hasLegacyRequiredProvider = requiredResourceProvidersLegacy.remove(ResourceResolverFactoryConfig.LEGACY_REQUIRED_PROVIDER_PID); + if ( !requiredResourceProvidersLegacy.isEmpty() ) { + LOG.error("ResourceResolverFactory is using deprecated required providers configuration " + + "(resource.resolver.required.providers). Please change to use the property " + + "resource.resolver.required.providernames for values: {}", requiredResourceProvidersLegacy); + } else { + requiredResourceProvidersLegacy = null; + } + } + + if ( hasLegacyRequiredProvider ) { + if (requiredResourceProviderNames == null) { + requiredResourceProviderNames = new HashSet<>(); + } + + // add default if not already present + final boolean hasRequiredProvider = !requiredResourceProviderNames.add(ResourceResolverFactoryConfig.REQUIRED_PROVIDER_NAME); + if ( hasRequiredProvider ) { + LOG.warn("ResourceResolverFactory is using deprecated required providers configuration " + + "(resource.resolver.required.providers) with value '{}'. Please remove this configuration " + + "property. '{}' is already contained in the property resource.resolver.required.providernames.", + ResourceResolverFactoryConfig.LEGACY_REQUIRED_PROVIDER_PID, + ResourceResolverFactoryConfig.REQUIRED_PROVIDER_NAME); + } else { + LOG.warn("ResourceResolverFactory is using deprecated required providers configuration " + + "(resource.resolver.required.providers) with value '{}'. Please remove this configuration " + + "property and add '{}' to the property resource.resolver.required.providernames.", + ResourceResolverFactoryConfig.LEGACY_REQUIRED_PROVIDER_PID, + ResourceResolverFactoryConfig.REQUIRED_PROVIDER_NAME); + } + } + + final List<RequiredProvider> rps = new ArrayList<>(); + if (requiredResourceProvidersLegacy != null) { + final Logger logger = LoggerFactory.getLogger(getClass()); + for (final String value : requiredResourceProvidersLegacy) { + RequiredProvider rp = new RequiredProvider(); + + if (value.startsWith("(")) { + try { + rp.filter = FrameworkUtil.createFilter(value); + } catch (final InvalidSyntaxException e) { + logger.warn("Ignoring invalid filter syntax for required provider: " + value, e); + rp = null; } + } else { + rp.pid = value; } - } - if ( namesConfiguration != null ) { - for(final String value : namesConfiguration) { - final RequiredProvider rp = new RequiredProvider(); - rp.name = value; - rps.add(rp); + if (rp != null) { + rps.add(rp); } } - this.requiredProviders = rps; } - } - - public void deactivate() { - synchronized ( this ) { - this.requiredProviders = null; - this.tracker = null; + if (requiredResourceProviderNames != null) { + for (final String value : requiredResourceProviderNames) { + final RequiredProvider rp = new RequiredProvider(); + rp.name = value; + rps.add(rp); + } } + return rps; } public boolean checkPreconditions(final String unavailableName, final String unavailableServicePid) { - synchronized ( this ) { - final List<RequiredProvider> localRequiredProviders = this.requiredProviders; - final ResourceProviderTracker localTracker = this.tracker; - boolean canRegister = localTracker != null; - if (localRequiredProviders != null && localTracker != null ) { - for (final RequiredProvider rp : localRequiredProviders) { - canRegister = false; - for (final ResourceProviderHandler h : localTracker.getResourceProviderStorage().getAllHandlers()) { - final ResourceProviderInfo info = h.getInfo(); - if ( info == null ) { - // provider has been deactivated in the meantime - // ignore and continue - continue; - } - @SuppressWarnings("rawtypes") - final ServiceReference ref = info.getServiceReference(); - final Object servicePid = ref.getProperty(Constants.SERVICE_PID); - if ( unavailableServicePid != null && unavailableServicePid.equals(servicePid) ) { - // ignore this service - continue; - } - if ( unavailableName != null && unavailableName.equals(info.getName()) ) { - // ignore this service - continue; - } - if ( rp.name != null && rp.name.equals(info.getName()) ) { - canRegister = true; - break; - } else if (rp.filter != null && rp.filter.match(ref)) { - canRegister = true; - break; - } else if (rp.pid != null && rp.pid.equals(servicePid)){ - canRegister = true; - break; - } else if (rp.pid != null && rp.pid.equals(ref.getProperty(LegacyResourceProviderWhiteboard.ORIGINAL_SERVICE_PID))) { - canRegister = true; - break; - } - } - if ( !canRegister ) { - break; - } + boolean canRegister = true; + for (final RequiredProvider rp : requiredProviders) { + canRegister = false; + for (final ResourceProviderHandler h : tracker.getResourceProviderStorage().getAllHandlers()) { + final ResourceProviderInfo info = h.getInfo(); + if (info == null) { + // provider has been deactivated in the meantime + // ignore and continue + continue; + } + @SuppressWarnings("rawtypes") final ServiceReference ref = info.getServiceReference(); + final Object servicePid = ref.getProperty(Constants.SERVICE_PID); + if (unavailableServicePid != null && unavailableServicePid.equals(servicePid)) { + // ignore this service + continue; + } + if (unavailableName != null && unavailableName.equals(info.getName())) { + // ignore this service + continue; } + if (rp.name != null && rp.name.equals(info.getName())) { + canRegister = true; + break; + } else if (rp.filter != null && rp.filter.match(ref)) { + canRegister = true; + break; + } else if (rp.pid != null && rp.pid.equals(servicePid)) { + canRegister = true; + break; + } else if (rp.pid != null && rp.pid.equals(ref.getProperty(LegacyResourceProviderWhiteboard.ORIGINAL_SERVICE_PID))) { + canRegister = true; + break; + } + } + if (!canRegister) { + break; } - return canRegister; } + return canRegister; } } diff --git a/src/main/java/org/apache/sling/resourceresolver/impl/FactoryRegistrationHandler.java b/src/main/java/org/apache/sling/resourceresolver/impl/FactoryRegistrationHandler.java new file mode 100644 index 0000000..94d8002 --- /dev/null +++ b/src/main/java/org/apache/sling/resourceresolver/impl/FactoryRegistrationHandler.java @@ -0,0 +1,176 @@ +/* + * Licensed to the Apache Software Foundation (ASF) under one + * or more contributor license agreements. See the NOTICE file + * distributed with this work for additional information + * regarding copyright ownership. The ASF licenses this file + * to you under the Apache License, Version 2.0 (the + * "License"); you may not use this file except in compliance + * with the License. You may obtain a copy of the License at + * + * http://www.apache.org/licenses/LICENSE-2.0 + * + * Unless required by applicable law or agreed to in writing, + * software distributed under the License is distributed on an + * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY + * KIND, either express or implied. See the License for the + * specific language governing permissions and limitations + * under the License. + */ +package org.apache.sling.resourceresolver.impl; + +import org.apache.sling.api.resource.ResourceResolverFactory; +import org.apache.sling.api.resource.runtime.RuntimeService; +import org.osgi.framework.Bundle; +import org.osgi.framework.BundleContext; +import org.osgi.framework.Constants; +import org.osgi.framework.ServiceFactory; +import org.osgi.framework.ServiceRegistration; +import org.slf4j.Logger; +import org.slf4j.LoggerFactory; + +import java.util.Dictionary; +import java.util.Hashtable; +import java.util.concurrent.ExecutorService; +import java.util.concurrent.Executors; +import java.util.concurrent.TimeUnit; + +public class FactoryRegistrationHandler implements AutoCloseable { + + private static final Logger LOG = LoggerFactory.getLogger(FactoryRegistrationHandler.class); + + private final ResourceResolverFactoryActivator activator; + + private final FactoryPreconditions factoryPreconditions; + + private final ExecutorService factoryRegistrationWorker; + + @SuppressWarnings("java:S3077") // The field is only ever set to null or to a new FactoryRegistration instance, which is safe. + private volatile FactoryRegistration factoryRegistration; + + public FactoryRegistrationHandler( + ResourceResolverFactoryActivator activator, FactoryPreconditions factoryPreconditions) { + this.factoryPreconditions = factoryPreconditions; + this.activator = activator; + this.factoryRegistrationWorker = Executors.newSingleThreadExecutor( + r -> new Thread(r, ResourceResolverFactory.class.getSimpleName() + " registration/deregistration")); + } + + @Override + public void close() { + unregisterFactory(); + factoryRegistrationWorker.shutdown(); + try { + if (!factoryRegistrationWorker.awaitTermination(5, TimeUnit.SECONDS)) { + factoryRegistrationWorker.shutdownNow(); + // make sure everything is unregistered, even if + // the factoryRegistrationWorker did not complete + doUnregisterFactory(); + } + } catch (InterruptedException e) { + Thread.currentThread().interrupt(); + } + } + + /** + * Check the preconditions and if it changed, either register factory or unregister + */ + void maybeRegisterFactory(final String unavailableName, final String unavailableServicePid) { + if (!factoryRegistrationWorker.isShutdown()) { + LOG.debug("submitting maybeRegisterFactory"); + factoryRegistrationWorker.submit(() -> { + final boolean preconditionsOk = factoryPreconditions.checkPreconditions(unavailableName, unavailableServicePid); + if ( preconditionsOk && this.factoryRegistration == null ) { + // check system bundle state - if stopping, don't register new factory + final Bundle systemBundle = activator.getBundleContext().getBundle(Constants.SYSTEM_BUNDLE_LOCATION); + if ( systemBundle != null && systemBundle.getState() != Bundle.STOPPING ) { + runWithThreadName("registration", this::doRegisterFactory); + } + } else if ( !preconditionsOk && this.factoryRegistration != null ) { + LOG.debug("performing unregisterFactory via maybeRegisterFactory"); + runWithThreadName("deregistration", this::doUnregisterFactory); + } + }); + } + } + + void unregisterFactory() { + LOG.debug("submitting unregisterFactory"); + factoryRegistrationWorker.submit(() -> runWithThreadName("deregistration", + this::doUnregisterFactory)); + } + + /** + * Register the factory. + */ + private void doRegisterFactory() { + LOG.debug("performing registerFactory, factoryRegistration == {}", factoryRegistration); + if (this.factoryRegistration == null) { + this.factoryRegistration = new FactoryRegistration(activator.getBundleContext(), activator); + } + LOG.debug("finished performing registerFactory, factoryRegistration == {}", factoryRegistration); + } + + /** + * Unregister the factory (if registered). + */ + private void doUnregisterFactory() { + LOG.debug("performing unregisterFactory, factoryRegistration == {}", factoryRegistration); + if (this.factoryRegistration != null) { + this.factoryRegistration.unregister(); + this.factoryRegistration = null; + } + LOG.debug("finished performing unregisterFactory, factoryRegistration == {}", factoryRegistration); + } + + private static void runWithThreadName(String threadNameSuffix, Runnable task) { + final String name = Thread.currentThread().getName(); + try { + Thread.currentThread().setName(ResourceResolverFactory.class.getSimpleName() + " " + threadNameSuffix); + task.run(); + } finally { + Thread.currentThread().setName(name); + } + } + + private final class FactoryRegistration { + /** Registration .*/ + private final ServiceRegistration<ResourceResolverFactory> factoryRegistration; + + /** Runtime registration. */ + private final ServiceRegistration<RuntimeService> runtimeRegistration; + + private final CommonResourceResolverFactoryImpl commonFactory; + + FactoryRegistration(BundleContext context, ResourceResolverFactoryActivator activator) { + commonFactory = new CommonResourceResolverFactoryImpl(activator); + commonFactory.activate(context); + + // activate and register factory + final Dictionary<String, Object> serviceProps = new Hashtable<>(); + serviceProps.put(Constants.SERVICE_VENDOR, "The Apache Software Foundation"); + serviceProps.put(Constants.SERVICE_DESCRIPTION, "Apache Sling Resource Resolver Factory"); + factoryRegistration = context.registerService(ResourceResolverFactory.class, new ServiceFactory<>() { + @Override + public ResourceResolverFactory getService(final Bundle bundle, final ServiceRegistration<ResourceResolverFactory> registration) { + if (FactoryRegistrationHandler.this.factoryRegistrationWorker.isShutdown()) { + return null; + } + return new ResourceResolverFactoryImpl(commonFactory, bundle, activator.getServiceUserMapper()); + } + + @Override + public void ungetService(final Bundle bundle, final ServiceRegistration<ResourceResolverFactory> registration, final ResourceResolverFactory service) { + // nothing to do + } + }, serviceProps); + + runtimeRegistration = context.registerService(RuntimeService.class, activator.getRuntimeService(), null); + } + + void unregister() { + runtimeRegistration.unregister(); + factoryRegistration.unregister(); + commonFactory.deactivate(); + } + } +} diff --git a/src/main/java/org/apache/sling/resourceresolver/impl/ResourceResolverFactoryActivator.java b/src/main/java/org/apache/sling/resourceresolver/impl/ResourceResolverFactoryActivator.java index 1f14e3c..1651e06 100644 --- a/src/main/java/org/apache/sling/resourceresolver/impl/ResourceResolverFactoryActivator.java +++ b/src/main/java/org/apache/sling/resourceresolver/impl/ResourceResolverFactoryActivator.java @@ -23,9 +23,7 @@ import java.lang.reflect.Method; import java.lang.reflect.Proxy; import java.util.ArrayList; import java.util.Collections; -import java.util.Dictionary; import java.util.HashSet; -import java.util.Hashtable; import java.util.List; import java.util.Map; import java.util.Optional; @@ -36,10 +34,10 @@ import org.apache.commons.collections4.BidiMap; import org.apache.commons.collections4.bidimap.TreeBidiMap; import org.apache.commons.lang3.StringUtils; import org.apache.sling.api.resource.ResourceDecorator; -import org.apache.sling.api.resource.ResourceResolverFactory; import org.apache.sling.api.resource.path.Path; import org.apache.sling.api.resource.runtime.RuntimeService; import org.apache.sling.resourceresolver.impl.helper.ResourceDecoratorTracker; +import org.apache.sling.resourceresolver.impl.mapping.MapEntries; import org.apache.sling.resourceresolver.impl.mapping.Mapping; import org.apache.sling.resourceresolver.impl.mapping.StringInterpolationProvider; import org.apache.sling.resourceresolver.impl.observation.ResourceChangeListenerWhiteboard; @@ -47,11 +45,7 @@ import org.apache.sling.resourceresolver.impl.providers.ResourceProviderTracker; import org.apache.sling.resourceresolver.impl.providers.ResourceProviderTracker.ChangeListener; import org.apache.sling.resourceresolver.impl.providers.RuntimeServiceImpl; import org.apache.sling.serviceusermapping.ServiceUserMapper; -import org.osgi.framework.Bundle; import org.osgi.framework.BundleContext; -import org.osgi.framework.Constants; -import org.osgi.framework.ServiceFactory; -import org.osgi.framework.ServiceRegistration; import org.osgi.service.component.annotations.Activate; import org.osgi.service.component.annotations.Component; import org.osgi.service.component.annotations.Deactivate; @@ -76,16 +70,6 @@ import org.slf4j.LoggerFactory; @Component(name = "org.apache.sling.jcr.resource.internal.JcrResourceResolverFactoryImpl") public class ResourceResolverFactoryActivator { - private static final class FactoryRegistration { - /** Registration .*/ - public volatile ServiceRegistration<ResourceResolverFactory> factoryRegistration; - - /** Runtime registration. */ - public volatile ServiceRegistration<RuntimeService> runtimeRegistration; - - public volatile CommonResourceResolverFactoryImpl commonFactory; - } - /** Logger. */ private final Logger logger = LoggerFactory.getLogger(this.getClass()); @@ -140,10 +124,8 @@ public class ResourceResolverFactoryActivator { /** Observation paths */ private volatile Path[] observationPaths; - private final FactoryPreconditions preconds = new FactoryPreconditions(); - - /** Factory registration. */ - private volatile FactoryRegistration factoryRegistration; + @SuppressWarnings("java:S3077") + private volatile FactoryRegistrationHandler factoryRegistrationHandler; private final VanityPathConfigurer vanityPathConfigurer = new VanityPathConfigurer(); { @@ -235,7 +217,7 @@ public class ResourceResolverFactoryActivator { * Activates this component (called by SCR before) */ @Activate - protected void activate(final BundleContext bundleContext, + protected void activate(final BundleContext bundleContext, final ResourceResolverFactoryConfig config, final VanityPathConfigurer.DeprecatedVanityConfig deprecatedVanityConfig) { this.vanityPathConfigurer.setConfiguration(config, deprecatedVanityConfig); @@ -315,85 +297,50 @@ public class ResourceResolverFactoryActivator { } } + // for testing: if we run unit test, both trackers are set from the outside + final boolean hasPreRegisteredResourceProviderTracker = this.resourceProviderTracker != null; + if (!hasPreRegisteredResourceProviderTracker) { + this.resourceProviderTracker = new ResourceProviderTracker(); + this.changeListenerWhiteboard = new ResourceChangeListenerWhiteboard(); + this.changeListenerWhiteboard.activate(this.bundleContext, this.resourceProviderTracker, searchPath); + } + // check for required property Set<String> requiredResourceProvidersLegacy = getStringSet(config.resource_resolver_required_providers()); Set<String> requiredResourceProviderNames = getStringSet(config.resource_resolver_required_providernames()); - boolean hasLegacyRequiredProvider = false; - if ( requiredResourceProvidersLegacy != null ) { - hasLegacyRequiredProvider = requiredResourceProvidersLegacy.remove(ResourceResolverFactoryConfig.LEGACY_REQUIRED_PROVIDER_PID); - if ( !requiredResourceProvidersLegacy.isEmpty() ) { - logger.error("ResourceResolverFactory is using deprecated required providers configuration (resource.resolver.required.providers" + - "). Please change to use the property resource.resolver.required.providernames for values: " + requiredResourceProvidersLegacy); - } else { - requiredResourceProvidersLegacy = null; - } - } - if ( hasLegacyRequiredProvider ) { - final boolean hasRequiredProvider; - if ( requiredResourceProviderNames != null ) { - hasRequiredProvider = !requiredResourceProviderNames.add(ResourceResolverFactoryConfig.REQUIRED_PROVIDER_NAME); - } else { - hasRequiredProvider = false; - requiredResourceProviderNames = Collections.singleton(ResourceResolverFactoryConfig.REQUIRED_PROVIDER_NAME); - } - if ( hasRequiredProvider ) { - logger.warn("ResourceResolverFactory is using deprecated required providers configuration (resource.resolver.required.providers" + - ") with value '" + ResourceResolverFactoryConfig.LEGACY_REQUIRED_PROVIDER_PID + ". Please remove this configuration property. " + - ResourceResolverFactoryConfig.REQUIRED_PROVIDER_NAME + " is already contained in the property resource.resolver.required.providernames."); - } else { - logger.warn("ResourceResolverFactory is using deprecated required providers configuration (resource.resolver.required.providers" + - ") with value '" + ResourceResolverFactoryConfig.LEGACY_REQUIRED_PROVIDER_PID + ". Please remove this configuration property and add " + - ResourceResolverFactoryConfig.REQUIRED_PROVIDER_NAME + " to the property resource.resolver.required.providernames."); - } - } + final FactoryPreconditions factoryPreconditions = new FactoryPreconditions( + resourceProviderTracker, + requiredResourceProviderNames, + requiredResourceProvidersLegacy); + factoryRegistrationHandler = new FactoryRegistrationHandler(this, factoryPreconditions); + factoryRegistrationHandler.maybeRegisterFactory(null, null); - // for testing: if we run unit test, both trackers are set from the outside - if ( this.resourceProviderTracker == null ) { - this.resourceProviderTracker = new ResourceProviderTracker(); - this.changeListenerWhiteboard = new ResourceChangeListenerWhiteboard(); - this.preconds.activate(this.bundleContext, - requiredResourceProvidersLegacy, - requiredResourceProviderNames, - resourceProviderTracker); - this.changeListenerWhiteboard.activate(this.bundleContext, - this.resourceProviderTracker, searchPath); - this.resourceProviderTracker.activate(this.bundleContext, - this.eventAdmin, + if (!hasPreRegisteredResourceProviderTracker) { + this.resourceProviderTracker.activate(this.bundleContext, this.eventAdmin, new ChangeListener() { @Override public void providerAdded() { - if ( factoryRegistration == null ) { - checkFactoryPreconditions(null, null); - } - + factoryRegistrationHandler.maybeRegisterFactory(null, null); } @Override public void providerRemoved(final String name, final String pid, final boolean stateful, final boolean isUsed) { - if ( factoryRegistration != null ) { - if ( isUsed && (stateful || config.resource_resolver_providerhandling_paranoid()) ) { - unregisterFactory(); - } - checkFactoryPreconditions(name, pid); + if ( isUsed && (stateful || config.resource_resolver_providerhandling_paranoid()) ) { + factoryRegistrationHandler.unregisterFactory(); } + factoryRegistrationHandler.maybeRegisterFactory(name, pid); } }); - } else { - this.preconds.activate(this.bundleContext, - requiredResourceProvidersLegacy, - requiredResourceProviderNames, - resourceProviderTracker); - this.checkFactoryPreconditions(null, null); - } + } } /** * Modifies this component (called by SCR to update this component) */ @Modified - protected void modified(final BundleContext bundleContext, + protected void modified(final BundleContext bundleContext, final ResourceResolverFactoryConfig config, final VanityPathConfigurer.DeprecatedVanityConfig deprecatedVanityConfig) { this.deactivate(); @@ -405,8 +352,10 @@ public class ResourceResolverFactoryActivator { */ @Deactivate protected void deactivate() { - this.unregisterFactory(); + this.factoryRegistrationHandler.close(); + this.factoryRegistrationHandler = null; + // factoryRegistrationHandler must be closed before bundleContext is set to null this.bundleContext = null; this.config = DEFAULT_CONFIG; this.vanityPathConfigurer.setConfiguration(DEFAULT_CONFIG, null); @@ -414,88 +363,7 @@ public class ResourceResolverFactoryActivator { this.changeListenerWhiteboard = null; this.resourceProviderTracker.deactivate(); this.resourceProviderTracker = null; - - this.preconds.deactivate(); this.resourceDecoratorTracker.close(); - - // this is just a sanity call to make sure that unregister - // in the case that a registration happened again - // while deactivation - this.unregisterFactory(); - } - - /** - * Unregister the factory (if registered) - * This method might be called concurrently from deactivate and the - * background thread, therefore we need to synchronize. - */ - private void unregisterFactory() { - FactoryRegistration local = null; - synchronized ( this ) { - if ( this.factoryRegistration != null ) { - local = this.factoryRegistration; - this.factoryRegistration = null; - } - } - this.unregisterFactory(local); - } - - /** - * Unregister the provided factory - */ - private void unregisterFactory(final FactoryRegistration local) { - if ( local != null ) { - if ( local.factoryRegistration != null ) { - local.factoryRegistration.unregister(); - } - if ( local.runtimeRegistration != null ) { - local.runtimeRegistration.unregister(); - } - if ( local.commonFactory != null ) { - local.commonFactory.deactivate(); - } - } - } - - /** - * Try to register the factory. - */ - private void registerFactory(final BundleContext localContext) { - final FactoryRegistration local = new FactoryRegistration(); - - if ( localContext != null ) { - // activate and register factory - final Dictionary<String, Object> serviceProps = new Hashtable<>(); - serviceProps.put(Constants.SERVICE_VENDOR, "The Apache Software Foundation"); - serviceProps.put(Constants.SERVICE_DESCRIPTION, "Apache Sling Resource Resolver Factory"); - - local.commonFactory = new CommonResourceResolverFactoryImpl(this); - local.commonFactory.activate(localContext); - local.factoryRegistration = localContext.registerService( - ResourceResolverFactory.class, new ServiceFactory<ResourceResolverFactory>() { - - @Override - public ResourceResolverFactory getService(final Bundle bundle, final ServiceRegistration<ResourceResolverFactory> registration) { - if ( ResourceResolverFactoryActivator.this.bundleContext == null ) { - return null; - } - final ResourceResolverFactoryImpl r = new ResourceResolverFactoryImpl( - local.commonFactory, bundle, - ResourceResolverFactoryActivator.this.getServiceUserMapper()); - return r; - } - - @Override - public void ungetService(final Bundle bundle, final ServiceRegistration<ResourceResolverFactory> registration, final ResourceResolverFactory service) { - // nothing to do - } - }, serviceProps); - - local.runtimeRegistration = localContext.registerService(RuntimeService.class, - this.getRuntimeService(), null); - - this.factoryRegistration = local; - } } /** @@ -514,35 +382,6 @@ public class ResourceResolverFactoryActivator { return this.bundleContext; } - /** - * Check the preconditions and if it changed, either register factory or unregister - */ - private void checkFactoryPreconditions(final String unavailableName, final String unavailableServicePid) { - final BundleContext localContext = this.getBundleContext(); - if ( localContext != null ) { - final boolean result = this.preconds.checkPreconditions(unavailableName, unavailableServicePid); - if ( result && this.factoryRegistration == null ) { - // check system bundle state - if stopping, don't register new factory - final Bundle systemBundle = localContext.getBundle(Constants.SYSTEM_BUNDLE_LOCATION); - if ( systemBundle != null && systemBundle.getState() != Bundle.STOPPING ) { - boolean create = true; - synchronized ( this ) { - if ( this.factoryRegistration == null ) { - this.factoryRegistration = new FactoryRegistration(); - } else { - create = false; - } - } - if ( create ) { - this.registerFactory(localContext); - } - } - } else if ( !result && this.factoryRegistration != null ) { - this.unregisterFactory(); - } - } - } - /** * Bind a resource decorator. */ diff --git a/src/main/java/org/apache/sling/resourceresolver/impl/providers/stateful/AuthenticatedResourceProvider.java b/src/main/java/org/apache/sling/resourceresolver/impl/providers/stateful/AuthenticatedResourceProvider.java index 453b5cf..7207b57 100644 --- a/src/main/java/org/apache/sling/resourceresolver/impl/providers/stateful/AuthenticatedResourceProvider.java +++ b/src/main/java/org/apache/sling/resourceresolver/impl/providers/stateful/AuthenticatedResourceProvider.java @@ -52,7 +52,7 @@ public class AuthenticatedResourceProvider { private static final Logger logger = LoggerFactory.getLogger(ResourceResolverImpl.class); - public static final AuthenticatedResourceProvider UNAUTHENTICATED_PROVIDER = new AuthenticatedResourceProvider(null, false, null, null); + public static final AuthenticatedResourceProvider UNAUTHENTICATED_PROVIDER = new AuthenticatedResourceProvider(); private final ResourceProviderHandler providerHandler; @@ -79,6 +79,13 @@ public class AuthenticatedResourceProvider { this.useRAS = useRAS; } + private AuthenticatedResourceProvider() { + this.providerHandler = null; + this.resolveContext = null; + this.tracker = null; + this.useRAS = false; + } + /** * Get the resolve context. * @return The resolve context diff --git a/src/test/java/org/apache/sling/resourceresolver/impl/EtcMappingResourceResolverTest.java b/src/test/java/org/apache/sling/resourceresolver/impl/EtcMappingResourceResolverTest.java index d87aa22..5873414 100644 --- a/src/test/java/org/apache/sling/resourceresolver/impl/EtcMappingResourceResolverTest.java +++ b/src/test/java/org/apache/sling/resourceresolver/impl/EtcMappingResourceResolverTest.java @@ -31,6 +31,7 @@ import org.apache.sling.resourceresolver.impl.providers.ResourceProviderStorage; import org.apache.sling.resourceresolver.impl.providers.ResourceProviderTracker; import org.apache.sling.serviceusermapping.ServiceUserMapper; import org.apache.sling.spi.resource.provider.ResourceProvider; +import org.junit.After; import org.junit.Before; import org.junit.Test; import org.mockito.Mock; @@ -144,6 +145,11 @@ public class EtcMappingResourceResolverTest { http = buildResource("/etc/map/http", map, resourceResolver, resourceProvider); } + @After + public void cleanup() { + resourceResolver.close(); + } + List<MapConfigurationProvider.VanityPathConfig> getVanityPathConfigs() { return new ArrayList<>(); } diff --git a/src/test/java/org/apache/sling/resourceresolver/impl/FactoryPreconditionsTest.java b/src/test/java/org/apache/sling/resourceresolver/impl/FactoryPreconditionsTest.java index e14a115..b930503 100644 --- a/src/test/java/org/apache/sling/resourceresolver/impl/FactoryPreconditionsTest.java +++ b/src/test/java/org/apache/sling/resourceresolver/impl/FactoryPreconditionsTest.java @@ -43,32 +43,15 @@ public class FactoryPreconditionsTest { final ResourceProviderStorage storage = Mockito.mock(ResourceProviderStorage.class); Mockito.when(tracker.getResourceProviderStorage()).thenReturn(storage); - FactoryPreconditions conditions = new FactoryPreconditions(); - conditions.activate(null, null, null, tracker); + FactoryPreconditions conditions = new FactoryPreconditions(tracker, null, null); assertTrue(conditions.checkPreconditions(null, null)); - conditions = new FactoryPreconditions(); - conditions.activate(null, Collections.<String> emptySet(), Collections.<String> emptySet(), tracker); + conditions = new FactoryPreconditions(tracker, Collections.<String> emptySet(), Collections.<String> emptySet()); assertTrue(conditions.checkPreconditions(null, null)); } - @Test public void testDeactivated() { - final ResourceProviderTracker tracker = Mockito.mock(ResourceProviderTracker.class); - final ResourceProviderStorage storage = Mockito.mock(ResourceProviderStorage.class); - Mockito.when(tracker.getResourceProviderStorage()).thenReturn(storage); - - FactoryPreconditions conditions = new FactoryPreconditions(); - conditions.activate(null, null, null, tracker); - - assertTrue(conditions.checkPreconditions(null, null)); - - conditions.deactivate(); - - assertFalse(conditions.checkPreconditions(null, null)); - } - private List<ResourceProviderHandler> getResourceProviderHandlers(String[] pids) { final List<ResourceProviderHandler> result = new ArrayList<ResourceProviderHandler>(); @@ -108,8 +91,7 @@ public class FactoryPreconditionsTest { final ResourceProviderStorage storage = Mockito.mock(ResourceProviderStorage.class); Mockito.when(tracker.getResourceProviderStorage()).thenReturn(storage); - FactoryPreconditions conditions = new FactoryPreconditions(); - conditions.activate(null, new HashSet<>(Arrays.asList("pid1", "pid3")), null, tracker); + FactoryPreconditions conditions = new FactoryPreconditions(tracker, null, new HashSet<>(Arrays.asList("pid1", "pid3"))); final List<ResourceProviderHandler> handlers1 = getResourceProviderHandlers(new String[] {"pid2"}); Mockito.when(storage.getAllHandlers()).thenReturn(handlers1); @@ -129,8 +111,7 @@ public class FactoryPreconditionsTest { final ResourceProviderStorage storage = Mockito.mock(ResourceProviderStorage.class); Mockito.when(tracker.getResourceProviderStorage()).thenReturn(storage); - FactoryPreconditions conditions = new FactoryPreconditions(); - conditions.activate(null, null,new HashSet<>(Arrays.asList("n1", "n2")), tracker); + FactoryPreconditions conditions = new FactoryPreconditions(tracker, new HashSet<>(Arrays.asList("n1", "n2")), null); final List<ResourceProviderHandler> handlers1 = getResourceProviderHandlersWithNames(new String[] {"n2"}); Mockito.when(storage.getAllHandlers()).thenReturn(handlers1); @@ -150,8 +131,7 @@ public class FactoryPreconditionsTest { final ResourceProviderStorage storage = Mockito.mock(ResourceProviderStorage.class); Mockito.when(tracker.getResourceProviderStorage()).thenReturn(storage); - FactoryPreconditions conditions = new FactoryPreconditions(); - conditions.activate(null, new HashSet<>(Arrays.asList("pid1", "pid3")), null, tracker); + FactoryPreconditions conditions = new FactoryPreconditions(tracker, null, new HashSet<>(Arrays.asList("pid1", "pid3"))); final List<ResourceProviderHandler> handlers2 = getResourceProviderHandlers(new String[] {"pid1", "pid2", "pid3"}); Mockito.when(storage.getAllHandlers()).thenReturn(handlers2); diff --git a/src/test/java/org/apache/sling/resourceresolver/impl/FactoryRegistrationHandlerTest.java b/src/test/java/org/apache/sling/resourceresolver/impl/FactoryRegistrationHandlerTest.java new file mode 100644 index 0000000..0f66b7f --- /dev/null +++ b/src/test/java/org/apache/sling/resourceresolver/impl/FactoryRegistrationHandlerTest.java @@ -0,0 +1,181 @@ +/* + * Licensed to the Apache Software Foundation (ASF) under one + * or more contributor license agreements. See the NOTICE file + * distributed with this work for additional information + * regarding copyright ownership. The ASF licenses this file + * to you under the Apache License, Version 2.0 (the + * "License"); you may not use this file except in compliance + * with the License. You may obtain a copy of the License at + * + * http://www.apache.org/licenses/LICENSE-2.0 + * + * Unless required by applicable law or agreed to in writing, + * software distributed under the License is distributed on an + * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY + * KIND, either express or implied. See the License for the + * specific language governing permissions and limitations + * under the License. + */ +package org.apache.sling.resourceresolver.impl; + +import org.apache.commons.collections4.bidimap.TreeBidiMap; +import org.apache.sling.api.resource.ResourceResolverFactory; +import org.apache.sling.resourceresolver.impl.providers.ResourceProviderStorage; +import org.apache.sling.resourceresolver.impl.providers.ResourceProviderTracker; +import org.apache.sling.serviceusermapping.ServiceUserMapper; +import org.apache.sling.testing.mock.osgi.junit.OsgiContext; +import org.junit.Before; +import org.junit.Rule; +import org.junit.Test; +import org.mockito.Mockito; +import org.mockito.internal.stubbing.defaultanswers.ReturnsSmartNulls; +import org.osgi.framework.BundleContext; +import org.osgi.framework.ServiceEvent; +import org.osgi.framework.ServiceListener; +import org.osgi.framework.ServiceReference; +import org.slf4j.Logger; +import org.slf4j.LoggerFactory; + +import java.util.concurrent.CountDownLatch; +import java.util.concurrent.TimeUnit; +import java.util.function.Predicate; + +import static org.junit.Assert.assertTrue; +import static org.mockito.ArgumentMatchers.isNull; +import static org.mockito.Mockito.doReturn; +import static org.mockito.Mockito.when; + +public class FactoryRegistrationHandlerTest { + + private static final Logger LOG = LoggerFactory.getLogger(FactoryRegistrationHandlerTest.class); + + public static final ReturnsSmartNulls DEFAULT_ANSWER = new ReturnsSmartNulls(); + + @Rule + public OsgiContext osgi = new OsgiContext(); + + private ResourceResolverFactoryActivator activator; + + @Before + public void setUp() { + final ResourceProviderTracker resourceProviderTracker = mock(ResourceProviderTracker.class); + doReturn(mock(ResourceProviderStorage.class)).when(resourceProviderTracker).getResourceProviderStorage(); + + activator = mock(ResourceResolverFactoryActivator.class); + doReturn(osgi.bundleContext()).when(activator).getBundleContext(); + doReturn(resourceProviderTracker).when(activator).getResourceProviderTracker(); + doReturn(mock(ServiceUserMapper.class)).when(activator).getServiceUserMapper(); + doReturn(new TreeBidiMap<>()).when(activator).getVirtualURLMap(); + doReturn(null).when(activator).getEventAdmin(); + + } + + private static <T> T mock(Class<T> classToMock) { + return Mockito.mock(classToMock, DEFAULT_ANSWER); + } + + @Test + public void testFactoryRegistration() throws InterruptedException { + final FactoryPreconditions preconditions = mock(FactoryPreconditions.class); + when(preconditions.checkPreconditions(isNull(), isNull())).thenReturn(true); + + try (AwaitingListener unregistration = + AwaitingListener.unregistration(osgi.bundleContext(), ResourceResolverFactory.class)) { + try (FactoryRegistrationHandler factoryRegistrationHandler = + new FactoryRegistrationHandler(activator, preconditions); + AwaitingListener registration = AwaitingListener.registration(osgi.bundleContext(), ResourceResolverFactory.class)) { + factoryRegistrationHandler.maybeRegisterFactory(null, null); + assertTrue("Expected ResourceResolverFactory service to be registered", + registration.await(5, TimeUnit.SECONDS)); + } + assertTrue("Expected ResourceResolverFactory service to be unregistered", + unregistration.await(5, TimeUnit.SECONDS)); + } + } + + @Test + public void testFactoryDeregistrationWhenConditionsUnsatisfied() throws InterruptedException { + final BundleContext ctx = osgi.bundleContext(); + final FactoryPreconditions preconditions = mock(FactoryPreconditions.class); + try (FactoryRegistrationHandler factoryRegistrationHandler = + new FactoryRegistrationHandler(activator, preconditions); + AwaitingListener registration = AwaitingListener.registration(ctx, ResourceResolverFactory.class); + AwaitingListener unregistration = AwaitingListener.unregistration(ctx, ResourceResolverFactory.class)) { + + when(preconditions.checkPreconditions(isNull(), isNull())).thenReturn(true); + factoryRegistrationHandler.maybeRegisterFactory(null, null); + assertTrue("Expected ResourceResolverFactory service to be registered", + registration.await(5, TimeUnit.SECONDS)); + + when(preconditions.checkPreconditions(isNull(), isNull())).thenReturn(false); + factoryRegistrationHandler.maybeRegisterFactory(null, null); + assertTrue("Expected ResourceResolverFactory service to be unregistered", + unregistration.await(5, TimeUnit.SECONDS)); + } + } + + private static class AwaitingListener implements ServiceListener, AutoCloseable { + + private final BundleContext bundleContext; + + private final Predicate<ServiceEvent> expectedEventPredicate; + + private final CountDownLatch latch; + + public AwaitingListener(BundleContext ctx, Predicate<ServiceEvent> expectedEventPredicate) { + this(ctx, expectedEventPredicate, 1); + } + + public AwaitingListener(BundleContext ctx, Predicate<ServiceEvent> expectedEventPredicate, int count) { + this.bundleContext = ctx; + this.expectedEventPredicate = expectedEventPredicate; + this.latch = new CountDownLatch(count); + this.bundleContext.addServiceListener(this); + } + + public static AwaitingListener registration(BundleContext bundleContext, Class<?> clazz) { + return new AwaitingListener(bundleContext, serviceEvent -> { + if (serviceEvent.getType() == ServiceEvent.REGISTERED) { + return isInstance(bundleContext, serviceEvent.getServiceReference(), clazz); + } + return false; + }); + } + + public static AwaitingListener unregistration(BundleContext bundleContext, Class<?> clazz) { + return new AwaitingListener(bundleContext, serviceEvent -> { + if (serviceEvent.getType() == ServiceEvent.UNREGISTERING) { + return isInstance(bundleContext, serviceEvent.getServiceReference(), clazz); + } + return false; + }); + } + + private static boolean isInstance(BundleContext bundleContext, ServiceReference<?> ref, Class<?> clazz) { + try { + final Object service = bundleContext.getService(ref); + final boolean isInstance = clazz.isInstance(service); + LOG.info("{} == isInstance({}, {})", isInstance, service.getClass(), clazz); + return isInstance; + } finally { + bundleContext.ungetService(ref); + } + } + + @Override + public void serviceChanged(ServiceEvent event) { + if (expectedEventPredicate.test(event)) { + latch.countDown(); + } + } + + public boolean await(long timeout, TimeUnit timeUnit) throws InterruptedException { + return latch.await(timeout, timeUnit); + } + + @Override + public void close() { + bundleContext.removeServiceListener(this); + } + } +} \ No newline at end of file diff --git a/src/test/java/org/apache/sling/resourceresolver/impl/MockedResourceResolverImplTest.java b/src/test/java/org/apache/sling/resourceresolver/impl/MockedResourceResolverImplTest.java index 8cfd4e0..2870da3 100644 --- a/src/test/java/org/apache/sling/resourceresolver/impl/MockedResourceResolverImplTest.java +++ b/src/test/java/org/apache/sling/resourceresolver/impl/MockedResourceResolverImplTest.java @@ -23,14 +23,17 @@ import static org.junit.Assert.assertEquals; import static org.junit.Assert.assertFalse; import static org.junit.Assert.assertNotEquals; import static org.junit.Assert.assertNotNull; +import static org.junit.Assert.assertThrows; import static org.junit.Assert.assertTrue; import static org.mockito.ArgumentMatchers.any; import static org.mockito.ArgumentMatchers.anyString; +import static org.mockito.ArgumentMatchers.same; import static org.mockito.Mockito.mock; import static org.mockito.Mockito.when; import java.lang.annotation.Annotation; import java.util.ArrayList; +import java.util.Collection; import java.util.Collections; import java.util.Dictionary; import java.util.HashMap; @@ -40,6 +43,8 @@ import java.util.List; import java.util.Map; import java.util.Random; import java.util.Set; +import java.util.concurrent.CountDownLatch; +import java.util.concurrent.TimeUnit; import javax.servlet.http.HttpServletRequest; @@ -71,11 +76,13 @@ import org.mockito.ArgumentCaptor; import org.mockito.Mock; import org.mockito.Mockito; import org.mockito.MockitoAnnotations; +import org.mockito.invocation.Invocation; import org.osgi.framework.Bundle; import org.osgi.framework.BundleContext; import org.osgi.framework.Constants; import org.osgi.framework.ServiceFactory; import org.osgi.framework.ServiceReference; +import org.osgi.framework.ServiceRegistration; import static org.mockito.ArgumentMatchers.nullable; @@ -117,10 +124,6 @@ public class MockedResourceResolverImplTest { @Mock private QueryLanguageProvider queryProvider; - private Map<String, Object> services = new HashMap<String, Object>(); - - private Map<String, Object> serviceProperties = new HashMap<String, Object>(); - private ResourceResolverFactoryImpl resourceResolverFactory; @Mock @@ -151,13 +154,20 @@ public class MockedResourceResolverImplTest { @SuppressWarnings("unchecked") @Before - public void before() throws LoginException { + public void before() throws LoginException, InterruptedException { activator = new ResourceResolverFactoryActivator(); // system bundle access final Bundle systemBundle = mock(Bundle.class); Mockito.when(systemBundle.getState()).thenReturn(Bundle.ACTIVE); Mockito.when(bundleContext.getBundle(Constants.SYSTEM_BUNDLE_LOCATION)).thenReturn(systemBundle); + CountDownLatch factoryRegistrationDone = new CountDownLatch(1); + Mockito.when(bundleContext.registerService(same(ResourceResolverFactory.class), any(ServiceFactory.class), any(Dictionary.class))) + .thenAnswer(invocation -> { + factoryRegistrationDone.countDown(); + return mock(ServiceRegistration.class); + }); + activator.resourceAccessSecurityTracker = new ResourceAccessSecurityTracker(); activator.resourceProviderTracker = resourceProviderTracker; activator.changeListenerWhiteboard = resourceChangeListenerWhiteboard; @@ -312,31 +322,20 @@ public class MockedResourceResolverImplTest { Mockito.when(usingBundle.getBundleContext()).thenReturn(usingBundleContext); Mockito.when(usingBundleContext.getBundle()).thenReturn(usingBundle); - // extract any services that were registered into a map. - ArgumentCaptor<Class> classesCaptor = ArgumentCaptor.forClass(Class.class); - ArgumentCaptor<ServiceFactory> serviceCaptor = ArgumentCaptor.forClass(ServiceFactory.class); - @SuppressWarnings("rawtypes") - ArgumentCaptor<Dictionary> propertiesCaptor = ArgumentCaptor.forClass(Dictionary.class); + factoryRegistrationDone.await(5, TimeUnit.SECONDS); + + ArgumentCaptor<ServiceFactory<ResourceResolverFactory>> serviceCaptor = argumentCaptorForClass(ServiceFactory.class); Mockito.verify(bundleContext, Mockito.atLeastOnce()).registerService( - (Class<ResourceResolverFactory>)classesCaptor.capture(), (ServiceFactory<ResourceResolverFactory>)serviceCaptor.capture(), - propertiesCaptor.capture()); - - int si = 0; - List<ServiceFactory> serviceList = serviceCaptor.getAllValues(); - @SuppressWarnings({ "unused", "rawtypes" }) - List<Dictionary> servicePropertiesList = propertiesCaptor.getAllValues(); - for (Class serviceClass : classesCaptor.getAllValues()) { - services.put(serviceClass.getName(), serviceList.get(si)); - serviceProperties.put(serviceClass.getName(), serviceProperties.get(si)); - si++; - } + same(ResourceResolverFactory.class), + serviceCaptor.capture(), + any(Dictionary.class)); + // verify that a ResourceResolverFactoryImpl was created and registered. - Assert.assertNotNull("" + services, services.get(ResourceResolverFactory.class.getName())); - Object rrf = services.get(ResourceResolverFactory.class.getName()); - if (rrf instanceof ServiceFactory) { - rrf = ((ServiceFactory) rrf).getService(usingBundle, null); - } - Assert.assertTrue(rrf instanceof ResourceResolverFactoryImpl); + ServiceFactory<ResourceResolverFactory> rrfServiceFactory = serviceCaptor.getValue(); + Assert.assertNotNull("ServiceFactory<ResourceResolverFactory>", rrfServiceFactory); + final ResourceResolverFactory rrf = rrfServiceFactory.getService(usingBundle, null); + assertNotNull("ResourceResolverFactory", rrf); + assertTrue("ResourceResolverFactoryImpl", rrf instanceof ResourceResolverFactoryImpl); resourceResolverFactory = (ResourceResolverFactoryImpl) rrf; // ensure allowed alias locations are *not* ending with a slash (invalid absolut path) @@ -350,6 +349,11 @@ public class MockedResourceResolverImplTest { getInaccessibleField("commonFactory",rrf,CommonResourceResolverFactoryImpl.class).getMapEntries()); } + @SuppressWarnings("unchecked") + private <T> ArgumentCaptor<T> argumentCaptorForClass(Class<?> clazz) { + return (ArgumentCaptor<T>) ArgumentCaptor.forClass(clazz); + } + public static ResourceProviderHandler createRPHandler(ResourceProvider<?> rp, String pid, long ranking, String path) { ServiceReference ref = mock(ServiceReference.class); @@ -470,11 +474,13 @@ public class MockedResourceResolverImplTest { */ @Test public void testGetResolver() throws LoginException { - ResourceResolver resourceResolver = resourceResolverFactory.getResourceResolver(null); - Assert.assertNotNull(resourceResolver); + try (ResourceResolver resourceResolver = resourceResolverFactory.getResourceResolver(null)) { + Assert.assertNotNull(resourceResolver); + } Map<String, Object> authenticationInfo = new HashMap<String, Object>(); - resourceResolver = resourceResolverFactory.getAdministrativeResourceResolver(authenticationInfo); - Assert.assertNotNull(resourceResolver); + try (ResourceResolver resourceResolver = resourceResolverFactory.getAdministrativeResourceResolver(authenticationInfo)) { + Assert.assertNotNull(resourceResolver); + } } /** @@ -483,14 +489,11 @@ public class MockedResourceResolverImplTest { */ @Test public void testResolverMisc() throws LoginException { - ResourceResolver resourceResolver = resourceResolverFactory.getResourceResolver(null); - try { - resourceResolver.getAttribute(null); - Assert.fail("Should have thrown a NPE"); - } catch ( NullPointerException e) { - // this is expected. + try (ResourceResolver resourceResolver = resourceResolverFactory.getResourceResolver(null)) { + assertThrows("Should have thrown a NPE", NullPointerException.class, + () -> resourceResolver.getAttribute(null)); + Assert.assertArrayEquals(new String[]{"/apps/","/libs/"}, resourceResolver.getSearchPath()); } - Assert.assertArrayEquals(new String[]{"/apps/","/libs/"}, resourceResolver.getSearchPath()); } /** @@ -499,12 +502,14 @@ public class MockedResourceResolverImplTest { */ @Test public void testGetAuthenticatedResolve() throws LoginException { - ResourceResolver resourceResolver = resourceResolverFactory.getAdministrativeResourceResolver(null); - Assert.assertNotNull(resourceResolver); - Map<String, Object> authenticationInfo = new HashMap<String, Object>(); + try (ResourceResolver resourceResolver = resourceResolverFactory.getAdministrativeResourceResolver(null)) { + Assert.assertNotNull(resourceResolver); + } - resourceResolver = resourceResolverFactory.getAdministrativeResourceResolver(authenticationInfo); - Assert.assertNotNull(resourceResolver); + Map<String, Object> authenticationInfo = new HashMap<String, Object>(); + try (ResourceResolver resourceResolver = resourceResolverFactory.getAdministrativeResourceResolver(authenticationInfo)) { + Assert.assertNotNull(resourceResolver); + } } /** @@ -513,11 +518,12 @@ public class MockedResourceResolverImplTest { */ @Test public void testGetResource() throws LoginException { - ResourceResolver resourceResolver = resourceResolverFactory.getResourceResolver(null); - Assert.assertNotNull(resourceResolver); - Resource singleResource = buildResource("/single/test", EMPTY_RESOURCE_LIST, resourceResolver, resourceProvider); - Resource resource = resourceResolver.getResource("/single/test"); - Assert.assertEquals(singleResource, resource); + try (ResourceResolver resourceResolver = resourceResolverFactory.getResourceResolver(null)) { + Assert.assertNotNull(resourceResolver); + Resource singleResource = buildResource("/single/test", EMPTY_RESOURCE_LIST, resourceResolver, resourceProvider); + Resource resource = resourceResolver.getResource("/single/test"); + Assert.assertEquals(singleResource, resource); + } } /** @@ -526,11 +532,12 @@ public class MockedResourceResolverImplTest { */ @Test public void testGetResourceSLING864() throws LoginException { - ResourceResolver resourceResolver = resourceResolverFactory.getResourceResolver(null); - Assert.assertNotNull(resourceResolver); - Resource singleResource = buildResource("/single/test.with/extra.dots/inthepath", EMPTY_RESOURCE_LIST,resourceResolver, resourceProvider); - Resource resource = resourceResolver.getResource("/single/test.with/extra.dots/inthepath"); - Assert.assertEquals(singleResource, resource); + try (ResourceResolver resourceResolver = resourceResolverFactory.getResourceResolver(null)) { + Assert.assertNotNull(resourceResolver); + Resource singleResource = buildResource("/single/test.with/extra.dots/inthepath", EMPTY_RESOURCE_LIST, resourceResolver, resourceProvider); + Resource resource = resourceResolver.getResource("/single/test.with/extra.dots/inthepath"); + Assert.assertEquals(singleResource, resource); + } } @@ -540,14 +547,15 @@ public class MockedResourceResolverImplTest { */ @Test public void testRelativeResource() throws LoginException { - ResourceResolver resourceResolver = resourceResolverFactory.getResourceResolver(null); - Assert.assertNotNull(resourceResolver); - Resource appResource = buildResource("/apps/store/inventory", EMPTY_RESOURCE_LIST, resourceResolver, appsResourceProvider); - Resource libResource = buildResource("/libs/store/catalog", EMPTY_RESOURCE_LIST, resourceResolver, appsResourceProvider); - Resource testResource = resourceResolver.getResource("store/inventory"); - Assert.assertEquals(appResource, testResource); - testResource = resourceResolver.getResource("store/catalog"); - Assert.assertEquals(libResource, testResource); + try (ResourceResolver resourceResolver = resourceResolverFactory.getResourceResolver(null)) { + Assert.assertNotNull(resourceResolver); + Resource appResource = buildResource("/apps/store/inventory", EMPTY_RESOURCE_LIST, resourceResolver, appsResourceProvider); + Resource libResource = buildResource("/libs/store/catalog", EMPTY_RESOURCE_LIST, resourceResolver, appsResourceProvider); + Resource testResource = resourceResolver.getResource("store/inventory"); + Assert.assertEquals(appResource, testResource); + testResource = resourceResolver.getResource("store/catalog"); + Assert.assertEquals(libResource, testResource); + } } /** @@ -558,36 +566,37 @@ public class MockedResourceResolverImplTest { */ @Test public void testMapping() throws LoginException { - ResourceResolver resourceResolver = resourceResolverFactory.getResourceResolver(null); - buildResource("/single/test", EMPTY_RESOURCE_LIST, resourceResolver, resourceProvider); - HttpServletRequest request = mock(HttpServletRequest.class); - Mockito.when(request.getScheme()).thenReturn("http"); - Mockito.when(request.getServerPort()).thenReturn(80); - Mockito.when(request.getServerName()).thenReturn("localhost"); - - String path = resourceResolver.map(request,"/single/test?q=123123"); - Assert.assertEquals("/single/test?q=123123", path); - buildResource("/single/test", EMPTY_RESOURCE_LIST, resourceResolver, resourceProvider); - path = resourceResolver.map(request,"/single/test"); - Assert.assertEquals("/single/test", path); - - buildResource("/single/test", EMPTY_RESOURCE_LIST, resourceResolver, resourceProvider); - // test path mapping without a request. - path = resourceResolver.map("/single/test"); - Assert.assertEquals("/single/test", path); - - buildResource("/content", EMPTY_RESOURCE_LIST, resourceResolver, resourceProvider); - path = resourceResolver.map("/content.html"); - Assert.assertEquals("/content.html", path); - - path = resourceResolver.map(request,"some/relative/path/test"); - Assert.assertEquals("some/relative/path/test", path); - - buildResource("/", EMPTY_RESOURCE_LIST, resourceResolver, resourceProvider); - buildResource("/single", EMPTY_RESOURCE_LIST, resourceResolver, resourceProvider); - buildResource("/single/test", EMPTY_RESOURCE_LIST, resourceResolver, resourceProvider); - path = resourceResolver.map("/single//test.html"); - Assert.assertEquals("/single/test.html", path); + try (ResourceResolver resourceResolver = resourceResolverFactory.getResourceResolver(null)) { + buildResource("/single/test", EMPTY_RESOURCE_LIST, resourceResolver, resourceProvider); + HttpServletRequest request = mock(HttpServletRequest.class); + Mockito.when(request.getScheme()).thenReturn("http"); + Mockito.when(request.getServerPort()).thenReturn(80); + Mockito.when(request.getServerName()).thenReturn("localhost"); + + String path = resourceResolver.map(request, "/single/test?q=123123"); + Assert.assertEquals("/single/test?q=123123", path); + buildResource("/single/test", EMPTY_RESOURCE_LIST, resourceResolver, resourceProvider); + path = resourceResolver.map(request, "/single/test"); + Assert.assertEquals("/single/test", path); + + buildResource("/single/test", EMPTY_RESOURCE_LIST, resourceResolver, resourceProvider); + // test path mapping without a request. + path = resourceResolver.map("/single/test"); + Assert.assertEquals("/single/test", path); + + buildResource("/content", EMPTY_RESOURCE_LIST, resourceResolver, resourceProvider); + path = resourceResolver.map("/content.html"); + Assert.assertEquals("/content.html", path); + + path = resourceResolver.map(request, "some/relative/path/test"); + Assert.assertEquals("some/relative/path/test", path); + + buildResource("/", EMPTY_RESOURCE_LIST, resourceResolver, resourceProvider); + buildResource("/single", EMPTY_RESOURCE_LIST, resourceResolver, resourceProvider); + buildResource("/single/test", EMPTY_RESOURCE_LIST, resourceResolver, resourceProvider); + path = resourceResolver.map("/single//test.html"); + Assert.assertEquals("/single/test.html", path); + } } @@ -599,22 +608,23 @@ public class MockedResourceResolverImplTest { */ @Test public void testListChildren() throws LoginException { - ResourceResolver resourceResolver = resourceResolverFactory.getResourceResolver(null); - buildResource("/single/test/withchildren", buildChildResources("/single/test/withchildren"), resourceResolver, resourceProvider ); + try (ResourceResolver resourceResolver = resourceResolverFactory.getResourceResolver(null)) { + buildResource("/single/test/withchildren", buildChildResources("/single/test/withchildren"), resourceResolver, resourceProvider); - Resource resource = resourceResolver.getResource("/single/test/withchildren"); - Assert.assertNotNull(resource); + Resource resource = resourceResolver.getResource("/single/test/withchildren"); + Assert.assertNotNull(resource); - // test via the resource list children itself, this really just tests this test case. - Iterator<Resource> resourceIterator = resource.listChildren(); - Assert.assertNotNull(resourceResolver); - int i = 0; - while(resourceIterator.hasNext()) { - Assert.assertEquals("m"+i, resourceIterator.next().getName()); - i++; + // test via the resource list children itself, this really just tests this test case. + Iterator<Resource> resourceIterator = resource.listChildren(); + Assert.assertNotNull(resourceResolver); + int i = 0; + while (resourceIterator.hasNext()) { + Assert.assertEquals("m" + i, resourceIterator.next().getName()); + i++; + } + Assert.assertEquals(5, i); } - Assert.assertEquals(5, i); } /** @@ -623,22 +633,23 @@ public class MockedResourceResolverImplTest { */ @Test public void testResourceResolverListChildren() throws LoginException { - ResourceResolver resourceResolver = resourceResolverFactory.getResourceResolver(null); - buildResource("/single/test/withchildren", buildChildResources("/single/test/withchildren"), resourceResolver, resourceProvider); + try (ResourceResolver resourceResolver = resourceResolverFactory.getResourceResolver(null)) { + buildResource("/single/test/withchildren", buildChildResources("/single/test/withchildren"), resourceResolver, resourceProvider); - Resource resource = resourceResolver.getResource("/single/test/withchildren"); - Assert.assertNotNull(resource); + Resource resource = resourceResolver.getResource("/single/test/withchildren"); + Assert.assertNotNull(resource); - // test via the resource list children itself, this really just tests this test case. - Iterator<Resource> resourceIterator = resourceResolver.listChildren(resource); - Assert.assertNotNull(resourceResolver); - int i = 0; - while(resourceIterator.hasNext()) { - Assert.assertEquals("m"+i, resourceIterator.next().getName()); - i++; + // test via the resource list children itself, this really just tests this test case. + Iterator<Resource> resourceIterator = resourceResolver.listChildren(resource); + Assert.assertNotNull(resourceResolver); + int i = 0; + while (resourceIterator.hasNext()) { + Assert.assertEquals("m" + i, resourceIterator.next().getName()); + i++; + } + Assert.assertEquals(5, i); } - Assert.assertEquals(5,i); } /** @@ -647,22 +658,23 @@ public class MockedResourceResolverImplTest { */ @Test public void testResourceResolverGetChildren() throws LoginException { - ResourceResolver resourceResolver = resourceResolverFactory.getResourceResolver(null); - buildResource("/single/test/withchildren", buildChildResources("/single/test/withchildren"), resourceResolver, resourceProvider); + try (ResourceResolver resourceResolver = resourceResolverFactory.getResourceResolver(null)) { + buildResource("/single/test/withchildren", buildChildResources("/single/test/withchildren"), resourceResolver, resourceProvider); - Resource resource = resourceResolver.getResource("/single/test/withchildren"); - Assert.assertNotNull(resource); + Resource resource = resourceResolver.getResource("/single/test/withchildren"); + Assert.assertNotNull(resource); - // test via the resource list children itself, this really just tests this test case. - Iterable<Resource> resourceIterator = resourceResolver.getChildren(resource); - Assert.assertNotNull(resourceResolver); - int i = 0; - for(Resource r : resourceIterator) { - Assert.assertEquals("m"+i, r.getName()); - i++; + // test via the resource list children itself, this really just tests this test case. + Iterable<Resource> resourceIterator = resourceResolver.getChildren(resource); + Assert.assertNotNull(resourceResolver); + int i = 0; + for (Resource r : resourceIterator) { + Assert.assertEquals("m" + i, r.getName()); + i++; + } + Assert.assertEquals(5, i); } - Assert.assertEquals(5,i); } @SuppressWarnings("unchecked") @@ -674,41 +686,42 @@ public class MockedResourceResolverImplTest { Mockito.when(queryProvider.queryResources(Mockito.nullable(ResolveContext.class), Mockito.nullable(String.class), Mockito.nullable(String.class))) .thenReturn(buildValueMapCollection(n, "A_").iterator()); - final ResourceResolver rr = resourceResolverFactory.getResourceResolver(null); - buildResource("/search/test/withchildren", buildChildResources("/search/test/withchildren"), rr, resourceProvider); - final Iterator<Map<String, Object>> it = rr.queryResources("/search", FAKE_QUERY_LANGUAGE); - final Set<String> toFind = new HashSet<String>(); - for(int i=0; i < n; i++) { - toFind.add("A_" + i); - } + try (ResourceResolver rr = resourceResolverFactory.getResourceResolver(null)) { + buildResource("/search/test/withchildren", buildChildResources("/search/test/withchildren"), rr, resourceProvider); + final Iterator<Map<String, Object>> it = rr.queryResources("/search", FAKE_QUERY_LANGUAGE); + final Set<String> toFind = new HashSet<String>(); + for (int i = 0; i < n; i++) { + toFind.add("A_" + i); + } - assertTrue("Expecting non-empty result (" + n + ")", it.hasNext()); - while(it.hasNext()) { - final Map<String, Object> m = it.next(); - toFind.remove(m.get(PATH)); + assertTrue("Expecting non-empty result (" + n + ")", it.hasNext()); + while (it.hasNext()) { + final Map<String, Object> m = it.next(); + toFind.remove(m.get(PATH)); + } + assertTrue("Expecting no leftovers (" + n + ") in" + toFind, toFind.isEmpty()); } - assertTrue("Expecting no leftovers (" + n + ") in" + toFind, toFind.isEmpty()); } @Test public void test_versions() throws LoginException { - ResourceResolver resourceResolver = resourceResolverFactory.getResourceResolver(null); - - Resource resource = resourceResolver.resolve("/content/test.html;v=1.0"); - Map<String, String> parameters = resource.getResourceMetadata().getParameterMap(); - assertEquals("/content/test.html", resource.getPath()); - assertEquals("test.html", resource.getName()); - assertEquals(Collections.singletonMap("v", "1.0"), parameters); - - resource = resourceResolver.resolve("/content/test;v='1.0'.html"); - parameters = resource.getResourceMetadata().getParameterMap(); - assertEquals("/content/test.html", resource.getPath()); - assertEquals("test.html", resource.getName()); - assertEquals(Collections.singletonMap("v", "1.0"), parameters); - - buildResource("/single/test/withchildren", buildChildResources("/single/test/withchildren"), resourceResolver, resourceProvider); - resource = resourceResolver.getResource("/single/test/withchildren;v='1.0'"); - assertNotNull(resource); - assertEquals("/single/test/withchildren", resource.getPath()); - assertEquals("withchildren", resource.getName()); + try (ResourceResolver resourceResolver = resourceResolverFactory.getResourceResolver(null)) { + Resource resource = resourceResolver.resolve("/content/test.html;v=1.0"); + Map<String, String> parameters = resource.getResourceMetadata().getParameterMap(); + assertEquals("/content/test.html", resource.getPath()); + assertEquals("test.html", resource.getName()); + assertEquals(Collections.singletonMap("v", "1.0"), parameters); + + resource = resourceResolver.resolve("/content/test;v='1.0'.html"); + parameters = resource.getResourceMetadata().getParameterMap(); + assertEquals("/content/test.html", resource.getPath()); + assertEquals("test.html", resource.getName()); + assertEquals(Collections.singletonMap("v", "1.0"), parameters); + + buildResource("/single/test/withchildren", buildChildResources("/single/test/withchildren"), resourceResolver, resourceProvider); + resource = resourceResolver.getResource("/single/test/withchildren;v='1.0'"); + assertNotNull(resource); + assertEquals("/single/test/withchildren", resource.getPath()); + assertEquals("withchildren", resource.getName()); + } } } diff --git a/src/test/java/org/apache/sling/resourceresolver/impl/mapping/AbstractMappingMapEntriesTest.java b/src/test/java/org/apache/sling/resourceresolver/impl/mapping/AbstractMappingMapEntriesTest.java index 36c35c0..fd66d0e 100644 --- a/src/test/java/org/apache/sling/resourceresolver/impl/mapping/AbstractMappingMapEntriesTest.java +++ b/src/test/java/org/apache/sling/resourceresolver/impl/mapping/AbstractMappingMapEntriesTest.java @@ -110,8 +110,8 @@ public abstract class AbstractMappingMapEntriesTest { when(resourceResolverFactory.getMapRoot()).thenReturn(MapEntries.DEFAULT_MAP_ROOT); when(resourceResolverFactory.getMaxCachedVanityPathEntries()).thenReturn(-1L); when(resourceResolverFactory.isMaxCachedVanityPathEntriesStartup()).thenReturn(true); - when(resourceResolver.findResources(anyString(), eq("JCR-SQL2"))).thenReturn( - Collections.<Resource> emptySet().iterator()); + when(resourceResolver.findResources(anyString(), eq("JCR-SQL2"))).thenReturn(Collections.emptyIterator()); + when(resourceResolver.findResources(anyString(), eq("sql"))).thenReturn(Collections.emptyIterator()); map = setupEtcMapResource("/etc", "map"); http = setupEtcMapResource("http", map); @@ -162,7 +162,7 @@ public abstract class AbstractMappingMapEntriesTest { String path = (parent == null ? parentPath : parent.getPath()) + "/" + name; when(resource.getPath()).thenReturn(path); when(resource.getName()).thenReturn(name); - ValueMap valueMap = buildValueMap(valueMapPairs); + ValueMap valueMap = buildValueMap((Object[]) valueMapPairs); when(resource.getValueMap()).thenReturn(valueMap); when(resource.adaptTo(ValueMap.class)).thenReturn(valueMap); when(resourceResolver.getResource(resource.getPath())).thenReturn(resource); diff --git a/src/test/java/org/apache/sling/resourceresolver/impl/mapping/InMemoryResourceProvider.java b/src/test/java/org/apache/sling/resourceresolver/impl/mapping/InMemoryResourceProvider.java index fe74988..4f63a85 100644 --- a/src/test/java/org/apache/sling/resourceresolver/impl/mapping/InMemoryResourceProvider.java +++ b/src/test/java/org/apache/sling/resourceresolver/impl/mapping/InMemoryResourceProvider.java @@ -105,7 +105,7 @@ public class InMemoryResourceProvider extends ResourceProvider<Void>{ // we don't explicitly filter paths under jcr:system, but we don't expect to have such resources either // and this stub provider is not the proper location to test JCR queries - if ( "sql".equals(language) && "SELECT sling:alias FROM nt:base AS page WHERE (NOT ISDESCENDANTNODE(page,\"/jcr:system\")) AND sling:alias IS NOT NULL".equals(query) ) { + if ( "sql".equals(language) && "SELECT sling:alias FROM nt:base AS page WHERE (NOT ISDESCENDANTNODE(page,'/jcr:system')) AND sling:alias IS NOT NULL".equals(query) ) { return resourcesWithProperty(ctx, "sling:alias") .iterator(); } diff --git a/src/test/java/org/apache/sling/resourceresolver/impl/mapping/ResourceMapperImplTest.java b/src/test/java/org/apache/sling/resourceresolver/impl/mapping/ResourceMapperImplTest.java index ddfd024..42bbfe0 100644 --- a/src/test/java/org/apache/sling/resourceresolver/impl/mapping/ResourceMapperImplTest.java +++ b/src/test/java/org/apache/sling/resourceresolver/impl/mapping/ResourceMapperImplTest.java @@ -32,6 +32,7 @@ import java.util.Arrays; import java.util.Collection; import java.util.HashSet; import java.util.Set; +import java.util.concurrent.TimeUnit; import javax.servlet.http.HttpServletRequest; @@ -53,6 +54,7 @@ import org.junit.Test; import org.junit.runner.RunWith; import org.junit.runners.Parameterized; import org.junit.runners.Parameterized.Parameters; +import org.osgi.util.tracker.ServiceTracker; /** * Validates that the {@link ResourceMapperImpl} correctly queries all sources of mappings @@ -94,7 +96,7 @@ public class ResourceMapperImplTest { } @Before - public void prepare() throws LoginException { + public void prepare() throws LoginException, InterruptedException { ctx.registerInjectActivateService(new ServiceUserMapperImpl()); ctx.registerInjectActivateService(new ResourceAccessSecurityTracker()); @@ -139,7 +141,15 @@ public class ResourceMapperImplTest { ctx.registerInjectActivateService(new ResourceResolverFactoryActivator(), "resource.resolver.optimize.alias.resolution", optimiseAliasResolution); - ResourceResolverFactory factory = ctx.getService(ResourceResolverFactory.class); + final ResourceResolverFactory factory; + final ServiceTracker<ResourceResolverFactory, ResourceResolverFactory> tracker = + new ServiceTracker<>(ctx.bundleContext(), ResourceResolverFactory.class, null); + try { + tracker.open(); + factory = tracker.waitForService(TimeUnit.SECONDS.toMillis(5)); + } finally { + tracker.close(); + } assertNotNull(factory);