You are viewing a plain text version of this content. The canonical link for it is here.
Posted to dev@avro.apache.org by "dervan (via GitHub)" <gi...@apache.org> on 2023/07/10 12:29:40 UTC
[GitHub] [avro] dervan opened a new pull request, #2334: AVRO-3795: [Java] Raise exception for nonexistent imports in maven-plugin
dervan opened a new pull request, #2334:
URL: https://github.com/apache/avro/pull/2334
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.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: dev-unsubscribe@avro.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [avro] dervan commented on a diff in pull request #2334: AVRO-3795: [Java] Raise exception for nonexistent imports in maven-plugin
Posted by "dervan (via GitHub)" <gi...@apache.org>.
dervan commented on code in PR #2334:
URL: https://github.com/apache/avro/pull/2334#discussion_r1262395378
##########
lang/java/maven-plugin/src/main/java/org/apache/avro/mojo/AbstractAvroMojo.java:
##########
@@ -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()) {
Review Comment:
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_?
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@avro.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [avro] clesaec commented on a diff in pull request #2334: AVRO-3795: [Java] Raise exception for nonexistent imports in maven-plugin
Posted by "clesaec (via GitHub)" <gi...@apache.org>.
clesaec commented on code in PR #2334:
URL: https://github.com/apache/avro/pull/2334#discussion_r1258455087
##########
lang/java/maven-plugin/src/main/java/org/apache/avro/mojo/AbstractAvroMojo.java:
##########
@@ -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()) {
Review Comment:
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.
##########
lang/java/maven-plugin/src/test/resources/unit/schema/pom-nonexistent-file.pom:
##########
@@ -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>
Review Comment:
Check also the case where first file exists (to check getIncludedFiles methods)
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@avro.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [avro] dervan commented on a diff in pull request #2334: AVRO-3795: [Java] Raise exception for nonexistent imports in maven-plugin
Posted by "dervan (via GitHub)" <gi...@apache.org>.
dervan commented on code in PR #2334:
URL: https://github.com/apache/avro/pull/2334#discussion_r1262388818
##########
lang/java/maven-plugin/src/test/resources/unit/schema/pom-nonexistent-file.pom:
##########
@@ -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>
Review Comment:
I've added another test for the case where first file exist and the second one doesn't.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@avro.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [avro] RyanSkraba merged pull request #2334: AVRO-3795: [Java] Raise exception for nonexistent imports in maven-plugin
Posted by "RyanSkraba (via GitHub)" <gi...@apache.org>.
RyanSkraba merged PR #2334:
URL: https://github.com/apache/avro/pull/2334
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: dev-unsubscribe@avro.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [avro] dervan commented on a diff in pull request #2334: AVRO-3795: [Java] Raise exception for nonexistent imports in maven-plugin
Posted by "dervan (via GitHub)" <gi...@apache.org>.
dervan commented on code in PR #2334:
URL: https://github.com/apache/avro/pull/2334#discussion_r1262395378
##########
lang/java/maven-plugin/src/main/java/org/apache/avro/mojo/AbstractAvroMojo.java:
##########
@@ -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()) {
Review Comment:
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 anyway beneficial to check it anyway in _getIncludedFiles_?
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@avro.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [avro] dervan commented on a diff in pull request #2334: AVRO-3795: [Java] Raise exception for nonexistent imports in maven-plugin
Posted by "dervan (via GitHub)" <gi...@apache.org>.
dervan commented on code in PR #2334:
URL: https://github.com/apache/avro/pull/2334#discussion_r1262527249
##########
lang/java/maven-plugin/src/main/java/org/apache/avro/mojo/AbstractAvroMojo.java:
##########
@@ -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()) {
Review Comment:
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?
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@avro.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [avro] clesaec commented on a diff in pull request #2334: AVRO-3795: [Java] Raise exception for nonexistent imports in maven-plugin
Posted by "clesaec (via GitHub)" <gi...@apache.org>.
clesaec commented on code in PR #2334:
URL: https://github.com/apache/avro/pull/2334#discussion_r1262482522
##########
lang/java/maven-plugin/src/main/java/org/apache/avro/mojo/AbstractAvroMojo.java:
##########
@@ -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()) {
Review Comment:
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.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@avro.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [avro] clesaec commented on a diff in pull request #2334: AVRO-3795: [Java] Raise exception for nonexistent imports in maven-plugin
Posted by "clesaec (via GitHub)" <gi...@apache.org>.
clesaec commented on code in PR #2334:
URL: https://github.com/apache/avro/pull/2334#discussion_r1262482522
##########
lang/java/maven-plugin/src/main/java/org/apache/avro/mojo/AbstractAvroMojo.java:
##########
@@ -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()) {
Review Comment:
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 execute method. So, even if used method (isFile & isDirectory) won't fails, i found that checking before using cleaner.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@avro.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org