apache · snazy · Apr 4, 2025 · Mar 21, 2025
@@ -17,6 +17,8 @@
  * under the License.
  */
 
+import java.util.Properties
+import net.ltgt.gradle.errorprone.CheckSeverity
 import net.ltgt.gradle.errorprone.errorprone
 import org.gradle.api.tasks.compile.JavaCompile
 import org.gradle.api.tasks.testing.Test
@@ -41,18 +43,23 @@ tasks.withType(JavaCompile::class.java).configureEach {
   options.errorprone.disableWarningsInGeneratedCode = true
   options.errorprone.excludedPaths =
     ".*/${project.layout.buildDirectory.get().asFile.relativeTo(projectDir)}/generated/.*"
-  options.errorprone.error(
-    "DefaultCharset",
-    "FallThrough",
-    "MissingCasesInEnumSwitch",
-    "MissingOverride",
-    "ModifiedButNotUsed",
-    "OrphanedFormatString",
-    "PatternMatchingInstanceof",
-    "StringCaseLocaleUsage",
-  )
+  val errorproneRules = rootProject.projectDir.resolve("codestyle/errorprone-rules.properties")
+  inputs.file(errorproneRules).withPathSensitivity(PathSensitivity.RELATIVE)
+  options.errorprone.checks.putAll(provider { memoizedErrorproneRules(errorproneRules) })
 }
 
+private fun memoizedErrorproneRules(rulesFile: File): Map<String, CheckSeverity> =
+  rulesFile.reader().use {
+    val rules = Properties()
+    rules.load(it)
+    rules
+      .mapKeys { e -> (e.key as String).trim() }
+      .mapValues { e -> (e.value as String).trim() }
+      .filter { e -> e.key.isNotEmpty() && e.value.isNotEmpty() }
+      .mapValues { e -> CheckSeverity.valueOf(e.value) }
+      .toMap()
+  }
+
 tasks.register("compileAll").configure {
   group = "build"
   description = "Runs all compilation and jar tasks"

@@ -0,0 +1,284 @@
+#
+# Licensed to the Apache Software Foundation (ASF) under one
+# or more contributor license agreements.  See the NOTICE file
+# distributed with this work for additional information
+# regarding copyright ownership.  The ASF licenses this file
+# to you under the Apache License, Version 2.0 (the
+# "License"); you may not use this file except in compliance
+# with the License.  You may obtain a copy of the License at
+#
+#   http://www.apache.org/licenses/LICENSE-2.0
+#
+# Unless required by applicable law or agreed to in writing,
+# software distributed under the License is distributed on an
+# "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+# KIND, either express or implied.  See the License for the
+# specific language governing permissions and limitations
+# under the License.
+#
+
+####################################################################################################
+# On by default : ERROR
+# See https://errorprone.info/bugpatterns
+####################################################################################################
+
+####################################################################################################
+# On by default : WARNING
+# See https://errorprone.info/bugpatterns
+####################################################################################################
+
+AnnotateFormatMethod=ERROR
+# This method passes a pair of parameters through to String.format, but the enclosing method wasn't annotated @FormatMethod. Doing so gives compile-time rather than run-time protection against malformed format strings.
+
+ArrayAsKeyOfSetOrMap=ERROR
+# Arrays do not override equals() or hashCode, so comparisons will be done on reference equality only. If neither deduplication nor lookup are needed, consider using a List instead. Otherwise, use IdentityHashMap/Set, a Map from a library that handles object arrays, or an Iterable/List of pairs.
+
+AssertEqualsArgumentOrderChecker=ERROR
+# Arguments are swapped in assertEquals-like call
+
+AssertThrowsMultipleStatements=ERROR
+# The lambda passed to assertThrows should contain exactly one statement
+
+AssertionFailureIgnored=ERROR
+# This assertion throws an AssertionError if it fails, which will be caught by an enclosing try block.
+
+BadImport=ERROR
+# Importing nested classes/static methods/static fields with commonly-used names can make code harder to read, because it may not be clear from the context exactly which type is being referred to. Qualifying the name with that of the containing class can make the code clearer.
+
+BadInstanceof=ERROR
+# instanceof used in a way that is equivalent to a null check.
+
+BareDotMetacharacter=ERROR
+# "." is rarely useful as a regex, as it matches any character. To match a literal '.' character, instead write "\.".
+
+BigDecimalEquals=ERROR
+# BigDecimal#equals has surprising behavior: it also compares scale.
+
+BigDecimalLiteralDouble=ERROR
+# new BigDecimal(double) loses precision in this case.
+
+BoxedPrimitiveConstructor=ERROR
+# valueOf or autoboxing provides better time and space performance
+
+ByteBufferBackingArray=ERROR
+# ByteBuffer.array() shouldn't be called unless ByteBuffer.arrayOffset() is used or if the ByteBuffer was initialized using ByteBuffer.wrap() or ByteBuffer.allocate().
+
+CanIgnoreReturnValueSuggester=OFF
+# Methods that always 'return this' should be annotated with @CanIgnoreReturnValue
+
+CatchAndPrintStackTrace=ERROR
+# Logging or rethrowing exceptions should usually be preferred to catching and calling printStackTrace
+
+ClassCanBeStatic=ERROR
+# Inner class is non-static but does not reference enclosing class
+
+ClassNewInstance=ERROR
+# Class.newInstance() bypasses exception checking; prefer getDeclaredConstructor().newInstance()
+
+DateFormatConstant=ERROR
+# DateFormat is not thread-safe, and should not be used as a constant field.
+
+DefaultCharset=ERROR
+# Implicit use of the platform default charset, which can result in differing behaviour between JVM executions or incorrect behavior if the encoding of the data source doesn't match expectations.
+
+DistinctVarargsChecker=ERROR
+# Method expects distinct arguments at some/all positions
+
+DoubleCheckedLocking=ERROR
+# Double-checked locking on non-volatile fields is unsafe
+
+# TODO enable: EqualsGetClass=ERROR
+# Prefer instanceof to getClass when implementing Object#equals.
+
+EqualsIncompatibleType=ERROR
+# An equality test between objects with incompatible types always returns false
+
+EqualsUnsafeCast=ERROR
+# The contract of #equals states that it should return false for incompatible types, while this implementation may throw ClassCastException.
+
+EqualsUsingHashCode=ERROR
+# Implementing #equals by just comparing hashCodes is fragile. Hashes collide frequently, and this will lead to false positives in #equals.
+
+ErroneousBitwiseExpression=ERROR
+# This expression evaluates to 0. If this isn't an error, consider expressing it as a literal 0.
+
+ErroneousThreadPoolConstructorChecker=ERROR
+# Thread pool size will never go beyond corePoolSize if an unbounded queue is used
+
+EscapedEntity=ERROR
+# HTML entities in @code/@literal tags will appear literally in the rendered javadoc.
+
+FallThrough=ERROR
+# Switch case may fall through
+
+FloatCast=ERROR
+# Use parentheses to make the precedence explicit
+
+FloatingPointAssertionWithinEpsilon=ERROR
+# This fuzzy equality check is using a tolerance less than the gap to the next number. You may want a less restrictive tolerance, or to assert equality.
+
+FloatingPointLiteralPrecision=ERROR
+# Floating point literal loses precision
+
+FutureReturnValueIgnored=ERROR
+# Return value of methods returning Future must be checked. Ignoring returned Futures suppresses exceptions thrown from the code that completes the Future.
+
+GetClassOnEnum=ERROR
+# Calling getClass() on an enum may return a subclass of the enum type
+
+InconsistentHashCode=ERROR
+# Including fields in hashCode which are not compared in equals violates the contract of hashCode.
+
+IntLongMath=ERROR
+# Expression of type int may overflow before being assigned to a long
+
+JavaLangClash=ERROR
+# Never reuse class names from java.lang
+
+JdkObsolete=ERROR
+# Suggests alternatives to obsolete JDK classes.
+
+LockNotBeforeTry=ERROR
+# Calls to Lock#lock should be immediately followed by a try block which releases the lock.
+
+LongDoubleConversion=ERROR
+# Conversion from long to double may lose precision; use an explicit cast to double if this was intentional
+
+LongFloatConversion=ERROR
+# Conversion from long to float may lose precision; use an explicit cast to float if this was intentional
+
+MissingCasesInEnumSwitch=ERROR
+# Switches on enum types should either handle all values, or have a default case.
+
+MissingOverride=ERROR
+# method overrides method in supertype; expected @Override
+
+ModifiedButNotUsed=ERROR
+# A collection or proto builder was created, but its values were never accessed.
+
+MockNotUsedInProduction=ERROR
+# This mock is instantiated and configured, but is never passed to production code. It should be
+# either removed or used.
+
+NonAtomicVolatileUpdate=ERROR
+# This update of a volatile variable is non-atomic
+
+NonCanonicalType=ERROR
+# This type is referred to by a non-canonical name, which may be misleading.
+
+NotJavadoc=ERROR
+# Avoid using /** for comments which aren't actually Javadoc.
+
+NullOptional=ERROR
+# Passing a literal null to an Optional parameter is almost certainly a mistake. Did you mean to provide an empty Optional?
+
+ObjectEqualsForPrimitives=ERROR
+# Avoid unnecessary boxing by using plain == for primitive types.
+
+OperatorPrecedence=ERROR
+# Use grouping parenthesis to make the operator precedence explicit
+
+OrphanedFormatString=ERROR
+# String literal contains format specifiers, but is not passed to a format method
+
+Overrides=ERROR
+# Varargs doesn't agree for overridden method
+
+# TODO PatternMatchingInstanceof=ERROR
+# This code can be simplified to use a pattern-matching instanceof.
+
+StreamToIterable=ERROR
+# Using stream::iterator creates a one-shot Iterable, which may cause surprising failures.
+
+SynchronizeOnNonFinalField=ERROR
+# Synchronizing on non-final fields is not safe: if the field is ever updated, different threads may end up locking on different objects.
+
+ThreadLocalUsage=ERROR
+# ThreadLocals should be stored in static fields
+
+URLEqualsHashCode=ERROR
+# Avoid hash-based containers of java.net.URL–the containers rely on equals() and hashCode(), which cause java.net.URL to make blocking internet connections.
+
+UnnecessaryLambda=ERROR
+# Returning a lambda from a helper method or saving it in a constant is unnecessary; prefer to implement the functional interface method directly and use a method reference instead.
+
+# TODO enable: UnusedMethod=ERROR
+# Unused.
+
+UnusedNestedClass=ERROR
+# This nested class is unused, and can be removed.
+
+UnusedTypeParameter=ERROR
+# This type parameter is unused and can be removed.
+
+UseCorrectAssertInTests=ERROR
+# Java assert is used in test. For testing purposes Assert.* matchers should be used.
+
+####################################################################################################
+# Experimental : ERROR
+# See https://errorprone.info/bugpatterns
+####################################################################################################
+
+####################################################################################################
+# Experimental : WARNING
+# See https://errorprone.info/bugpatterns
+####################################################################################################
+
+ConstantPatternCompile=ERROR
+# Variables initialized with Pattern#compile calls on constants can be constants
+
+PrimitiveArrayPassedToVarargsMethod=ERROR
+# Passing a primitive array to a varargs method is usually wrong
+
+RedundantOverride=ERROR
+# This overriding method is redundant, and can be removed.
+
+RedundantThrows=ERROR
+# Thrown exception is a subtype of another
+
+StringCaseLocaleUsage=ERROR
+# Specify a `Locale` when calling `String#to{Lower,Upper}Case`. (Note: there are multiple suggested fixes; the third may be most appropriate if you're dealing with ASCII Strings.)
+
+StronglyTypeByteString=WARN
+# This primitive byte array is only used to construct ByteStrings. It would be clearer to strongly type the field instead.
+
+StronglyTypeTime=ERROR
+# This primitive integral type is only used to construct time types. It would be clearer to strongly type the field instead.
+
+TestExceptionChecker=ERROR
+# Using @Test(expected=…) is discouraged, since the test will pass if any statement in the test method throws the expected exception
+
+TransientMisuse=ERROR
+# Static fields are implicitly transient, so the explicit modifier is unnecessary
+
+UrlInSee=ERROR
+# URLs should not be used in @see tags; they are designed for Java elements which could be used with @link.
+
+####################################################################################################
+# Experimental : SUGGESTION
+# See https://errorprone.info/bugpatterns
+####################################################################################################
+
+FieldCanBeStatic=ERROR
+# A final field initialized at compile-time with an instance of an immutable type can be static.
+
+ForEachIterable=ERROR
+# This loop can be replaced with an enhanced for loop.
+
+MixedArrayDimensions=ERROR
+# C-style array declarations should not be used
+
+PackageLocation=ERROR
+# Package names should match the directory they are declared in
+
+TryFailRefactoring=ERROR
+# Prefer assertThrows to try/fail
+
+UnnecessaryBoxedAssignment=WARN
+# This expression can be implicitly boxed.
+
+UnnecessaryBoxedVariable=ERROR
+# It is unnecessary for this variable to be boxed. Use the primitive instead.
+
+UseEnumSwitch=ERROR
+# Prefer using a switch instead of a chained if-else for enums
@@ -77,7 +77,12 @@ public class PolarisEclipseLinkMetaStoreSessionImpl extends AbstractTransactiona
   private static final ConcurrentHashMap<String, EntityManagerFactory> realmFactories =
       new ConcurrentHashMap<>();
   private final EntityManagerFactory emf;
+
+  // TODO this has to be refactored, see https://github.com/apache/polaris/issues/463 and
+  //  https://errorprone.info/bugpattern/ThreadLocalUsage
+  @SuppressWarnings("ThreadLocalUsage")
   private final ThreadLocal<EntityManager> localSession = new ThreadLocal<>();
+
   private final PolarisEclipseLinkStore store;
   private final PolarisStorageIntegrationProvider storageIntegrationProvider;
   private final PrincipalSecretsGenerator secretsGenerator;

@@ -40,6 +40,7 @@ default boolean ready() {
   List<Error> getErrors();
 
   @PolarisImmutable
+  @SuppressWarnings("JavaLangClash")
   interface Error {
 
     static Error of(String message, String offendingProperty) {

@@ -36,6 +36,8 @@ public class AwsStorageConfigurationInfo extends PolarisStorageConfigurationInfo
   @JsonIgnore
   public static final String ROLE_ARN_PATTERN = "^arn:(aws|aws-us-gov):iam::(\\d{12}):role/.+$";
 
+  private static final Pattern ROLE_ARN_PATTERN_COMPILED = Pattern.compile(ROLE_ARN_PATTERN);
+
   // AWS role to be assumed
   private final @Nonnull String roleARN;
 
@@ -131,8 +133,7 @@ public String getAwsPartition() {
 
   private static String parseAwsAccountId(String arn) {
     validateArn(arn);
-    Pattern pattern = Pattern.compile(ROLE_ARN_PATTERN);
-    Matcher matcher = pattern.matcher(arn);
+    Matcher matcher = ROLE_ARN_PATTERN_COMPILED.matcher(arn);
     if (matcher.matches()) {
       return matcher.group(2);
     } else {
@@ -142,8 +143,7 @@ private static String parseAwsAccountId(String arn) {
 
   private static String parseAwsPartition(String arn) {
     validateArn(arn);
-    Pattern pattern = Pattern.compile(ROLE_ARN_PATTERN);
-    Matcher matcher = pattern.matcher(arn);
+    Matcher matcher = ROLE_ARN_PATTERN_COMPILED.matcher(arn);
     if (matcher.matches()) {
       return matcher.group(1);
     } else {

@@ -57,6 +57,7 @@ void testBasic() {
    * only allow "maxTokens" requests
    */
   @Test
+  @SuppressWarnings("FutureReturnValueIgnored") // implementation looks okay
   void testConcurrent() throws InterruptedException {
     int maxTokens = 100;
     int numTasks = 50000;

@@ -23,6 +23,7 @@
 /** An OAuth Error Token Response as defined by the Iceberg REST API OpenAPI Spec. */
 public class OAuthTokenErrorResponse {
 
+  @SuppressWarnings("JavaLangClash")
   public enum Error {
     invalid_request("The request is invalid"),
     invalid_client("The Client is invalid"),

@@ -18,7 +18,6 @@
  */
 package org.apache.polaris.service.config;
 
-import com.fasterxml.jackson.core.JacksonException;
 import com.fasterxml.jackson.core.JsonParser;
 import com.fasterxml.jackson.core.TreeNode;
 import com.fasterxml.jackson.databind.DeserializationContext;
@@ -70,7 +69,7 @@ public static final class CreateCatalogRequestDeserializer
       extends JsonDeserializer<CreateCatalogRequest> {
     @Override
     public CreateCatalogRequest deserialize(JsonParser p, DeserializationContext ctxt)
-        throws IOException, JacksonException {
+        throws IOException {
       TreeNode treeNode = p.readValueAsTree();
       if (treeNode.isObject() && ((ObjectNode) treeNode).has("catalog")) {
         return CreateCatalogRequest.builder()
@@ -92,7 +91,7 @@ public static final class CreatePrincipalRequestDeserializer
       extends JsonDeserializer<CreatePrincipalRequest> {
     @Override
     public CreatePrincipalRequest deserialize(JsonParser p, DeserializationContext ctxt)
-        throws IOException, JacksonException {
+        throws IOException {
       TreeNode treeNode = p.readValueAsTree();
       if (treeNode.isObject() && ((ObjectNode) treeNode).has("principal")) {
         return CreatePrincipalRequest.builder()
@@ -118,7 +117,7 @@ public static final class CreatePrincipalRoleRequestDeserializer
       extends JsonDeserializer<CreatePrincipalRoleRequest> {
     @Override
     public CreatePrincipalRoleRequest deserialize(JsonParser p, DeserializationContext ctxt)
-        throws IOException, JacksonException {
+        throws IOException {
       TreeNode treeNode = p.readValueAsTree();
       if (treeNode.isObject() && ((ObjectNode) treeNode).has("principalRole")) {
         return CreatePrincipalRoleRequest.builder()
@@ -141,7 +140,7 @@ public static final class GrantPrincipalRoleRequestDeserializer
       extends JsonDeserializer<GrantPrincipalRoleRequest> {
     @Override
     public GrantPrincipalRoleRequest deserialize(JsonParser p, DeserializationContext ctxt)
-        throws IOException, JacksonException {
+        throws IOException {
       TreeNode treeNode = p.readValueAsTree();
       if (treeNode.isObject() && ((ObjectNode) treeNode).has("principalRole")) {
         return GrantPrincipalRoleRequest.builder()
@@ -164,7 +163,7 @@ public static final class CreateCatalogRoleRequestDeserializer
       extends JsonDeserializer<CreateCatalogRoleRequest> {
     @Override
     public CreateCatalogRoleRequest deserialize(JsonParser p, DeserializationContext ctxt)
-        throws IOException, JacksonException {
+        throws IOException {
       TreeNode treeNode = p.readValueAsTree();
       if (treeNode.isObject() && ((ObjectNode) treeNode).has("catalogRole")) {
         return CreateCatalogRoleRequest.builder()
@@ -187,7 +186,7 @@ public static final class GrantCatalogRoleRequestDeserializer
       extends JsonDeserializer<GrantCatalogRoleRequest> {
     @Override
     public GrantCatalogRoleRequest deserialize(JsonParser p, DeserializationContext ctxt)
-        throws IOException, JacksonException {
+        throws IOException {
       TreeNode treeNode = p.readValueAsTree();
       if (treeNode.isObject() && ((ObjectNode) treeNode).has("catalogRole")) {
         return GrantCatalogRoleRequest.builder()
@@ -208,7 +207,7 @@ public GrantCatalogRoleRequest deserialize(JsonParser p, DeserializationContext
   public static final class AddGrantRequestDeserializer extends JsonDeserializer<AddGrantRequest> {
     @Override
     public AddGrantRequest deserialize(JsonParser p, DeserializationContext ctxt)
-        throws IOException, JacksonException {
+        throws IOException {
       TreeNode treeNode = p.readValueAsTree();
       if (treeNode.isObject() && ((ObjectNode) treeNode).has("grant")) {
         return AddGrantRequest.builder()
@@ -230,7 +229,7 @@ public static final class RevokeGrantRequestDeserializer
       extends JsonDeserializer<RevokeGrantRequest> {
     @Override
     public RevokeGrantRequest deserialize(JsonParser p, DeserializationContext ctxt)
-        throws IOException, JacksonException {
+        throws IOException {
       TreeNode treeNode = p.readValueAsTree();
       if (treeNode.isObject() && ((ObjectNode) treeNode).has("grant")) {
         return RevokeGrantRequest.builder()

@@ -29,7 +29,7 @@ public class TokenBucket {
   private final long maxTokens;
   private final InstantSource instantSource;
 
-  private double tokens;
+  private long tokens;
   private long lastTokenGenerationMillis;
 
   public TokenBucket(long tokensPerSecond, long maxTokens, InstantSource instantSource) {
@@ -51,7 +51,7 @@ public synchronized boolean tryAcquire() {
     long t = instantSource.millis();
     long millisPassed = Math.subtractExact(t, lastTokenGenerationMillis);
     lastTokenGenerationMillis = t;
-    tokens = Math.min(maxTokens, tokens + (millisPassed * tokensPerMilli));
+    tokens = Math.min(maxTokens, tokens + ((long) (millisPassed * tokensPerMilli)));
 
     // Take a token if they have one available
     if (tokens >= 1) {

@@ -82,6 +82,7 @@ public void addTaskHandler(TaskHandler taskHandler) {
    * asynchronously with a clone of the provided {@link CallContext}.
    */
   @Override
+  @SuppressWarnings("FutureReturnValueIgnored") // it _should_ be okay in this particular case
   public void addTaskHandlerContext(long taskEntityId, CallContext callContext) {
     // Unfortunately CallContext is a request-scoped bean and must be cloned now,
     // because its usage inside the TaskExecutor thread pool will outlive its