This is an automated email from the ASF dual-hosted git repository.
cziegeler pushed a commit to branch master
in repository
https://gitbox.apache.org/repos/asf/sling-org-apache-sling-adapter.git
The following commit(s) were added to refs/heads/master by this push:
new a03221c SLING-10885 : Improve adapter factory handling
a03221c is described below
commit a03221cf0b9bdbaaf33b8104a54e5116371d3ba8
Author: Carsten Ziegeler <[email protected]>
AuthorDate: Fri Oct 29 13:43:48 2021 +0200
SLING-10885 : Improve adapter factory handling
---
pom.xml | 6 +-
.../adapter/internal/AdapterFactoryDescriptor.java | 10 ++-
.../sling/adapter/internal/AdapterManagerImpl.java | 89 +++++++++-------------
.../sling/adapter/internal/AdapterManagerTest.java | 50 +++++++++++-
4 files changed, 97 insertions(+), 58 deletions(-)
diff --git a/pom.xml b/pom.xml
index 98b1359..716a15a 100644
--- a/pom.xml
+++ b/pom.xml
@@ -96,9 +96,9 @@
<artifactId>slf4j-api</artifactId>
</dependency>
<dependency>
- <groupId>org.osgi</groupId>
- <artifactId>org.osgi.util.converter</artifactId>
- <version>1.0.0</version>
+ <groupId>org.apache.felix</groupId>
+ <artifactId>org.apache.felix.converter</artifactId>
+ <version>1.0.18</version>
<scope>provided</scope>
</dependency>
<dependency>
diff --git
a/src/main/java/org/apache/sling/adapter/internal/AdapterFactoryDescriptor.java
b/src/main/java/org/apache/sling/adapter/internal/AdapterFactoryDescriptor.java
index be1bf22..a9ed454 100644
---
a/src/main/java/org/apache/sling/adapter/internal/AdapterFactoryDescriptor.java
+++
b/src/main/java/org/apache/sling/adapter/internal/AdapterFactoryDescriptor.java
@@ -33,13 +33,17 @@ public class AdapterFactoryDescriptor {
private final String[] adapters;
+ private final String[] adaptables;
+
private volatile ServiceRegistration<Adaption> adaption;
public AdapterFactoryDescriptor(
final AdapterFactory factory,
- final String[] adapters) {
+ final String[] adapters,
+ final String[] adaptables) {
this.factory = factory;
this.adapters = adapters;
+ this.adaptables = adaptables;
}
public AdapterFactory getFactory() {
@@ -50,6 +54,10 @@ public class AdapterFactoryDescriptor {
return adapters;
}
+ public String[] getAdaptables() {
+ return adaptables;
+ }
+
public ServiceRegistration<Adaption> getAdaption() {
return adaption;
}
diff --git
a/src/main/java/org/apache/sling/adapter/internal/AdapterManagerImpl.java
b/src/main/java/org/apache/sling/adapter/internal/AdapterManagerImpl.java
index 16a661b..8053a92 100644
--- a/src/main/java/org/apache/sling/adapter/internal/AdapterManagerImpl.java
+++ b/src/main/java/org/apache/sling/adapter/internal/AdapterManagerImpl.java
@@ -38,7 +38,6 @@ import org.apache.sling.api.adapter.AdapterManager;
import org.apache.sling.api.adapter.SlingAdaptable;
import org.osgi.framework.Constants;
import org.osgi.framework.ServiceReference;
-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;
@@ -119,7 +118,7 @@ public class AdapterManagerImpl implements AdapterManager {
if (descList != null && descList.size() > 0) {
for (AdapterFactoryDescriptor desc : descList) {
- final AdapterFactory factory = desc == null ? null :
desc.getFactory();
+ final AdapterFactory factory = desc.getFactory();
// have the factory adapt the adaptable if the factory exists
if (factory != null) {
@@ -167,7 +166,7 @@ public class AdapterManagerImpl implements AdapterManager {
/**
* Bind a new adapter factory.
*/
- @Reference(name="AdapterFactory", service=AdapterFactory.class,
+ @Reference(service=AdapterFactory.class, updated = "updatedAdapterFactory",
cardinality=ReferenceCardinality.MULTIPLE,
policy=ReferencePolicy.DYNAMIC)
protected void bindAdapterFactory(final AdapterFactory factory, final
ServiceReference<AdapterFactory> reference) {
registerAdapterFactory(factory, reference);
@@ -180,6 +179,14 @@ public class AdapterManagerImpl implements AdapterManager {
unregisterAdapterFactory(reference);
}
+ /**
+ * Modify a adapter factory.
+ */
+ protected void updatedAdapterFactory(final AdapterFactory factory, final
ServiceReference<AdapterFactory> reference) {
+ unregisterAdapterFactory(reference);
+ registerAdapterFactory(factory, reference);
+ }
+
// ---------- unit testing stuff only
--------------------------------------
/**
@@ -228,7 +235,7 @@ public class AdapterManagerImpl implements AdapterManager {
}
}
- final AdapterFactoryDescriptor factoryDesc = new
AdapterFactoryDescriptor(factory, adapters);
+ final AdapterFactoryDescriptor factoryDesc = new
AdapterFactoryDescriptor(factory, adapters, adaptables);
for (final String adaptable : adaptables) {
AdapterFactoryDescriptorMap adfMap = null;
@@ -290,63 +297,43 @@ public class AdapterManagerImpl implements AdapterManager
{
* <code>reference</code> from the registry.
*/
private void unregisterAdapterFactory(final
ServiceReference<AdapterFactory> reference) {
- final Converter converter = Converters.standardConverter();
- final String[] adaptables =
converter.convert(reference.getProperty(ADAPTABLE_CLASSES)).to(String[].class);
- final String[] adapters =
converter.convert(reference.getProperty(ADAPTER_CLASSES)).to(String[].class);
-
- if (adaptables == null || adaptables.length == 0 || adapters == null
|| adapters.length == 0) {
- return;
+ final List<AdapterFactoryDescriptorMap> list = new ArrayList<>();
+ synchronized ( this.descriptors ) {
+ for(final AdapterFactoryDescriptorMap map :
this.descriptors.values()) {
+ list.add(map);
+ }
}
-
- boolean factoriesModified = false;
- AdapterFactoryDescriptorMap adfMap = null;
-
AdapterFactoryDescriptor removedDescriptor = null;
- for (final String adaptable : adaptables) {
- synchronized ( this.descriptors ) {
- adfMap = this.descriptors.get(adaptable);
- }
- if (adfMap != null) {
- synchronized ( adfMap ) {
- AdapterFactoryDescriptor factoryDesc =
adfMap.remove(reference);
- if (factoryDesc != null) {
- factoriesModified = true;
- // A single ServiceReference should correspond to a
single Adaption service being registered
- // Since the code paths above does not fully guarantee
it though, let's keep this check in place
- if (removedDescriptor != null && removedDescriptor !=
factoryDesc) {
- log.error("When unregistering reference {} got
duplicate service descriptors {} and {}. Unregistration of {} services may be
incomplete.",
- new Object[] { reference,
removedDescriptor, factoryDesc, Adaption.class.getName()} );
- }
- removedDescriptor = factoryDesc;
- }
+ for(final AdapterFactoryDescriptorMap map : list) {
+ synchronized (map ) {
+ final AdapterFactoryDescriptor factoryDesc =
map.remove(reference);
+ if (factoryDesc != null) {
+ removedDescriptor = factoryDesc;
}
}
}
- // only remove cache if some adapter factories have actually been
- // removed
- if (factoriesModified) {
- this.factoryCache.clear();
- }
-
// unregister adaption
if (removedDescriptor != null) {
+ // only remove cache if some adapter factories have actually been
+ // removed
+ this.factoryCache.clear();
+
removedDescriptor.getAdaption().unregister();
if (log.isDebugEnabled()) {
log.debug("Unregistered service {} with {} : {} and {} : {}",
new Object[] { Adaption.class.getName(),
- SlingConstants.PROPERTY_ADAPTABLE_CLASSES,
Arrays.toString(adaptables),
- SlingConstants.PROPERTY_ADAPTER_CLASSES,
Arrays.toString(adapters) });
+ SlingConstants.PROPERTY_ADAPTABLE_CLASSES,
Arrays.toString(removedDescriptor.getAdaptables()),
+ SlingConstants.PROPERTY_ADAPTER_CLASSES,
Arrays.toString(removedDescriptor.getAdapters()) });
}
- }
- // send event
- final EventAdmin localEA = this.eventAdmin;
- if ( localEA != null ) {
- final Dictionary<String, Object> props = new Hashtable<>();
- props.put(SlingConstants.PROPERTY_ADAPTABLE_CLASSES, adaptables);
- props.put(SlingConstants.PROPERTY_ADAPTER_CLASSES, adapters);
- localEA.postEvent(new
Event(SlingConstants.TOPIC_ADAPTER_FACTORY_REMOVED,
- props));
+ // send event
+ final EventAdmin localEA = this.eventAdmin;
+ if ( localEA != null ) {
+ final Dictionary<String, Object> props = new Hashtable<>();
+ props.put(SlingConstants.PROPERTY_ADAPTABLE_CLASSES,
removedDescriptor.getAdaptables());
+ props.put(SlingConstants.PROPERTY_ADAPTER_CLASSES,
removedDescriptor.getAdapters());
+ localEA.postEvent(new
Event(SlingConstants.TOPIC_ADAPTER_FACTORY_REMOVED, props));
+ }
}
}
@@ -448,12 +435,8 @@ public class AdapterManagerImpl implements AdapterManager {
// for each target class copy the entry to dest and put it in the list
or create the list
for (Map.Entry<String, List<AdapterFactoryDescriptor>> entry :
scMap.entrySet()) {
- List<AdapterFactoryDescriptor> factoryDescriptors =
dest.get(entry.getKey());
+ final List<AdapterFactoryDescriptor> factoryDescriptors =
dest.computeIfAbsent(entry.getKey(), id -> new ArrayList<>());
- if (factoryDescriptors == null) {
- factoryDescriptors = new ArrayList<>();
- dest.put(entry.getKey(), factoryDescriptors);
- }
for (AdapterFactoryDescriptor descriptor : entry.getValue()) {
factoryDescriptors.add(descriptor);
}
diff --git
a/src/test/java/org/apache/sling/adapter/internal/AdapterManagerTest.java
b/src/test/java/org/apache/sling/adapter/internal/AdapterManagerTest.java
index 8b35ddf..4b047b2 100644
--- a/src/test/java/org/apache/sling/adapter/internal/AdapterManagerTest.java
+++ b/src/test/java/org/apache/sling/adapter/internal/AdapterManagerTest.java
@@ -36,6 +36,7 @@ import org.osgi.framework.Bundle;
import org.osgi.framework.BundleContext;
import org.osgi.framework.Constants;
import org.osgi.framework.ServiceReference;
+import org.osgi.framework.ServiceRegistration;
import org.osgi.service.packageadmin.ExportedPackage;
import org.osgi.service.packageadmin.PackageAdmin;
@@ -92,8 +93,30 @@ public class AdapterManagerTest {
}
@Test
- public void testBindAfterActivate() throws Exception {
+ public void testInvalidRegistrations() throws Exception {
+ ServiceReference<AdapterFactory> ref = createServiceReference(0, null,
new String[] {TestAdapter.class.getName()});
+ am.bindAdapterFactory(Mockito.mock(AdapterFactory.class), ref);
+ assertTrue("AdapterFactoryDescriptors must be empty",
am.getFactories().isEmpty());
+
+ ref = createServiceReference(0, new String[0], new String[]
{TestAdapter.class.getName()});
+ am.bindAdapterFactory(Mockito.mock(AdapterFactory.class), ref);
+ assertTrue("AdapterFactoryDescriptors must be empty",
am.getFactories().isEmpty());
+
+ ref = createServiceReference(0, new String[]
{TestSlingAdaptable.class.getName()}, null);
+ am.bindAdapterFactory(Mockito.mock(AdapterFactory.class), ref);
+ assertTrue("AdapterFactoryDescriptors must be empty",
am.getFactories().isEmpty());
+
+ ref = createServiceReference(0, new String[]
{TestSlingAdaptable.class.getName()}, new String[0]);
+ am.bindAdapterFactory(Mockito.mock(AdapterFactory.class), ref);
+ assertTrue("AdapterFactoryDescriptors must be empty",
am.getFactories().isEmpty());
+ }
+
+ @Test
+ public void testBindUnbind() throws Exception {
final ServiceReference<AdapterFactory> ref = createServiceReference();
+ final ServiceRegistration<Adaption> registration =
Mockito.mock(ServiceRegistration.class);
+
Mockito.when(ref.getBundle().getBundleContext().registerService(Mockito.eq(Adaption.class),
+ Mockito.eq(AdaptionImpl.INSTANCE),
Mockito.any())).thenReturn(registration);
am.bindAdapterFactory(Mockito.mock(AdapterFactory.class), ref);
// check that a service is registered
@@ -103,6 +126,7 @@ public class AdapterManagerTest {
// expect the factory, but cache is empty
assertNotNull("AdapterFactoryDescriptors must not be null",
am.getFactories());
assertEquals("AdapterFactoryDescriptors must contain one entry", 1,
am.getFactories().size());
+ assertEquals(1,
am.getFactories().get(TestSlingAdaptable.class.getName()).size());
assertTrue("AdapterFactory cache must be empty",
am.getFactoryCache().isEmpty());
Map<String, AdapterFactoryDescriptorMap> f = am.getFactories();
@@ -117,6 +141,30 @@ public class AdapterManagerTest {
assertEquals(ITestAdapter.class.getName(), afd.getAdapters()[0]);
assertNull(f.get(TestSlingAdaptable2.class.getName()));
+
+ Mockito.verify(registration, Mockito.never()).unregister();
+ am.unbindAdapterFactory(ref);
+ Mockito.verify(registration).unregister();
+
assertTrue(am.getFactories().get(TestSlingAdaptable.class.getName()).isEmpty());
+ assertTrue("AdapterFactory cache must be empty",
am.getFactoryCache().isEmpty());
+ }
+
+ @Test
+ public void testBindModifiedUnbind() throws Exception {
+ ServiceReference<AdapterFactory> ref = createServiceReference();
+ final ServiceRegistration<Adaption> registration =
Mockito.mock(ServiceRegistration.class);
+
Mockito.when(ref.getBundle().getBundleContext().registerService(Mockito.eq(Adaption.class),
+ Mockito.eq(AdaptionImpl.INSTANCE),
Mockito.any())).thenReturn(registration);
+ am.bindAdapterFactory(Mockito.mock(AdapterFactory.class), ref);
+
+ assertEquals("AdapterFactoryDescriptors must contain one entry", 1,
am.getFactories().size());
+ assertEquals(1,
am.getFactories().get(TestSlingAdaptable.class.getName()).size());
+
+
Mockito.when(ref.getProperty(AdapterFactory.ADAPTABLE_CLASSES)).thenReturn(new
String[] {TestSlingAdaptable2.class.getName()});
+ am.updatedAdapterFactory(Mockito.mock(AdapterFactory.class), ref);
+ assertEquals("AdapterFactoryDescriptors must contain two entries", 2,
am.getFactories().size());
+ assertEquals(0,
am.getFactories().get(TestSlingAdaptable.class.getName()).size());
+ assertEquals(1,
am.getFactories().get(TestSlingAdaptable2.class.getName()).size());
}
@Test
