Skip to content
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.

Sign up for GitHub

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

Jump to bottom

AVRO-3795: [Java] Raise exception for nonexistent imports in maven-plugin #2334

Merged
merged 4 commits into from
Jul 14, 2023
Merged

AVRO-3795: [Java] Raise exception for nonexistent imports in maven-plugin #2334

Show file tree
Hide file tree
Changes from 2 commits
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
0e873b9
AVRO-3795: [Java] Testcase for nonexistent files in maven-plugin
dervan Jul 10, 2023
38710a0
AVRO-3795: [Java] Raise exception for nonexistent imports in maven-pl…
dervan Jul 7, 2023
165deeb
AVRO-3795: [Java] Fix testcases for nonexistent files in maven-plugin
dervan Jul 13, 2023
c3c3cbd
AVRO-3795: [Java] Check imports for maven-plugin in separate method
dervan Jul 13, 2023
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
4 changes: 3 additions & 1 deletion lang/java/maven-plugin/src/main/java/org/apache/avro/mojo/AbstractAvroMojo.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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()) {
Copy link
Contributor

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.

Copy link
Contributor Author

@dervan dervan Jul 13, 2023 •
edited
Loading

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?

Copy link
Contributor

@clesaec clesaec Jul 13, 2023 •
edited
Loading

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.

Copy link
Contributor Author

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?

throw new MojoExecutionException("Path " + file.getAbsolutePath() + " does not exist");
} else if (file.isDirectory()) {
String[] includedFiles = getIncludedFiles(file.getAbsolutePath(), excludes, getIncludes());
getLog().info("Importing Directory: " + file.getAbsolutePath());
getLog().debug("Importing Directory Files: " + Arrays.toString(includedFiles));
Expand Down
11 changes: 11 additions & 0 deletions lang/java/maven-plugin/src/test/java/org/apache/avro/mojo/TestSchemaMojo.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -25,6 +25,8 @@
import java.util.HashSet;
import java.util.Set;

import org.apache.maven.plugin.MojoExecutionException;

/**
* Test the Schema Mojo.
*/
Expand All @@ -33,6 +35,8 @@ public class TestSchemaMojo extends AbstractAvroMojoTest {
private File testPom = new File(getBasedir(), "src/test/resources/unit/schema/pom.xml");
private File injectingVelocityToolsTestPom = new File(getBasedir(),
"src/test/resources/unit/schema/pom-injecting-velocity-tools.xml");
private File testNonexistentFilePom = new File(getBasedir(),
"src/test/resources/unit/schema/pom-nonexistent-file.xml");

@Test
public void testSchemaMojo() throws Exception {
Expand Down Expand Up @@ -67,4 +71,11 @@ public void testSetCompilerVelocityAdditionalTools() throws Exception {
final String schemaUserContent = FileUtils.fileRead(new File(outputDir, "SchemaUser.java"));
assertTrue("Got " + schemaUserContent + " instead", schemaUserContent.contains("It works!"));
}

@Test(expected = MojoExecutionException.class)
public void throwsErrorForNonexistentFile() throws Exception {
final SchemaMojo mojo = (SchemaMojo) lookupMojo("schema", testNonexistentFilePom);

mojo.execute();
}
}
69 changes: 69 additions & 0 deletions lang/java/maven-plugin/src/test/resources/unit/schema/pom-nonexistent-file.pom
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,69 @@
<?xml version="1.0" encoding="UTF-8"?>
<!--
Licensed to the Apache Software Foundation (ASF) under one or more
contributor license agreements. See the NOTICE file distributed with
this work for additional information regarding copyright ownership.
The ASF licenses this file to You under the Apache License, Version 2.0
(the "License"); you may not use this file except in compliance with
the License. You may obtain a copy of the License at

https://www.apache.org/licenses/LICENSE-2.0

Unless required by applicable law or agreed to in writing, software
distributed under the License is distributed on an "AS IS" BASIS,
WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
See the License for the specific language governing permissions and
limitations under the License.
-->
<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>
<artifactId>avro-parent</artifactId>
<groupId>org.apache.avro</groupId>
<version>1.12.0-SNAPSHOT</version>
<relativePath>../../../../../../../../../pom.xml</relativePath>
</parent>

<artifactId>avro-maven-plugin-test</artifactId>
<packaging>jar</packaging>

<name>testproject</name>

<build>
<plugins>
<plugin>
<artifactId>avro-maven-plugin</artifactId>
<executions>
<execution>
<id>schema</id>
<goals>
<goal>schema</goal>
</goals>
</execution>
</executions>
<configuration>
<sourceDirectory>${basedir}/src/test/avro</sourceDirectory>
<outputDirectory>${basedir}/target/test-harness/schema</outputDirectory>
<imports>
<import>${basedir}/src/test/avro/nonexistent-dir</import>
Copy link
Contributor

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)

Copy link
Contributor Author

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.

</imports>
<project implementation="org.apache.maven.plugin.testing.stubs.MavenProjectStub"/>
</configuration>
</plugin>
</plugins>
</build>
<dependencies>
<dependency>
<groupId>org.apache.avro</groupId>
<artifactId>avro</artifactId>
<version>${parent.version}</version>
</dependency>
<dependency>
<groupId>com.fasterxml.jackson.core</groupId>
<artifactId>jackson-databind</artifactId>
<version>${jackson.version}</version>
</dependency>
</dependencies>
</project>
Loading