-
Notifications
You must be signed in to change notification settings - Fork 1.6k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
AVRO-3795: [Java] Raise exception for nonexistent imports in maven-plugin #2334
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Need to check on getIncludedFiles method (or create a "checkImports" method to run it once)
@@ -213,7 +213,9 @@ public void execute() throws MojoExecutionException { | |||
if (hasImports) { | |||
for (String importedFile : imports) { | |||
File file = new File(importedFile); | |||
if (file.isDirectory()) { | |||
if (!file.exists()) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Nice !!
Same logic should apply to getIncludedFiles method, line 259 ?
(or may be put this control in another method as "checkImportFiles", as it concerns only "imports" variables.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I was considering that, but it seems that if imports field is non-null, then we may be sure that during the execution we will check every imported file in line 216. Indeed, even if getIncludedFiles would execute first, the nonexistent file will be found by line 216 in a next iteration. Do you think it's beneficial to check it anyway in getIncludedFiles?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
On line 256, getIncludedFiles method scan for all imports ...
for (String importFile : this.imports) {
File file = new File(importFile);
...
So, if you have 2 files, and only the first exists, this second method will encountered this unexisting file before the "execute()" method. So, even if used method (isFile & isDirectory) won't fails, i found that checking before using cleaner.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Of course I understand that - I just pointed out that getIncludedFiles touches this file earlier, but with no real downside. Anyway, I moved existence check to the separate method that checks everything in the first place - will that work for you?
<sourceDirectory>${basedir}/src/test/avro</sourceDirectory> | ||
<outputDirectory>${basedir}/target/test-harness/schema</outputDirectory> | ||
<imports> | ||
<import>${basedir}/src/test/avro/nonexistent-dir</import> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Check also the case where first file exists (to check getIncludedFiles methods)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I've added another test for the case where first file exist and the second one doesn't.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM
…ugin (apache#2334) * AVRO-3795: [Java] Testcase for nonexistent files in maven-plugin * AVRO-3795: [Java] Raise exception for nonexistent imports in maven-plugin * AVRO-3795: [Java] Fix testcases for nonexistent files in maven-plugin * AVRO-3795: [Java] Check imports for maven-plugin in separate method
This PR fixes bug reported in https://issues.apache.org/jira/browse/AVRO-3795
Checking if the path exists before processing seems to have no real drawbacks and provides more intuitive behavior, so I think it should be merged. I hope you don't have serious objections.