diff --git a/build-logic/src/main/kotlin/org.jabref.gradle.base.dependency-rules.gradle.kts b/build-logic/src/main/kotlin/org.jabref.gradle.base.dependency-rules.gradle.kts index e9ee267d7d9..a080fbbc9bb 100644 --- a/build-logic/src/main/kotlin/org.jabref.gradle.base.dependency-rules.gradle.kts +++ b/build-logic/src/main/kotlin/org.jabref.gradle.base.dependency-rules.gradle.kts @@ -276,13 +276,23 @@ extraJavaModuleInfo { module("com.github.javaparser:javaparser-symbol-solver-core", "com.github.javaparser.symbolsolver.core") module("net.sf.jopt-simple:jopt-simple", "net.sf.jopt.simple") - module("com.tngtech.archunit:archunit-junit5-api", "com.tngtech.archunit.junit5.api") - module("com.tngtech.archunit:archunit-junit5-engine", "com.tngtech.archunit.junit5.engine") - module("com.tngtech.archunit:archunit-junit5-engine-api", "com.tngtech.archunit.junit5.engineapi") + module("com.tngtech.archunit:archunit-junit5-api", "com.tngtech.archunit.junit5.api") { + exportAllPackages() + requireAllDefinedDependencies() + } + module("com.tngtech.archunit:archunit-junit5-engine", "com.tngtech.archunit.junit5.engine") { + exportAllPackages() + requireAllDefinedDependencies() + } + module("com.tngtech.archunit:archunit-junit5-engine-api", "com.tngtech.archunit.junit5.engineapi") { + exportAllPackages() + requireAllDefinedDependencies() + } module("com.tngtech.archunit:archunit", "com.tngtech.archunit") { exportAllPackages() + requireAllDefinedDependencies() requires("java.logging") - requires("org.slf4j") + uses("com.tngtech.archunit.lang.extension.ArchUnitExtension") } module("org.glassfish.hk2.external:aopalliance-repackaged", "org.aopalliance") diff --git a/docs/code-howtos/faq.md b/docs/code-howtos/faq.md index d3b818118fc..b8764be33fd 100644 --- a/docs/code-howtos/faq.md +++ b/docs/code-howtos/faq.md @@ -61,13 +61,13 @@ Check the directory `jablib/src/main/resources/csl-locales`. If it is missing or empty, run `git submodule update`. If still not fixed, run `git reset --hard` **inside that directory**. -### `org.jabref.architecture.MainArchitectureTest restrictStandardStreams` FAILED +### `org.jabref.support.CommonArchitectureTest restrictStandardStreams` FAILED Check if you've used `System.out.println(...)` (the standard output stream) to log anything into the console. This is an architectural violation, as you should use the Logger instead for logging. More details on [how to log](https://devdocs.jabref.org/code-howtos/logging.html). -### `org.jabref.architecture.MainArchitectureTest doNotUseLogicInModel` FAILED +### `org.jabref.support.CommonArchitectureTest doNotUseLogicInModel` FAILED One common case when this test fails is when you put any class purely containing business logic inside the `model` package (i.e., inside the directory `org/jabref/model/`). To fix this, shift the class to a sub-package within the `logic` package (i.e., the directory`org/jabref/logic/`). diff --git a/jabgui/build.gradle.kts b/jabgui/build.gradle.kts index 9363d779a2e..6dd0ed0aa90 100644 --- a/jabgui/build.gradle.kts +++ b/jabgui/build.gradle.kts @@ -121,6 +121,10 @@ dependencies { testImplementation("com.github.javaparser:javaparser-symbol-solver-core") testImplementation("org.ow2.asm:asm") + + testImplementation("com.tngtech.archunit:archunit") + testImplementation("com.tngtech.archunit:archunit-junit5-api") + testRuntimeOnly("com.tngtech.archunit:archunit-junit5-engine") } application { @@ -218,6 +222,9 @@ javaModuleTesting.whitebox(testing.suites["test"]) { requires.add("wiremock") requires.add("wiremock.slf4j.spi.shim") + + requires.add("com.tngtech.archunit") + requires.add("com.tngtech.archunit.junit5.api") } tasks.test { diff --git a/jabgui/src/main/java/org/jabref/gui/ClipBoardManager.java b/jabgui/src/main/java/org/jabref/gui/ClipBoardManager.java index 3e1455908e0..d9c8617b526 100644 --- a/jabgui/src/main/java/org/jabref/gui/ClipBoardManager.java +++ b/jabgui/src/main/java/org/jabref/gui/ClipBoardManager.java @@ -28,7 +28,7 @@ import org.slf4j.Logger; import org.slf4j.LoggerFactory; -@AllowedToUseAwt("Requires ava.awt.datatransfer.Clipboard") +@AllowedToUseAwt("Requires java.awt.datatransfer.Clipboard") public class ClipBoardManager { public static final DataFormat XML = new DataFormat("application/xml"); diff --git a/jabgui/src/test/java/org/jabref/gui/JabGuiArchitectureTests.java b/jabgui/src/test/java/org/jabref/gui/JabGuiArchitectureTests.java new file mode 100644 index 00000000000..1dacb99fae4 --- /dev/null +++ b/jabgui/src/test/java/org/jabref/gui/JabGuiArchitectureTests.java @@ -0,0 +1,10 @@ +package org.jabref.gui; + +import org.jabref.support.CommonArchitectureTest; + +import com.tngtech.archunit.core.importer.ImportOption; +import com.tngtech.archunit.junit.AnalyzeClasses; + +@AnalyzeClasses(packages = "org.jabref", importOptions = ImportOption.DoNotIncludeTests.class) +public class JabGuiArchitectureTests extends CommonArchitectureTest { +} diff --git a/jablib/build.gradle.kts b/jablib/build.gradle.kts index d441ccc3348..998df49b311 100644 --- a/jablib/build.gradle.kts +++ b/jablib/build.gradle.kts @@ -207,8 +207,10 @@ dependencies { testImplementation("org.xmlunit:xmlunit-core") testImplementation("org.xmlunit:xmlunit-matchers") testImplementation("org.junit.jupiter:junit-jupiter-api") - testRuntimeOnly("com.tngtech.archunit:archunit-junit5-engine") + + testImplementation("com.tngtech.archunit:archunit") testImplementation("com.tngtech.archunit:archunit-junit5-api") + testRuntimeOnly("com.tngtech.archunit:archunit-junit5-engine") testImplementation("org.hamcrest:hamcrest") @@ -563,4 +565,7 @@ javaModuleTesting.whitebox(testing.suites["test"]) { requires.add("org.xmlunit.matchers") requires.add("wiremock") requires.add("wiremock.slf4j.spi.shim") + + requires.add("com.tngtech.archunit") + requires.add("com.tngtech.archunit.junit5.api") } diff --git a/jablib/src/main/java/org/jabref/logic/importer/fetcher/EuropePmcFetcher.java b/jablib/src/main/java/org/jabref/logic/importer/fetcher/EuropePmcFetcher.java index e65d0835e6b..c3e5eda6c29 100644 --- a/jablib/src/main/java/org/jabref/logic/importer/fetcher/EuropePmcFetcher.java +++ b/jablib/src/main/java/org/jabref/logic/importer/fetcher/EuropePmcFetcher.java @@ -26,12 +26,15 @@ import kong.unirest.core.json.JSONArray; import kong.unirest.core.json.JSONException; import kong.unirest.core.json.JSONObject; +import org.slf4j.Logger; +import org.slf4j.LoggerFactory; public class EuropePmcFetcher implements IdBasedParserFetcher { + private static final Logger LOGGER = LoggerFactory.getLogger(EuropePmcFetcher.class); @Override public URL getUrlForIdentifier(String identifier) throws URISyntaxException, MalformedURLException { - return URI.create("https://www.ebi.ac.uk/europepmc/webservices/rest/search?query=" + identifier + "&resultType=core&format=json").toURL(); + return new URI("https://www.ebi.ac.uk/europepmc/webservices/rest/search?query=" + identifier + "&resultType=core&format=json").toURL(); } @Override @@ -49,7 +52,7 @@ private BibEntry jsonItemToBibEntry(JSONObject item) throws ParseException { try { JSONObject result = item.getJSONObject("resultList").getJSONArray("result").getJSONObject(0); - System.out.println(result.toString(2)); + LOGGER.debug(result.toString(2)); EntryType entryType = StandardEntryType.Article; if (result.has("pubTypeList")) { diff --git a/jablib/src/main/java/org/jabref/logic/remote/RemotePreferences.java b/jablib/src/main/java/org/jabref/logic/remote/RemotePreferences.java index 79f37d38a41..562120e0c3e 100644 --- a/jablib/src/main/java/org/jabref/logic/remote/RemotePreferences.java +++ b/jablib/src/main/java/org/jabref/logic/remote/RemotePreferences.java @@ -2,6 +2,7 @@ import java.net.InetAddress; import java.net.URI; +import java.net.URISyntaxException; import java.net.UnknownHostException; import javafx.beans.property.BooleanProperty; @@ -9,6 +10,7 @@ import javafx.beans.property.SimpleBooleanProperty; import javafx.beans.property.SimpleIntegerProperty; +import org.jspecify.annotations.NonNull; import org.slf4j.Logger; import org.slf4j.LoggerFactory; @@ -75,12 +77,17 @@ public static InetAddress getIpAddress() throws UnknownHostException { return InetAddress.getByName("localhost"); } - public URI getHttpServerUri() { + public @NonNull URI getHttpServerUri() { try { - return URI.create("http://" + RemotePreferences.getIpAddress().getHostAddress() + ":23119"); - } catch (UnknownHostException e) { + return new URI("http://" + RemotePreferences.getIpAddress().getHostAddress() + ":23119"); + } catch (UnknownHostException | URISyntaxException e) { LOGGER.error("Could not create HTTP server URI. Falling back to default.", e); - return URI.create("http://localhost:23119"); + try { + return new URI("http://localhost:23119"); + } catch (URISyntaxException ex) { + LOGGER.error("Should never happen, raw string is already valid uri"); + throw new RuntimeException(ex); + } } } } diff --git a/jablib/src/main/java/org/jabref/model/strings/StringUtil.java b/jablib/src/main/java/org/jabref/model/strings/StringUtil.java index 97310445a6a..24d81e4239f 100644 --- a/jablib/src/main/java/org/jabref/model/strings/StringUtil.java +++ b/jablib/src/main/java/org/jabref/model/strings/StringUtil.java @@ -15,6 +15,7 @@ import javafx.util.Pair; import org.jabref.architecture.AllowedToUseApacheCommonsLang3; +import org.jabref.architecture.AllowedToUseLogic; import org.jabref.logic.bibtex.FieldWriter; import org.jabref.logic.os.OS; @@ -23,6 +24,7 @@ @SuppressWarnings("checkstyle:NoMultipleClosingBracesAtEndOfLine") @AllowedToUseApacheCommonsLang3("There is no equivalent in Google's Guava") +@AllowedToUseLogic("OS.NewLine is most basic") public class StringUtil { // Non-letters which are used to denote accents in LaTeX-commands, e.g., in {\"{a}} diff --git a/jablib/src/test/java/org/jabref/logic/JabLibArchitectureTests.java b/jablib/src/test/java/org/jabref/logic/JabLibArchitectureTests.java new file mode 100644 index 00000000000..c544f4865f2 --- /dev/null +++ b/jablib/src/test/java/org/jabref/logic/JabLibArchitectureTests.java @@ -0,0 +1,10 @@ +package org.jabref.logic; + +import org.jabref.support.CommonArchitectureTest; + +import com.tngtech.archunit.core.importer.ImportOption; +import com.tngtech.archunit.junit.AnalyzeClasses; + +@AnalyzeClasses(packages = "org.jabref", importOptions = ImportOption.DoNotIncludeTests.class) +public class JabLibArchitectureTests extends CommonArchitectureTest { +} diff --git a/jabsrv/src/main/java/org/jabref/http/server/Server.java b/jabsrv/src/main/java/org/jabref/http/server/Server.java index 439db74d17c..78b743c4479 100644 --- a/jabsrv/src/main/java/org/jabref/http/server/Server.java +++ b/jabsrv/src/main/java/org/jabref/http/server/Server.java @@ -62,9 +62,9 @@ public HttpServer run(List files, URI uri) { // GUI uses HttpServerManager Runtime.getRuntime().addShutdownHook(new Thread(() -> { try { - System.out.println("Shutting down jabsrv..."); + LOGGER.debug("Shutting down jabsrv..."); httpServer.shutdownNow(); - System.out.println("Done, exit."); + LOGGER.debug("Done, exit."); } catch (Exception e) { LOGGER.error("Could not shut down server", e); } diff --git a/jabsrv/src/main/java/org/jabref/http/server/cayw/CAYWResource.java b/jabsrv/src/main/java/org/jabref/http/server/cayw/CAYWResource.java index 350723f56c1..25429375985 100644 --- a/jabsrv/src/main/java/org/jabref/http/server/cayw/CAYWResource.java +++ b/jabsrv/src/main/java/org/jabref/http/server/cayw/CAYWResource.java @@ -17,6 +17,7 @@ import javafx.application.Platform; +import org.jabref.architecture.AllowedToUseAwt; import org.jabref.http.server.cayw.format.FormatterService; import org.jabref.http.server.cayw.gui.CAYWEntry; import org.jabref.http.server.cayw.gui.SearchDialog; @@ -40,6 +41,7 @@ import org.slf4j.Logger; import org.slf4j.LoggerFactory; +@AllowedToUseAwt("Requires java.awt.datatransfer.Clipboard") @Path("better-bibtex/cayw") public class CAYWResource { private static final Logger LOGGER = LoggerFactory.getLogger(CAYWResource.class); diff --git a/test-support/build.gradle.kts b/test-support/build.gradle.kts index d0296bc0bcb..3033d2da9a3 100644 --- a/test-support/build.gradle.kts +++ b/test-support/build.gradle.kts @@ -25,4 +25,7 @@ dependencies { implementation("net.bytebuddy:byte-buddy") implementation("org.jspecify:jspecify") + + implementation("com.tngtech.archunit:archunit") + implementation("com.tngtech.archunit:archunit-junit5-api") } diff --git a/test-support/src/main/java/module-info.java b/test-support/src/main/java/module-info.java index 440fb50addc..b8d2d3e2a68 100644 --- a/test-support/src/main/java/module-info.java +++ b/test-support/src/main/java/module-info.java @@ -2,6 +2,9 @@ requires org.junit.jupiter.api; requires org.mockito; requires org.jabref.jablib; + requires com.tngtech.archunit; + requires com.tngtech.archunit.junit5.api; + requires javafx.graphics; exports org.jabref.support; } diff --git a/test-support/src/main/java/org/jabref/support/CommonArchitectureTest.java b/test-support/src/main/java/org/jabref/support/CommonArchitectureTest.java new file mode 100644 index 00000000000..15574947e7e --- /dev/null +++ b/test-support/src/main/java/org/jabref/support/CommonArchitectureTest.java @@ -0,0 +1,142 @@ +package org.jabref.support; + +import java.net.URI; +import java.nio.file.Paths; + +import org.jabref.architecture.AllowedToUseApacheCommonsLang3; +import org.jabref.architecture.AllowedToUseAwt; +import org.jabref.architecture.AllowedToUseClassGetResource; +import org.jabref.architecture.AllowedToUseLogic; +import org.jabref.architecture.AllowedToUseStandardStreams; +import org.jabref.architecture.AllowedToUseSwing; + +import com.tngtech.archunit.core.domain.JavaClasses; +import com.tngtech.archunit.core.importer.ImportOption; +import com.tngtech.archunit.junit.AnalyzeClasses; +import com.tngtech.archunit.junit.ArchTest; +import com.tngtech.archunit.lang.syntax.ArchRuleDefinition; +import com.tngtech.archunit.library.GeneralCodingRules; + +/** + * This class checks JabRef's shipped classes for architecture quality. + * Does not analyze test classes. Hint from StackOverflow + */ +@AnalyzeClasses(packages = "org.jabref", importOptions = ImportOption.DoNotIncludeTests.class) +public class CommonArchitectureTest { + + private static final String PACKAGE_JAVAX_SWING = "javax.swing.."; + private static final String PACKAGE_JAVA_AWT = "java.awt.."; + 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 void doNotUseApacheCommonsLang3(JavaClasses classes) { + ArchRuleDefinition.noClasses().that().areNotAnnotatedWith(AllowedToUseApacheCommonsLang3.class) + .should().accessClassesThat().resideInAPackage("org.apache.commons.lang3") + .check(classes); + } + + @ArchTest + public void doNotUseSwing(JavaClasses classes) { + // This checks for all Swing packages, but not the UndoManager + ArchRuleDefinition.noClasses().that().areNotAnnotatedWith(AllowedToUseSwing.class) + .should().accessClassesThat() + .resideInAnyPackage("javax.swing", + "javax.swing.border..", + "javax.swing.colorchooser..", + "javax.swing.event..", + "javax.swing.filechooser..", + "javax.swing.plaf..", + "javax.swing.table..", + "javax.swing.text..", + "javax.swing.tree..") + .check(classes); + } + + @ArchTest + public void doNotUseAssertJ(JavaClasses classes) { + ArchRuleDefinition.noClasses().should().accessClassesThat().resideInAPackage("org.assertj..") + .check(classes); + } + + @ArchTest + public void doNotUseJavaAWT(JavaClasses classes) { + ArchRuleDefinition.noClasses().that().areNotAnnotatedWith(AllowedToUseAwt.class) + .should().accessClassesThat().resideInAPackage(PACKAGE_JAVA_AWT) + .check(classes); + } + + @ArchTest + public void doNotUsePaths(JavaClasses classes) { + ArchRuleDefinition.noClasses().should() + .accessClassesThat() + .belongToAnyOf(Paths.class) + .because("Path.of(...) should be used instead") + .check(classes); + } + + @ArchTest + public void useStreamsOfResources(JavaClasses classes) { + // Reason: https://github.com/oracle/graal/issues/7682#issuecomment-1786704111 + ArchRuleDefinition.noClasses().that().haveNameNotMatching(".*Test") + .and().areNotAnnotatedWith(AllowedToUseClassGetResource.class) + .and().areNotAssignableFrom("org.jabref.logic.importer.fileformat.ImporterTestEngine") + .should() + .callMethod(Class.class, "getResource", String.class) + .because("getResourceAsStream(...) should be used instead") + .check(classes); + } + + // TODO: no org.jabref.gui package may reside in org.jabref.logic + + @ArchTest + public void doNotUseLogicInModel(JavaClasses classes) { + ArchRuleDefinition.noClasses().that().resideInAPackage(PACKAGE_ORG_JABREF_MODEL) + .and().areNotAnnotatedWith(AllowedToUseLogic.class) + .should().dependOnClassesThat().resideInAPackage(PACKAGE_ORG_JABREF_LOGIC) + .check(classes); + } + + @ArchTest + public void restrictUsagesInModel(JavaClasses classes) { + // Until we switch to Lucene, we need to access Globals.stateManager().getActiveDatabase() from the search classes, + // because the PDFSearch needs to access the index of the corresponding database + ArchRuleDefinition.noClasses().that().areNotAssignableFrom("org.jabref.model.search.rules.ContainBasedSearchRule") + .and().areNotAssignableFrom("org.jabref.model.search.rules.RegexBasedSearchRule") + .and().areNotAssignableFrom("org.jabref.model.search.rules.GrammarBasedSearchRule") + .and().resideInAPackage(PACKAGE_ORG_JABREF_MODEL) + .should().dependOnClassesThat().resideInAPackage(PACKAGE_JAVAX_SWING) + .check(classes); + } + + @ArchTest + public void restrictUsagesInLogic(JavaClasses classes) { + ArchRuleDefinition.noClasses().that().resideInAPackage(PACKAGE_ORG_JABREF_LOGIC) + .and().areNotAnnotatedWith(AllowedToUseSwing.class) + .and().areNotAssignableFrom("org.jabref.logic.search.DatabaseSearcherWithBibFilesTest") + .should().dependOnClassesThat().resideInAPackage(PACKAGE_JAVAX_SWING) + .check(classes); + } + + @ArchTest + public void restrictStandardStreams(JavaClasses classes) { + ArchRuleDefinition.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); + } + + /// Use constructor new URI(...) instead + @ArchTest + public void shouldNotCallUriCreateMethod(JavaClasses classes) { + ArchRuleDefinition.noClasses() + .that() + .resideInAPackage("org.jabref..") + .should().callMethod(URI.class, "create", String.class) + .check(classes); + } +} diff --git a/versions/build.gradle.kts b/versions/build.gradle.kts index 7dacb99242d..e6726b42872 100644 --- a/versions/build.gradle.kts +++ b/versions/build.gradle.kts @@ -56,6 +56,7 @@ dependencies.constraints { api("com.squareup.okhttp3:okhttp:4.12.0") api("com.squareup.okio:okio-jvm:3.12.0") api("com.squareup.retrofit2:retrofit:3.0.0") + api("com.tngtech.archunit:archunit:1.4.1") api("com.tngtech.archunit:archunit-junit5-api:1.4.1") api("com.tngtech.archunit:archunit-junit5-engine:1.4.1") api("com.vladsch.flexmark:flexmark-html2md-converter:0.64.8")