Details
Description
org.apache.avro.mojo.AbstractAvroMojo#getIncludedFiles contains the following code:
251 // exclude imports directory since it has already been compiled. 252 if (imports != null) { 253 String importExclude = null; 254 255 for (String importFile : this.imports) { 256 File file = new File(importFile); 257 258 if (file.isDirectory()) { 259 importExclude = file.getName() + "/**"; 260 } else if (file.isFile()) { 261 importExclude = "**/" + file.getName(); 262 } 263 264 fs.addExclude(importExclude); 265 } 266 }
which is supposed to explicitly exclude any directories that have been added through an <import>...</import> configuration property.
Unfortunately, when the configuration property does not resolve to an actual file or directory, e.g.:
<imports> <!-- note the erroneous extra / at the beginning --> <import>/${basedir}/src/main/resources/schemas/</import> </imports>
Then both checks at line no. 258 and 260 evaluate to false, meaning that in line no. 264 the code:
fs.addExclude(null);
is executed.
This leads to the behaviour that the following condition in the org.apache.maven.shared.model.fileset.util.FileSetManager#scan code:
656 if ( excludesArray.length > 0 )
657 {
658 scanner.setExcludes( excludesArray );
659 }
evaluates to true, causing an exception to occur in org.apache.maven.shared.utils.io.DirectoryScanner#setExcludes
357 pattern = excludes[i].trim().replace( '/', File.separatorChar ).replace( '\\', File.separatorChar );
with the Maven error:
[ERROR] Failed to execute goal org.apache.avro:avro-maven-plugin:1.11.0:schema (default) on project app: Execution default of goal org.apache.avro:avro-maven-plugin:1.11.0:schema failed: Cannot invoke "String.trim()" because "excludes[i]" is null -> [Help 1]
which is highly unintuitive and has cost me almost a day of debugging now. The error is ultimately caused by the user (me!), but IMO org.apache.avro.mojo.AbstractAvroMojo#getIncludedFiles should simply throw an exception if the file path cannot be resolve to an actual directory or file.
IMO, fail-fast behaviour when given incorrect configuration is a must for robust software, instead of getting a cryptic message from the underlying platform code.
Proposed change attached. An alternative solution via file.exists() would also work. Strictly speaking, throwing a FileNotFoundException would be more semantically correct, but I did not wish to add more checked exceptions. The current one is already rethrown by all calling methods.
If the change seems OK, I can include a test case as well (should be very simple).