Merge pull request #11 from bosschaert/SLING-9501
SLING-9501 Packages provided by a non-api-region enabled feature should be on par with the global region
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 d6a1881..fe5d634 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
@@ -63,21 +63,35 @@
if (!PackageNamespace.PACKAGE_NAMESPACE.equals(requirement.getNamespace()))
return;
+ if (candidates.isEmpty())
+ return;
+
+ Object pkg = candidates.iterator().next().getAttributes().get(PackageNamespace.PACKAGE_NAMESPACE);
+ if (!(pkg instanceof String)) {
+ return;
+ }
+ String packageName = (String) pkg;
+
Bundle reqBundle = requirement.getRevision().getBundle();
long reqBundleID = reqBundle.getBundleId();
- Set<String> reqRegions = new HashSet<>(this.configuration.getDefaultRegions());
+ Set<String> bareReqRegions = null; // Null means: not opting into API Regions
Set<String> reqFeatures = getFeaturesForBundle(reqBundle);
for (String feature : reqFeatures) {
Set<String> fr = this.configuration.getFeatureRegionMap().get(feature);
if (fr != null) {
- reqRegions.addAll(fr);
+ if (bareReqRegions == null)
+ bareReqRegions = new HashSet<>();
+ bareReqRegions.addAll(fr);
}
}
+ Set<String> reqRegions = new HashSet<>(this.configuration.getDefaultRegions());
+ if (bareReqRegions != null)
+ reqRegions.addAll(bareReqRegions);
Map<BundleCapability, String> coveredCaps = new HashMap<>();
Map<BundleCapability, String> bcFeatureMap = new HashMap<>();
- String packageName = null;
+
nextCapability:
for (BundleCapability bc : candidates) {
BundleRevision rev = bc.getRevision();
@@ -86,14 +100,19 @@
long capBundleID = capBundle.getBundleId();
if (capBundleID == 0) {
// always allow capability from the system bundle
- coveredCaps.put(bc, null); // null value means same bundle, same feature or system bundlee
+ coveredCaps.put(bc, null); // null value means same bundle, same feature or system bundle
continue nextCapability;
}
if (capBundleID == reqBundleID) {
// always allow capability from same bundle
- coveredCaps.put(bc, null); // null value means same bundle, same feature or system bundle
- continue nextCapability;
+
+ // Here we cover the case where the bundle is not in any feature which means that the package is in the 'global' region,
+ // however if the bundle is in a feature then it could be marked as more specific, with a 'null value', which may
+ // happen below.
+ coveredCaps.put(bc, RegionConstants.GLOBAL_REGION);
+
+ // note: don't continue to nextCapability here, this one may be overwritten later...
}
Set<String> capFeatures = getFeaturesForBundle(capBundle);
@@ -106,7 +125,9 @@
for (String capFeat : capFeatures) {
if (reqFeatures.contains(capFeat)) {
// Within a single feature everything can wire to everything else
- coveredCaps.put(bc, null); // null value means same bundle, same feature or system bundle
+
+ // null value means same bundle, same feature or system bundle, but if exported into global region, use 'global' instead
+ coveredCaps.put(bc, isInGlobalRegion(packageName, capFeat) ? RegionConstants.GLOBAL_REGION : null);
continue nextCapability;
}
@@ -121,32 +142,27 @@
List<String> sharedRegions = new ArrayList<>(reqRegions);
sharedRegions.retainAll(capRegions);
- Object pkg = bc.getAttributes().get(PackageNamespace.PACKAGE_NAMESPACE);
- if (pkg instanceof String) {
- packageName = (String) pkg;
-
- // Look at specific regions first as they take precedence over the global region
- for (String region : sharedRegions) {
- Set<String> regionPackages = this.configuration.getRegionPackageMap().get(region);
- if (regionPackages != null && regionPackages.contains(packageName)) {
- // If the export is in a region that the feature is also in, then allow
- coveredCaps.put(bc, region);
- continue nextCapability;
- }
- }
-
- // Now check the global region
- Set<String> globalPackages = this.configuration.getRegionPackageMap().get(RegionConstants.GLOBAL_REGION);
- if (globalPackages != null && globalPackages.contains(packageName)) {
- // If the export is in the global region everyone can access
- coveredCaps.put(bc, RegionConstants.GLOBAL_REGION);
+ // Look at specific regions first as they take precedence over the global region
+ for (String region : sharedRegions) {
+ Set<String> regionPackages = this.configuration.getRegionPackageMap().get(region);
+ if (regionPackages != null && regionPackages.contains(packageName)) {
+ // If the export is in a region that the feature is also in, then allow
+ coveredCaps.put(bc, region);
continue nextCapability;
}
}
+
+ // Now check the global region
+ Set<String> globalPackages = this.configuration.getRegionPackageMap().get(RegionConstants.GLOBAL_REGION);
+ if (globalPackages != null && globalPackages.contains(packageName)) {
+ // If the export is in the global region everyone can access
+ coveredCaps.put(bc, RegionConstants.GLOBAL_REGION);
+ continue nextCapability;
+ }
}
}
- pruneCoveredCaps(reqRegions, coveredCaps);
+ pruneCoveredCaps(bareReqRegions, coveredCaps);
List<BundleCapability> removedCandidates = new ArrayList<>(candidates);
// Remove any capabilities that are not covered
@@ -184,15 +200,41 @@
}
}
- /*
+ /**
+ * Check if the package is exported in the global region
+ * @param packageName The package
+ * @param capFeat The feature where it is found
+ * @return If the feature exports to the global region and the package is exported into the global region
+ */
+ private boolean isInGlobalRegion(String packageName, String capFeat) {
+ Set<String> capRegions = this.configuration.getFeatureRegionMap().get(capFeat);
+ if (capRegions != null && capRegions.contains(RegionConstants.GLOBAL_REGION)) {
+ Set<String> globalPackages = this.configuration.getRegionPackageMap().get(RegionConstants.GLOBAL_REGION);
+ if (globalPackages.contains(packageName)) {
+ return true;
+ }
+ }
+ return false;
+ }
+
+ /**
* If there are multiple choices of capabilities and some of the capabilities are in the global
- * region while others are in another named region, take out the capabilities from the global
- * region so that the requirement gets wired to the more 'specifc' one than the global one.
- * Capabilities from bundle 0 (the system bundle), the same bundle as the requirer and from the
- * same feature as the requirer should always be kept. These are marked in the capMap with a
- * {@code null} region value.
+ * region while others are in another named region or in a feature-private region, take out the
+ * capabilities from the global region so that the requirement gets wired to the more 'specific'
+ * one than the global one.
+ * Capabilities that are considered specific but not in a named region are marked in the CapMap
+ * with a {@code null} region value.
+ *
+ * @param reqRegions The regions declared in the requiring feature. If {@code null} is passed in
+ * the requirement did not opt into the API Regions
+ * @param capMap The map of capabilities to regions where they are found in
*/
private void pruneCoveredCaps(Set<String> reqRegions, Map<BundleCapability,String> capMap) {
+ if (reqRegions == null) {
+ // No regions (other than global) for the requirement: do nothing
+ return;
+ }
+
Set<String> reqNonGlobalRegions = new HashSet<>(reqRegions);
reqNonGlobalRegions.remove(RegionConstants.GLOBAL_REGION);
@@ -201,12 +243,6 @@
return;
}
- if (reqRegions.size() == 0
- || Collections.singleton(RegionConstants.GLOBAL_REGION).equals(reqRegions)) {
- // No regions (other than global) for the requirement: do nothing
- return;
- }
-
List<BundleCapability> specificCaps = new ArrayList<>();
for (Iterator<Map.Entry<BundleCapability,String>> it = capMap.entrySet().iterator(); it.hasNext(); ) {
Map.Entry<BundleCapability,String> entry = it.next();
@@ -236,7 +272,7 @@
BundleCapability cap = it.next();
if (!specificCaps.contains(cap)) {
it.remove();
- Activator.LOG.log(Level.INFO, "Removing candidate {0} which is in region {1} as more specific candidates are available in regions {2}",
+ Activator.LOG.log(Level.INFO, "Removing candidate {0} which is in region {1} as more specific candidate(s) are available in regions {2}",
new Object[] {
cap, capMap.get(cap),
specificCaps.stream().map(c -> "" + c + " region " + capMap.get(c)).collect(Collectors.joining("/"))
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 e9b0468..714eba9 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
@@ -443,8 +443,8 @@
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);
+ BundleCapability cap1 = mockCapability("org.foo.bar", "b1", new Version(1,0,0), ctx);
+ BundleCapability cap2 = mockCapability("org.foo.bar", "b2", new Version(1,2,3), ctx);
ArrayList<BundleCapability> caps1 = new ArrayList<>(Arrays.asList(cap1, cap2));
rhi.filterMatches(req, caps1);
@@ -502,13 +502,17 @@
return req;
}
- private BundleCapability mockCapability(String bsn, Version bver, BundleContext mockContext) {
+ private BundleCapability mockCapability(String pkg, String bsn, Version bver, BundleContext mockContext) {
+ Map<String, Object> attrs =
+ Collections.<String, Object>singletonMap(PackageNamespace.PACKAGE_NAMESPACE, pkg);
+
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;
+ BundleCapability cap = Mockito.mock(BundleCapability.class);
+ Mockito.when(cap.getRevision()).thenReturn(br);
+ Mockito.when(cap.getAttributes()).thenReturn(attrs);
+ Mockito.when(cap.getNamespace()).thenReturn(PackageNamespace.PACKAGE_NAMESPACE);
+ return cap;
}
private BundleRevision mockBundleRevision(String bsn, Version bver, BundleContext mockContext) {
diff --git a/src/test/java/org/apache/sling/feature/apiregions/impl/ResolverHookImplTest.java b/src/test/java/org/apache/sling/feature/apiregions/impl/ResolverHookImplTest.java
index 2525030..bf9d5fb 100644
--- a/src/test/java/org/apache/sling/feature/apiregions/impl/ResolverHookImplTest.java
+++ b/src/test/java/org/apache/sling/feature/apiregions/impl/ResolverHookImplTest.java
@@ -205,6 +205,35 @@
}
@Test
+ public void testOwnBundleInRegionHasPrecedenceOverGlobal() {
+ Map<Entry<String, Version>, List<String>> bsnvermap = new HashMap<>();
+ bsnvermap.put(new AbstractMap.SimpleEntry<String,Version>(
+ "providing.bundle.infeature", new Version(1,0,0)), Collections.singletonList("b1"));
+ bsnvermap.put(new AbstractMap.SimpleEntry<String,Version>(
+ "providing.bundle.inglobal", new Version(1,0,0)), Collections.singletonList("b2"));
+
+ Map<String, Set<String>> bfmap = new HashMap<>();
+ bfmap.put("b1", Collections.singleton("f1"));
+
+ Map<String, Set<String>> frmap = new HashMap<>();
+ frmap.put("f1", Collections.emptySet());
+
+ Map<String, Set<String>> rpmap = new HashMap<>();
+
+ ResolverHookImpl rh = new ResolverHookImpl(new RegionConfiguration(bsnvermap, bfmap, frmap, rpmap, Collections.singleton("global")));
+
+ BundleRequirement req1 = mockRequirement("b1", bsnvermap);
+ BundleCapability cap1 = mockCapability("org.foo.bar", "b1", bsnvermap);
+ BundleCapability cap2 = mockCapability("org.foo.bar", "b2", bsnvermap);
+ List<BundleCapability> candidates = new ArrayList<>(Arrays.asList(cap1, cap2));
+ rh.filterMatches(req1, candidates);
+
+ assertEquals("Only the candidate from b1 should be selected, even through b2 is in the global region, "
+ + "because b1 is not exported and as such more specific",
+ Collections.singletonList(cap1), candidates);
+ }
+
+ @Test
public void testOwnFeatureHasPrecendenceOverGlobal() {
Map<Entry<String, Version>, List<String>> bsnvermap = new HashMap<>();
bsnvermap.put(new AbstractMap.SimpleEntry<String,Version>(
@@ -241,6 +270,44 @@
}
@Test
+ public void testGlobalAndNonAPIRegionsFeatureAreEqual() {
+ Map<Entry<String, Version>, List<String>> bsnvermap = new HashMap<>();
+
+ bsnvermap.put(new AbstractMap.SimpleEntry<String,Version>(
+ "providing.bundle.infeature", new Version(1,0,0)), Collections.singletonList("b101"));
+ bsnvermap.put(new AbstractMap.SimpleEntry<String,Version>(
+ "providing.bundle.inglobal", new Version(1,0,0)), Collections.singletonList("b102"));
+ bsnvermap.put(new AbstractMap.SimpleEntry<String,Version>(
+ "requiring.bundle", new Version(1,0,0)), Collections.singletonList("b99"));
+
+ Map<String, Set<String>> bfmap = new HashMap<>();
+ bfmap.put("b99", Collections.singleton("f1"));
+ bfmap.put("b101", Collections.singleton("f1"));
+
+ Map<String, Set<String>> frmap = new HashMap<>();
+ frmap.put("f1", new HashSet<>(Arrays.asList("r1", "global")));
+
+ Map<String, Set<String>> rpmap = new HashMap<>();
+ rpmap.put("r1", Collections.singleton("org.blah.blah"));
+ rpmap.put(RegionConstants.GLOBAL_REGION, Collections.singleton("org.foo.bar"));
+
+ ResolverHookImpl rh = new ResolverHookImpl(new RegionConfiguration(bsnvermap, bfmap, frmap, rpmap, Collections.singleton("global")));
+
+ // b2 needs to resolve 'org.foo.bar' and there are 2 candidates:
+ // b101 is in the same feature as b2
+ // b102 is in the no feature
+ // In this case both should still be available as both are in the global region
+ BundleRequirement req1 = mockRequirement("b99", bsnvermap);
+ BundleCapability cap1 = mockCapability("org.foo.bar", "b101", bsnvermap);
+ BundleCapability cap2 = mockCapability("org.foo.bar", "b102", bsnvermap);
+ List<BundleCapability> candidates = new ArrayList<>(Arrays.asList(cap1, cap2));
+ Set<BundleCapability> orgCandidates = new HashSet<>(candidates);
+ rh.filterMatches(req1, candidates);
+
+ assertEquals(orgCandidates, new HashSet<>(candidates));
+ }
+
+ @Test
public void testMultipleGlobalOptionsOnly() {
Map<Entry<String, Version>, List<String>> bsnvermap = new HashMap<>();
bsnvermap.put(new AbstractMap.SimpleEntry<String,Version>(
@@ -661,6 +728,7 @@
Mockito.when(br.getBundle()).thenReturn(bundle);
BundleCapability cap = Mockito.mock(BundleCapability.class);
+ Mockito.when(cap.getNamespace()).thenReturn(PackageNamespace.PACKAGE_NAMESPACE);
Mockito.when(cap.getAttributes()).thenReturn(attrs);
Mockito.when(cap.getRevision()).thenReturn(br);
return cap;
