Merge pull request #66 from aecio/issue-52

Provide better error message when the formatter fails

src/integTest/groovy/com/github/sherter/googlejavaformatgradleplugin/LoggingSpec.groovy

@@ -66,7 +66,7 @@ class LoggingSpec extends AbstractIntegrationSpec {
         result.output =~ /Foo\.java: formatted successfully/
         result.output =~ /Bar\.java: UP-TO-DATE/
         // (?s) makes the regex match newlines with . (dot) operators
-        result.output =~ /(?s)Detected Java syntax errors in the following files.*Baz\.java/
-        result.output =~ /(?s)Detected Java syntax errors in the following files.*OtherNon\.java/
+        result.output =~ /(?s)Failed to format the following files.*Baz\.java/
+        result.output =~ /(?s)Failed to format the following files.*OtherNon\.java/
     }
 }

src/main/groovy/com/github/sherter/googlejavaformatgradleplugin/GoogleJavaFormat.groovy

@@ -53,6 +53,7 @@ class GoogleJavaFormat extends FormatTask {
 
   @TaskAction
   void formatSources() {
+    Map<String, String> errors = [:]
     boolean successful = true
     if (!Iterables.isEmpty(filteredSources)) {
       Formatter formatter = sharedContext.formatter()
@@ -70,6 +71,7 @@ class GoogleJavaFormat extends FormatTask {
           mapper.putIfNewer(info);
           if (info.state() == FileState.INVALID) {
             invalidSources.add(info.path())
+            errors.put(info.path().toString(), info.error());
           }
         } catch (ExecutionException e) {
           def pathException = e.getCause() as PathException
@@ -80,12 +82,12 @@ class GoogleJavaFormat extends FormatTask {
     }
     if (Iterables.size(invalidSources) > 0) {
       successful = false
-      logger.error('\n\nDetected Java syntax errors in the following files ({} "{}" {}):\n',
+      logger.error('\n\nFailed to format the following files ({} "{}" {}):\n',
               'you can exclude them from this task, see',
               'https://github.com/sherter/google-java-format-gradle-plugin',
               'for details')
       for (Path file : invalidSources) {
-        logger.error(file.toString())
+        logger.error("{}\n > Reason: {}\n", file.toString(), errors.get(file.toString()))
       }
     }
     if (!successful) {

src/main/groovy/com/github/sherter/googlejavaformatgradleplugin/VerifyGoogleJavaFormat.groovy

@@ -49,7 +49,7 @@ class VerifyGoogleJavaFormat extends FormatTask implements VerificationTask {
             }
         }
         if (invalid.size() > 0) {
-            logger.lifecycle('\n\nDetected Java syntax errors in the following files ({} "{}" {}):\n',
+            logger.lifecycle('\n\nFailed to verify format of the following files ({} "{}" {}):\n',
                     'you can configure this task to exclude them, see',
                     'https://github.com/sherter/google-java-format-gradle-plugin',
                     'for details')

src/main/java/com/github/sherter/googlejavaformatgradleplugin/FileInfo.java

@@ -20,12 +20,25 @@ abstract class FileInfo {
    * @throws IllegalArgumentException if {@code state} == {@link FileState#UNKNOWN}
    */
   static FileInfo create(Path path, FileTime lastModified, long size, FileState state) {
+    return create(path, lastModified, size, state, "");
+  }
+
+  /**
+   * Constructs a new object of type {@link FileInfo} storing the given values.
+   *
+   * <p>If {@code path} is not already absolute and normalized, an absolute and normalized path that
+   * is equivalent to {@code path} is stored instead.
+   *
+   * @throws IllegalArgumentException if {@code state} == {@link FileState#UNKNOWN}
+   */
+  static FileInfo create(
+      Path path, FileTime lastModified, long size, FileState state, String error) {
     if (state == FileState.UNKNOWN) {
       throw new IllegalArgumentException(
           "constructing file info with state UNKNOWN is not allowed");
     }
     Path modifiedPath = path.toAbsolutePath().normalize();
-    return new AutoValue_FileInfo(modifiedPath, lastModified, size, state);
+    return new AutoValue_FileInfo(modifiedPath, lastModified, size, state, error);
   }
 
   /**
@@ -40,6 +53,8 @@ static FileInfo create(Path path, FileTime lastModified, long size, FileState st
 
   abstract FileState state();
 
+  abstract String error();
+
   /**
    * Returns true if and only if {@code this} FileInfo represents the file at a strictly later point
    * in time than the given FileInfo.

src/main/java/com/github/sherter/googlejavaformatgradleplugin/FileInfoDecoder.java

@@ -10,6 +10,7 @@
 import java.nio.file.Path;
 import java.nio.file.attribute.FileTime;
 import java.util.ArrayList;
+import java.util.Base64;
 import java.util.List;
 import java.util.Objects;
 import java.util.concurrent.TimeUnit;
@@ -39,14 +40,15 @@ class FileInfoDecoder {
    */
   FileInfo decode(CharSequence serializedFileInfo) {
     String[] elements = Iterables.toArray(Splitter.on(',').split(serializedFileInfo), String.class);
-    if (elements.length != 4) {
+    if (elements.length != 5) {
       throw new IllegalArgumentException("Invalid number of elements");
     }
     return FileInfo.create(
         decodePath(elements[0]),
         FileTime.from(decodeLong(elements[1]), TimeUnit.NANOSECONDS),
         decodeLong(elements[2]),
-        decodeState(elements[3]));
+        decodeState(elements[3]),
+        decodeError(elements[4]));
   }
 
   private Path decodePath(CharSequence path) {
@@ -80,4 +82,8 @@ private long decodeLong(String number) {
       throw new IllegalArgumentException("Not a valid long value: " + number);
     }
   }
+
+  private String decodeError(String error) {
+    return new String(Base64.getDecoder().decode(error));
+  }
 }

src/main/java/com/github/sherter/googlejavaformatgradleplugin/FileInfoEncoder.java

@@ -7,6 +7,7 @@
 import java.nio.charset.StandardCharsets;
 import java.nio.file.Path;
 import java.util.ArrayList;
+import java.util.Base64;
 import java.util.List;
 import java.util.Objects;
 import java.util.concurrent.TimeUnit;
@@ -35,7 +36,9 @@ String encode(FileInfo fileInfo) {
         + ','
         + fileInfo.size()
         + ','
-        + fileInfo.state().name();
+        + fileInfo.state().name()
+        + ','
+        + encodeError(fileInfo.error());
   }
 
   private String encodePath(Path p) {
@@ -52,4 +55,9 @@ private String encodePath(Path p) {
     }
     return Joiner.on('/').join(urlEncodedElements);
   }
+
+  /* Encode the error string as base64 to make sure that there are no commas in the text. */
+  private String encodeError(String error) {
+    return Base64.getEncoder().encodeToString(error.getBytes(StandardCharsets.UTF_8));
+  }
 }

src/main/java/com/github/sherter/googlejavaformatgradleplugin/FormatFileCallable.java

@@ -38,8 +38,9 @@ public FileInfo call() throws PathException {
       try {
         formatted = formatter.format(utf8Decoded);
       } catch (FormatterException e) {
+        String error = e.getMessage();
         return FileInfo.create(
-            file, Files.getLastModifiedTime(file), content.length, FileState.INVALID);
+            file, Files.getLastModifiedTime(file), content.length, FileState.INVALID, error);
       }
       if (utf8Decoded.equals(formatted)) {
         logger.info("{}: UP-TO-DATE", file);

src/test/groovy/com/github/sherter/googlejavaformatgradleplugin/FileInfoDecoderTest.groovy

@@ -19,9 +19,9 @@ class FileInfoDecoderTest extends Specification {
 
         where:
         encoded | decoded
-        'foo,13000000,10,UNFORMATTED' | create(Paths.get('foo'), FileTime.fromMillis(13), 10, UNFORMATTED)
-        'b%2Cr,-231000000,0,INVALID' | create(Paths.get('b,r'), FileTime.fromMillis(-231), 0, INVALID)
-        '../foo/bar,0,0,FORMATTED' | create(Paths.get('../foo/bar'), FileTime.fromMillis(0), 0, FORMATTED)
+        'foo,13000000,10,UNFORMATTED,' | create(Paths.get('foo'), FileTime.fromMillis(13), 10, UNFORMATTED)
+        'b%2Cr,-231000000,0,INVALID,ZXJyb3I=' | create(Paths.get('b,r'), FileTime.fromMillis(-231), 0, INVALID, "error")
+        '../foo/bar,0,0,FORMATTED,' | create(Paths.get('../foo/bar'), FileTime.fromMillis(0), 0, FORMATTED)
     }
 
     def 'decode invalid'() {
@@ -37,9 +37,9 @@ class FileInfoDecoderTest extends Specification {
 
         where:
         encoded | errorMessage
-        'b,r,-231,0,UNKNOWN' | 'Invalid number of elements'
-        'b%r,231,0,UNKNOWN' | 'URLDecoder: Incomplete trailing escape (%) pattern'
-        'foo,0,0,STATE' | 'Not a valid state: STATE'
-        'foo,abc,0,STATE' | 'Not a valid long value: abc'
+        'b,r,t,-231,0,UNKNOWN,' | 'Invalid number of elements'
+        'b%r,231,0,UNKNOWN,' | 'URLDecoder: Incomplete trailing escape (%) pattern'
+        'foo,0,0,STATE,' | 'Not a valid state: STATE'
+        'foo,abc,0,STATE,' | 'Not a valid long value: abc'
     }
 }

src/test/groovy/com/github/sherter/googlejavaformatgradleplugin/FileInfoEncoderTest.groovy

@@ -7,6 +7,7 @@ import java.nio.file.Paths
 import java.nio.file.attribute.FileTime
 
 import static com.github.sherter.googlejavaformatgradleplugin.FileInfo.create
+import static com.github.sherter.googlejavaformatgradleplugin.FileState.INVALID
 import static com.github.sherter.googlejavaformatgradleplugin.FileState.UNFORMATTED
 
 class FileInfoEncoderTest extends Specification {
@@ -19,9 +20,11 @@ class FileInfoEncoderTest extends Specification {
 
         where:
         info | serialized
-        create(Paths.get('foo'), FileTime.fromMillis(0), 0, UNFORMATTED) | 'foo,0,0,UNFORMATTED'
+
+        create(Paths.get('foo'), FileTime.fromMillis(0), 0, INVALID, "error") | 'foo,0,0,INVALID,ZXJyb3I='
+        create(Paths.get('foo'), FileTime.fromMillis(0), 0, UNFORMATTED) | 'foo,0,0,UNFORMATTED,'
         create(Paths.get('/foo'), FileTime.fromMillis(0), 0, UNFORMATTED) |
                 (1..encoder.basePath.nameCount).inject('') {result, i -> result + '../'} +
-                'foo,0,0,UNFORMATTED'
+                'foo,0,0,UNFORMATTED,'
     }
 }

src/test/groovy/com/github/sherter/googlejavaformatgradleplugin/FileInfoStoreTest.groovy

@@ -25,17 +25,17 @@ class FileInfoStoreTest extends Specification {
         given:
         def store = new FileInfoStore(log, backingFile, encoder, decoder)
         Files.write(backingFile,
-                '''foo,0,0,FORMATTED
+                '''foo,0,0,FORMATTED,
                   |this-one-errors
-                  |baz,0,0,INVALID
+                  |baz,0,0,INVALID,ZXJyb3I=
                   |'''.stripMargin().getBytes(StandardCharsets.UTF_8))
 
         when:
         def readResult = store.read()
 
         then:
-        readResult.equals([ FileInfo.create(fs.getPath('foo'), FileTime.fromMillis(0), 0, FileState.FORMATTED),
-                            FileInfo.create(fs.getPath('baz'), FileTime.fromMillis(0), 0, FileState.INVALID) ] as Set)
+        readResult.equals([ FileInfo.create(fs.getPath('foo'), FileTime.fromMillis(0), 0, FileState.FORMATTED, ""),
+                            FileInfo.create(fs.getPath('baz'), FileTime.fromMillis(0), 0, FileState.INVALID,"error") ] as Set)
         1 * log.error(*_) >> { args ->
             assert MessageFormatter.arrayFormat(args[0], args[1]).message ==
                     "/base/storage.txt:2: couldn't decode 'this-one-errors': Invalid number of elements"
@@ -46,26 +46,26 @@ class FileInfoStoreTest extends Specification {
         given:
         def store = new FileInfoStore(log, backingFile, encoder, decoder)
         when:
-        store.update([ FileInfo.create(fs.getPath('foo'), FileTime.fromMillis(0), 0, FileState.FORMATTED),
+        store.update([ FileInfo.create(fs.getPath('foo'), FileTime.fromMillis(0), 0, FileState.FORMATTED, ""),
                        FileInfo.create(fs.getPath('baz'), FileTime.fromMillis(0), 0, FileState.UNFORMATTED),
-                       FileInfo.create(fs.getPath('foo'), FileTime.fromMillis(0), 10, FileState.INVALID)])
+                       FileInfo.create(fs.getPath('foo'), FileTime.fromMillis(0), 10, FileState.INVALID,"error")])
 
         then:
         def lines = Files.readAllLines(backingFile, StandardCharsets.UTF_8)
-        (lines as Set).equals([ 'baz,0,0,UNFORMATTED', 'foo,0,10,INVALID'] as Set)
+        (lines as Set).equals([ 'baz,0,0,UNFORMATTED,', 'foo,0,10,INVALID,ZXJyb3I='] as Set)
     }
 
     def 'replace existing info'() {
         given:
         def store = new FileInfoStore(log, backingFile, encoder, decoder)
-        Files.write(backingFile, 'foo,0,0,FORMATTED\n'.getBytes(StandardCharsets.UTF_8))
+        Files.write(backingFile, 'foo,0,0,FORMATTED,\n'.getBytes(StandardCharsets.UTF_8))
 
         when:
         store.update([ FileInfo.create(fs.getPath('foo'), FileTime.fromMillis(10), 10, FileState.UNFORMATTED) ])
 
         then:
         def lines = Files.readAllLines(backingFile, StandardCharsets.UTF_8)
-        (lines as Set).equals([ 'foo,10000000,10,UNFORMATTED' ] as Set)
+        (lines as Set).equals([ 'foo,10000000,10,UNFORMATTED,' ] as Set)
     }
 
     def 'create parent directories of backing file if necessary'() {

src/test/groovy/com/github/sherter/googlejavaformatgradleplugin/FormatFileCallableTest.groovy

@@ -35,7 +35,7 @@ class FormatFileCallableTest extends Specification {
         def result = task.call()
 
         then:
-        1 * formatter.format('Hello World!') >> { throw new FormatterException() }
+        1 * formatter.format('Hello World!') >> { throw new FormatterException("", new Exception()) }
         result.path() == file
         result.state() == FileState.INVALID
         result.lastModified() == modifiedTime

src/test/groovy/com/github/sherter/googlejavaformatgradleplugin/VerifyFileCallableTest.groovy

@@ -34,7 +34,7 @@ class VerifyFileCallableTest extends Specification {
         def result = task.call()
 
         then:
-        1 * formatter.format('foo') >> { throw new FormatterException() }
+        1 * formatter.format('foo') >> { throw new FormatterException("error", new Exception()) }
         result.state() == FileState.INVALID
         result.lastModified() == modified
         result.size() == Files.size(file)

subprojects/format/src/main/groovy/com/github/sherter/googlejavaformatgradleplugin/format/FormatterException.java

@@ -2,5 +2,7 @@
 
 /** Wraps any exceptions thrown by implementations of the {@link Formatter} interface. */
 public class FormatterException extends Exception {
-  FormatterException() {}
+  FormatterException(String message, Throwable cause) {
+    super(message, cause);
+  }
 }

subprojects/format/src/main/groovy/com/github/sherter/googlejavaformatgradleplugin/format/OneDotOneFactory.groovy

@@ -25,15 +25,22 @@ class OneDotOneFactory extends AbstractFormatterFactory {
         def reorderImports = constructReorderImportsClosure()
         def removeUnusedImports = constructRemoveUnusedImportsClosure()
         return { String source ->
+            Error cause;
             try {
                 def tmp = reorderImports.call(source)
                 tmp = removeUnusedImports.call(tmp)
                 return formatter.formatSource(tmp)
             } catch (e) {
-                throw new FormatterException()
+                if ("com.google.googlejavaformat.java.FormatterException".equals(e.getClass().getCanonicalName())) {
+                    String error = "Google Java Formatter error: " + e.getMessage()
+                    throw new FormatterException(error, e)
+                }
+                cause = e; // Unknown error
             } catch (Error e) {
-                throw new FormatterException()
+                cause = e; // Unknown error
             }
+            String error = "An unexpected error happened: " + cause.toString()
+            throw new FormatterException(error, cause)
         }
     }
 

subprojects/format/src/main/groovy/com/github/sherter/googlejavaformatgradleplugin/format/OneDotZeroFactory.groovy

@@ -24,12 +24,21 @@ final class OneDotZeroFactory extends AbstractFormatterFactory {
         def formatter = constructFormatter()
         def reorderImports = constructReorderImportsClosure()
         return { String source ->
+            Error cause;
             try {
                 def importOrderedSource = reorderImports.call(source)
                 return formatter.formatSource(importOrderedSource)
             } catch (e) {
-                throw new FormatterException()
+                if ("com.google.googlejavaformat.java.FormatterException".equals(e.getClass().getCanonicalName())) {
+                    String error = "Google Java Formatter error: " + e.getMessage()
+                    throw new FormatterException(error, e)
+                }
+                cause = e; // Unknown error
+            } catch (Error e) {
+                cause = e; // Unknown error
             }
+            String error = "An unexpected error happened: " + cause.toString()
+            throw new FormatterException(error, cause)
         }
     }