[
https://issues.apache.org/jira/browse/AVRO-3582?page=com.atlassian.jira.plugin.system.issuetabpanels:all-tabpanel
]
Filip updated AVRO-3582:
------------------------
Description:
{{org.apache.avro.mojo.AbstractAvroMojo#getIncludedFiles}} contains the
following code:
{code:java}
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 } {code}
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.:
{code:java}
<imports>
<import>/${basedir}/src/main/resources/schemas/</import> <!-- note the
erroneous extra / at the beginning -->
</imports>{code}
Then both checks at line no. {{258}} and {{260}} evaluate to {{{}false{}}},
meaning that in line no. {{264}} the code:
{code:java}
fs.addExclude(null);
{code}
is executed.
This leads to the behaviour that the following condition in the
{{org.apache.maven.shared.model.fileset.util.FileSetManager#scan}} code:
{code:java}
656 if ( excludesArray.length > 0 )
657 {
658 scanner.setExcludes( excludesArray );
659 } {code}
evaluates to {{{}true{}}}, causing an exception to occur in
{{org.apache.maven.shared.utils.io.DirectoryScanner#setExcludes}}
{code:java}
357 pattern = excludes[i].trim().replace( '/', File.separatorChar ).replace(
'\\', File.separatorChar );{code}
with the Maven error:
{code:java}
[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]
{code}
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).
was:
{{org.apache.avro.mojo.AbstractAvroMojo#getIncludedFiles}} contains the
following code:
{code:java}
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 } {code}
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.:
{code:java}
<imports>
<import>/src/main/resources/schemas/</import> <!-- note the erroneous extra /
at the beginning -->
</imports>{code}
Then both checks at line no. {{258}} and {{260}} evaluate to {{{}false{}}},
meaning that in line no. {{264}} the code:
{code:java}
fs.addExclude(null);
{code}
is executed.
This leads to the behaviour that the following condition in the
{{org.apache.maven.shared.model.fileset.util.FileSetManager#scan}} code:
{code:java}
656 if ( excludesArray.length > 0 )
657 {
658 scanner.setExcludes( excludesArray );
659 } {code}
evaluates to {{{}true{}}}, causing an exception to occur in
{{org.apache.maven.shared.utils.io.DirectoryScanner#setExcludes}}
{code:java}
357 pattern = excludes[i].trim().replace( '/', File.separatorChar ).replace(
'\\', File.separatorChar );{code}
with the Maven error:
{code:java}
[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]
{code}
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).
> Avro Maven plug in should fail if imports do not resolve to actual
> file/directory
> ---------------------------------------------------------------------------------
>
> Key: AVRO-3582
> URL: https://issues.apache.org/jira/browse/AVRO-3582
> Project: Apache Avro
> Issue Type: Improvement
> Components: java, tools
> Affects Versions: 1.10.2, 1.12.0
> Reporter: Filip
> Priority: Minor
> Attachments: abstractAvroMojo.patch
>
>
> {{org.apache.avro.mojo.AbstractAvroMojo#getIncludedFiles}} contains the
> following code:
> {code:java}
> 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 } {code}
> 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.:
> {code:java}
> <imports>
> <import>/${basedir}/src/main/resources/schemas/</import> <!-- note the
> erroneous extra / at the beginning -->
> </imports>{code}
> Then both checks at line no. {{258}} and {{260}} evaluate to {{{}false{}}},
> meaning that in line no. {{264}} the code:
> {code:java}
> fs.addExclude(null);
> {code}
> is executed.
> This leads to the behaviour that the following condition in the
> {{org.apache.maven.shared.model.fileset.util.FileSetManager#scan}} code:
> {code:java}
> 656 if ( excludesArray.length > 0 )
> 657 {
> 658 scanner.setExcludes( excludesArray );
> 659 } {code}
> evaluates to {{{}true{}}}, causing an exception to occur in
> {{org.apache.maven.shared.utils.io.DirectoryScanner#setExcludes}}
> {code:java}
> 357 pattern = excludes[i].trim().replace( '/', File.separatorChar ).replace(
> '\\', File.separatorChar );{code}
> with the Maven error:
> {code:java}
> [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]
> {code}
> 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).
--
This message was sent by Atlassian Jira
(v8.20.10#820010)
