refactor: take PR comments into account

mathieucarbou · Jan 16, 2025 · befea6b · befea6b
1 parent fcad2a6
commit befea6b
Show file tree

Hide file tree

Showing 12 changed files with 81 additions and 37 deletions.
diff --git a/docs/reports/5.0.0-SNAPSHOT/license-maven-plugin/license-plugin-report.xml b/docs/reports/5.0.0-SNAPSHOT/license-maven-plugin/license-plugin-report.xml
@@ -1,5 +1,5 @@
 <?xml version="1.0" encoding="UTF-8" standalone="no"?>
-<licensePluginReport goal="CHECK" timestamp="1736757823011">
+<licensePluginReport goal="CHECK" timestamp="1737017928065">
     <module artifactId="license-maven-plugin" groupId="com.mycila" version="5.0.0-SNAPSHOT"/>
     <files>
         <file path="pom.xml" result="PRESENT"/>
@@ -16,6 +16,7 @@
         <file path="src/main/java/com/mycila/maven/plugin/license/Multi.java" result="PRESENT"/>
         <file path="src/main/java/com/mycila/maven/plugin/license/PropertiesProvider.java" result="PRESENT"/>
         <file path="src/main/java/com/mycila/maven/plugin/license/Report.java" result="PRESENT"/>
+        <file path="src/main/java/com/mycila/maven/plugin/license/WorkSpace.java" result="PRESENT"/>
         <file path="src/main/java/com/mycila/maven/plugin/license/dependencies/AbstractLicensePolicyEnforcer.java" result="PRESENT"/>
         <file path="src/main/java/com/mycila/maven/plugin/license/dependencies/AggregateLicensePolicyEnforcer.java" result="PRESENT"/>
         <file path="src/main/java/com/mycila/maven/plugin/license/dependencies/ArtifactLicensePolicyEnforcer.java" result="PRESENT"/>

diff --git a/docs/reports/5.0.0-SNAPSHOT/license-plugin-report.xml b/docs/reports/5.0.0-SNAPSHOT/license-plugin-report.xml
@@ -1,5 +1,5 @@
 <?xml version="1.0" encoding="UTF-8" standalone="no"?>
-<licensePluginReport goal="CHECK" timestamp="1736757784315">
+<licensePluginReport goal="CHECK" timestamp="1737017925393">
     <module artifactId="license-maven-plugin-parent" groupId="com.mycila" version="5.0.0-SNAPSHOT"/>
     <files>
         <file path=".mvn/extensions.xml" result="PRESENT"/>

diff --git a/...lugin-fs/src/test/java/com/mycila/maven/plugin/license/fs/CopyrightRangeProviderTest.java b/...lugin-fs/src/test/java/com/mycila/maven/plugin/license/fs/CopyrightRangeProviderTest.java
@@ -42,7 +42,7 @@ class CopyrightRangeProviderTest {
   void copyrightRange() {
     Map<String, String> props = new HashMap<>();
     final LicenseCheckMojo mojo = new LicenseCheckMojo();
-    mojo.legacyDefaultBasedir = fsRepoRoot.toFile();
+    mojo.workspace.basedir = fsRepoRoot.toFile();
     try (CopyrightRangeProvider provider = new CopyrightRangeProvider()) {
       provider.init(mojo, props);
 

diff --git a/...plugin-git/src/main/java/com/mycila/maven/plugin/license/git/CopyrightAuthorProvider.java b/...plugin-git/src/main/java/com/mycila/maven/plugin/license/git/CopyrightAuthorProvider.java
@@ -40,7 +40,7 @@ public class CopyrightAuthorProvider implements PropertiesProvider {
 
   @Override
   public void init(AbstractLicenseMojo mojo, Map<String, String> currentProperties) {
-    gitLookup = GitLookup.create(mojo.legacyDefaultBasedir, currentProperties);
+    gitLookup = GitLookup.create(mojo.workspace.basedir, currentProperties);
 
     // One-time warning for shallow repo
     if (mojo.warnIfShallow && gitLookup.isShallowRepository()) {

diff --git a/...-plugin-git/src/main/java/com/mycila/maven/plugin/license/git/CopyrightRangeProvider.java b/...-plugin-git/src/main/java/com/mycila/maven/plugin/license/git/CopyrightRangeProvider.java
@@ -43,7 +43,7 @@ public class CopyrightRangeProvider implements PropertiesProvider {
 
   @Override
   public void init(AbstractLicenseMojo mojo, Map<String, String> currentProperties) {
-    gitLookup = GitLookup.create(mojo.legacyDefaultBasedir, currentProperties);
+    gitLookup = GitLookup.create(mojo.workspace.basedir, currentProperties);
 
     // One-time warning for shallow repo
     if (mojo.warnIfShallow && gitLookup.isShallowRepository()) {

diff --git a/...in-git/src/test/java/com/mycila/maven/plugin/license/git/CopyrightAuthorProviderTest.java b/...in-git/src/test/java/com/mycila/maven/plugin/license/git/CopyrightAuthorProviderTest.java
@@ -41,7 +41,7 @@ class CopyrightAuthorProviderTest {
   void copyrightAuthor() {
     Map<String, String> props = new HashMap<>();
     final LicenseCheckMojo mojo = new LicenseCheckMojo();
-    mojo.legacyDefaultBasedir = gitRepoRoot.toFile();
+    mojo.workspace.basedir = gitRepoRoot.toFile();
 
     try (CopyrightAuthorProvider provider = new CopyrightAuthorProvider()) {
       provider.init(mojo, props);

diff --git a/...gin-git/src/test/java/com/mycila/maven/plugin/license/git/CopyrightRangeProviderTest.java b/...gin-git/src/test/java/com/mycila/maven/plugin/license/git/CopyrightRangeProviderTest.java
@@ -41,7 +41,7 @@ class CopyrightRangeProviderTest {
   void copyrightRange() {
     Map<String, String> props = new HashMap<>();
     final LicenseCheckMojo mojo = new LicenseCheckMojo();
-    mojo.legacyDefaultBasedir = gitRepoRoot.toFile();
+    mojo.workspace.basedir = gitRepoRoot.toFile();
     try (CopyrightRangeProvider provider = new CopyrightRangeProvider()) {
       provider.init(mojo, props);
 

diff --git a/license-maven-plugin/src/main/java/com/mycila/maven/plugin/license/AbstractLicenseMojo.java b/license-maven-plugin/src/main/java/com/mycila/maven/plugin/license/AbstractLicenseMojo.java
@@ -81,6 +81,7 @@
 import static java.lang.String.format;
 import static java.util.Arrays.asList;
 import static java.util.Arrays.deepToString;
+import static java.util.Objects.requireNonNull;
 
 public abstract class AbstractLicenseMojo extends AbstractMojo {
 
@@ -103,7 +104,7 @@ public abstract class AbstractLicenseMojo extends AbstractMojo {
    * @deprecated use {@link WorkSpace#basedir}
    */
   @Deprecated
-  @Parameter(property = "license.basedir", defaultValue = "${project.basedir}", alias = "basedir", required = true)
+  @Parameter(property = "license.basedir", alias = "basedir")
   public File legacyDefaultBasedir;
 
   /**
@@ -499,26 +500,34 @@ protected final void execute(final Callback callback) throws MojoExecutionExcept
         throw new MojoExecutionException("Use of legacy parameters has been prohibited by configuration.");
       }
 
-      // make default base dir canonical
-      workspace.basedir = getCanonicalFile(firstNonNull(workspace.basedir, legacyDefaultBasedir), "license.workspace.basedir");
+      // use canonical base dir
+      workspace.basedir = getCanonicalFile(workspace.basedir, "license.workspace.basedir");
+      if (workspace.basedir == null) {
+        if (legacyDefaultBasedir == null) {
+          workspace.basedir = getCanonicalFile(project.getBasedir(), "project.basedir");
+        } else {
+          workspace.basedir = getCanonicalFile(legacyDefaultBasedir, "license.basedir");
+        }
+      }
+      requireNonNull(workspace.basedir);
 
       // collect all the license sets together
       final LicenseSet[] allLicenseSets;
 
       // if we abandon the legacy config this contiguous block can be removed
       final LicenseSet legacyLicenseSet = convertLegacyConfigToLicenseSet();
 
-      if (workspace.basedir != null) {
-        if (legacyLicenseSet != null && legacyLicenseSet.basedir != null) {
-          if (!FileUtils.isSubfolder(legacyLicenseSet.basedir, workspace.basedir)) {
-            throw new MojoExecutionException("Legacy basedir parameter should be a subfolder of the workspace basedir.");
-          }
+      if (legacyLicenseSet != null && legacyLicenseSet.basedir != null) {
+        if (!FileUtils.isSameOrSubFolder(legacyLicenseSet.basedir, workspace.basedir)) {
+          throw new MojoExecutionException("Legacy basedir parameter should be a subfolder of the workspace basedir.");
         }
-        for (LicenseSet licenseSet : licenseSets) {
-          if (licenseSet.basedir != null) {
-            if (!FileUtils.isSubfolder(licenseSet.basedir, workspace.basedir)) {
-              throw new MojoExecutionException(String.format("LicenseSet basedir parameter [%s] should be a subfolder of the workspace basedir.", licenseSet.basedir.getPath()));
-            }
+      }
+      for (LicenseSet licenseSet : licenseSets) {
+        if (licenseSet.basedir == null) {
+          licenseSet.basedir = workspace.basedir;
+        } else {
+          if (!FileUtils.isSameOrSubFolder(licenseSet.basedir, workspace.basedir)) {
+            throw new MojoExecutionException(String.format("LicenseSet basedir parameter [%s] should be a subfolder of the workspace basedir.", licenseSet.basedir.getPath()));
           }
         }
       }
@@ -610,7 +619,7 @@ private LicenseSet convertLegacyConfigToLicenseSet() {
   }
 
   private void executeForLicenseSet(final LicenseSet licenseSet, final Callback callback) throws MojoExecutionException, MojoFailureException {
-    final ResourceFinder finder = new ResourceFinder(firstNonNull(asPath(licenseSet.basedir), asPath(workspace.basedir)));
+    final ResourceFinder finder = new ResourceFinder(asPath(licenseSet.basedir));
     try {
       finder.setCompileClassPath(project.getCompileClasspathElements());
     } catch (DependencyResolutionRequiredException e) {
@@ -697,7 +706,7 @@ private void executeForLicenseSet(final LicenseSet licenseSet, final Callback ca
       };
 
       final DocumentFactory documentFactory = new DocumentFactory(
-          firstNonNull(licenseSet.basedir, workspace.basedir), buildMapping(),
+          licenseSet.basedir, buildMapping(),
           buildHeaderDefinitions(licenseSet, finder), Charset.forName(encoding), licenseSet.keywords,
           perDocumentProperties);
 
@@ -812,9 +821,9 @@ private Map<String, String> getDefaultProperties() {
   private String[] listSelectedFiles(final LicenseSet licenseSet) {
     final boolean useDefaultExcludes = (licenseSet.useDefaultExcludes != null ? licenseSet.useDefaultExcludes : defaultUseDefaultExcludes);
     final Selection selection = new Selection(
-        firstNonNull(licenseSet.basedir, workspace.basedir), licenseSet.includes, buildExcludes(licenseSet), useDefaultExcludes,
+        licenseSet.basedir, licenseSet.includes, buildExcludes(licenseSet), useDefaultExcludes,
         getLog());
-    debug("From: %s", firstNonNull(licenseSet.basedir, workspace.basedir));
+    debug("From: %s", licenseSet.basedir);
     debug("Including: %s", deepToString(selection.getIncluded()));
     debug("Excluding: %s", deepToString(selection.getExcluded()));
     return selection.getSelectedFiles();
@@ -948,8 +957,4 @@ public Credentials findCredentials(String serverID) {
     }
     return null;
   }
-
-  private static <T> T firstNonNull(final T t1, final T t2) {
-    return t1 == null ? t2 : t1;
-  }
 }
diff --git a/license-maven-plugin/src/main/java/com/mycila/maven/plugin/license/WorkSpace.java b/license-maven-plugin/src/main/java/com/mycila/maven/plugin/license/WorkSpace.java
@@ -28,12 +28,12 @@ public class WorkSpace {
    * This default value can be overridden in each LicenseSet by setting {@link LicenseSet#basedir}.
    */
 
-    @Parameter(property = "license.workspace.basedir", defaultValue = "${project.basedir}", alias = "basedir")
-    File basedir;
+    @Parameter(property = "license.workspace.basedir", alias = "basedir")
+    public File basedir;
 
     @Parameter
-    String[] includes = new String[0];
+    public String[] includes = new String[0];
 
     @Parameter
-    String[] excludes = new String[0];
+    public String[] excludes = new String[0];
 }
diff --git a/license-maven-plugin/src/main/java/com/mycila/maven/plugin/license/util/FileUtils.java b/license-maven-plugin/src/main/java/com/mycila/maven/plugin/license/util/FileUtils.java
@@ -39,6 +39,7 @@
 import java.util.stream.Stream;
 
 import static java.nio.file.StandardCopyOption.REPLACE_EXISTING;
+import static java.util.Objects.requireNonNull;
 
 public final class FileUtils {
 
@@ -134,9 +135,9 @@ public static void copyFilesToFolder(File src, File dst) {
     });
   }
 
-  public static boolean isSubfolder(File subfolder, File folder) {
-    String subfolderPath = subfolder.getAbsolutePath();
-    String folderPath = folder.getAbsolutePath();
-    return subfolderPath.startsWith(folderPath) && subfolderPath.length() > folderPath.length() && subfolderPath.charAt(folderPath.length()) == File.separatorChar;
+  public static boolean isSameOrSubFolder(File subfolder, File folder) {
+    requireNonNull(subfolder);
+    requireNonNull(folder);
+    return subfolder.toPath().startsWith(folder.toPath());
   }
 }
diff --git a/license-maven-plugin/src/test/java/com/mycila/maven/plugin/license/LicenseSetTest.java b/license-maven-plugin/src/test/java/com/mycila/maven/plugin/license/LicenseSetTest.java
@@ -47,7 +47,7 @@ void multipleLicenseSets() throws Exception {
     check.licenseSets = licenseSets;
     check.project = new MavenProjectStub();
     check.strictCheck = false;
-    check.legacyDefaultBasedir = new File("src/test/resources");
+    check.legacyDefaultBasedir = new File("src/test/resources/check");
     final MockedLog logger = new MockedLog();
     check.setLog(new DefaultLog(logger));
     check.execute();

diff --git a/license-maven-plugin/src/test/java/com/mycila/maven/plugin/license/util/FileUtilsTest.java b/license-maven-plugin/src/test/java/com/mycila/maven/plugin/license/util/FileUtilsTest.java
@@ -17,8 +17,10 @@
 
 import org.junit.jupiter.api.Assertions;
 import org.junit.jupiter.api.Test;
+import org.junit.jupiter.api.io.TempDir;
 
 import java.io.File;
+import java.io.IOException;
 import java.net.URL;
 import java.nio.ByteBuffer;
 import java.nio.charset.Charset;
@@ -51,4 +53,39 @@ void test_read_first_lines() throws Exception {
     Assertions.assertTrue(s.contains("c"));
     Assertions.assertFalse(s.contains("d"));
   }
+
+  @Test
+  void test_IsSameOrSubFolder_folder_is_direct_child_of_folder(@TempDir File folder) throws IOException {
+    Assertions.assertTrue(FileUtils.isSameOrSubFolder(folder, folder));
+  }
+
+  @Test
+  void test_IsSameOrSubFolder_subfolder_is_direct_child_of_folder(@TempDir File folder) throws IOException {
+    File subfolder = createSubfolder(folder, "subfolder");
+
+    Assertions.assertTrue(FileUtils.isSameOrSubFolder(subfolder, folder));
+  }
+
+  @Test
+  void test_IsSameOrSubFolder_subfolder_is_indirect_child_of_folder(@TempDir File folder) throws IOException {
+    File subfolder = createSubfolder(folder, "subfolder/subsubfolder");
+
+    Assertions.assertTrue(FileUtils.isSameOrSubFolder(subfolder, folder));
+  }
+
+  @Test
+  void test_IsSameOrSubFolder_subfolder_is_not_child_of_folder(@TempDir File folder, @TempDir File otherFolder)
+      throws IOException {
+    File subfolder = createSubfolder(folder, "subfolder");
+
+    Assertions.assertFalse(FileUtils.isSameOrSubFolder(subfolder, otherFolder));
+  }
+
+  private File createSubfolder(File parent, String path) throws IOException {
+    File subfolder = new File(parent, path);
+    if (!subfolder.mkdirs()) {
+      throw new IOException("Failed to create subfolder");
+    }
+    return subfolder;
+  }
 }