Merge pull request #27274 from vespa-engine/warn-for-using-non-PublicApi

Warn for using non public api

10 files changed, 228 insertions, 36 deletions

diff --git a/bundle-plugin-test/integration-test/pom.xml b/bundle-plugin-test/integration-test/pom.xml
index 7384bf3aea6..acd075d0365 100644
--- a/bundle-plugin-test/integration-test/pom.xml
+++ b/bundle-plugin-test/integration-test/pom.xml
@@ -56,7 +56,14 @@
       <classifier>bundle</classifier>
       <version>${project.version}</version>
     </dependency>
+    <dependency>
+      <groupId>com.yahoo.vespa.bundle-plugin</groupId>
+      <artifactId>non-public-api-usage</artifactId>
+      <classifier>bundle</classifier>
+      <version>${project.version}</version>
+    </dependency>
   </dependencies>
+
   <build>
     <plugins>
       <plugin>
diff --git a/bundle-plugin-test/integration-test/src/test/java/com/yahoo/container/plugin/BundleTest.java b/bundle-plugin-test/integration-test/src/test/java/com/yahoo/container/plugin/BundleTest.java
index e06ce45a5f5..673d7d8e09e 100644
--- a/bundle-plugin-test/integration-test/src/test/java/com/yahoo/container/plugin/BundleTest.java
+++ b/bundle-plugin-test/integration-test/src/test/java/com/yahoo/container/plugin/BundleTest.java
@@ -30,9 +30,9 @@ import static org.junit.jupiter.api.Assertions.assertTrue;
 public class BundleTest {
     static final String TEST_BUNDLE_PATH = System.getProperty("test.bundle.path", ".") + "/";
 
-    // If bundle-plugin-test is compiled in a mvn command that also built dependencies, e.g. jrt,
-    // the artifact is jrt.jar, otherwise the installed and versioned artifact
-    // is used: jrt-7-SNAPSHOT.jar or e.g. jrt-7.123.45.jar.
+    // If bundle-plugin-test is compiled in a mvn command that also built dependencies, e.g. 'defaults',
+    // the artifact is defaults.jar, otherwise the installed and versioned artifact
+    // is used: defaults-7-SNAPSHOT.jar or e.g. defaults-7.123.45.jar.
     private static final String snapshotOrVersionOrNone = "(-\\d+((-SNAPSHOT)|((\\.\\d+(\\.\\d+)?)?))?)?\\.jar";
 
     private JarFile jarFile;
@@ -104,38 +104,36 @@ public class BundleTest {
     }
 
     @Test
-    void require_that_manifest_contains_public_api() {
-        assertEquals("com.yahoo.test", mainAttributes.getValue("X-JDisc-PublicApi-Package"));
+    void require_that_manifest_contains_public_api_for_this_bundle_and_embedded_bundles() {
+        assertEquals("com.yahoo.test,com.yahoo.vespa.defaults", mainAttributes.getValue("X-JDisc-PublicApi-Package"));
     }
 
-    // TODO: use another jar than jrt, which now pulls in a lot of dependencies that pollute the manifest of the
-    //       generated bundle. (It's compile scoped in pom.xml to be added to the bundle-cp.)
     @Test
     void require_that_manifest_contains_bundle_class_path() {
         String bundleClassPath = mainAttributes.getValue("Bundle-ClassPath");
         assertTrue(bundleClassPath.contains(".,"));
 
-        Pattern jrtPattern = Pattern.compile("dependencies/jrt" + snapshotOrVersionOrNone);
-        assertTrue(jrtPattern.matcher(bundleClassPath).find(), "Bundle class path did not contain jrt.");
+        Pattern jrtPattern = Pattern.compile("dependencies/defaults" + snapshotOrVersionOrNone);
+        assertTrue(jrtPattern.matcher(bundleClassPath).find(), "Bundle class path did not contain 'defaults''.");
     }
 
     @Test
     void require_that_component_jar_file_contains_compile_artifacts() {
-        String depJrt = "dependencies/jrt";
-        Pattern jrtPattern = Pattern.compile(depJrt + snapshotOrVersionOrNone);
-        ZipEntry jrtEntry = null;
+        String requiredDep = "dependencies/defaults";
+        Pattern depPattern = Pattern.compile(requiredDep + snapshotOrVersionOrNone);
+        ZipEntry depEntry = null;
 
         Enumeration<JarEntry> entries = jarFile.entries();
         while (entries.hasMoreElements()) {
             var e = entries.nextElement();
-            if (e.getName().startsWith(depJrt)) {
-                if (jrtPattern.matcher(e.getName()).matches()) {
-                    jrtEntry = e;
+            if (e.getName().startsWith(requiredDep)) {
+                if (depPattern.matcher(e.getName()).matches()) {
+                    depEntry = e;
                     break;
                 }
             }
         }
-        assertNotNull(jrtEntry, "Component jar file did not contain jrt dependency.");
+        assertNotNull(depEntry, "Component jar file did not contain 'defaults' dependency.");
     }
 
 
diff --git a/bundle-plugin-test/integration-test/src/test/java/com/yahoo/container/plugin/NonPublicApiDetectionTest.java b/bundle-plugin-test/integration-test/src/test/java/com/yahoo/container/plugin/NonPublicApiDetectionTest.java
new file mode 100644
index 00000000000..42ac99c65e5
--- /dev/null
+++ b/bundle-plugin-test/integration-test/src/test/java/com/yahoo/container/plugin/NonPublicApiDetectionTest.java
@@ -0,0 +1,44 @@
+package com.yahoo.container.plugin;
+
+import org.junit.jupiter.api.BeforeAll;
+import org.junit.jupiter.api.Test;
+
+import java.io.File;
+import java.io.IOException;
+import java.util.Arrays;
+import java.util.Set;
+import java.util.jar.Attributes;
+import java.util.jar.JarFile;
+import java.util.stream.Collectors;
+
+import static com.yahoo.container.plugin.BundleTest.findBundleJar;
+import static org.junit.jupiter.api.Assertions.assertEquals;
+import static org.junit.jupiter.api.Assertions.assertTrue;
+
+/**
+ * @author gjoranv
+ */
+public class NonPublicApiDetectionTest {
+
+    private static Set<String> usedNonPublicApi;
+
+    @BeforeAll
+    public static void setup() {
+        try {
+            File componentJar = findBundleJar("non-public-api-usage");
+            Attributes mainAttributes = new JarFile(componentJar).getManifest().getMainAttributes();
+            var nonPublicApiAttribute = mainAttributes.getValue("X-JDisc-Non-PublicApi-Import-Package");
+            usedNonPublicApi = Arrays.stream(nonPublicApiAttribute.split(",")).collect(Collectors.toSet());
+        } catch (IOException e) {
+            throw new RuntimeException(e);
+        }
+    }
+
+    @Test
+    void usage_of_non_publicApi_packages_is_detected() {
+        assertEquals(2, usedNonPublicApi.size());
+        assertTrue(usedNonPublicApi.contains("ai.vespa.http"));
+        assertTrue(usedNonPublicApi.contains("com.yahoo.io"));
+    }
+
+}
diff --git a/bundle-plugin-test/test-bundles/main/pom.xml b/bundle-plugin-test/test-bundles/main/pom.xml
index 603e0e95aa4..a6cf45947f3 100644
--- a/bundle-plugin-test/test-bundles/main/pom.xml
+++ b/bundle-plugin-test/test-bundles/main/pom.xml
@@ -17,7 +17,7 @@
   <dependencies>
     <dependency>
       <groupId>com.yahoo.vespa</groupId>
-      <artifactId>jrt</artifactId>
+      <artifactId>defaults</artifactId>
       <version>${project.version}</version>
     </dependency>
     <dependency>
diff --git a/bundle-plugin-test/test-bundles/non-public-api-usage/pom.xml b/bundle-plugin-test/test-bundles/non-public-api-usage/pom.xml
new file mode 100644
index 00000000000..5386346b8f7
--- /dev/null
+++ b/bundle-plugin-test/test-bundles/non-public-api-usage/pom.xml
@@ -0,0 +1,45 @@
+<?xml version="1.0"?>
+<!-- Copyright Yahoo. Licensed under the terms of the Apache 2.0 license. See LICENSE in the project root. -->
+<project xmlns="http://maven.apache.org/POM/4.0.0"
+         xmlns:xsi="http://www.w3.org/2001/XMLSchema-instance"
+         xsi:schemaLocation="http://maven.apache.org/POM/4.0.0
+                             http://maven.apache.org/xsd/maven-4.0.0.xsd">
+  <modelVersion>4.0.0</modelVersion>
+  <parent>
+    <groupId>com.yahoo.vespa.bundle-plugin</groupId>
+    <artifactId>test-bundles</artifactId>
+    <version>8-SNAPSHOT</version>
+    <relativePath>../pom.xml</relativePath>
+  </parent>
+  <artifactId>non-public-api-usage</artifactId>
+  <version>8-SNAPSHOT</version>
+  <packaging>container-plugin</packaging>
+
+  <dependencies>
+    <dependency>
+      <groupId>com.yahoo.vespa</groupId>
+      <artifactId>defaults</artifactId>
+      <version>${project.version}</version>
+      <scope>provided</scope>
+    </dependency>
+    <dependency>
+      <groupId>com.yahoo.vespa</groupId>
+      <artifactId>vespajlib</artifactId>
+      <version>${project.version}</version>
+      <scope>provided</scope>
+    </dependency>
+  </dependencies>
+
+  <build>
+    <plugins>
+      <plugin>
+        <groupId>com.yahoo.vespa</groupId>
+        <artifactId>bundle-plugin</artifactId>
+        <extensions>true</extensions>
+        <configuration>
+          <failOnWarnings>false</failOnWarnings>
+        </configuration>
+      </plugin>
+    </plugins>
+  </build>
+</project>
diff --git a/bundle-plugin-test/test-bundles/non-public-api-usage/src/main/java/com/yahoo/test/UsingBothPublicApiAndNonPublicApiPackages.java b/bundle-plugin-test/test-bundles/non-public-api-usage/src/main/java/com/yahoo/test/UsingBothPublicApiAndNonPublicApiPackages.java
new file mode 100644
index 00000000000..f2c64661ad6
--- /dev/null
+++ b/bundle-plugin-test/test-bundles/non-public-api-usage/src/main/java/com/yahoo/test/UsingBothPublicApiAndNonPublicApiPackages.java
@@ -0,0 +1,15 @@
+// Copyright Yahoo. Licensed under the terms of the Apache 2.0 license. See LICENSE in the project root.
+package com.yahoo.test;
+
+public class UsingBothPublicApiAndNonPublicApiPackages {
+
+    com.yahoo.vespa.defaults.Defaults publicFromDefaults = null;
+
+    com.yahoo.text.BooleanParser publicFromVespajlib = null;
+
+
+    ai.vespa.http.DomainName nonPublic1 = null;
+
+    com.yahoo.io.ByteWriter nonPublic2 = null;
+
+}
diff --git a/bundle-plugin-test/test-bundles/pom.xml b/bundle-plugin-test/test-bundles/pom.xml
index 3af10826adc..34c6b2e4540 100644
--- a/bundle-plugin-test/test-bundles/pom.xml
+++ b/bundle-plugin-test/test-bundles/pom.xml
@@ -50,6 +50,7 @@
     <modules>
         <module>artifact-version-for-exports</module>
         <module>artifact-version-for-exports-dep</module>
+        <module>non-public-api-usage</module>
         <module>main</module>
     </modules>
 
diff --git a/bundle-plugin/src/main/java/com/yahoo/container/plugin/bundle/AnalyzeBundle.java b/bundle-plugin/src/main/java/com/yahoo/container/plugin/bundle/AnalyzeBundle.java
index 2b5941cc5aa..af6c82023ab 100644
--- a/bundle-plugin/src/main/java/com/yahoo/container/plugin/bundle/AnalyzeBundle.java
+++ b/bundle-plugin/src/main/java/com/yahoo/container/plugin/bundle/AnalyzeBundle.java
@@ -7,12 +7,14 @@ import com.yahoo.container.plugin.util.JarFiles;
 
 import java.io.File;
 import java.util.ArrayList;
+import java.util.Arrays;
 import java.util.Collection;
 import java.util.Collections;
 import java.util.List;
 import java.util.Optional;
 import java.util.Set;
 import java.util.jar.Manifest;
+import java.util.stream.Collectors;
 
 /**
  * Static utilities for analyzing jar files.
@@ -34,20 +36,42 @@ public class AnalyzeBundle {
     }
 
     static List<Export> exportedPackages(File jarFile) {
+        var manifest = getOsgiManifest(jarFile);
+        if (manifest == null) return Collections.emptyList();
         try {
-            Optional<Manifest> jarManifest = JarFiles.getManifest(jarFile);
-            if (jarManifest.isPresent()) {
-                Manifest manifest = jarManifest.get();
-                if (isOsgiManifest(manifest)) {
-                    return parseExports(manifest);
-                }
-            }
-            return Collections.emptyList();
+            return parseExports(manifest);
         } catch (Exception e) {
             throw new RuntimeException(String.format("Invalid manifest in bundle '%s'", jarFile.getPath()), e);
         }
     }
 
+    public static List<String> publicApiPackagesAggregated(Collection<File> jarFiles) {
+        return jarFiles.stream()
+                .map(AnalyzeBundle::publicApiPackages)
+                .flatMap(List::stream)
+                .distinct()
+                .toList();
+    }
+
+    static List<String> publicApiPackages(File jarFile) {
+        var manifest = getOsgiManifest(jarFile);
+        if (manifest == null) return Collections.emptyList();
+        return getMainAttributeValue(manifest, "X-JDisc-PublicApi-Package")
+                .map(s -> Arrays.asList(s.split(",")))
+                .orElseGet(ArrayList::new);
+    }
+
+    private static Manifest getOsgiManifest(File jarFile) {
+        Optional<Manifest> jarManifest = JarFiles.getManifest(jarFile);
+        if (jarManifest.isPresent()) {
+            Manifest manifest = jarManifest.get();
+            if (isOsgiManifest(manifest)) {
+                return manifest;
+            }
+        }
+        return null;
+    }
+
     public static Optional<String> bundleSymbolicName(File jarFile) {
         return JarFiles.getManifest(jarFile).flatMap(AnalyzeBundle::getBundleSymbolicName);
     }
diff --git a/bundle-plugin/src/main/java/com/yahoo/container/plugin/classanalysis/Packages.java b/bundle-plugin/src/main/java/com/yahoo/container/plugin/classanalysis/Packages.java
index 9eef8a55c01..48a128c2f0d 100644
--- a/bundle-plugin/src/main/java/com/yahoo/container/plugin/classanalysis/Packages.java
+++ b/bundle-plugin/src/main/java/com/yahoo/container/plugin/classanalysis/Packages.java
@@ -1,8 +1,13 @@
 // Copyright Yahoo. Licensed under the terms of the Apache 2.0 license. See LICENSE in the project root.
 package com.yahoo.container.plugin.classanalysis;
 
+import com.yahoo.container.plugin.osgi.ImportPackages;
+
 import java.util.HashSet;
+import java.util.List;
+import java.util.Map;
 import java.util.Set;
+import java.util.stream.Collectors;
 
 /**
  * Utility methods related to packages.
@@ -31,6 +36,22 @@ public class Packages {
         }
     }
 
+    /**
+     * Returns the imported Vespa packages that don't exist in the given set of allowed packages.
+     */
+    public static List<String> disallowedVespaImports(Map<String, ImportPackages.Import> imports, List<String> allowed) {
+        if (imports == null || imports.isEmpty()) return List.of();
+
+        var publicApi = allowed == null ? Set.of() : new HashSet<>(allowed);
+
+        Set<String> yahooImports = imports.keySet().stream()
+                .filter(pkg -> pkg.startsWith("com.yahoo") || pkg.startsWith("ai.vespa."))
+                .collect(Collectors.toSet());
+
+        List<String> disallowedImports = yahooImports.stream().collect(Collectors.groupingBy(publicApi::contains)).get(false);
+        return disallowedImports == null ? List.of() : disallowedImports;
+    }
+
     public static PackageMetaData analyzePackages(Set<ClassFileMetaData> allClasses) {
         Set<String> definedPackages = new HashSet<>();
         Set<String> referencedPackages = new HashSet<>();
diff --git a/bundle-plugin/src/main/java/com/yahoo/container/plugin/mojo/GenerateOsgiManifestMojo.java b/bundle-plugin/src/main/java/com/yahoo/container/plugin/mojo/GenerateOsgiManifestMojo.java
index 2781decdf79..eef74365b92 100644
--- a/bundle-plugin/src/main/java/com/yahoo/container/plugin/mojo/GenerateOsgiManifestMojo.java
+++ b/bundle-plugin/src/main/java/com/yahoo/container/plugin/mojo/GenerateOsgiManifestMojo.java
@@ -26,6 +26,8 @@ import java.util.stream.Collectors;
 import java.util.stream.Stream;
 
 import static com.yahoo.container.plugin.bundle.AnalyzeBundle.exportedPackagesAggregated;
+import static com.yahoo.container.plugin.bundle.AnalyzeBundle.publicApiPackagesAggregated;
+import static com.yahoo.container.plugin.classanalysis.Packages.disallowedVespaImports;
 import static com.yahoo.container.plugin.osgi.ExportPackages.exportsByPackageName;
 import static com.yahoo.container.plugin.osgi.ImportPackages.calculateImports;
 import static com.yahoo.container.plugin.util.Files.allDescendantFiles;
@@ -66,6 +68,16 @@ public class GenerateOsgiManifestMojo extends AbstractGenerateOsgiManifestMojo {
     private BundleType bundleType = BundleType.USER;
 
     @Parameter(defaultValue = "false")
+    private boolean suppressWarningMissingImportPackages;
+    @Parameter(defaultValue = "false")
+    private boolean suppressWarningPublicApi;
+    @Parameter(defaultValue = "false")
+    private boolean suppressWarningOverlappingPackages;
+
+    @Parameter(defaultValue = "false")
+    private boolean failOnWarnings;
+
+    @Parameter(defaultValue = "false")
     private boolean buildLegacyVespaPlatformBundle;
 
     public void execute() throws MojoExecutionException {
@@ -78,10 +90,12 @@ public class GenerateOsgiManifestMojo extends AbstractGenerateOsgiManifestMojo {
             if (! isContainerDiscArtifact(project.getArtifact()))
                 throwIfInternalContainerArtifactsAreIncluded(artifactSet.getJarArtifactsToInclude());
 
-            List<Export> exportedPackagesFromProvidedJars = exportedPackagesAggregated(
-                    artifactSet.getJarArtifactsProvided().stream().map(Artifact::getFile).toList());
+            List<Artifact> providedJarArtifacts = artifactSet.getJarArtifactsProvided();
+            List<File> providedJarFiles = providedJarArtifacts.stream().map(Artifact::getFile).toList();
+            List<Export> exportedPackagesFromProvidedJars = exportedPackagesAggregated(providedJarFiles);
+            List<String> publicApiPackagesFromProvidedJars = publicApiPackagesAggregated(providedJarFiles);
 
-            // Packages from Export-Package headers in provided scoped jars
+            // Packages from Export-Package/PublicApi headers in provided scoped jars
             Set<String> exportedPackagesFromProvidedDeps = ExportPackages.packageNames(exportedPackagesFromProvidedJars);
 
             // Packaged defined in this project's code
@@ -95,11 +109,11 @@ public class GenerateOsgiManifestMojo extends AbstractGenerateOsgiManifestMojo {
 
             logDebugPackageSets(exportedPackagesFromProvidedJars, includedPackages);
 
-            if (hasJdiscCoreProvided(artifactSet.getJarArtifactsProvided())) {
+            if (hasJdiscCoreProvided(providedJarArtifacts)) {
                 // jdisc_core being provided guarantees that log output does not contain its exported packages
                 logMissingPackages(exportedPackagesFromProvidedDeps, projectPackages, compileJarsPackages, includedPackages);
-            } else {
-                getLog().warn(("This project does not have '%s' as provided dependency, so the generated 'Import-Package' " +
+            } else if (! suppressWarningMissingImportPackages) {
+                warnOrThrow(("This project does not have '%s' as provided dependency, so the generated 'Import-Package' " +
                         "OSGi header may be missing important packages.").formatted(wantedProvidedDependency()));
             }
             logOverlappingPackages(projectPackages, exportedPackagesFromProvidedDeps);
@@ -109,9 +123,12 @@ public class GenerateOsgiManifestMojo extends AbstractGenerateOsgiManifestMojo {
                                                                      includedPackages.definedPackages(),
                                                                      exportsByPackageName(exportedPackagesFromProvidedJars));
 
+            List<String> nonPublicApiUsed = disallowedVespaImports(calculatedImports, publicApiPackagesFromProvidedJars);
+            logNonPublicApiUsage(nonPublicApiUsed);
 
             Map<String, String> manifestContent = generateManifestContent(artifactSet.getJarArtifactsToInclude(), calculatedImports, includedPackages);
             addAdditionalManifestProperties(manifestContent, includedPackages);
+            addManifestPropertiesForUserBundles(manifestContent, nonPublicApiUsed);
             createManifestFile(Paths.get(project.getBuild().getOutputDirectory()), manifestContent);
 
         } catch (Exception e) {
@@ -142,6 +159,16 @@ public class GenerateOsgiManifestMojo extends AbstractGenerateOsgiManifestMojo {
         addIfNotEmpty(manifestContent, "WebInfUrl", webInfUrl);
     }
 
+    private void addManifestPropertiesForUserBundles(Map<String, String> manifestContent, List<String> nonPublicApiUsed) {
+        if (effectiveBundleType() != BundleType.USER) return;
+        addIfNotEmpty(manifestContent, "X-JDisc-Non-PublicApi-Import-Package", String.join(",", nonPublicApiUsed));
+    }
+
+    private void logNonPublicApiUsage(List<String> nonPublicApiUsed) {
+        if (suppressWarningPublicApi || effectiveBundleType() != BundleType.USER || nonPublicApiUsed.isEmpty()) return;
+        warnOrThrow("This project uses packages that are not part of Vespa's public api: %s".formatted(nonPublicApiUsed));
+    }
+
     private static String publicApi(PackageTally tally) {
         return tally.publicApiPackages().stream().sorted().collect(Collectors.joining(","));
     }
@@ -181,10 +208,12 @@ public class GenerateOsgiManifestMojo extends AbstractGenerateOsgiManifestMojo {
 
     private void logOverlappingPackages(PackageTally projectPackages,
                                         Set<String> exportedPackagesFromProvidedDeps) {
+        if (suppressWarningOverlappingPackages) return;
+
         Set<String> overlappingProjectPackages = Sets.intersection(projectPackages.definedPackages(), exportedPackagesFromProvidedDeps);
         if (! overlappingProjectPackages.isEmpty()) {
-            getLog().warn("This project defines packages that are also defined in provided scoped dependencies " +
-                          "(overlapping packages are strongly discouraged): " + overlappingProjectPackages);
+            warnOrThrow("This project defines packages that are also defined in provided scoped dependencies " +
+                         "(overlapping packages are strongly discouraged): " + overlappingProjectPackages);
         }
     }
 
@@ -211,9 +240,8 @@ public class GenerateOsgiManifestMojo extends AbstractGenerateOsgiManifestMojo {
         List<Artifact> unsupportedArtifacts = nonJarArtifacts.stream().filter(a -> ! a.getType().equals("pom"))
                 .toList();
 
-        unsupportedArtifacts.forEach(artifact -> getLog()
-                .warn(String.format("Unsupported artifact '%s': Type '%s' is not supported. Please file a feature request.",
-                        artifact.getId(), artifact.getType())));
+        unsupportedArtifacts.forEach(artifact -> warnOrThrow(String.format("Unsupported artifact '%s': Type '%s' is not supported. Please file a feature request.",
+                                                                           artifact.getId(), artifact.getType())));
     }
 
     private void throwIfInternalContainerArtifactsAreIncluded(Collection<Artifact> includedArtifacts) throws MojoExecutionException {
@@ -246,4 +274,13 @@ public class GenerateOsgiManifestMojo extends AbstractGenerateOsgiManifestMojo {
 
         return PackageTally.fromAnalyzedClassFiles(analyzedClasses);
     }
+
+    private void warnOrThrow(String... messages){
+        String message = String.join("\n", messages);
+        if (failOnWarnings) {
+            throw new RuntimeException(message);
+        }
+        getLog().warn(message);
+    }
+
 }