Repository: checkstyle/checkstyle

Problem statement:
JavadocMetadataScraper fails silently if it can't write the XML files
Identified through https://github.com/sevntu-checkstyle/sevntu.checkstyle/issues/857 ,

At https://github.com/checkstyle/checkstyle/blob/13481f2c410e4944ecf5ab93ec49948a523a0c82/src/main/java/com/puppycrawl/tools/checkstyle/meta/XmlMetaWriter.java#L179 , checkstyle will fail silently if there are no descriptions to write for the file it scrapped.

I was expecting the files to be generated, or give me an error why they couldn't be generated. Since there was no message and no files, this required me to debug through the code to find out why it wasn't generating my files or complaining.

I feel we should print an error message while continuing to process other files.

I also feel `MetadataGeneratorUtil` should report any errors back somehow so the client and calling tests can handle the situation appropriately.


Hints:
I am working on it.
The issue is split into 2 PRs. The first will add the error message to the module. The second will have the util print out the errors to the user when it is run.
@Vyom-Yadav, what is left to be done ?
@romani, Currently the error messages will not be shown to users as `JavadovMetadataScraper` is not executed as a normal check on all the files, we have to separately run it on all the files in `MetadataGeneratorUtilTest`. So modifying `generate(String... args)` method in `MetadataGeneratorUtil` to give the user the violations by adding `DefaultLogger` and a few more modifications. I will send a PR shortly. 

Patch:
diff --git a/src/main/java/com/puppycrawl/tools/checkstyle/MetadataGeneratorLogger.java b/src/main/java/com/puppycrawl/tools/checkstyle/MetadataGeneratorLogger.java
new file mode 100644
--- /dev/null
+++ b/src/main/java/com/puppycrawl/tools/checkstyle/MetadataGeneratorLogger.java
@@ -0,0 +1,111 @@
+////////////////////////////////////////////////////////////////////////////////
+// checkstyle: Checks Java source code for adherence to a set of rules.
+// Copyright (C) 2001-2022 the original author or authors.
+//
+// This library is free software; you can redistribute it and/or
+// modify it under the terms of the GNU Lesser General Public
+// License as published by the Free Software Foundation; either
+// version 2.1 of the License, or (at your option) any later version.
+//
+// This library is distributed in the hope that it will be useful,
+// but WITHOUT ANY WARRANTY; without even the implied warranty of
+// MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the GNU
+// Lesser General Public License for more details.
+//
+// You should have received a copy of the GNU Lesser General Public
+// License along with this library; if not, write to the Free Software
+// Foundation, Inc., 59 Temple Place, Suite 330, Boston, MA  02111-1307  USA
+////////////////////////////////////////////////////////////////////////////////
+
+package com.puppycrawl.tools.checkstyle;
+
+import java.io.OutputStream;
+import java.io.OutputStreamWriter;
+import java.io.PrintWriter;
+import java.io.Writer;
+import java.nio.charset.StandardCharsets;
+
+import com.puppycrawl.tools.checkstyle.api.AuditEvent;
+import com.puppycrawl.tools.checkstyle.api.AuditListener;
+import com.puppycrawl.tools.checkstyle.api.AutomaticBean;
+import com.puppycrawl.tools.checkstyle.api.SeverityLevel;
+
+/**
+ * Simple logger for metadata generator util.
+ */
+public class MetadataGeneratorLogger extends AutomaticBean implements AuditListener {
+
+    /**
+     * Where to write error messages.
+     */
+    private final PrintWriter errorWriter;
+
+    /**
+     * Formatter for the log message.
+     */
+    private final AuditEventFormatter formatter;
+
+    /**
+     * Close output stream in audit finished.
+     */
+    private final boolean closeErrorWriter;
+
+    /**
+     * Creates a new MetadataGeneratorLogger instance.
+     *
+     * @param outputStream where to log audit events
+     * @param outputStreamOptions if {@code CLOSE} error should be closed in auditFinished()
+     */
+    public MetadataGeneratorLogger(OutputStream outputStream,
+            OutputStreamOptions outputStreamOptions) {
+        final Writer errorStreamWriter = new OutputStreamWriter(outputStream,
+                StandardCharsets.UTF_8);
+        errorWriter = new PrintWriter(errorStreamWriter);
+        formatter = new AuditEventDefaultFormatter();
+        closeErrorWriter = outputStreamOptions == OutputStreamOptions.CLOSE;
+    }
+
+    @Override
+    public void auditStarted(AuditEvent event) {
+        errorWriter.flush();
+    }
+
+    @Override
+    public void auditFinished(AuditEvent event) {
+        errorWriter.flush();
+        if (closeErrorWriter) {
+            errorWriter.close();
+        }
+    }
+
+    @Override
+    public void fileStarted(AuditEvent event) {
+        // No code by default.
+    }
+
+    @Override
+    public void fileFinished(AuditEvent event) {
+        errorWriter.flush();
+    }
+
+    @Override
+    public void addError(AuditEvent event) {
+        final SeverityLevel severityLevel = event.getSeverityLevel();
+        if (severityLevel != SeverityLevel.IGNORE) {
+            final String errorMessage = formatter.format(event);
+            errorWriter.println(errorMessage);
+        }
+    }
+
+    @Override
+    public void addException(AuditEvent event, Throwable throwable) {
+        synchronized (errorWriter) {
+            throwable.printStackTrace(errorWriter);
+        }
+    }
+
+    @Override
+    protected void finishLocalSetup() {
+        // No code by default.
+    }
+}
diff --git a/src/main/java/com/puppycrawl/tools/checkstyle/meta/JavadocMetadataScraper.java b/src/main/java/com/puppycrawl/tools/checkstyle/meta/JavadocMetadataScraper.java
--- a/src/main/java/com/puppycrawl/tools/checkstyle/meta/JavadocMetadataScraper.java
+++ b/src/main/java/com/puppycrawl/tools/checkstyle/meta/JavadocMetadataScraper.java
@@ -236,7 +236,9 @@ public void finishJavadocTree(DetailNode rootAst) {
         moduleDetails.setDescription(getDescriptionText());
         if (isTopLevelClassJavadoc()) {
             if (moduleDetails.getDescription().isEmpty()) {
-                log(rootAst.getLineNumber(), MSG_DESC_MISSING, moduleDetails.getName());
+                final String fullQualifiedName = moduleDetails.getFullQualifiedName();
+                log(rootAst.getLineNumber(), MSG_DESC_MISSING,
+                        fullQualifiedName.substring(fullQualifiedName.lastIndexOf('.') + 1));
             }
             else if (writeXmlOutput) {
                 try {
diff --git a/src/main/java/com/puppycrawl/tools/checkstyle/meta/MetadataGeneratorUtil.java b/src/main/java/com/puppycrawl/tools/checkstyle/meta/MetadataGeneratorUtil.java
--- a/src/main/java/com/puppycrawl/tools/checkstyle/meta/MetadataGeneratorUtil.java
+++ b/src/main/java/com/puppycrawl/tools/checkstyle/meta/MetadataGeneratorUtil.java
@@ -21,6 +21,7 @@
 
 import java.io.File;
 import java.io.IOException;
+import java.io.OutputStream;
 import java.nio.charset.StandardCharsets;
 import java.nio.file.Files;
 import java.nio.file.Path;
@@ -32,7 +33,9 @@
 
 import com.puppycrawl.tools.checkstyle.Checker;
 import com.puppycrawl.tools.checkstyle.DefaultConfiguration;
+import com.puppycrawl.tools.checkstyle.MetadataGeneratorLogger;
 import com.puppycrawl.tools.checkstyle.TreeWalker;
+import com.puppycrawl.tools.checkstyle.api.AutomaticBean;
 import com.puppycrawl.tools.checkstyle.api.CheckstyleException;
 
 /** Class which handles all the metadata generation and writing calls. */
@@ -46,11 +49,12 @@ private MetadataGeneratorUtil() {
      * Generate metadata from the module source files available in the input argument path.
      *
      * @param path arguments
+     * @param out OutputStream for error messages
      * @param moduleFolders folders to check
      * @throws IOException ioException
      * @throws CheckstyleException checkstyleException
      */
-    public static void generate(String path, String... moduleFolders)
+    public static void generate(String path, OutputStream out, String... moduleFolders)
             throws IOException, CheckstyleException {
         JavadocMetadataScraper.resetModuleDetailsStore();
 
@@ -58,13 +62,18 @@ public static void generate(String path, String... moduleFolders)
         checker.setModuleClassLoader(Checker.class.getClassLoader());
         final DefaultConfiguration scraperCheckConfig =
                         new DefaultConfiguration(JavadocMetadataScraper.class.getName());
-        final DefaultConfiguration defaultConfiguration = new DefaultConfiguration("configuration");
+        final DefaultConfiguration defaultConfiguration =
+                new DefaultConfiguration("configuration");
         final DefaultConfiguration treeWalkerConfig =
                 new DefaultConfiguration(TreeWalker.class.getName());
         defaultConfiguration.addProperty("charset", StandardCharsets.UTF_8.name());
         defaultConfiguration.addChild(treeWalkerConfig);
         treeWalkerConfig.addChild(scraperCheckConfig);
         checker.configure(defaultConfiguration);
+
+        checker.addListener(new MetadataGeneratorLogger(out,
+                AutomaticBean.OutputStreamOptions.NONE));
+
         dumpMetadata(checker, path, moduleFolders);
     }
 
@@ -94,7 +103,6 @@ private static void dumpMetadata(Checker checker, String path, String... moduleF
                         .collect(Collectors.toList()));
             }
         }
-
         checker.process(validFiles);
     }
 }


Test patch:
diff --git a/src/test/java/com/puppycrawl/tools/checkstyle/MetadataGeneratorLoggerTest.java b/src/test/java/com/puppycrawl/tools/checkstyle/MetadataGeneratorLoggerTest.java
new file mode 100644
--- /dev/null
+++ b/src/test/java/com/puppycrawl/tools/checkstyle/MetadataGeneratorLoggerTest.java
@@ -0,0 +1,112 @@
+////////////////////////////////////////////////////////////////////////////////
+// checkstyle: Checks Java source code for adherence to a set of rules.
+// Copyright (C) 2001-2022 the original author or authors.
+//
+// This library is free software; you can redistribute it and/or
+// modify it under the terms of the GNU Lesser General Public
+// License as published by the Free Software Foundation; either
+// version 2.1 of the License, or (at your option) any later version.
+//
+// This library is distributed in the hope that it will be useful,
+// but WITHOUT ANY WARRANTY; without even the implied warranty of
+// MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the GNU
+// Lesser General Public License for more details.
+//
+// You should have received a copy of the GNU Lesser General Public
+// License along with this library; if not, write to the Free Software
+// Foundation, Inc., 59 Temple Place, Suite 330, Boston, MA  02111-1307  USA
+////////////////////////////////////////////////////////////////////////////////
+
+package com.puppycrawl.tools.checkstyle;
+
+import static com.google.common.truth.Truth.assertWithMessage;
+
+import java.io.ByteArrayOutputStream;
+import java.io.IOException;
+import java.io.OutputStream;
+
+import org.junit.jupiter.api.Test;
+
+import com.puppycrawl.tools.checkstyle.api.AuditEvent;
+import com.puppycrawl.tools.checkstyle.api.AutomaticBean;
+import com.puppycrawl.tools.checkstyle.api.SeverityLevel;
+import com.puppycrawl.tools.checkstyle.api.Violation;
+import com.puppycrawl.tools.checkstyle.internal.utils.CloseAndFlushTestByteArrayOutputStream;
+
+public class MetadataGeneratorLoggerTest {
+
+    @Test
+    public void testIgnoreSeverityLevel() {
+        final OutputStream outputStream = new ByteArrayOutputStream();
+        final MetadataGeneratorLogger logger = new MetadataGeneratorLogger(outputStream,
+                AutomaticBean.OutputStreamOptions.CLOSE);
+        final AuditEvent event = new AuditEvent(this, "fileName",
+                new Violation(1, 2, "bundle", "key",
+                        null, SeverityLevel.IGNORE, null, getClass(), "customViolation"));
+        logger.finishLocalSetup();
+        logger.addError(event);
+        logger.auditFinished(event);
+        assertWithMessage("Output stream should be empty")
+                .that(outputStream.toString())
+                .isEmpty();
+    }
+
+    @Test
+    public void testAddException() {
+        final OutputStream outputStream = new ByteArrayOutputStream();
+        final MetadataGeneratorLogger logger = new MetadataGeneratorLogger(outputStream,
+                AutomaticBean.OutputStreamOptions.CLOSE);
+        final AuditEvent event = new AuditEvent(1);
+        logger.addException(event, new IllegalStateException("Test Exception"));
+        logger.auditFinished(event);
+        assertWithMessage("Violation should contain exception message")
+                .that(outputStream.toString())
+                .contains("java.lang.IllegalStateException: Test Exception");
+    }
+
+    @Test
+    public void testClose() throws IOException {
+        try (CloseAndFlushTestByteArrayOutputStream outputStream =
+                     new CloseAndFlushTestByteArrayOutputStream()) {
+            final MetadataGeneratorLogger logger = new MetadataGeneratorLogger(outputStream,
+                    AutomaticBean.OutputStreamOptions.CLOSE);
+            logger.auditFinished(new AuditEvent(1));
+            assertWithMessage("Unexpected close count")
+                    .that(outputStream.getCloseCount())
+                    .isEqualTo(1);
+        }
+    }
+
+    @Test
+    public void testCloseOutputStreamOptionNone() throws IOException {
+        try (CloseAndFlushTestByteArrayOutputStream outputStream =
+                     new CloseAndFlushTestByteArrayOutputStream()) {
+            final MetadataGeneratorLogger logger = new MetadataGeneratorLogger(outputStream,
+                    AutomaticBean.OutputStreamOptions.NONE);
+            final AuditEvent event = new AuditEvent(1);
+            logger.auditFinished(event);
+            assertWithMessage("Unexpected close count")
+                    .that(outputStream.getCloseCount())
+                    .isEqualTo(0);
+        }
+    }
+
+    @Test
+    public void testFlushStreams() throws Exception {
+        try (CloseAndFlushTestByteArrayOutputStream outputStream =
+                     new CloseAndFlushTestByteArrayOutputStream()) {
+            final MetadataGeneratorLogger logger = new MetadataGeneratorLogger(outputStream,
+                    AutomaticBean.OutputStreamOptions.NONE);
+            final AuditEvent event = new AuditEvent(1);
+            logger.auditStarted(event);
+            logger.fileFinished(event);
+            assertWithMessage("Unexpected flush count")
+                    .that(outputStream.getFlushCount())
+                    .isEqualTo(2);
+            logger.auditFinished(event);
+            assertWithMessage("Unexpected flush count")
+                    .that(outputStream.getFlushCount())
+                    .isEqualTo(3);
+        }
+    }
+}
diff --git a/src/test/java/com/puppycrawl/tools/checkstyle/meta/JavadocMetadataScraperTest.java b/src/test/java/com/puppycrawl/tools/checkstyle/meta/JavadocMetadataScraperTest.java
--- a/src/test/java/com/puppycrawl/tools/checkstyle/meta/JavadocMetadataScraperTest.java
+++ b/src/test/java/com/puppycrawl/tools/checkstyle/meta/JavadocMetadataScraperTest.java
@@ -199,7 +199,7 @@ public void testEmptyDescription() throws Exception {
 
         final String[] expected = {
             "19: " + getCheckMessage(MSG_DESC_MISSING,
-                    "InputJavadocMetadataScraperAbstractSuper"),
+                    "InputJavadocMetadataScraperAbstractSuperCheck"),
         };
         verifyWithInlineConfigParser(getPath(
                 "InputJavadocMetadataScraperAbstractSuperCheck.java"), expected);
diff --git a/src/test/java/com/puppycrawl/tools/checkstyle/meta/MetadataGeneratorUtilTest.java b/src/test/java/com/puppycrawl/tools/checkstyle/meta/MetadataGeneratorUtilTest.java
--- a/src/test/java/com/puppycrawl/tools/checkstyle/meta/MetadataGeneratorUtilTest.java
+++ b/src/test/java/com/puppycrawl/tools/checkstyle/meta/MetadataGeneratorUtilTest.java
@@ -20,6 +20,7 @@
 package com.puppycrawl.tools.checkstyle.meta;
 
 import static com.google.common.truth.Truth.assertWithMessage;
+import static com.puppycrawl.tools.checkstyle.meta.JavadocMetadataScraper.MSG_DESC_MISSING;
 
 import java.nio.file.Files;
 import java.nio.file.Path;
@@ -28,14 +29,21 @@
 import java.util.LinkedHashSet;
 import java.util.List;
 import java.util.Set;
+import java.util.regex.Matcher;
+import java.util.regex.Pattern;
 import java.util.stream.Collectors;
 import java.util.stream.Stream;
 
+import org.itsallcode.io.Capturable;
+import org.itsallcode.junit.sysextensions.SystemOutGuard;
 import org.junit.jupiter.api.Test;
+import org.junit.jupiter.api.extension.ExtendWith;
 
+import com.puppycrawl.tools.checkstyle.AbstractModuleTestSupport;
 import com.puppycrawl.tools.checkstyle.internal.utils.CheckUtil;
 
-public final class MetadataGeneratorUtilTest {
+@ExtendWith(SystemOutGuard.class)
+public final class MetadataGeneratorUtilTest extends AbstractModuleTestSupport {
 
     private final List<String> modulesContainingNoMetadataFile = Arrays.asList(
             "Checker",
@@ -43,14 +51,55 @@ public final class MetadataGeneratorUtilTest {
             "JavadocMetadataScraper"
     );
 
+    @Override
+    protected String getPackageLocation() {
+        return null;
+    }
+
+    /**
+     * Generates metadata for checkstyle modules and verifies number of
+     * generated metadata modules match the number of checkstyle modules.
+     * Also verifies whether every checkstyle module contains description.
+     *
+     * @param systemOut wrapper for {@code System.out}
+     * @throws Exception if exception occurs during generating metadata or
+     *                   if an I/O error is thrown when accessing the starting file.
+     * @noinspection UseOfSystemOutOrSystemErr
+     */
     @Test
-    public void testMetadataFilesGenerationAllFiles() throws Exception {
+    public void testMetadataFilesGenerationAllFiles(@SystemOutGuard.SysOut Capturable systemOut)
+            throws Exception {
+        systemOut.captureMuted();
 
         MetadataGeneratorUtil.generate(System.getProperty("user.dir")
                         + "/src/main/java/com/puppycrawl/tools/checkstyle",
-                "checks", "filters", "filefilters");
-        final Set<String> metaFiles;
+                System.out, "checks", "filters", "filefilters");
 
+        final String[] expectedErrorMessages = {
+            "31: " + getCheckMessage(MSG_DESC_MISSING, "AbstractSuperCheck"),
+            "45: " + getCheckMessage(MSG_DESC_MISSING, "AbstractHeaderCheck"),
+            "42: " + getCheckMessage(MSG_DESC_MISSING, "AbstractJavadocCheck"),
+            "45: " + getCheckMessage(MSG_DESC_MISSING, "AbstractClassCouplingCheck"),
+            "26: " + getCheckMessage(MSG_DESC_MISSING, "AbstractAccessControlNameCheck"),
+            "30: " + getCheckMessage(MSG_DESC_MISSING, "AbstractNameCheck"),
+            "29: " + getCheckMessage(MSG_DESC_MISSING, "AbstractParenPadCheck"),
+        };
+
+        final String[] actualViolations = systemOut.getCapturedData().split("\\n");
+        final Pattern violationExtractingPattern = Pattern.compile("((?<=:)\\d.*:.*(?=\\s\\[))");
+
+        Arrays.setAll(actualViolations, id -> {
+            final Matcher matcher = violationExtractingPattern.matcher(actualViolations[id]);
+            matcher.find();
+            return matcher.group(1);
+        });
+
+        assertWithMessage("Expected and actual errors do not match")
+                .that(expectedErrorMessages)
+                .asList()
+                .containsExactlyElementsIn(actualViolations);
+
+        final Set<String> metaFiles;
         try (Stream<Path> fileStream = Files.walk(
                 Paths.get(System.getProperty("user.dir") + "/src/main/resources/com/puppycrawl"
                         + "/tools/checkstyle/meta"))) {
@@ -67,8 +116,10 @@ public void testMetadataFilesGenerationAllFiles() throws Exception {
                 .sorted()
                 .collect(Collectors.toCollection(LinkedHashSet::new));
         checkstyleModules.removeAll(modulesContainingNoMetadataFile);
-        assertWithMessage("Number of generated metadata files dont match with number of checkstyle "
-                + "module").that(metaFiles).isEqualTo(checkstyleModules);
+        assertWithMessage("Number of generated metadata files dont match with "
+                + "number of checkstyle module")
+                .that(metaFiles)
+                .isEqualTo(checkstyleModules);
     }
 
     /**