Restrict use of standard streams (#8816)

* Added architecture test * Refined test * Applied test results * Add reason * Update src/test/java/org/jabref/logic/exporter/ModsExportFormatFilesTest.java * Update src/test/java/org/jabref/logic/util/io/FileUtilTest.java * Checkstyle Co-authored-by: Christoph <[email protected]>
JabRef · May 17, 2022 · 82aad89 · 82aad89
1 parent e0460dd
commit 82aad89
Show file tree

Hide file tree

Showing 8 changed files with 57 additions and 19 deletions.
diff --git a/src/main/java/org/jabref/architecture/AllowedToUseStandardStreams.java b/src/main/java/org/jabref/architecture/AllowedToUseStandardStreams.java
@@ -0,0 +1,10 @@
+package org.jabref.architecture;
+
+/**
+ * Annotation to indicate that this class can use System.Out.* instead of using the logging framework
+ */
+public @interface AllowedToUseStandardStreams {
+
+    // The rationale
+    String value();
+}
diff --git a/src/main/java/org/jabref/gui/desktop/os/Linux.java b/src/main/java/org/jabref/gui/desktop/os/Linux.java
@@ -28,17 +28,17 @@ private void nativeOpenFile(String filePath) {
             try {
                 File file = new File(filePath);
                 Desktop.getDesktop().open(file);
-                System.out.println("Open file in default application with Desktop integration");
+                LOGGER.debug("Open file in default application with Desktop integration");
             } catch (IllegalArgumentException e) {
-                System.out.println("Fail back to xdg-open");
+                LOGGER.debug("Fail back to xdg-open");
                 try {
                     String[] cmd = {"xdg-open", filePath};
                     Runtime.getRuntime().exec(cmd);
                 } catch (Exception e2) {
-                    System.out.println("Open operation not successful: " + e2);
+                    LOGGER.warn("Open operation not successful: " + e2);
                 }
             } catch (IOException e) {
-                System.out.println("Native open operation not successful: " + e);
+                LOGGER.warn("Native open operation not successful: " + e);
             }
         });
     }

diff --git a/src/main/java/org/jabref/logic/util/io/XMLUtil.java b/src/main/java/org/jabref/logic/util/io/XMLUtil.java
@@ -14,6 +14,8 @@
 import javax.xml.transform.dom.DOMSource;
 import javax.xml.transform.stream.StreamResult;
 
+import org.jabref.architecture.AllowedToUseStandardStreams;
+
 import org.slf4j.Logger;
 import org.slf4j.LoggerFactory;
 import org.w3c.dom.Document;
@@ -25,6 +27,7 @@
 /**
  * Currently used for debugging only
  */
+@AllowedToUseStandardStreams("Used for debugging only")
 public class XMLUtil {
     private static final Logger LOGGER = LoggerFactory.getLogger(XMLUtil.class);
 

diff --git a/src/test/java/org/jabref/architecture/MainArchitectureTests.java b/src/test/java/org/jabref/architecture/MainArchitectureTests.java
@@ -6,6 +6,7 @@
 import com.tngtech.archunit.junit.AnalyzeClasses;
 import com.tngtech.archunit.junit.ArchIgnore;
 import com.tngtech.archunit.junit.ArchTest;
+import com.tngtech.archunit.library.GeneralCodingRules;
 
 import static com.tngtech.archunit.lang.syntax.ArchRuleDefinition.noClasses;
 import static com.tngtech.archunit.library.Architectures.layeredArchitecture;
@@ -20,6 +21,7 @@ class MainArchitectureTests {
     private static final String PACKAGE_ORG_JABREF_GUI = "org.jabref.gui..";
     private static final String PACKAGE_ORG_JABREF_LOGIC = "org.jabref.logic..";
     private static final String PACKAGE_ORG_JABREF_MODEL = "org.jabref.model..";
+    private static final String PACKAGE_ORG_JABREF_CLI = "org.jabref.cli..";
 
     @ArchTest
     public static void doNotUseApacheCommonsLang3(JavaClasses classes) {
@@ -92,7 +94,7 @@ public static void respectLayeredArchitecture(JavaClasses classes) {
                 .layer("Gui").definedBy(PACKAGE_ORG_JABREF_GUI)
                 .layer("Logic").definedBy(PACKAGE_ORG_JABREF_LOGIC)
                 .layer("Model").definedBy(PACKAGE_ORG_JABREF_MODEL)
-                .layer("Cli").definedBy("org.jabref.cli..")
+                .layer("Cli").definedBy(PACKAGE_ORG_JABREF_CLI)
                 .layer("Migrations").definedBy("org.jabref.migrations..") // TODO: Move to logic
                 .layer("Preferences").definedBy("org.jabref.preferences..")
                 .layer("Styletester").definedBy("org.jabref.styletester..")
@@ -135,4 +137,14 @@ public static void restrictUsagesInLogic(JavaClasses classes) {
                    .orShould().dependOnClassesThat().haveFullyQualifiedName(CLASS_ORG_JABREF_GLOBALS)
                    .check(classes);
     }
+
+    @ArchTest
+    public static void restrictStandardStreams(JavaClasses classes) {
+        noClasses().that().resideOutsideOfPackages(PACKAGE_ORG_JABREF_CLI)
+                   .and().resideOutsideOfPackages("org.jabref.gui.openoffice..") // Uses LibreOffice SDK
+                   .and().areNotAnnotatedWith(AllowedToUseStandardStreams.class)
+                   .should(GeneralCodingRules.ACCESS_STANDARD_STREAMS)
+                   .because("logging framework should be used instead or the class be marked explicitly as @AllowedToUseStandardStreams")
+                   .check(classes);
+    }
 }
diff --git a/...c/exporter/ModsExportFormatTestFiles.java → ...c/exporter/ModsExportFormatFilesTest.java b/...c/exporter/ModsExportFormatTestFiles.java → ...c/exporter/ModsExportFormatFilesTest.java
@@ -5,7 +5,6 @@
 import java.nio.file.Files;
 import java.nio.file.Path;
 import java.util.List;
-import java.util.stream.Collectors;
 import java.util.stream.Stream;
 
 import org.jabref.logic.bibtex.BibEntryAssert;
@@ -22,14 +21,18 @@
 import org.junit.jupiter.params.provider.MethodSource;
 import org.mockito.Answers;
 import org.mockito.Mockito;
+import org.slf4j.Logger;
+import org.slf4j.LoggerFactory;
 
 import static org.junit.jupiter.api.Assertions.assertEquals;
 import static org.mockito.Mockito.mock;
 
-public class ModsExportFormatTestFiles {
-
+public class ModsExportFormatFilesTest {
+    private static final Logger LOGGER = LoggerFactory.getLogger(ModsExportFormatFilesTest.class);
     private static Path resourceDir;
+
     public Charset charset;
+
     private BibDatabaseContext databaseContext;
     private Path exportedFile;
     private ModsExporter exporter;
@@ -39,12 +42,11 @@ public class ModsExportFormatTestFiles {
 
     public static Stream<String> fileNames() throws Exception {
         resourceDir = Path.of(MSBibExportFormatTestFiles.class.getResource("ModsExportFormatTestAllFields.bib").toURI()).getParent();
-        System.out.println(resourceDir);
+        LOGGER.debug("Mods export resouce dir {}", resourceDir);
 
         try (Stream<Path> stream = Files.list(resourceDir)) {
-            //            stream.forEach(n -> System.out.println(n));
             return stream.map(n -> n.getFileName().toString()).filter(n -> n.endsWith(".bib"))
-                         .filter(n -> n.startsWith("Mods")).collect(Collectors.toList()).stream();
+                         .filter(n -> n.startsWith("Mods")).toList().stream();
         }
     }
 
@@ -65,10 +67,10 @@ public void setUp(@TempDir Path testFolder) throws Exception {
     @ParameterizedTest
     @MethodSource("fileNames")
     public final void testPerformExport(String filename) throws Exception {
-        importFile = Path.of(ModsExportFormatTestFiles.class.getResource(filename).toURI());
+        importFile = Path.of(ModsExportFormatFilesTest.class.getResource(filename).toURI());
         String xmlFileName = filename.replace(".bib", ".xml");
         List<BibEntry> entries = bibtexImporter.importDatabase(importFile).getDatabase().getEntries();
-        Path expectedFile = Path.of(ModsExportFormatTestFiles.class.getResource(xmlFileName).toURI());
+        Path expectedFile = Path.of(ModsExportFormatFilesTest.class.getResource(xmlFileName).toURI());
 
         exporter.export(databaseContext, exportedFile, entries);
 
@@ -80,7 +82,7 @@ public final void testPerformExport(String filename) throws Exception {
     @ParameterizedTest
     @MethodSource("fileNames")
     public final void testExportAsModsAndThenImportAsMods(String filename) throws Exception {
-        importFile = Path.of(ModsExportFormatTestFiles.class.getResource(filename).toURI());
+        importFile = Path.of(ModsExportFormatFilesTest.class.getResource(filename).toURI());
         List<BibEntry> entries = bibtexImporter.importDatabase(importFile).getDatabase().getEntries();
 
         exporter.export(databaseContext, exportedFile, entries);
@@ -90,9 +92,9 @@ public final void testExportAsModsAndThenImportAsMods(String filename) throws Ex
     @ParameterizedTest
     @MethodSource("fileNames")
     public final void testImportAsModsAndExportAsMods(String filename) throws Exception {
-        importFile = Path.of(ModsExportFormatTestFiles.class.getResource(filename).toURI());
+        importFile = Path.of(ModsExportFormatFilesTest.class.getResource(filename).toURI());
         String xmlFileName = filename.replace(".bib", ".xml");
-        Path xmlFile = Path.of(ModsExportFormatTestFiles.class.getResource(xmlFileName).toURI());
+        Path xmlFile = Path.of(ModsExportFormatFilesTest.class.getResource(xmlFileName).toURI());
 
         List<BibEntry> entries = modsImporter.importDatabase(xmlFile).getDatabase().getEntries();
 

diff --git a/src/test/java/org/jabref/logic/git/SlrGitHandlerTest.java b/src/test/java/org/jabref/logic/git/SlrGitHandlerTest.java
@@ -10,10 +10,14 @@
 import org.junit.jupiter.api.BeforeEach;
 import org.junit.jupiter.api.Test;
 import org.junit.jupiter.api.io.TempDir;
+import org.slf4j.Logger;
+import org.slf4j.LoggerFactory;
 
 import static org.junit.jupiter.api.Assertions.assertEquals;
 
 class SlrGitHandlerTest {
+    private static final Logger LOGGER = LoggerFactory.getLogger(SlrGitHandlerTest.class);
+
     @TempDir
     Path repositoryPath;
     private SlrGitHandler gitHandler;
@@ -44,7 +48,7 @@ void calculateDiffOnBranch() throws IOException, GitAPIException {
         Files.writeString(Path.of(repositoryPath.toString(), "TestFolder", "Test1.txt"), "This is a new line of text 2\n" + Files.readString(Path.of(repositoryPath.toString(), "TestFolder", "Test1.txt")));
         gitHandler.createCommitOnCurrentBranch("Commit 2 on branch1", false);
 
-        System.out.println(gitHandler.calculatePatchOfNewSearchResults("branch1"));
+        LOGGER.debug(gitHandler.calculatePatchOfNewSearchResults("branch1"));
         assertEquals(expectedPatch, gitHandler.calculatePatchOfNewSearchResults("branch1"));
     }
 

diff --git a/src/test/java/org/jabref/logic/net/URLDownloadTest.java b/src/test/java/org/jabref/logic/net/URLDownloadTest.java
@@ -11,12 +11,15 @@
 
 import kong.unirest.UnirestException;
 import org.junit.jupiter.api.Test;
+import org.slf4j.Logger;
+import org.slf4j.LoggerFactory;
 
 import static org.junit.jupiter.api.Assertions.assertNotNull;
 import static org.junit.jupiter.api.Assertions.assertThrows;
 import static org.junit.jupiter.api.Assertions.assertTrue;
 
 public class URLDownloadTest {
+    private static final Logger LOGGER = LoggerFactory.getLogger(URLDownloadTest.class);
 
     @Test
     public void testStringDownloadWithSetEncoding() throws IOException {
@@ -42,7 +45,7 @@ public void testFileDownload() throws IOException {
         } finally {
             // cleanup
             if (!destination.delete()) {
-                System.err.println("Cannot delete downloaded file");
+                LOGGER.error("Cannot delete downloaded file");
             }
         }
     }

diff --git a/src/test/java/org/jabref/logic/util/io/FileUtilTest.java b/src/test/java/org/jabref/logic/util/io/FileUtilTest.java
@@ -20,6 +20,8 @@
 import org.junit.jupiter.api.Test;
 import org.junit.jupiter.api.io.TempDir;
 import org.mockito.Answers;
+import org.slf4j.Logger;
+import org.slf4j.LoggerFactory;
 
 import static org.junit.jupiter.api.Assertions.assertEquals;
 import static org.junit.jupiter.api.Assertions.assertFalse;
@@ -29,6 +31,8 @@
 import static org.mockito.Mockito.mock;
 
 class FileUtilTest {
+    private static final Logger LOGGER = LoggerFactory.getLogger(FileUtilTest.class);
+
     private final Path nonExistingTestPath = Path.of("nonExistingTestPath");
     private Path existingTestFile;
     private Path otherExistingTestFile;
@@ -319,7 +323,7 @@ void testRenameFileSuccessful(@TempDir Path otherTemporaryFolder) {
         // in the @BeforeEach method.
         Path temp = Path.of(otherTemporaryFolder.resolve("123").toString());
 
-        System.out.println(temp);
+        LOGGER.debug("Temp dir {}", temp);
         FileUtil.renameFile(existingTestFile, temp);
         assertFalse(Files.exists(existingTestFile));
     }