Checkstyle cleanup (#4501)

* cleaning up checkstyle files * added exlusions for files at base project level so checkstyle doesn't error out * duplicate error code from merge * One more fix for #4467 * changing lifecycle goal for all module checkstyle check * moving checkstyle to base pom file, changing exectution phase on base check, cleaning dependency, resolving duplicate error code * wip * trying to figure out why pipeline cannot copy files * removing modules that don't actually need to be built. * I messed up the version * missing model --------- Co-authored-by: James Agnew <[email protected]>
hapifhir · Feb 3, 2023 · 64d776a · 64d776a
1 parent 9665476
commit 64d776a
Show file tree

Hide file tree

Showing 31 changed files with 365 additions and 531 deletions.
diff --git a/azure-pipelines.yml b/azure-pipelines.yml
@@ -80,16 +80,12 @@ stages:
  module: hapi-fhir-server-mdm
  - name: hapi_fhir_server_openapi
  module: hapi-fhir-server-openapi
- - name: hapi_fhir_serviceloaders
- module: hapi-fhir-serviceloaders
  - name: hapi_fhir_caching_api
  module: hapi-fhir-serviceloaders/hapi-fhir-caching-api
  - name: hapi_fhir_caching_caffeine
  module: hapi-fhir-serviceloaders/hapi-fhir-caching-caffeine
  - name: hapi_fhir_caching_guava
  module: hapi-fhir-serviceloaders/hapi-fhir-caching-guava
- - name: hapi_fhir_caching_testing
- module: hapi-fhir-serviceloaders/hapi-fhir-caching-testing
  - name: hapi_fhir_spring_boot_autoconfigure
  module: hapi-fhir-spring-boot/hapi-fhir-spring-boot-autoconfigure
  - name: hapi_fhir_spring_boot_sample_server_jersey

diff --git a/hapi-deployable-pom/pom.xml b/hapi-deployable-pom/pom.xml
@@ -1,7 +1,8 @@
-<project xmlns="http:https://maven.apache.org/POM/4.0.0" xmlns:xsi="http:https://www.w3.org/2001/XMLSchema-instance" xsi:schemaLocation="http:https://maven.apache.org/POM/4.0.0 http:https://maven.apache.org/xsd/maven-4.0.0.xsd">
- <modelVersion>4.0.0</modelVersion>
+<project xmlns="http:https://maven.apache.org/POM/4.0.0" xmlns:xsi="http:https://www.w3.org/2001/XMLSchema-instance"
+ xsi:schemaLocation="http:https://maven.apache.org/POM/4.0.0 http:https://maven.apache.org/xsd/maven-4.0.0.xsd">
+ <modelVersion>4.0.0</modelVersion>
 
- <parent>
+ <parent>
  <groupId>ca.uhn.hapi.fhir</groupId>
  <artifactId>hapi-fhir</artifactId>
  <version>6.5.0-SNAPSHOT</version>
@@ -13,9 +14,6 @@
 
  <name>HAPI FHIR - Deployable Artifact Parent POM</name>
 
- <dependencies>
- </dependencies>
-
  <build>
  <plugins>
  <plugin>
@@ -120,27 +118,6 @@
  </ignoredResourcePatterns>
  </configuration>
  </plugin>
- <plugin>
- <groupId>org.apache.maven.plugins</groupId>
- <artifactId>maven-checkstyle-plugin</artifactId>
- <executions>
- <execution>
- <phase>process-sources</phase>
- <goals>
- <goal>check</goal>
- </goals>
- <configuration>
- <logViolationsToConsole>true</logViolationsToConsole>
- <failsOnError>true</failsOnError>
- <suppressionsLocation>${maven.multiModuleProjectDirectory}/src/checkstyle/checkstyle_suppressions.xml</suppressionsLocation>
- <enableRulesSummary>true</enableRulesSummary>
- <enableSeveritySummary>true</enableSeveritySummary>
- <consoleOutput>true</consoleOutput>
- <configLocation>${maven.multiModuleProjectDirectory}/src/checkstyle/checkstyle.xml</configLocation>
- </configuration>
- </execution>
- </executions>
- </plugin>
  </plugins>
  </build>
 
@@ -208,7 +185,6 @@
  <execution>
  <phase>package</phase>
  <goals>
- <!--<goal>aggregate-jar</goal>-->
  <goal>jar</goal>
  </goals>
  </execution>
@@ -252,5 +228,4 @@
  </build>
  </profile>
  </profiles>
-
 </project>
diff --git a/hapi-fhir-base/pom.xml b/hapi-fhir-base/pom.xml
@@ -196,6 +196,62 @@
  </execution>
  </executions>
  </plugin>
+ <plugin>
+ <groupId>org.apache.maven.plugins</groupId>
+ <artifactId>maven-antrun-plugin</artifactId>
+ <version>1.8</version>
+ <configuration>
+ <target>
+ <!--suppress UnresolvedMavenProperty -->
+ <delete dir="${pom.basedir}/target/" includes="checkstyle*"/>
+ </target>
+ </configuration>
+ <inherited>true</inherited>
+ <executions>
+ <execution>
+ <id>delete-module-cache-file</id>
+ <phase>validate</phase>
+ <goals>
+ <goal>run</goal>
+ </goals>
+ </execution>
+ </executions>
+ </plugin>
+ <plugin>
+ <groupId>org.apache.maven.plugins</groupId>
+ <artifactId>maven-checkstyle-plugin</artifactId>
+ <version>${maven_checkstyle_version}</version>
+ <configuration>
+ <!--suppress UnresolvedMavenProperty -->
+ <configLocation>${maven.multiModuleProjectDirectory}/hapi-fhir-checkstyle/src/checkstyle/hapi-base-checkstyle.xml</configLocation>
+ <!--suppress UnresolvedMavenProperty -->
+ <suppressionsLocation>${maven.multiModuleProjectDirectory}/hapi-fhir-checkstyle/src/checkstyle/hapi-base-checkstyle-suppression.xml</suppressionsLocation>
+ <inputEncoding>UTF-8</inputEncoding>
+ <consoleOutput>true</consoleOutput>
+ </configuration>
+ <inherited>true</inherited>
+ <executions>
+ <execution>
+ <id>hapi-single-module-checkstyle</id>
+ <phase>validate</phase>
+ <goals>
+ <goal>check</goal>
+ </goals>
+ </execution>
+ </executions>
+ <dependencies>
+ <dependency>
+ <groupId>ca.uhn.hapi.fhir</groupId>
+ <artifactId>hapi-fhir-checkstyle</artifactId>
+ <version>${project.version}</version>
+ </dependency>
+ <dependency>
+ <groupId>com.puppycrawl.tools</groupId>
+ <artifactId>checkstyle</artifactId>
+ <version>${checkstyle_version}</version>
+ </dependency>
+ </dependencies>
+ </plugin>
  </plugins>
  <resources>
  <resource>

diff --git a/hapi-fhir-base/src/main/java/ca/uhn/fhir/validation/FhirValidator.java b/hapi-fhir-base/src/main/java/ca/uhn/fhir/validation/FhirValidator.java
@@ -333,7 +333,7 @@ static List<SingleValidationMessage> buildValidationMessages(List<ConcurrentVali
  retval.addAll(messages);
  }
  } catch (InterruptedException | ExecutionException exp) {
- throw new InternalErrorException(Msg.code(1975) + exp);
+ throw new InternalErrorException(Msg.code(2246) + exp);
  }
  return retval;
  }

diff --git a/hapi-fhir-checkstyle/pom.xml b/hapi-fhir-checkstyle/pom.xml
@@ -18,6 +18,7 @@
  <dependency>
  <groupId>com.puppycrawl.tools</groupId>
  <artifactId>checkstyle</artifactId>
+ <version>${checkstyle_version}</version>
  </dependency>
  <dependency>
  <groupId>commons-io</groupId>
@@ -34,6 +35,31 @@
  </dependency>
  </dependencies>
 
+ <build>
+ <pluginManagement>
+ <plugins>
+ <plugin>
+ <groupId>org.apache.maven.plugins</groupId>
+ <artifactId>maven-checkstyle-plugin</artifactId>
+ <version>3.2.0</version>
+ <dependencies>
+ <dependency>
+ <groupId>com.puppycrawl.tools</groupId>
+ <artifactId>checkstyle</artifactId>
+ <version>${checkstyle_version}</version>
+ </dependency>
+ <dependency>
+ <groupId>ca.uhn.hapi.fhir</groupId>
+ <artifactId>hapi-fhir-checkstyle</artifactId>
+ <!-- Remember to bump this when you upgrade the version -->
+ <version>${project.version}</version>
+ </dependency>
+ </dependencies>
+ </plugin>
+ </plugins>
+ </pluginManagement>
+ </build>
+
  <profiles>
  <!-- For releases, we need to generate javadoc and sources JAR -->
  <profile>
@@ -75,5 +101,44 @@
  </plugins>
  </build>
  </profile>
+ <profile>
+ <id>CI</id>
+ <build>
+ <plugins>
+ <plugin>
+ <groupId>org.apache.maven.plugins</groupId>
+ <artifactId>maven-checkstyle-plugin</artifactId>
+ <version>${maven_checkstyle_version}</version>
+ <configuration>
+ <excludes>**/osgi/**/*, **/.mvn/**/*, **/.mvn_/**/*</excludes>
+ <sourceDirectories>
+ <!--scan base project directory to include all modules-->
+ <!--suppress UnresolvedMavenProperty -->
+ <directory>${maven.multiModuleProjectDirectory}/</directory>
+ </sourceDirectories>
+ <!--suppress UnresolvedMavenProperty -->
+ <configLocation>${maven.multiModuleProjectDirectory}/hapi-fhir-checkstyle/src/checkstyle/hapi-base-checkstyle.xml</configLocation>
+ <!--suppress UnresolvedMavenProperty -->
+ <suppressionsLocation>${maven.multiModuleProjectDirectory}/hapi-fhir-checkstyle/src/checkstyle/hapi-base-checkstyle-suppression.xml</suppressionsLocation>
+ <inputEncoding>UTF-8</inputEncoding>
+ <consoleOutput>true</consoleOutput>
+ </configuration>
+ <executions>
+ <execution>
+ <id>checkstyle-across-all-modules</id>
+ <phase>install</phase>
+ <goals>
+ <goal>check</goal>
+ </goals>
+ </execution>
+ <execution>
+ <id>hapi-single-module-checkstyle</id>
+ <phase>none</phase>
+ </execution>
+ </executions>
+ </plugin>
+ </plugins>
+ </build>
+ </profile>
  </profiles>
 </project>
diff --git a/src/checkstyle/checkstyle_suppressions.xml → ...tyle/hapi-base-checkstyle-suppression.xml b/src/checkstyle/checkstyle_suppressions.xml → ...tyle/hapi-base-checkstyle-suppression.xml
@@ -1,10 +1,18 @@
 <?xml version="1.0"?>
-
 <!DOCTYPE suppressions PUBLIC
- "-//Puppy Crawl//DTD Suppressions 1.1//EN"
- "http:https://www.puppycrawl.com/dtds/suppressions_1_1.dtd">
+ "-//Checkstyle//DTD SuppressionFilter Configuration 1.2//EN"
+ "https:https://checkstyle.org/dtds/suppressions_1_2.dtd">
 
 <suppressions>
+ <suppress files="[/\\]test[/\\]" checks="[a-zA-Z0-9]*" />
+ <suppress files="[/\\]resources[/\\]" checks="[a-zA-Z0-9]*" />
+ <suppress files="[\\/]target[\\/]" checks="[a-zA-Z0-9]*" />
+ <suppress files="[\\/]hapi-fhir-docs[\\/]" checks="[a-zA-Z0-9]*" />
+ <suppress files="[\\/]hapi-fhir-checkstyle[\\/]" checks="[a-zA-Z0-9]*" />
+ <suppress files="[\\/]hapi-fhir-jpaserver-test-utilities[\\/]" checks="[a-zA-Z0-9]*" />
+ <suppress files="[\\/]hapi-tinder-plugin[\\/]" checks="[a-zA-Z0-9]*" />
+ <suppress files="[\\/]osgi[\\/]" checks="[a-zA-Z0-9]*"/>
+
  <!-- Name suppressions -->
  <suppress files="Base64BinaryDt\.java" checks="AbstractClassName"/>
  <!-- TODO KHS cdr instantiates these. Remove them once cdr has been fixed. -->
@@ -16,4 +24,6 @@
  <suppress files="RequestDetails\.java" checks="AbstractClassName"/>
  <suppress files="RestfulClientFactory\.java" checks="AbstractClassName"/>
  <suppress files="MatchUrlService\.java" checks="AbstractClassName"/>
+ <suppress files="BaseController\.java" checks="AbstractClassName"/>
+
 </suppressions>
diff --git a/hapi-fhir-checkstyle/src/checkstyle/hapi-base-checkstyle.xml b/hapi-fhir-checkstyle/src/checkstyle/hapi-base-checkstyle.xml
@@ -0,0 +1,41 @@
+<?xml version="1.0"?>
+<!DOCTYPE module PUBLIC
+ "-//Puppy Crawl//DTD Check Configuration 1.3//EN"
+ "http:https://www.puppycrawl.com/dtds/configuration_1_3.dtd">
+
+<module name="Checker">
+
+ <property name="severity" value="error"/>
+ <property name="charset" value="UTF-8"/>
+ <property name="fileExtensions" value="java, properties, xml, js, json"/>
+
+ <module name="TreeWalker">
+ <!-- Run custom HapiErrorCodeCheck to find duplicate error codes -->
+ <module name="ca.uhn.fhir.checks.HapiErrorCodeCheck"/>
+ <!-- Throw error if any FIX ME is left in the code -->
+ <module name="TodoComment">
+ <!-- The (?i) below means Case Insensitive -->
+ <property name="format" value="(?i)FIXME"/>
+ </module>
+
+ <module name="RegexpSinglelineJava">
+ <property name="format" value="System\.out\.println"/>
+ <property name="ignoreComments" value="true"/>
+ </module>
+ <module name="RegexpSinglelineJava">
+ <property name="format" value="org\.jetbrains\.annotations\.NotNull"/>
+ </module>
+ <module name="RegexpSinglelineJava">
+ <property name="format" value="org\.jetbrains\.annotations\.Nullable"/>
+ </module>
+ <!-- Should always use the Spring transactional interface, per: https://stackoverflow.com/questions/26387399/javax-transaction-transactional-vs-org-springframework-transaction-annotation-tr -->
+ <module name="RegexpSinglelineJava">
+ <property name="format" value="javax\.transaction\.Transactional"/>
+ <property name="message"
+ value="Wrong @Transactional annotation used, use instead: org.springframework.transaction.annotation.Transactional"/>
+ </module>
+ <module name="AbstractClassName">
+ <property name="format" value="^(Base|Abstract).+$"/>
+ </module>
+ </module>
+</module>
diff --git a/hapi-fhir-checkstyle/src/main/java/ca/uhn/fhir/checks/HapiErrorCodeCheck.java b/hapi-fhir-checkstyle/src/main/java/ca/uhn/fhir/checks/HapiErrorCodeCheck.java
@@ -1,6 +1,6 @@
 package ca.uhn.fhir.checks;
 
-import com.puppycrawl.tools.checkstyle.StatelessCheck;
+import com.puppycrawl.tools.checkstyle.FileStatefulCheck;
 import com.puppycrawl.tools.checkstyle.api.AbstractCheck;
 import com.puppycrawl.tools.checkstyle.api.DetailAST;
 import com.puppycrawl.tools.checkstyle.api.TokenTypes;
@@ -9,15 +9,13 @@
 
 import java.util.HashMap;
 import java.util.Map;
+import java.util.Set;
 
-/**
- * mvn -P CI checkstyle:check
- */
-@StatelessCheck
+@FileStatefulCheck
 public final class HapiErrorCodeCheck extends AbstractCheck {
  private static final Logger ourLog = LoggerFactory.getLogger(HapiErrorCodeCheck.class);
 
- private static final Map<Integer, String> ourCodesUsed = new HashMap<>();
+ private static final ErrorCodeCache ourCache = ErrorCodeCache.INSTANCE;
 
  @Override
  public int[] getDefaultTokens() {
@@ -42,10 +40,6 @@ public void visitToken(DetailAST ast) {
  }
 
  private void validateMessageCode(DetailAST theAst) {
- // TODO KHS this should be done in the checkstyle plugin pom config, not here
- if (getFileContents().getFileName().contains("/generated-sources/")) {
- return;
- }
  DetailAST instantiation = theAst.getFirstChild().getFirstChild();
  // We only expect message codes on new exception instantiations
  if (TokenTypes.LITERAL_NEW != instantiation.getType()) {
@@ -68,14 +62,13 @@ private void validateMessageCode(DetailAST theAst) {
  DetailAST numberNode = msgNode.getParent().getNextSibling().getFirstChild().getFirstChild();
  if (TokenTypes.NUM_INT == numberNode.getType()) {
  Integer code = Integer.valueOf(numberNode.getText());
- if (ourCodesUsed.containsKey(code)) {
+ if (ourCache.containsKey(code)) {
  log(theAst.getLineNo(), "Two different exception messages call Msg.code(" +
- code +
- "). Each thrown exception throw call Msg.code() with a different code. " +
- "Previously found at: " + ourCodesUsed.get(code));
+ code + "). \nEach thrown exception must call Msg.code() with a different code. " +
+ "\nPreviously found at: " + ourCache.get(code));
  } else {
- String location = getFileContents().getFileName() + ":" + instantiation.getLineNo() + ":" + instantiation.getColumnNo() + "(" + code + ")";
- ourCodesUsed.put(code, location);
+ String location = getFilePath() + ":" + instantiation.getLineNo() + ":" + instantiation.getColumnNo() + "(" + code + ")";
+ ourCache.put(code, location);
  }
  } else {
  log(theAst.getLineNo(), "Called Msg.code() with a non-integer argument");
@@ -110,5 +103,30 @@ private DetailAST getMsgNodeOrNull(DetailAST theNode) {
  }
  return null;
  }
+
+ public enum ErrorCodeCache {
+ INSTANCE;
+
+ private static final Map<Integer, String> ourCodesUsed = new HashMap<>();
+
+ ErrorCodeCache() {
+ }
+
+ public boolean containsKey(Integer s) {
+ return ourCodesUsed.containsKey(s);
+ }
+
+ public String get(Integer i) {
+ return ourCodesUsed.get(i);
+ }
+
+ public String put(Integer code, String location) {
+ return ourCodesUsed.put(code, location);
+ }
+
+ public Set<Integer> keySet() {
+ return ourCodesUsed.keySet();
+ }
+ }
 }