4.x: Checkstyle suppression in code

DEV-GUIDELINES.md

@@ -12,10 +12,11 @@ Some of these rules are enforced by checkstyle, some are checked during code rev
 # General coding rules
 1. Use unchecked Throwables - descendants of RuntimeException in API
     1. Never use RuntimeException directly - always create a descendant appropriate for your module, or
-        use an existing exception declared in the module
+       use an existing exception declared in the module
     2. Our APIs should never throw a checked exception unless enforced by implemented/extended interface - e.g. when
-        we implement a java.io.Closeable, we must declare the checked exception. 
-1. Usage of `null` is discouraged and should not exist in any public APIs of Helidon
+       we implement a java.io.Closeable, we must declare the checked exception.
+    3. In some cases we may use existing runtime exceptions if they fit the problem (`NoSuchElementException`, `IllegalStateException`)
+2. Usage of `null` is discouraged and should not exist in any public APIs of Helidon
     1. If a method accepts a `null`, refactor it to a different approach
         - a setter: create a method to remove the field value rather than setting a `null` value 
             (such as `host(String)` to set a host, and `unsetHost()` to revert to default value)
@@ -70,9 +71,7 @@ Some of these rules are enforced by checkstyle, some are checked during code rev
         2. Default: component has a well defined and documented default value for such a property
         3. Optional: component behaves in a well defined and documented manner if such a property is not 
             configured (e.g. a component may expect tracing endpoint - if not defined, tracing may be disabled)         
-5. We have introduced `helidon-builder` module, that provides capability to generate builders that support both programmatic and configuration approach. See [helidon-builder](builder/README.md) for more details about modules, and [helidon-builder-api](builder/api/README.md) for explanation of APIs and naming rules. The `Blueprint` approach should be used for all builders (and the API of the prototype). Exceptions must be consulted with project architect (this may result in either changing the processor to support the required feature, or in removing such feature and redesigning the problem, or in an exception (documented) to the rule)  
-
-Example: [io.helidon.faulttolerance.RetryConfigBlueprint](nima/fault-tolerance/fault-tolerance/src/main/java/io/helidon/nima/faulttolerance/RetryConfigBlueprint.java)
+5. We have introduced `helidon-builder` module, see [Builders](#Builders)
 
 # Getters and Setters
 1. We do not use the verb, e.g. when a property "port" exists, the following methods are used:  
@@ -90,6 +89,11 @@ Example: [io.helidon.security.providers.oidc.common.OidcConfig](security/provide
     2. When using control methods (such as Server server = Server.create().start())
 
 # Builders
+We have introduced code generation for builders that enforces the rules mentioned below.
+
+See [helidon-builder](builder/README.md) for more details about modules, and [helidon-builder-api](builder/api/README.md) for explanation of APIs and naming rules. The `Blueprint` approach should be used for all builders (and the API of the prototype). Exceptions must be consulted with project architect (this may result in either changing the processor to support the required feature, or in removing such feature and redesigning the problem, or in an exception (documented) to the rule).
+
+
 1. We use builders to create instances that need any parameters for construction
     1. **This implies that there are no public API classes that would use public constructors**
     2. Allowed exceptions to this rule:
@@ -112,24 +116,16 @@ Example: [io.helidon.security.providers.oidc.common.OidcConfig](security/provide
             "success(Subject subject)" etc. - **these methods MUST use builder to build the instance internally**
         5. An internal class named "Builder" that is building instances of the containing class
             1. it is allowed to have the builder as a top level class, in such a case the name must reflect the class it is 
-                building (e.g. FooBarBuilder) 
-3. Builder class
-    1. Must have:
-        1. Implements "io.helidon.common.Builder<FooBar>"
-        2. All methods configuring the builder return a builder instance with updated configuration
-        3. Validation done either on setters or in method build() (latest) - e.g. we should fail to
-            build an instance if the configuration is not correct
-    2. May have:
-        1. May accept other classes that are built using a builder, either directly, or as Supplier<T> 
-            (as builder implements Supplier, this allows us to pass a builder to such a method, as well as a nice lambda)
-
-Example: [io.helidon.security.providers.oidc.common.OidcConfig](security/providers/oidc-common/src/main/java/io/helidon/security/oidc/common/OidcConfig.java)
+                building (e.g. FooBarBuilder)
+
+Example: [io.helidon.faulttolerance.RetryConfigBlueprint](nima/fault-tolerance/fault-tolerance/src/main/java/io/helidon/nima/faulttolerance/RetryConfigBlueprint.java)
 
 # JPMS
 1. Each java module that is released has a `module-info.java`
 2. Provided services of released modules are declared ONLY in `module-info.java`, `META-INF/services` is generated  
         automatically by a Maven plugin. `META-INF/services` in sources of released modules will fail the build
 3. Javadoc is using modules, so do not forget to add javadoc to `module-info.java`
+4. Provided and used services should be always using fully qualified class names
 
 # Testing
 

config/config/src/main/java/io/helidon/config/ClasspathConfigSource.java

@@ -1,5 +1,5 @@
 /*
- * Copyright (c) 2017, 2022 Oracle and/or its affiliates.
+ * Copyright (c) 2017, 2023 Oracle and/or its affiliates.
  *
  * Licensed under the Apache License, Version 2.0 (the "License");
  * you may not use this file except in compliance with the License.

etc/checkstyle-suppressions.xml

@@ -19,85 +19,38 @@
 
 <!DOCTYPE suppressions PUBLIC
         "-//Puppy Crawl//DTD Suppressions 1.1//EN"
-        "http://checkstyle.sourceforge.net/dtds/suppressions_1_1.dtd">
+        "https://checkstyle.sourceforge.net/dtds/suppressions_1_1.dtd">
 
+<!--
+Use in Helidon:
+This file is used when running the aggregate report (and configuration in modules is ignored in that case).
+This file SHOULD ONLY contain exclusions for whole modules where appropriate.
+Single check exclusions should be handled through @SuppressWarning("checkstyle:....") annotation.
+
+Only exceptional cases that cannot be handled through annotations may be provided here.
+This is to keep the checkstyle exclusions co-located with the code, so if the code changes, we do not leave an outdated
+record here.
+-->
 <suppressions>
-    <!-- to do comment suppression for to do project -->
-    <suppress checks="TodoComment"
-            files="examples/todo-app/.*"/>
-
-    <suppress checks="FileLength"
-            files="config/config/src/main/java/io/helidon/config/Config.java|integrations/cdi/jpa-cdi/src/main/java/io/helidon/integrations/cdi/jpa/JpaExtension\.java|security/providers/oidc-common/src/main/java/io/helidon/security/providers/oidc/common/OidcConfig\.java"
-            lines="1"/>
-
-    <!-- Java comments with AsciiDoc tag:: and end:: in import section incorrectly flagged
-         as unacceptable blank lines within a package's imports. -->
-    <suppress checks="ImportOrder"
-            files="examples/guides/se-restful-webservice/src/main/java/io/helidon/guides/se/restfulwebservice/Main.java"/>
-    <suppress checks="ImportOrder"
-            files="examples/guides/se-restful-webservice/src/main/java/io/helidon/guides/se/restfulwebservice/GreetService.java"/>
-    <suppress checks="NoWhitespaceBefore|SeparatorWrap"
-            files="examples/guides/mp-restful-webservice/src/main/java/io/helidon/guides/mp/restfulwebservice/GreetApplication.java"/>
     <!--
-     The following files are work taken over from other projects,
-        where we want to keep the author tag untouched
+    - File exclusions (exceptions)
+    - FileLength cannot be excluded using a @SuppressWarnings
     -->
-    <!-- Common HTTP project -->
-    <suppress id="Javadoc.javadocNoAuthor"
-            files="common/http/Preconditions\.java|common/http/Ascii\.java|common/http/CharMatcher\.java"/>
-    <!-- Webserver project -->
-    <suppress id="Javadoc.javadocNoAuthor"
-            files="webserver/UriComponent\.java"/>
-
-    <!-- Building a Graph involves a lot of instanceof checks and state manipulation.  -->
-    <suppress checks="MethodLength"
-              files="HelidonReactiveStreamsEngine\.java"/>
-
-    <!-- PKCS#1 private keys - required for OCI Instance Principal Authentication -->
-    <suppress checks="IllegalImport"
-            files="DerUtils\.java"/>
-
-    <!-- this is a record style, all parameters are always needed, no benefit of changing to builder -->
-    <suppress files="webserver/http2/src/main/java/io/helidon/webserver/http2/spi/Http2SubProtocolSelector.java"
-            checks="ParameterNumber"/>
-
-    <!-- this is a record style, all parameters are always needed, no benefit of changing to builder -->
-    <suppress files="webserver/webserver/src/main/java/io/helidon/webserver/ConnectionContext.java"
-            checks="ParameterNumber"/>
-
-    <!-- this is a record style, all parameters are always needed, no benefit of changing to builder -->
-    <suppress files="webserver/webserver/src/main/java/io/helidon/webserver/http1/Http1ServerRequest.java"
-            checks="ParameterNumber"/>
-
     <!-- the huffman constants are long, but this is not actual code, just a set of constants -->
-    <suppress files="http/http2/src/main/java/io/helidon/http/http2/Http2HuffmanConstants.java"
+    <suppress files="http/http2/src/main/java/io/helidon/http/http2/Http2HuffmanConstants\.java"
             checks="FileLength"/>
 
-    <suppress files="webserver/benchmark/"
-            checks=".*"/>
-
-    <!-- builder brings no benefit, all parameters always needed -->
-    <suppress files="webserver/http2/webserver/src/main/java/io/helidon/webserver/http2/spi/Http2SubProtocolProvider.java"
-            checks="ParameterNumber"/>
-
-    <!-- builder brings no benefit, all parameters always needed, and a private method -->
-    <suppress files="src/main/java/io/helidon/metrics/serviceapi/PrometheusFormat.java"
-            checks="ParameterNumber"/>
+    <suppress files="integrations/cdi/jpa-cdi/src/main/java/io/helidon/integrations/cdi/jpa/JpaExtension\.java"
+            checks="FileLength"/>
 
-    <!-- builder brings no benefit, all parameters always needed, and a private method -->
-    <suppress files="microprofile/lra/jax-rs/src/main/java/io/helidon/microprofile/lra/NonJaxRsResource.java"
-            checks="ParameterNumber"/>
+    <!--
+    - Module exclusions
+    -->
 
-    <!-- builder exclusions -->
-    <suppress files="builder/tests/builder/src/main/java/io/helidon/builder/test/testsubjects/.*"
+    <!-- JMH benchmark is not required to follow code style -->
+    <suppress files="webserver/benchmark/"
             checks=".*"/>
 
-    <!-- interfaces implemented by generated code, ensuring methods don't interfere with the interlaced methods from the user -->
-    <suppress files="builder/builder-config/src/main/java/io/helidon/builder/config/spi/GeneratedConfig.*.java"
-            checks="MethodName"/>
-    <suppress files="builder/builder-config/src/main/java/io/helidon/builder/config/spi/ConfigProvider.java"
-            checks="MethodName"/>
-
     <!-- injection tests need to violate to check different user scenarios -->
     <suppress files="builder/tests/"
             checks=".*"/>

etc/checkstyle.xml

@@ -1,7 +1,7 @@
 <?xml version="1.0" encoding="UTF-8"?>
 <!--
 
-    Copyright (c) 2016, 2022 Oracle and/or its affiliates.
+    Copyright (c) 2016, 2023 Oracle and/or its affiliates.
 
     Licensed under the Apache License, Version 2.0 (the "License");
     you may not use this file except in compliance with the License.
@@ -17,7 +17,8 @@
 
 -->
 
-<!DOCTYPE module PUBLIC "-//Puppy Crawl//DTD Check Configuration 1.3//EN" "http://www.puppycrawl.com/dtds/configuration_1_3.dtd">
+<!DOCTYPE module PUBLIC "-//Puppy Crawl//DTD Check Configuration 1.3//EN"
+        "https://www.puppycrawl.com/dtds/configuration_1_3.dtd">
 <!--
     This file is based on maven-checkstyle-plugin config/sun_checks.xml file.
     Checkstyle configuration that checks the coding conventions based on:

microprofile/lra/jax-rs/src/main/java/io/helidon/microprofile/lra/NonJaxRsResource.java

@@ -119,6 +119,7 @@ private void handleRequest(ServerRequest req, ServerResponse res) {
         }
     }
 
+    @SuppressWarnings("checkstyle:ParameterNumber") // all parameters required, no benefit using a record wrapper
     private void handleRequest(ServerRequest req,
                                ServerResponse res,
                                String type,

microprofile/reactive-streams/src/main/java/io/helidon/microprofile/reactive/HelidonReactiveStreamsEngine.java

@@ -1,5 +1,5 @@
 /*
- * Copyright (c) 2020, 2021 Oracle and/or its affiliates.
+ * Copyright (c) 2020, 2023 Oracle and/or its affiliates.
  *
  * Licensed under the Apache License, Version 2.0 (the "License");
  * you may not use this file except in compliance with the License.
@@ -126,7 +126,7 @@ static void requireNullTerminal(Object o, Stage stage) {
         }
     }
 
-    @SuppressWarnings({ "unchecked", "rawtypes" })
+    @SuppressWarnings({ "unchecked", "rawtypes", "checkstyle:MethodLength" }) // lot of instanceof checks required, long method
     static Object build(Iterable<Stage> graph, Mode mode) throws UnsupportedStageException {
         Flow.Subscriber graphInlet = null;
         Multi result = null;

webserver/http2/src/main/java/io/helidon/webserver/http2/spi/Http2SubProtocolSelector.java

@@ -53,6 +53,7 @@ public interface Http2SubProtocolSelector {
      * @param router             router
      * @return sub-protocol result
      */
+    @SuppressWarnings("checkstyle:ParameterNumber") // all parameters required, no benefit using a record wrapper
     SubProtocolResult subProtocol(ConnectionContext ctx,
                                   HttpPrologue prologue,
                                   Http2Headers headers,

webserver/webserver/src/main/java/io/helidon/webserver/http1/Http1ServerRequest.java

@@ -81,6 +81,7 @@ static Http1ServerRequest create(ConnectionContext ctx,
     /*
      * Create a new request with an entity.
      */
+    @SuppressWarnings("checkstyle:ParameterNumber") // all parameters are always needed, record would not bring any benefit
     static Http1ServerRequest create(ConnectionContext ctx,
                                      Http1Connection connection,
                                      Http1Config http1Config,