Copilot commented on code in PR #23:
URL:
https://github.com/apache/sling-org-apache-sling-feature-extension-apiregions/pull/23#discussion_r2807504943
##########
src/main/java/org/apache/sling/feature/extension/apiregions/analyser/CheckDeprecatedApi.java:
##########
@@ -199,41 +195,45 @@ Set<String> getAllowedRegions(final ApiRegion region) {
return allowedNames;
}
- Set<ApiExport> calculateDeprecatedPackages(
+ Map<String, DeprecatedPackage> calculateDeprecatedPackages(
final ApiRegion region, final Map<BundleDescriptor, Set<String>>
bundleRegions) {
- final Set<ApiExport> result = new LinkedHashSet<>();
- for (final ApiExport export : region.listAllExports()) {
- if (export.getDeprecation().getPackageInfo() != null) {
- final String version = getVersion(bundleRegions,
region.getName(), export.getName());
- // create new ApiExport to add version
- final ApiExport clone = new ApiExport(export.getName());
-
clone.getDeprecation().setPackageInfo(export.getDeprecation().getPackageInfo());
- if (version != null) {
- clone.getProperties().put(PROP_VERSION, version);
+ final Map<String, DeprecatedPackage> result = new HashMap<>();
+ ApiRegion current = region;
+ while (current != null) {
+ for (final ApiExport export : current.listExports()) {
+ if (export.getDeprecation().getPackageInfo() != null &&
!result.containsKey(export.getName())) {
+ final DeprecatedPackage pck =
getDeprecatedPackage(bundleRegions, current, export);
+ result.put(export.getName(), pck);
}
- result.add(clone);
}
+ current = current.getParent();
}
return result;
}
- String getVersion(
- final Map<BundleDescriptor, Set<String>> bundleRegions, final
String regionName, final String packageName) {
- String version = null;
+ DeprecatedPackage getDeprecatedPackage(
+ final Map<BundleDescriptor, Set<String>> bundleRegions, final
ApiRegion region, final ApiExport export) {
+ final List<PackageInfo> deprecatedList = new ArrayList<>();
+ final List<PackageInfo> nonDeprecatedList = new ArrayList<>();
+
+ final ArtifactId[] regionOrigins = region.getFeatureOrigins();
+
for (final Map.Entry<BundleDescriptor, Set<String>> entry :
bundleRegions.entrySet()) {
- if (entry.getValue().contains(regionName)) {
+ final ArtifactId[] bundleOrigins =
entry.getKey().getArtifact().getFeatureOrigins();
+ if (entry.getValue().contains(region.getName())) {
for (final PackageInfo info :
entry.getKey().getExportedPackages()) {
- if (info.getName().equals(packageName)) {
- version = info.getVersion();
- break;
+ if (info.getName().equals(export.getName())) {
+ if (regionOrigins.length == 0
+ || (bundleOrigins.length > 0 &&
bundleOrigins[0].isSame(regionOrigins[0]))) {
Review Comment:
The feature origins comparison only checks the first origin
(bundleOrigins[0] and regionOrigins[0]). If a bundle or region has multiple
feature origins, only the first one is considered when deciding whether to
treat the package as deprecated or non-deprecated. Consider whether all origins
should be compared, or document why only the first origin matters.
##########
src/test/java/org/apache/sling/feature/extension/apiregions/analyser/CheckDeprecatedApiTest.java:
##########
@@ -138,6 +141,145 @@ public void testOptionalImportReportedWhenEnabled()
throws Exception {
Mockito.eq(ArtifactId.fromMvnId("g:b:1.0.0")),
Mockito.contains("org.foo.deprecated"));
}
+ @Test
+ public void testVersionRangeChecking() throws Exception {
+ // Test that version ranges are properly checked
+ final CheckDeprecatedApi analyser = new CheckDeprecatedApi();
+
+ // Create a feature with deprecated package exported at version 2.0.0
+ final Feature feature = new
Feature(ArtifactId.fromMvnId("g:feature:1"));
+ final Extension extension =
+ new Extension(ExtensionType.JSON, ApiRegions.EXTENSION_NAME,
ExtensionState.OPTIONAL);
+
extension.setJSON("[{\"name\":\"global\",\"feature-origins\":[\"g:feature:1\"],"
+ +
"\"exports\":[{\"name\":\"org.foo.deprecated\",\"deprecated\":\"This package is
deprecated\"}]}]");
+ feature.getExtensions().add(extension);
+
+ final FeatureDescriptor fd = new FeatureDescriptorImpl(feature);
+
+ // Bundle that exports the deprecated package at version 2.0.0
+ final Artifact exportBundle = new
Artifact(ArtifactId.fromMvnId("g:exporter:1.0.0"));
+ exportBundle.setFeatureOrigins(feature.getId());
+ final BundleDescriptor exportBd = new
TestBundleDescriptor(exportBundle);
+ exportBd.getExportedPackages()
+ .add(new PackageInfo("org.foo.deprecated", "2.0.0", false,
Collections.emptySet()));
+ fd.getBundleDescriptors().add(exportBd);
+
+ // Bundle that imports with version range [1.0.0,2.0.0) - should NOT
be flagged
+ final Artifact importBundle1 = new
Artifact(ArtifactId.fromMvnId("g:importer1:1.0.0"));
+ importBundle1.setFeatureOrigins(feature.getId());
+ final BundleDescriptor importBd1 = new
TestBundleDescriptor(importBundle1);
+ final PackageInfo import1 =
+ new PackageInfo("org.foo.deprecated", "[1.0.0,2.0.0)", false,
Collections.emptySet());
+ importBd1.getImportedPackages().add(import1);
+ fd.getBundleDescriptors().add(importBd1);
+
+ // Bundle that imports with version range [2.0.0,3.0.0) - SHOULD be
flagged
+ final Artifact importBundle2 = new
Artifact(ArtifactId.fromMvnId("g:importer2:1.0.0"));
+ importBundle2.setFeatureOrigins(feature.getId());
+ final BundleDescriptor importBd2 = new
TestBundleDescriptor(importBundle2);
+ final PackageInfo import2 =
+ new PackageInfo("org.foo.deprecated", "[2.0.0,3.0.0)", false,
Collections.emptySet());
+ importBd2.getImportedPackages().add(import2);
+ fd.getBundleDescriptors().add(importBd2);
+
+ // Bundle that imports without version - SHOULD be flagged (matches
any version)
+ final Artifact importBundle3 = new
Artifact(ArtifactId.fromMvnId("g:importer3:1.0.0"));
+ importBundle3.setFeatureOrigins(feature.getId());
+ final BundleDescriptor importBd3 = new
TestBundleDescriptor(importBundle3);
+ final PackageInfo import3 = new PackageInfo("org.foo.deprecated",
null, false, Collections.emptySet());
+ importBd3.getImportedPackages().add(import3);
+ fd.getBundleDescriptors().add(importBd3);
+
+ final AnalyserTaskContext ctx =
Mockito.mock(AnalyserTaskContext.class);
+ Mockito.when(ctx.getFeature()).thenReturn(feature);
+ Mockito.when(ctx.getFeatureDescriptor()).thenReturn(fd);
+
Mockito.when(ctx.getConfiguration()).thenReturn(Collections.emptyMap());
+
+ analyser.execute(ctx);
+
+ // importer1 should NOT be flagged (version range excludes 2.0.0)
+ Mockito.verify(ctx, never())
+
.reportArtifactWarning(Mockito.eq(ArtifactId.fromMvnId("g:importer1:1.0.0")),
Mockito.anyString());
+
+ // importer2 SHOULD be flagged (version range includes 2.0.0)
+ Mockito.verify(ctx)
+ .reportArtifactWarning(
+ Mockito.eq(ArtifactId.fromMvnId("g:importer2:1.0.0")),
Mockito.contains("org.foo.deprecated"));
+
+ // importer3 SHOULD be flagged (no version constraint)
+ Mockito.verify(ctx)
+ .reportArtifactWarning(
+ Mockito.eq(ArtifactId.fromMvnId("g:importer3:1.0.0")),
Mockito.contains("org.foo.deprecated"));
+ }
+
+ @Test
+ public void testMultipleExportVersions() throws Exception {
+ // Test that when multiple versions of a deprecated package exist,
+ // only imports matching the actual exported versions are flagged
+ final CheckDeprecatedApi analyser = new CheckDeprecatedApi();
+
+ final Feature feature = new
Feature(ArtifactId.fromMvnId("g:feature:1"));
+ final Extension extension =
+ new Extension(ExtensionType.JSON, ApiRegions.EXTENSION_NAME,
ExtensionState.OPTIONAL);
+
extension.setJSON("[{\"name\":\"global\",\"feature-origins\":[\"g:feature:1\"],"
+ +
"\"exports\":[{\"name\":\"org.foo.deprecated\",\"deprecated\":\"This package is
deprecated\"}]}]");
+ feature.getExtensions().add(extension);
+
+ final FeatureDescriptor fd = new FeatureDescriptorImpl(feature);
+
+ // Bundle that exports the deprecated package at version 1.0.0
+ final Artifact exportBundle1 = new
Artifact(ArtifactId.fromMvnId("g:exporter1:1.0.0"));
+ exportBundle1.setFeatureOrigins(feature.getId());
+ final BundleDescriptor exportBd1 = new
TestBundleDescriptor(exportBundle1);
+ exportBd1
+ .getExportedPackages()
+ .add(new PackageInfo("org.foo.deprecated", "1.0.0", false,
Collections.emptySet()));
+ fd.getBundleDescriptors().add(exportBd1);
+
+ // DIFFERENT bundle that exports the SAME package at version 3.0.0
+ final Artifact exportBundle2 = new
Artifact(ArtifactId.fromMvnId("g:exporter2:1.0.0"));
+ exportBundle2.setFeatureOrigins(feature.getId());
+ final BundleDescriptor exportBd2 = new
TestBundleDescriptor(exportBundle2);
+ exportBd2
+ .getExportedPackages()
+ .add(new PackageInfo("org.foo.deprecated", "3.0.0", false,
Collections.emptySet()));
+ fd.getBundleDescriptors().add(exportBd2);
+
+ // Bundle that imports version [1.0.0,2.0.0) - matches only the 1.0.0
export
+ final Artifact importBundle1 = new
Artifact(ArtifactId.fromMvnId("g:importer1:1.0.0"));
+ importBundle1.setFeatureOrigins(feature.getId());
+ final BundleDescriptor importBd1 = new
TestBundleDescriptor(importBundle1);
+ importBd1
+ .getImportedPackages()
+ .add(new PackageInfo("org.foo.deprecated", "[1.0.0,2.0.0)",
false, Collections.emptySet()));
+ fd.getBundleDescriptors().add(importBd1);
+
+ // Bundle that imports version [2.5.0,4.0.0) - matches only the 3.0.0
export
+ final Artifact importBundle2 = new
Artifact(ArtifactId.fromMvnId("g:importer2:1.0.0"));
+ importBundle2.setFeatureOrigins(feature.getId());
+ final BundleDescriptor importBd2 = new
TestBundleDescriptor(importBundle2);
+ importBd2
+ .getImportedPackages()
+ .add(new PackageInfo("org.foo.deprecated", "[2.5.0,4.0.0)",
false, Collections.emptySet()));
+ fd.getBundleDescriptors().add(importBd2);
+
+ final AnalyserTaskContext ctx =
Mockito.mock(AnalyserTaskContext.class);
+ Mockito.when(ctx.getFeature()).thenReturn(feature);
+ Mockito.when(ctx.getFeatureDescriptor()).thenReturn(fd);
+
Mockito.when(ctx.getConfiguration()).thenReturn(Collections.emptyMap());
+
+ analyser.execute(ctx);
+
+ // BOTH importers should be flagged since both versions (1.0.0 and
3.0.0) are available
+ // The implementation checks all matching exported versions for this
package.
+ Mockito.verify(ctx)
+ .reportArtifactWarning(
+ Mockito.eq(ArtifactId.fromMvnId("g:importer1:1.0.0")),
Mockito.contains("org.foo.deprecated"));
+ Mockito.verify(ctx)
+ .reportArtifactWarning(
+ Mockito.eq(ArtifactId.fromMvnId("g:importer2:1.0.0")),
Mockito.contains("org.foo.deprecated"));
+ }
+
Review Comment:
The new tests don't actually test the main scenario described in the PR
title "Check for deprecated API does not consider other bundles exporting the
API". Both new tests (testVersionRangeChecking and testMultipleExportVersions)
have all bundles with the same feature origin. There should be a test where a
deprecated package from one feature is also exported by a bundle from a
DIFFERENT feature, and verify that imports matching only the non-deprecated
version are not flagged.
```suggestion
@Test
public void testDeprecatedExportFromOtherFeatureDoesNotTriggerWarning()
throws Exception {
final Feature feature = new
Feature(ArtifactId.fromMvnId("g:feature:1"));
final Extension extension =
new Extension(ExtensionType.JSON, ApiRegions.EXTENSION_NAME,
ExtensionState.OPTIONAL);
extension.setJSON(API_REGIONS_JSON);
feature.getExtensions().add(extension);
final FeatureDescriptor fd = new FeatureDescriptorImpl(feature);
// Importer bundle belonging to the current feature, importing only
the newer (non-deprecated) version
final Artifact importBundle = new
Artifact(ArtifactId.fromMvnId("g:importer:1.0.0"));
importBundle.setFeatureOrigins(feature.getId());
final BundleDescriptor importBd = new
TestBundleDescriptor(importBundle);
importBd
.getImportedPackages()
.add(new PackageInfo("org.foo.deprecated", "[3.0.0,4.0.0)",
false, Collections.emptySet()));
fd.getBundleDescriptors().add(importBd);
// Deprecated export coming from the current feature
final Artifact deprecatedExportBundle =
new
Artifact(ArtifactId.fromMvnId("g:deprecated-exporter:1.0.0"));
deprecatedExportBundle.setFeatureOrigins(feature.getId());
final BundleDescriptor deprecatedExportBd = new
TestBundleDescriptor(deprecatedExportBundle);
deprecatedExportBd
.getExportedPackages()
.add(new PackageInfo("org.foo.deprecated", "1.0.0", false,
Collections.emptySet()));
fd.getBundleDescriptors().add(deprecatedExportBd);
// Non-deprecated export coming from a different feature
final Artifact otherFeatureExportBundle =
new Artifact(ArtifactId.fromMvnId("g:other-exporter:1.0.0"));
otherFeatureExportBundle.setFeatureOrigins(ArtifactId.fromMvnId("g:otherfeature:1"));
final BundleDescriptor otherFeatureExportBd = new
TestBundleDescriptor(otherFeatureExportBundle);
otherFeatureExportBd
.getExportedPackages()
.add(new PackageInfo("org.foo.deprecated", "3.0.0", false,
Collections.emptySet()));
fd.getBundleDescriptors().add(otherFeatureExportBd);
final AnalyserTaskContext ctx =
Mockito.mock(AnalyserTaskContext.class);
Mockito.when(ctx.getFeature()).thenReturn(feature);
Mockito.when(ctx.getFeatureDescriptor()).thenReturn(fd);
Mockito.when(ctx.getConfiguration()).thenReturn(Collections.emptyMap());
analyser.execute(ctx);
// The importer only matches the non-deprecated 3.0.0 export from
the other feature, so it must not be flagged
Mockito.verify(ctx, never())
.reportArtifactWarning(
Mockito.eq(ArtifactId.fromMvnId("g:importer:1.0.0")), Mockito.anyString());
}
```
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
