This is an automated email from the ASF dual-hosted git repository.
davidb pushed a commit to branch master
in repository
https://gitbox.apache.org/repos/asf/sling-org-apache-sling-feature-apiregions.git
The following commit(s) were added to refs/heads/master by this push:
new 27de130 SLING-9410 BSN to feature cache does not get udpated when new
configuration arrives
new db42dc1 Merge pull request #8 from bosschaert/SLING-9410-alt
27de130 is described below
commit 27de130d9306f90d28752379a3c4210d9defc4cd
Author: David Bosschaert <[email protected]>
AuthorDate: Fri May 1 12:12:03 2020 +0100
SLING-9410 BSN to feature cache does not get udpated when new configuration
arrives
---
pom.xml | 1 +
.../sling/feature/apiregions/impl/Activator.java | 2 +-
.../apiregions/impl/RegionConfiguration.java | 114 ++++++++++----------
.../feature/apiregions/impl/ResolverHookImpl.java | 14 ++-
.../apiregions/impl/RegionConfigurationTest.java | 120 ++++++++++++++++++++-
src/test/resources/props4/bundles.properties | 4 +
src/test/resources/props4/features.properties | 3 +
src/test/resources/props4/idbsnver.properties | 5 +
src/test/resources/props4/regions.properties | 4 +
9 files changed, 202 insertions(+), 65 deletions(-)
diff --git a/pom.xml b/pom.xml
index d3483f1..d9cb4af 100644
--- a/pom.xml
+++ b/pom.xml
@@ -68,6 +68,7 @@
<exclude>src/test/resources/props1/*</exclude>
<exclude>src/test/resources/props2/*</exclude>
<exclude>src/test/resources/props3/*</exclude>
+ <exclude>src/test/resources/props4/*</exclude>
</excludes>
</configuration>
</plugin>
diff --git
a/src/main/java/org/apache/sling/feature/apiregions/impl/Activator.java
b/src/main/java/org/apache/sling/feature/apiregions/impl/Activator.java
index 285d91a..b3a6701 100644
--- a/src/main/java/org/apache/sling/feature/apiregions/impl/Activator.java
+++ b/src/main/java/org/apache/sling/feature/apiregions/impl/Activator.java
@@ -107,7 +107,7 @@ public class Activator implements BundleActivator,
FrameworkListener {
// All services automatically get unregistered by the framework.
if (configuration != null) {
- configuration.storePersistedConfiguration(context);
+ configuration.storeLocationToConfigMap(context);
}
if ( this.configAdminTracker != null ) {
this.configAdminTracker.close();
diff --git
a/src/main/java/org/apache/sling/feature/apiregions/impl/RegionConfiguration.java
b/src/main/java/org/apache/sling/feature/apiregions/impl/RegionConfiguration.java
index e375f2c..f0f25e6 100644
---
a/src/main/java/org/apache/sling/feature/apiregions/impl/RegionConfiguration.java
+++
b/src/main/java/org/apache/sling/feature/apiregions/impl/RegionConfiguration.java
@@ -46,7 +46,6 @@ import java.util.Set;
import java.util.concurrent.ConcurrentHashMap;
import java.util.concurrent.ConcurrentMap;
import java.util.logging.Level;
-import java.util.stream.Collectors;
import org.osgi.framework.BundleContext;
import org.osgi.framework.Version;
@@ -62,7 +61,6 @@ class RegionConfiguration {
final Set<String> defaultRegions;
private final Dictionary<String, Object> regProps = new Hashtable<>();
-
private final Map<String, Dictionary<String, Object>> factoryConfigs = new
ConcurrentHashMap<>();
private final Map<Map.Entry<String, Version>, List<String>> baseBsnVerMap;
@@ -70,11 +68,11 @@ class RegionConfiguration {
private final Map<String, Set<String>> baseFeatureRegionMap;
private final Map<String, Set<String>> baseRegionPackageMap;
- // This field stores the association between bundle location and features.
- // It is populated dynamically as bundles are getting resolved. If a
feature
- // cannot be found for a location, it is looked up through the other maps
in
- // this class.
- private final ConcurrentMap<String, Set<String>> bundleLocationFeatureMap =
+ // This field stores the association between bundle location and the
configuration
+ // to be used. The configuration is based on bsn+version. If the bundle is
updated
+ // the original bsn+version associated with the location still needs to be
used.
+ // It is populated dynamically as bundles are getting resolved.
+ private final ConcurrentMap<String, Map.Entry<String, Version>>
bundleLocationConfigMap =
new ConcurrentHashMap<>();
private final String toGlobalConfig;
@@ -95,7 +93,6 @@ class RegionConfiguration {
RegionConfiguration(final BundleContext context)
throws IOException, URISyntaxException {
-
URI idbsnverFile = getDataFileURI(context,
RegionConstants.IDBSNVER_FILENAME);
// Register the location as a service property for diagnostic purposes
regProps.put(RegionConstants.IDBSNVER_FILENAME,
idbsnverFile.toString());
@@ -143,11 +140,11 @@ class RegionConfiguration {
defaultRegions = Collections.emptySet();
}
- loadLocationToFeatureMap(context);
+ loadLocationToConfigMap(context);
updateConfiguration();
}
- private void loadLocationToFeatureMap(BundleContext context) {
+ private void loadLocationToConfigMap(BundleContext context) {
File file =
context.getBundle().getDataFile(BUNDLE_LOCATION_TO_FEATURE_FILE);
if (file != null && file.exists()) {
@@ -159,12 +156,33 @@ class RegionConfiguration {
}
for (String k : p.stringPropertyNames()) {
- bundleLocationFeatureMap.put(k,
stringToSetOfString(p.getProperty(k)));
+ Map.Entry<String, Version> bsnver =
parseBSNVer(p.getProperty(k));
+ if (bsnver != null) {
+ bundleLocationConfigMap.put(k, bsnver);
+ }
}
}
}
- void storePersistedConfiguration(BundleContext context) {
+ private Map.Entry<String, Version> parseBSNVer(String val) {
+ String[] bsnver = val.split("~");
+ if (bsnver.length == 2) {
+ String bsn = bsnver[0].trim();
+ Version ver = null;
+ try {
+ ver = Version.parseVersion(bsnver[1].trim());
+ } catch (Exception e) {
+ Activator.LOG.log(Level.WARNING, "Problem parsing " +
BUNDLE_LOCATION_TO_FEATURE_FILE, e);
+ }
+
+ if (ver != null) {
+ return new AbstractMap.SimpleEntry<String,Version>(bsn, ver);
+ }
+ }
+ return null;
+ }
+
+ void storeLocationToConfigMap(BundleContext context) {
File file =
context.getBundle().getDataFile(BUNDLE_LOCATION_TO_FEATURE_FILE);
if (file == null) {
Activator.LOG.warning("Cannot store " +
BUNDLE_LOCATION_TO_FEATURE_FILE
@@ -173,9 +191,8 @@ class RegionConfiguration {
}
Properties p = new Properties();
- for (Map.Entry<String, Set<String>> entry :
bundleLocationFeatureMap.entrySet()) {
- p.setProperty(entry.getKey(),
-
entry.getValue().stream().collect(Collectors.joining(",")));
+ for (Map.Entry<String, Map.Entry<String, Version>> entry :
bundleLocationConfigMap.entrySet()) {
+ p.setProperty(entry.getKey(), entry.getValue().getKey() + "~" +
entry.getValue().getValue());
}
if (p.size() > 0) {
try (OutputStream os = new BufferedOutputStream(new
FileOutputStream(file))) {
@@ -186,19 +203,6 @@ class RegionConfiguration {
}
}
- private Set<String> stringToSetOfString(String prop) {
- Set<String> res = new HashSet<>();
-
- for (String s : prop.split(",")) {
- String ts = s.trim();
- if (ts.length() > 0) {
- res.add(ts);
- }
- }
-
- return res;
- }
-
private synchronized void updateConfiguration() {
final Map<Entry<String, Version>, List<String>> bvm =
cloneMapOfLists(this.baseBsnVerMap);
final Map<String, Set<String>> bfm =
cloneMapOfSets(this.baseBundleFeatureMap);
@@ -214,43 +218,29 @@ class RegionConfiguration {
final String[] parts = val.split("=");
final String n = parts[0];
final String[] bsnver = parts[1].split("~");
- addBsnVerArtifact(bvm, bsnver[0], bsnver[1], n);
+ String bsn = bsnver[0];
+ String bver = bsnver[1];
+ addBsnVerArtifact(bvm, bsn, bver, n);
}
}
// bundle id to features
valObj = props.get(RegionConstants.PROP_bundleFeatures);
if ( valObj != null ) {
- for(final String val : convert(valObj)) {
- final String[] parts = val.split("=");
- final String n = parts[0];
- final String[] features = parts[1].split(",");
- addValuesToMap(bfm, n, Arrays.asList(features));
- }
+ handleMapConfig(valObj, bfm);
}
// feature id to regions
valObj = props.get(RegionConstants.PROP_featureRegions);
if ( valObj != null ) {
- for(final String val : convert(valObj)) {
- final String[] parts = val.split("=");
- final String n = parts[0];
- final String[] regions = parts[1].split(",");
- addValuesToMap(frm, n, Arrays.asList(regions));
- }
+ handleMapConfig(valObj, frm);
}
// region to packages
valObj = props.get(RegionConstants.PROP_regionPackage);
if ( valObj != null ) {
- for(final String val : convert(valObj)) {
- final String[] parts = val.split("=");
- final String n = parts[0];
- final String[] packages = parts[1].split(",");
- addValuesToMap(rpm, n, Arrays.asList(packages));
- }
+ handleMapConfig(valObj, rpm);
}
-
}
// join regions
@@ -263,7 +253,15 @@ class RegionConfiguration {
bundleFeatureMap = unmodifiableMapToSet(bfm);
featureRegionMap = unmodifiableMapToSet(frm);
regionPackageMap = unmodifiableMapToSet(rpm);
+ }
+ private void handleMapConfig(Object valObj, Map<String, Set<String>> map) {
+ for(final String val : convert(valObj)) {
+ final String[] parts = val.split("=");
+ final String n = parts[0];
+ final String[] features = parts[1].split(",");
+ addValuesToMap(map, n, Arrays.asList(features));
+ }
}
private static <K,V> Map<K, List<V>> cloneMapOfLists(Map<K, List<V>> m) {
@@ -334,7 +332,8 @@ class RegionConfiguration {
l = new ArrayList<>();
bsnVerMap.put(bsnVer, l);
}
- l.add(artifactId);
+ if (!l.contains(artifactId))
+ l.add(artifactId);
}
private static Map<String, Set<String>> populateBundleFeatureMap(URI
bundlesFile) throws IOException {
@@ -409,13 +408,20 @@ class RegionConfiguration {
/**
* Obtain a mutable concurrent map that contains an association between
- * bundle location and features. This map is persisted in bundle private
+ * bundle location and their configuration, which is the original
+ * Symbolic Name and Version. This map is persisted in bundle private
* storage. Initially this map is empty but as more bundles get resolved
- * this map is gradually filled in.
- * @return The bundle location to features map.
+ * this map is gradually filled in. <p>
+ *
+ * This map is used to find the API Regions information for a bundle even
+ * if the bundle is updated at some point. After the update the location
+ * will still be the same, so the original configuration can be found by
+ * looking up the original bsn+version associated with the location. That's
+ * what this method enables.
+ * @return The bundle location to bsnver map.
*/
- public ConcurrentMap<String, Set<String>> getBundleLocationFeatureMap() {
- return bundleLocationFeatureMap;
+ public ConcurrentMap<String, Map.Entry<String, Version>>
getBundleLocationConfigMap() {
+ return bundleLocationConfigMap;
}
public Map<String, Set<String>> getBundleFeatureMap() {
diff --git
a/src/main/java/org/apache/sling/feature/apiregions/impl/ResolverHookImpl.java
b/src/main/java/org/apache/sling/feature/apiregions/impl/ResolverHookImpl.java
index da2e562..9dbeccf 100644
---
a/src/main/java/org/apache/sling/feature/apiregions/impl/ResolverHookImpl.java
+++
b/src/main/java/org/apache/sling/feature/apiregions/impl/ResolverHookImpl.java
@@ -242,14 +242,18 @@ class ResolverHookImpl implements ResolverHook {
}
Set<String> getFeaturesForBundle(Bundle bundle) {
- return this.configuration.getBundleLocationFeatureMap()
- .computeIfAbsent(bundle.getLocation(), l ->
getFeaturesForBundleFromConfig(bundle));
+ // Look up the bsn and bundle version initially associated with the
location. If the bundle
+ // for the specified location was later updated, the initial
bsn+version is still used to look up the
+ // api regions configuration
+ Map.Entry<String, Version> bsnVer =
this.configuration.getBundleLocationConfigMap()
+ .computeIfAbsent(bundle.getLocation(), l -> new
AbstractMap.SimpleEntry<>(
+ bundle.getSymbolicName(), bundle.getVersion()));
+
+ return getFeaturesForBundleFromConfig(bsnVer.getKey(),
bsnVer.getValue());
}
- private Set<String> getFeaturesForBundleFromConfig(Bundle bundle) {
+ private Set<String> getFeaturesForBundleFromConfig(String bundleName,
Version bundleVersion) {
Set<String> newSet = new HashSet<>();
- String bundleName = bundle.getSymbolicName();
- Version bundleVersion = bundle.getVersion();
List<String> aids = this.configuration.getBsnVerMap().get(
new AbstractMap.SimpleEntry<String, Version>(bundleName,
bundleVersion));
if (aids != null) {
diff --git
a/src/test/java/org/apache/sling/feature/apiregions/impl/RegionConfigurationTest.java
b/src/test/java/org/apache/sling/feature/apiregions/impl/RegionConfigurationTest.java
index 9d2f4d5..e9b0468 100644
---
a/src/test/java/org/apache/sling/feature/apiregions/impl/RegionConfigurationTest.java
+++
b/src/test/java/org/apache/sling/feature/apiregions/impl/RegionConfigurationTest.java
@@ -52,6 +52,10 @@ import org.osgi.framework.Bundle;
import org.osgi.framework.BundleContext;
import org.osgi.framework.Version;
import org.osgi.framework.hooks.resolver.ResolverHook;
+import org.osgi.framework.namespace.PackageNamespace;
+import org.osgi.framework.wiring.BundleCapability;
+import org.osgi.framework.wiring.BundleRequirement;
+import org.osgi.framework.wiring.BundleRevision;
public class RegionConfigurationTest {
@Test
@@ -405,19 +409,125 @@ public class RegionConfigurationTest {
RegionConfiguration cfg = new RegionConfiguration(ctx);
- ConcurrentMap<String, Set<String>> m =
cfg.getBundleLocationFeatureMap();
- m.put("foo://bar", Collections.singleton("blah"));
- m.put("foo://tar", new HashSet<>(Arrays.asList("a", "b", "c")));
- cfg.storePersistedConfiguration(ctx);
+ ConcurrentMap<String, Map.Entry<String, Version>> m =
cfg.getBundleLocationConfigMap();
+ m.put("foo://bar", new AbstractMap.SimpleEntry<>("blah", new
Version(1,0,0,"suffix")));
+ m.put("foo://tar", new AbstractMap.SimpleEntry<>("a.b.c", new
Version(9,8,7)));
+ cfg.storeLocationToConfigMap(ctx);
RegionConfiguration cfg2 = new RegionConfiguration(ctx);
- ConcurrentMap<String, Set<String>> m2 =
cfg2.getBundleLocationFeatureMap();
+ ConcurrentMap<String, Map.Entry<String, Version>> m2 =
cfg2.getBundleLocationConfigMap();
assertEquals(m, m2);
} finally {
f.delete();
}
}
+ @Test
+ public void testUpdateLocationCacheOnConfigUpdate() throws Exception {
+ // Set up a typical configuration scenario
+
+ BundleContext ctx = Mockito.mock(BundleContext.class);
+ Mockito.when(ctx.getBundle()).thenReturn(Mockito.mock(Bundle.class));
+ Mockito.when(ctx.getProperty(PROPERTIES_FILE_LOCATION)).
+ thenReturn("classloader://props4");
+ RegionConfiguration cfg = new RegionConfiguration(ctx);
+
+ assertEquals(2, cfg.getBsnVerMap().size());
+ assertEquals(Collections.singletonList("g:b1:1"),
+ cfg.getBsnVerMap().get(new AbstractMap.SimpleEntry<>("b1", new
Version(1,0,0))));
+ assertEquals(Collections.singletonList("g:b2:1.2.3"),
+ cfg.getBsnVerMap().get(new AbstractMap.SimpleEntry<>("b2", new
Version(1,2,3))));
+
+ // Now start invoking the resolver hook, this should fill in some of
the location cache
+ assertEquals("Precondition", 0,
cfg.getBundleLocationConfigMap().size());
+ ResolverHookImpl rhi = new ResolverHookImpl(cfg);
+
+ BundleRequirement req = mockRequirement("b4", new
Version(9,9,9,"something"), ctx);
+ BundleCapability cap1 = mockCapability("b1", new Version(1,0,0), ctx);
+ BundleCapability cap2 = mockCapability("b2", new Version(1,2,3), ctx);
+
+ ArrayList<BundleCapability> caps1 = new
ArrayList<>(Arrays.asList(cap1, cap2));
+ rhi.filterMatches(req, caps1);
+ assertEquals("caps should be empty it b4 doesn't have regions
configuration yet",
+ Collections.emptyList(), caps1);
+
+ // At this point the bundle location to feature map should have some
cached info
+ assertEquals(3, cfg.getBundleLocationConfigMap().size());
+ assertEquals(new AbstractMap.SimpleEntry<>("b1", new Version(1,0,0)),
+ cfg.getBundleLocationConfigMap().get("http://b1";));
+ assertEquals(new AbstractMap.SimpleEntry<>("b2", new Version(1,2,3)),
+ cfg.getBundleLocationConfigMap().get("http://b2";));
+ assertEquals(new AbstractMap.SimpleEntry<>("b4", new
Version(9,9,9,"something")),
+ cfg.getBundleLocationConfigMap().get("http://b4";));
+
+
+ // Now update the configuration, there is new mappings for bundles b3
and b4 and
+ // the features of b2 have changed
+ Dictionary<String,Object> d = new Hashtable<>();
+ d.put("mapping.bundleid.bsnver",
+ new String[] {
+ "g:b3:1.2=a.b3.c~1.2",
+ "g:b4:9.9.9.something=b4~9.9.9.something"});
+ d.put("mapping.bundleid.features",
+ new String[] {
+ "g:b4:9.9.9.something=org.sling:something:1.2.3"
+ });
+
+ cfg.setConfig("my.factory.pid", d);
+ assertEquals(4, cfg.getBsnVerMap().size());
+ assertEquals(Collections.singletonList("g:b1:1"),
+ cfg.getBsnVerMap().get(new AbstractMap.SimpleEntry<>("b1", new
Version(1,0,0))));
+ assertEquals(Collections.singletonList("g:b2:1.2.3"),
+ cfg.getBsnVerMap().get(new AbstractMap.SimpleEntry<>("b2", new
Version(1,2,3))));
+ assertEquals(Collections.singletonList("g:b3:1.2"),
+ cfg.getBsnVerMap().get(new AbstractMap.SimpleEntry<>("a.b3.c",
new Version(1,2,0))));
+ assertEquals(Collections.singletonList("g:b4:9.9.9.something"),
+ cfg.getBsnVerMap().get(new AbstractMap.SimpleEntry<>("b4", new
Version(9,9,9,"something"))));
+
+ // Redo a resolve action and check that the cache is now re-filled
+ ArrayList<BundleCapability> caps2 = new
ArrayList<>(Arrays.asList(cap1, cap2));
+ rhi.filterMatches(req, caps2);
+
+ ArrayList<BundleCapability> expected = new
ArrayList<>(Arrays.asList(cap1, cap2));
+ assertEquals("Now b4 from the req should have visibility to both caps
as b4 is now in the feature",
+ expected, caps2);
+ }
+
+ private BundleRequirement mockRequirement(String bsn, Version bver,
BundleContext mockContext) {
+ BundleRevision br = mockBundleRevision(bsn, bver, mockContext);
+
+ BundleRequirement req = Mockito.mock(BundleRequirement.class);
+ Mockito.when(req.getRevision()).thenReturn(br);
+
Mockito.when(req.getNamespace()).thenReturn(PackageNamespace.PACKAGE_NAMESPACE);
+ return req;
+ }
+
+ private BundleCapability mockCapability(String bsn, Version bver,
BundleContext mockContext) {
+ BundleRevision br = mockBundleRevision(bsn, bver, mockContext);
+
+ BundleCapability req = Mockito.mock(BundleCapability.class);
+ Mockito.when(req.getRevision()).thenReturn(br);
+
Mockito.when(req.getNamespace()).thenReturn(PackageNamespace.PACKAGE_NAMESPACE);
+ return req;
+ }
+
+ private BundleRevision mockBundleRevision(String bsn, Version bver,
BundleContext mockContext) {
+ Bundle b = Mockito.mock(Bundle.class);
+ Mockito.when(b.getSymbolicName()).thenReturn(bsn);
+ Mockito.when(b.getVersion()).thenReturn(bver);
+ String bundleLocation = "http://"; + bsn;
+ Mockito.when(b.getLocation()).thenReturn(bundleLocation);
+ Mockito.when(b.getBundleId()).thenReturn(System.currentTimeMillis());
// Just some random unique ID
+
+ BundleRevision br = Mockito.mock(BundleRevision.class);
+ Mockito.when(br.getBundle()).thenReturn(b);
+
+ // Make the bundlecontext aware...
+ Mockito.when(mockContext.getBundle(bundleLocation)).thenReturn(b);
+
+ return br;
+ }
+
private void testDefaultRegions(String defProp, Set<String> expected)
throws IOException, URISyntaxException, NoSuchFieldException,
IllegalAccessException {
BundleContext ctx = Mockito.mock(BundleContext.class);
diff --git a/src/test/resources/props4/bundles.properties
b/src/test/resources/props4/bundles.properties
new file mode 100644
index 0000000..f5de1bb
--- /dev/null
+++ b/src/test/resources/props4/bundles.properties
@@ -0,0 +1,4 @@
+#Generated at Sat Nov 03 10:58:58 GMT 2018
+#Sat Nov 03 10:58:58 GMT 2018
+g\:b2\:1.2.3=org.sling\:something\:1.2.3
+g\:b1\:1=org.sling\:something\:1.2.3
\ No newline at end of file
diff --git a/src/test/resources/props4/features.properties
b/src/test/resources/props4/features.properties
new file mode 100644
index 0000000..9a47d1a
--- /dev/null
+++ b/src/test/resources/props4/features.properties
@@ -0,0 +1,3 @@
+#Generated at Sat Nov 03 11:10:29 GMT 2018
+#Sat Nov 03 11:10:29 GMT 2018
+org.sling\:something\:1.2.3=internal,global
diff --git a/src/test/resources/props4/idbsnver.properties
b/src/test/resources/props4/idbsnver.properties
new file mode 100644
index 0000000..0fd6539
--- /dev/null
+++ b/src/test/resources/props4/idbsnver.properties
@@ -0,0 +1,5 @@
+#Generated at Sat Nov 03 10:26:37 GMT 2018
+#Sat Nov 03 10:26:37 GMT 2018
+g\:b2\:1.2.3=b2~1.2.3
+g\:b1\:1=b1~1.0.0
+
diff --git a/src/test/resources/props4/regions.properties
b/src/test/resources/props4/regions.properties
new file mode 100644
index 0000000..a4982d7
--- /dev/null
+++ b/src/test/resources/props4/regions.properties
@@ -0,0 +1,4 @@
+#Generated at Sat Nov 03 11:10:29 GMT 2018
+#Sat Nov 03 11:10:29 GMT 2018
+internal=xyz
+global=d.e.f,test,a.b.c
