Add spotbugs & pmd for static analysis

Also move internal state validation into DoSomethingProvider since that sits in test code

pom.xml

@@ -54,6 +54,13 @@
       <scope>provided</scope>
     </dependency>
 
+    <dependency>
+      <!-- used so that lombok can generate suppressions for spotbugs. It needs to find it on the relevant classpath -->
+      <groupId>com.github.spotbugs</groupId>
+      <artifactId>spotbugs</artifactId>
+      <version>4.7.1</version>
+    </dependency>
+
     <dependency>
       <groupId>org.slf4j</groupId>
       <artifactId>slf4j-api</artifactId>
@@ -132,6 +139,7 @@
           </argLine>
         </configuration>
       </plugin>
+
       <plugin>
         <groupId>org.apache.maven.plugins</groupId>
         <artifactId>maven-failsafe-plugin</artifactId>
@@ -263,6 +271,54 @@
       </plugin>
       <!-- end sign -->
 
+      <plugin>
+        <groupId>org.apache.maven.plugins</groupId>
+        <artifactId>maven-pmd-plugin</artifactId>
+        <version>3.13.0</version>
+        <executions>
+          <execution>
+            <id>run-pmd</id>
+            <phase>verify</phase>
+            <goals>
+              <goal>check</goal>
+            </goals>
+          </execution>
+        </executions>
+      </plugin>
+
+      <plugin>
+        <groupId>com.github.spotbugs</groupId>
+        <artifactId>spotbugs-maven-plugin</artifactId>
+        <version>4.7.0.0</version>
+        <configuration>
+          <excludeFilterFile>spotbugs-exclusions.xml</excludeFilterFile>
+          <plugins>
+            <plugin>
+              <groupId>com.h3xstream.findsecbugs</groupId>
+              <artifactId>findsecbugs-plugin</artifactId>
+              <version>1.12.0</version>
+            </plugin>
+          </plugins>
+        </configuration>
+        <dependencies>
+          <!-- overwrite dependency on spotbugs if you want to specify the version of spotbugs -->
+          <dependency>
+            <groupId>com.github.spotbugs</groupId>
+            <artifactId>spotbugs</artifactId>
+            <version>4.7.1</version>
+          </dependency>
+        </dependencies>
+        <executions>
+          <execution>
+            <id>run-spotbugs</id>
+            <phase>verify</phase>
+            <goals>
+              <goal>check</goal>
+            </goals>
+          </execution>
+        </executions>
+      </plugin>
+
     </plugins>
   </build>
 

spotbugs-exclusions.xml

@@ -0,0 +1,29 @@
+<?xml version="1.0" encoding="UTF-8"?>
+<FindBugsFilter
+        xmlns="https://github.com/spotbugs/filter/3.0.0"
+        xmlns:xsi="http://www.w3.org/2001/XMLSchema-instance"
+        xsi:schemaLocation="https://github.com/spotbugs/filter/3.0.0 https://raw.githubusercontent.com/spotbugs/spotbugs/3.1.0/spotbugs/etc/findbugsfilter.xsd">
+
+    <!-- I'm reasonably confident that the singleton pattern isn't exposing internal representation -->
+    <And>
+        <Class name="dev.openfeature.javasdk.OpenFeatureAPI"/>
+        <Bug pattern="MS_EXPOSE_REP"/>
+    </And>
+    <!-- similarly, client using the singleton doesn't seem bad -->
+    <And>
+        <Class name="dev.openfeature.javasdk.OpenFeatureClient"/>
+        <Bug pattern="EI_EXPOSE_REP2"/>
+    </And>
+
+    <!-- Test class that should be excluded -->
+    <Match>
+        <Class name="dev.openfeature.javasdk.DoSomethingProvider"/>
+    </Match>
+    <!-- All bugs in test classes, except for JUnit-specific bugs -->
+    <Match>
+        <Class name="~.*\.*Test" />
+        <Not>
+            <Bug code="IJU" />
+        </Not>
+    </Match>
+</FindBugsFilter>

src/lombok.config

@@ -1,2 +1,2 @@
 lombok.addLombokGeneratedAnnotation = true
-
+lombok.extern.findbugs.addSuppressFBWarnings = true

src/main/java/dev/openfeature/javasdk/EvaluationContext.java

@@ -12,7 +12,7 @@ import java.util.Map;
 @SuppressWarnings("PMD.BeanMembersShouldSerialize")
 public class EvaluationContext {
     @Setter @Getter private String targetingKey;
-    private final Map<String, dev.openfeature.javasdk.internal.Pair<FlagValueType, Object>> attributes;
+    private final Map<String, Pair<FlagValueType, Object>> attributes;
 
     public EvaluationContext() {
         this.targetingKey = "";

src/main/java/dev/openfeature/javasdk/Metadata.java

@@ -4,5 +4,5 @@ package dev.openfeature.javasdk;
  * Holds identifying information about a given entity
  */
 public interface Metadata {
-    public String getName();
+    String getName();
 }

src/main/java/dev/openfeature/javasdk/NoOpProvider.java

@@ -9,12 +9,6 @@ public class NoOpProvider implements FeatureProvider {
     public static final String PASSED_IN_DEFAULT = "Passed in default";
     @Getter
     private final String name = "No-op Provider";
-    private EvaluationContext ctx;
-
-    public EvaluationContext getMergedContext() {
-        return ctx;
-    }
-
     @Override
     public Metadata getMetadata() {
         return new Metadata() {
@@ -27,7 +21,6 @@ public class NoOpProvider implements FeatureProvider {
 
     @Override
     public ProviderEvaluation<Boolean> getBooleanEvaluation(String key, Boolean defaultValue, EvaluationContext ctx, FlagEvaluationOptions options) {
-        this.ctx = ctx;
         return ProviderEvaluation.<Boolean>builder()
                 .value(defaultValue)
                 .variant(PASSED_IN_DEFAULT)
@@ -37,7 +30,6 @@ public class NoOpProvider implements FeatureProvider {
 
     @Override
     public ProviderEvaluation<String> getStringEvaluation(String key, String defaultValue, EvaluationContext ctx, FlagEvaluationOptions options) {
-        this.ctx = ctx;
         return ProviderEvaluation.<String>builder()
                 .value(defaultValue)
                 .variant(PASSED_IN_DEFAULT)
@@ -47,7 +39,6 @@ public class NoOpProvider implements FeatureProvider {
 
     @Override
     public ProviderEvaluation<Integer> getIntegerEvaluation(String key, Integer defaultValue, EvaluationContext ctx, FlagEvaluationOptions options) {
-        this.ctx = ctx;
         return ProviderEvaluation.<Integer>builder()
                 .value(defaultValue)
                 .variant(PASSED_IN_DEFAULT)
@@ -56,7 +47,6 @@ public class NoOpProvider implements FeatureProvider {
     }
     @Override
     public ProviderEvaluation<Double> getDoubleEvaluation(String key, Double defaultValue, EvaluationContext ctx, FlagEvaluationOptions options) {
-        this.ctx = ctx;
         return ProviderEvaluation.<Double>builder()
                 .value(defaultValue)
                 .variant(PASSED_IN_DEFAULT)
@@ -65,7 +55,6 @@ public class NoOpProvider implements FeatureProvider {
     }
     @Override
     public <T> ProviderEvaluation<T> getObjectEvaluation(String key, T defaultValue, EvaluationContext invocationContext, FlagEvaluationOptions options) {
-        this.ctx = ctx;
         return ProviderEvaluation.<T>builder()
                 .value(defaultValue)
                 .variant(PASSED_IN_DEFAULT)

src/main/java/dev/openfeature/javasdk/exceptions/FlagNotFoundError.java

@@ -6,6 +6,6 @@ import lombok.experimental.StandardException;
 
 @StandardException
 public class FlagNotFoundError extends OpenFeatureError {
-    private static long serialVersionUID = 1L;
+    private static final long serialVersionUID = 1L;
     @Getter private final ErrorCode errorCode = ErrorCode.GENERAL;
 }

src/main/java/dev/openfeature/javasdk/exceptions/GeneralError.java

@@ -6,6 +6,6 @@ import lombok.experimental.StandardException;
 
 @StandardException
 public class GeneralError extends OpenFeatureError{
-    private static long serialVersionUID = 1L;
+    private static final long serialVersionUID = 1L;
     @Getter private final ErrorCode errorCode = ErrorCode.GENERAL;
 }

src/main/java/dev/openfeature/javasdk/exceptions/OpenFeatureError.java

@@ -5,6 +5,6 @@ import lombok.experimental.StandardException;
 
 @StandardException
 public abstract class OpenFeatureError extends RuntimeException {
-    private static long serialVersionUID = 1L;
+    private static final long serialVersionUID = 1L;
     public abstract ErrorCode getErrorCode();
 }

src/main/java/dev/openfeature/javasdk/exceptions/ParseError.java

@@ -6,7 +6,7 @@ import lombok.experimental.StandardException;
 
 @StandardException
 public class ParseError extends OpenFeatureError {
-    private static long serialVersionUID = 1L;
+    private static final long serialVersionUID = 1L;
 
     @Getter private final ErrorCode errorCode = ErrorCode.PARSE_ERROR;
 

src/main/java/dev/openfeature/javasdk/exceptions/TypeMismatchError.java

@@ -6,7 +6,7 @@ import lombok.experimental.StandardException;
 
 @StandardException
 public class TypeMismatchError extends OpenFeatureError {
-    private static long serialVersionUID = 1L;
+    private static final long serialVersionUID = 1L;
 
     @Getter private final ErrorCode errorCode = ErrorCode.TYPE_MISMATCH;
 

src/test/java/dev/openfeature/javasdk/DoSomethingProvider.java

@@ -2,6 +2,12 @@ package dev.openfeature.javasdk;
 
 public class DoSomethingProvider implements FeatureProvider {
 
+    private EvaluationContext savedContext;
+
+    public EvaluationContext getMergedContext() {
+        return savedContext;
+    }
+
     @Override
     public Metadata getMetadata() {
         return () -> "test";
@@ -9,6 +15,7 @@ public class DoSomethingProvider implements FeatureProvider {
 
     @Override
     public ProviderEvaluation<Boolean> getBooleanEvaluation(String key, Boolean defaultValue, EvaluationContext ctx, FlagEvaluationOptions options) {
+        savedContext = ctx;
         return ProviderEvaluation.<Boolean>builder()
                 .value(!defaultValue).build();
     }
@@ -22,6 +29,7 @@ public class DoSomethingProvider implements FeatureProvider {
 
     @Override
     public ProviderEvaluation<Integer> getIntegerEvaluation(String key, Integer defaultValue, EvaluationContext ctx, FlagEvaluationOptions options) {
+        savedContext = ctx;
         return ProviderEvaluation.<Integer>builder()
                 .value(defaultValue * 100)
                 .build();
@@ -29,6 +37,7 @@ public class DoSomethingProvider implements FeatureProvider {
 
     @Override
     public ProviderEvaluation<Double> getDoubleEvaluation(String key, Double defaultValue, EvaluationContext ctx, FlagEvaluationOptions options) {
+        savedContext = ctx;
         return ProviderEvaluation.<Double>builder()
                 .value(defaultValue * 100)
                 .build();
@@ -36,6 +45,7 @@ public class DoSomethingProvider implements FeatureProvider {
 
     @Override
     public <T> ProviderEvaluation<T> getObjectEvaluation(String key, T defaultValue, EvaluationContext invocationContext, FlagEvaluationOptions options) {
+        savedContext = invocationContext;
         return ProviderEvaluation.<T>builder()
                 .value(null)
                 .build();

src/test/java/dev/openfeature/javasdk/FlagEvaluationSpecTest.java

@@ -218,7 +218,7 @@ class FlagEvaluationSpecTest implements HookFixtures {
     @Specification(number="3.2.2", text="Evaluation context MUST be merged in the order: API (global) - client - invocation, with duplicate values being overwritten.")
     @Test void multi_layer_context_merges_correctly() {
         OpenFeatureAPI api = OpenFeatureAPI.getInstance();
-        NoOpProvider provider = new NoOpProvider();
+        DoSomethingProvider provider = new DoSomethingProvider();
         api.setProvider(provider);
 
         EvaluationContext apiCtx = new EvaluationContext();
@@ -238,7 +238,8 @@ class FlagEvaluationSpecTest implements HookFixtures {
         clientCtx.addStringAttribute("common", "5");
         clientCtx.addStringAttribute("invocation", "6");
 
-        assertFalse(c.getBooleanValue("key", false, invocationCtx));
+        // dosomethingprovider inverts this value.
+        assertTrue(c.getBooleanValue("key", false, invocationCtx));
 
         EvaluationContext merged = provider.getMergedContext();
         assertEquals("6", merged.getStringAttribute("invocation"));