Merge pull request #14 from bosschaert/SLING-9937
SLING-9937 api-regions-crossfeature-dups should work if all exports come from API Regions
diff --git a/README.md b/README.md
index ad05a8c..10b567a 100644
--- a/README.md
+++ b/README.md
@@ -36,10 +36,8 @@
* `order`: A comma separated list of the region names declaring the order in which they should be found. Not all regions declared must be present, but if they are present this
order must be obeyed.
-* `api-regions-crossfeature-dups`: This analyser checks whether there are exported packages in a feature model
-that does _not_ opt in to the API Regions (i.e. it does not have an API-Regions section) that overlap with exported
-packages from API regions in other feature models. It can prevent against unwanted results when packages are
-exported from the outside which should be exported from an API Region.
+* `api-regions-crossfeature-dups`: This analyser checks whether the same package is exported
+into the same API Region from multiple features. It can prevent against unwanted results when packages are exported by a bundle in a platform feature in an API Region such as `global` as well as by a non-platform bundle.
This analyser only provides a useful result when run on
an aggregate feature model, i.e. a feature model that was created by aggregating a number of other feature models. It uses the
`feature-origins` metadata to find the features that bundles were initially declared in. It then matches this with the `feature-origins` found in the `api-regions` section. Exports from bundles from features that don't
@@ -47,6 +45,11 @@
is reported.
* Configuration parameters:
* `regions`: a comma separated list of regions to check. If not specified all regions found are checked. This configuration item can be used to exclude certain regions from the check.
+ * `definingFeatures`: comma separated list the features IDs that are allowed to define the API. If overlapping exports
+are done into the selected regions from other features this will cause an error. The suffix `*` is
+supported as a wildcard at the end of the feature ID. If this configuration is not specified, the list of defining features
+is built up from all features that opt-in to the API regions and ones that
+don't opt-in are assumed to be non-defining.
* `warningPackages`: if packages listed here are found to overlap, a warning instead of an error is reported. Supports either literal package names (e.g. `javax.servlet`) or wildcards with an asterisk at the end (e.g. `javax.*`).
* `ignoredPackages`: packages listed here are completely ignored in the analysis. Supports literal package names or wildcards with an asterisk at the end.
diff --git a/src/main/java/org/apache/sling/feature/extension/apiregions/analyser/CheckApiRegionsCrossFeatureDups.java b/src/main/java/org/apache/sling/feature/extension/apiregions/analyser/CheckApiRegionsCrossFeatureDups.java
index 4c0c8af..50c2bf9 100644
--- a/src/main/java/org/apache/sling/feature/extension/apiregions/analyser/CheckApiRegionsCrossFeatureDups.java
+++ b/src/main/java/org/apache/sling/feature/extension/apiregions/analyser/CheckApiRegionsCrossFeatureDups.java
@@ -30,6 +30,7 @@
import java.util.Collections;
import java.util.HashMap;
import java.util.HashSet;
+import java.util.Iterator;
import java.util.List;
import java.util.Map;
import java.util.Set;
@@ -52,10 +53,11 @@
Set<String> checkedRegions = splitListConfig(ctx.getConfiguration().get("regions"));
Set<String> ignoredPackages = splitListConfig(ctx.getConfiguration().get("ignoredPackages"));
Set<String> warningPackages = splitListConfig(ctx.getConfiguration().get("warningPackages"));
+ Set<String> definingFeatures = splitListConfig(ctx.getConfiguration().get("definingFeatures"));
Map<String, Set<String>> regionExports = new HashMap<>();
+ Set<ArtifactId> apiRegionsFeatures = new HashSet<>();
- List<ArtifactId> apiRegionsFeatures = new ArrayList<>();
for (ApiRegion r : apiRegions.listRegions()) {
apiRegionsFeatures.addAll(Arrays.asList(r.getFeatureOrigins()));
if (checkedRegions.isEmpty() || checkedRegions.contains(r.getName())) {
@@ -68,13 +70,19 @@
}
}
+ if (definingFeatures.isEmpty()) {
+ definingFeatures = apiRegionsFeatures
+ .stream()
+ .map(ArtifactId::toMvnId)
+ .collect(Collectors.toSet());
+ }
+
FeatureDescriptor f = ctx.getFeatureDescriptor();
for (BundleDescriptor bd : f.getBundleDescriptors()) {
List<ArtifactId> borgs = new ArrayList<>(Arrays.asList(bd.getArtifact().getFeatureOrigins()));
- borgs.removeAll(apiRegionsFeatures);
+ removeDefiningFeatures(definingFeatures, borgs);
if (!borgs.isEmpty()) {
- // This bundle was contributed by a feature that did not opt-in to the API Regions
Set<String> reportedPackages = new HashSet<>();
for (PackageInfo pi : bd.getExportedPackages()) {
String pkgName = pi.getName();
@@ -84,11 +92,16 @@
continue;
}
+ if (allOtherExportersNonDefining(pi, f, definingFeatures)) {
+ // If all exports are done by non-defining features then that's ok
+ continue;
+ }
+
reportedPackages.add(pi.getName());
String msg = "Package overlap found between region " + entry.getKey()
+ " and bundle " + bd.getBundleSymbolicName() + " " + bd.getBundleVersion()
- + " which comes from a feature without API Regions: " + borgs
+ + " which comes from feature: " + borgs
+ ". Both export package: " + pi.getName();
if (matchesSet(pkgName, warningPackages)) {
ctx.reportArtifactWarning(bd.getArtifact().getId(), msg);
@@ -102,6 +115,51 @@
}
}
+ // Check if all exports of this package are done by non-defining features
+ private boolean allOtherExportersNonDefining(PackageInfo pi, FeatureDescriptor f, Set<String> definingFeatures) {
+ List<ArtifactId> declaringFeatures = new ArrayList<>();
+
+ for (BundleDescriptor bd : f.getBundleDescriptors()) {
+ if (bd.getExportedPackages().contains(pi)) {
+ declaringFeatures.addAll(Arrays.asList(bd.getArtifact().getFeatureOrigins(f.getFeature().getId())));
+ }
+ }
+
+ for (ArtifactId feature : declaringFeatures) {
+ for (String definingFeature : definingFeatures) {
+ if (definingFeature.endsWith("*")) {
+ String prefix = definingFeature.substring(0, definingFeature.length() - 1);
+ if (feature.toMvnId().startsWith(prefix)) {
+ return false;
+ }
+ } else {
+ if (feature.toMvnId().equals(definingFeature)) {
+ return false;
+ }
+ }
+ }
+ }
+ return true;
+ }
+
+ private void removeDefiningFeatures(Set<String> definingFeatures, List<ArtifactId> features) {
+ for (Iterator<ArtifactId> it = features.iterator(); it.hasNext(); ) {
+ ArtifactId feature = it.next();
+ for (String definingFeature : definingFeatures) {
+ if (definingFeature.endsWith("*")) {
+ String prefix = definingFeature.substring(0, definingFeature.length() - 1);
+ if (feature.toMvnId().startsWith(prefix)) {
+ it.remove();
+ }
+ } else {
+ if (feature.toMvnId().equals(definingFeature)) {
+ it.remove();
+ }
+ }
+ }
+ }
+ }
+
private boolean matchesSet(String pkg, Set<String> set) {
for (String e : set) {
if (e.endsWith("*")) {
diff --git a/src/test/java/org/apache/sling/feature/extension/apiregions/analyser/CheckApiRegionsCrossFeatureDupsTest.java b/src/test/java/org/apache/sling/feature/extension/apiregions/analyser/CheckApiRegionsCrossFeatureDupsTest.java
index 1d8652e..1785daa 100644
--- a/src/test/java/org/apache/sling/feature/extension/apiregions/analyser/CheckApiRegionsCrossFeatureDupsTest.java
+++ b/src/test/java/org/apache/sling/feature/extension/apiregions/analyser/CheckApiRegionsCrossFeatureDupsTest.java
@@ -193,6 +193,73 @@
assertTrue(err1.getValue().contains("zzz.zzz"));
}
+ @Test
+ public void testDefiningFeatures() throws Exception {
+ Path fp = new File(getClass().getResource("/crossfeatdups/fm2.json").getFile()).toPath();
+ String fm = new String(Files.readAllBytes(fp));
+
+ Feature f = FeatureJSONReader.read(new StringReader(fm), null);
+
+ Scanner scanner = getScanner3();
+ AnalyserTask at = new CheckApiRegionsCrossFeatureDups();
+ Map<String, Map<String,String>> configs =
+ Collections.singletonMap("api-regions-crossfeature-dups",
+ Collections.singletonMap("definingFeatures", "g:f1:1"));
+ Analyser a = new Analyser(scanner, configs, at);
+ AnalyserResult res = a.analyse(f);
+
+ assertEquals(1, res.getErrors().size());
+ assertEquals(0, res.getWarnings().size());
+
+ String err = res.getErrors().get(0);
+ assertTrue(err.contains("org.foo.bar.bingo"));
+ assertTrue(err.contains("g:f2:1"));
+ assertTrue(err.contains("feature-export"));
+ }
+
+ @Test
+ public void testDefiningFeaturesRegex() throws Exception {
+ Path fp = new File(getClass().getResource("/crossfeatdups/fm2.json").getFile()).toPath();
+ String fm = new String(Files.readAllBytes(fp));
+
+ Feature f = FeatureJSONReader.read(new StringReader(fm), null);
+
+ Scanner scanner = getScanner3();
+ AnalyserTask at = new CheckApiRegionsCrossFeatureDups();
+ Map<String, Map<String,String>> configs =
+ Collections.singletonMap("api-regions-crossfeature-dups",
+ Collections.singletonMap("definingFeatures", "g:f1:*"));
+ Analyser a = new Analyser(scanner, configs, at);
+ AnalyserResult res = a.analyse(f);
+
+ assertEquals(1, res.getErrors().size());
+ assertEquals(0, res.getWarnings().size());
+
+ String err = res.getErrors().get(0);
+ assertTrue(err.contains("org.foo.bar.bingo"));
+ assertTrue(err.contains("g:f2:1"));
+ assertTrue(err.contains("feature-export"));
+ }
+
+ @Test
+ public void testMultipleExportsFromNondefiningFeatures() throws Exception {
+ Path fp = new File(getClass().getResource("/crossfeatdups/fm3.json").getFile()).toPath();
+ String fm = new String(Files.readAllBytes(fp));
+
+ Feature f = FeatureJSONReader.read(new StringReader(fm), null);
+
+ Scanner scanner = getScanner3();
+ AnalyserTask at = new CheckApiRegionsCrossFeatureDups();
+ Map<String, Map<String,String>> configs =
+ Collections.singletonMap("api-regions-crossfeature-dups",
+ Collections.singletonMap("definingFeatures", "g:f2:*"));
+ Analyser a = new Analyser(scanner, configs, at);
+ AnalyserResult res = a.analyse(f);
+
+ assertEquals(0, res.getErrors().size());
+ assertEquals(0, res.getWarnings().size());
+ }
+
private Scanner getScanner() throws IOException {
ArtifactProvider ap = new ArtifactProvider() {
@Override
@@ -206,6 +273,8 @@
return getClass().getResource("/crossfeatdups/test-bundles/no-exports.jar");
case "g:extra:1":
return getClass().getResource("/crossfeatdups/test-bundles/no-exports.jar");
+ case "g:extra:2":
+ return getClass().getResource("/crossfeatdups/test-bundles/feature-export4.jar");
}
return null;
}
@@ -227,6 +296,25 @@
return getClass().getResource("/crossfeatdups/test-bundles/no-exports.jar");
case "g:extra:1":
return getClass().getResource("/crossfeatdups/test-bundles/feature-export3.jar");
+ case "g:extra:2":
+ return getClass().getResource("/crossfeatdups/test-bundles/feature-export4.jar");
+ }
+ return null;
+ }
+ };
+ Scanner scanner = new Scanner(ap, Collections.singletonList(new ApiRegionsExtensionScanner()), Collections.emptyList());
+ return scanner;
+ }
+
+ private Scanner getScanner3() throws IOException {
+ ArtifactProvider ap = new ArtifactProvider() {
+ @Override
+ public URL provide(ArtifactId id) {
+ switch (id.toMvnId()) {
+ case "g:exp0:1":
+ return getClass().getResource("/crossfeatdups/test-bundles/feature-export.jar");
+ case "g:exp1:1":
+ return getClass().getResource("/crossfeatdups/test-bundles/feature-export1.jar");
}
return null;
}
diff --git a/src/test/resources/crossfeatdups/fm1.json b/src/test/resources/crossfeatdups/fm1.json
index b0a037b..7cba672 100644
--- a/src/test/resources/crossfeatdups/fm1.json
+++ b/src/test/resources/crossfeatdups/fm1.json
@@ -13,7 +13,11 @@
{
"id": "g:extra:1",
"feature-origins":"g:f3:1"
- }
+ },
+ {
+ "id": "g:extra:2",
+ "feature-origins":"g:f2:1"
+ }
],
"api-regions:JSON|false":[
{
diff --git a/src/test/resources/crossfeatdups/fm2.json b/src/test/resources/crossfeatdups/fm2.json
new file mode 100644
index 0000000..db41ae6
--- /dev/null
+++ b/src/test/resources/crossfeatdups/fm2.json
@@ -0,0 +1,25 @@
+{
+ "id":"g:fm1:1.2.3",
+ "bundles":[
+ {
+ "id":"g:exp0:1",
+ "feature-origins":"g:f2:1,g:f1:1"
+ },
+ {
+ "id":"g:exp1:1",
+ "feature-origins":"g:f1:1"
+ }
+ ],
+ "api-regions:JSON|false":[
+ {
+ "name":"global",
+ "exports":[
+ "org.foo.bar",
+ "org.foo.bar.bingo"
+ ],
+ "feature-origins":[
+ "g:f1:1","g:f2:1"
+ ]
+ }
+ ]
+}
diff --git a/src/test/resources/crossfeatdups/fm3.json b/src/test/resources/crossfeatdups/fm3.json
new file mode 100644
index 0000000..e1f50b8
--- /dev/null
+++ b/src/test/resources/crossfeatdups/fm3.json
@@ -0,0 +1,25 @@
+{
+ "id":"g:fm1:1.2.3",
+ "bundles":[
+ {
+ "id":"g:exp0:1",
+ "feature-origins":"g:f1:1"
+ },
+ {
+ "id":"g:exp1:1",
+ "feature-origins":"g:f1:1"
+ }
+ ],
+ "api-regions:JSON|false":[
+ {
+ "name":"global",
+ "exports":[
+ "org.foo.bar",
+ "org.foo.bar.bingo"
+ ],
+ "feature-origins":[
+ "g:f1:1"
+ ]
+ }
+ ]
+}
diff --git a/src/test/resources/crossfeatdups/test-bundles/feature-export1.jar b/src/test/resources/crossfeatdups/test-bundles/feature-export1.jar
new file mode 100644
index 0000000..617ee55
--- /dev/null
+++ b/src/test/resources/crossfeatdups/test-bundles/feature-export1.jar
Binary files differ
diff --git a/src/test/resources/crossfeatdups/test-bundles/feature-export4.jar b/src/test/resources/crossfeatdups/test-bundles/feature-export4.jar
new file mode 100644
index 0000000..5ca78d4
--- /dev/null
+++ b/src/test/resources/crossfeatdups/test-bundles/feature-export4.jar
Binary files differ
