Lab 02 — SpotBugs Static Analysis

Phase: 05 — Code Quality and Engineering Discipline
Difficulty: ⭐⭐⭐☆☆ | Estimated Time: 2–2.5 hours
Primary Artifact: ProductionConfig.java with 4 planted SpotBugs bugs
Verify: mvn spotbugs:check reports 0 HIGH-confidence bugs after your fixes

Goal

ProductionConfig.java is a configuration manager that compiles cleanly and passes Checkstyle — but it has 4 SpotBugs bugs that indicate real correctness and safety problems. Your job is to:

1. Run mvn spotbugs:check and read the bug report
2. Look up each bug category in the SpotBugs documentation
3. Understand why each pattern is a bug (not just a style issue)
4. Fix each one so the build goes green

Setup

cd phase-05-code-quality/lab-02-spotbugs
mvn spotbugs:check

The build will fail. Read the output — each bug is reported with:

• The bug category code (e.g., NP_NULL_ON_SOME_PATH)
• The class name and line number
• A one-line description

For the full HTML report with more context:

mvn spotbugs:spotbugs
open target/spotbugs.html       # macOS
xdg-open target/spotbugs.html   # Linux

The 4 Bugs

Find and fix each one. Do not expand the answers until you have attempted to understand the bug category from the SpotBugs description alone.

Steps

Step 1 — Read the report

mvn spotbugs:check 2>&1 | grep -E 'Bug|Priority|Type'

Note the bug code for each reported issue. You should see exactly 4 HIGH-confidence bugs.

Step 2 — Look up each bug code

For each bug code (e.g., NP_NULL_ON_SOME_PATH), look it up at:
https://spotbugs.readthedocs.io/en/latest/bugDescriptions.html

Read the description. Before looking at the fix, answer:

• What runtime failure does this cause?
• Under what inputs does it trigger?
• Is this a correctness bug, a safety bug, or a performance bug?

Step 3 — Fix each bug

Fix them in order from most dangerous to least:

1. NP_NULL_ON_SOME_PATH — will cause a production NPE on any missing config key
2. EI_EXPOSE_REP — silent data corruption when callers modify the returned map
3. MS_MUTABLE_COLLECTION — silent data corruption when callers modify REQUIRED_KEYS
4. DM_NUMBER_CTOR — deprecated API, minor performance concern

After each fix, run mvn spotbugs:check to confirm the count decreases by 1.

Step 4 — Green build

mvn spotbugs:check && echo "ALL CLEAN"

Step 5 — Design discussion

Bug #3 (MS_MUTABLE_COLLECTION) is particularly subtle because the code looks correct — the field is declared static final. Answer in a comment added to the top of ProductionConfig.java:

1. What does final actually guarantee for a reference type?
2. What does List.of(...) guarantee that Collections.unmodifiableList(new ArrayList<>()) does NOT?
3. If REQUIRED_KEYS needed to be built dynamically at class load time (the static {} block pattern), what would be a safe alternative to List.of()?

Verification

# Zero HIGH-confidence SpotBugs bugs
mvn spotbugs:check
echo "Exit code: $?"  # must be 0

# Code still compiles
mvn compile

Source Files

pom.xml

<?xml version="1.0" encoding="UTF-8"?>
<project xmlns="http://maven.apache.org/POM/4.0.0"
    xmlns:xsi="http://www.w3.org/2001/XMLSchema-instance"
    xsi:schemaLocation="http://maven.apache.org/POM/4.0.0 https://maven.apache.org/xsd/maven-4.0.0.xsd">
    <modelVersion>4.0.0</modelVersion>

    <groupId>org.example.quality</groupId>
    <artifactId>spotbugs-lab</artifactId>
    <version>1.0.0-SNAPSHOT</version>
    <packaging>jar</packaging>
    <name>Code Quality Lab 02 — SpotBugs</name>

    <properties>
        <maven.compiler.source>11</maven.compiler.source>
        <maven.compiler.target>11</maven.compiler.target>
        <project.build.sourceEncoding>UTF-8</project.build.sourceEncoding>
    </properties>

    <build>
        <plugins>
            <!--
        spotbugs-maven-plugin: runs SpotBugs against the compiled bytecode.

        IMPORTANT: SpotBugs analyzes .class files, not .java files.
        You must run `mvn compile` before `mvn spotbugs:check` (or use `mvn verify`
        which runs compile → test → package → verify in order).

        Goals:
          mvn spotbugs:check     — fail build if bugs found above threshold
          mvn spotbugs:spotbugs  — generate report only (no build failure)
          mvn spotbugs:gui       — launch the interactive SpotBugs viewer

        effort: analysis depth.
          max = most thorough (slowest) — use this in CI for maximum coverage
          default = balanced (good for dev iteration)
          min = fastest (fewer checks)

        threshold: minimum confidence level that causes a build failure.
          High   = only HIGH confidence bugs fail the build
          Medium = HIGH + MEDIUM confidence bugs fail the build
          Low    = all bugs fail the build (many false positives)

        For this lab we use effort=max and threshold=High so that only the
        4 deliberately planted HIGH-confidence bugs cause the build failure.
      -->
            <plugin>
                <groupId>com.github.spotbugs</groupId>
                <artifactId>spotbugs-maven-plugin</artifactId>
                <version>4.8.3.1</version>
                <configuration>
                    <effort>Max</effort>
                    <threshold>High</threshold>
                    <xmlOutput>true</xmlOutput>
                </configuration>
                <executions>
                    <execution>
                        <id>spotbugs-verify</id>
                        <phase>verify</phase>
                        <goals>
                            <goal>check</goal>
                        </goals>
                    </execution>
                </executions>
            </plugin>
        </plugins>
    </build>

</project>

ProductionConfig.java (before your fixes)

package org.example.quality;

import java.util.ArrayList;
import java.util.HashMap;
import java.util.List;
import java.util.Map;

/**
 * Manages application configuration as a key-value store.
 *
 * <p>
 * Configuration is loaded by the {@link #load(Map)} method and
 * validated against {@link #REQUIRED_KEYS} to ensure all mandatory
 * settings are present before the application starts.
 *
 * <p>
 * This class has 4 SpotBugs bugs. Find and fix them.
 * Run: mvn spotbugs:check
 */
public class ProductionConfig {

    // BUG 3 — MS_MUTABLE_COLLECTION
    // The 'final' keyword on a static field only prevents reassigning the
    // reference.
    // The List object itself is still mutable: callers can call
    // REQUIRED_KEYS.add("evil")
    // and corrupt every downstream validation.
    //
    // Fix: replace with List.of("host", "port", "db.name")
    // which returns a truly unmodifiable list.
    public static final List<String> REQUIRED_KEYS = new ArrayList<>();
    static {
        REQUIRED_KEYS.add("host");
        REQUIRED_KEYS.add("port");
        REQUIRED_KEYS.add("db.name");
    }

    // The internal mutable backing store.
    // Note: HashMap is fine here — the bug is in how we expose it (see BUG 2).
    private final Map<String, String> properties = new HashMap<>();

    /**
     * Loads all entries from the supplied map into the internal store.
     * Validates that all required keys are present.
     *
     * @param source the configuration source map (must not be null)
     * @throws IllegalArgumentException if any required key is missing
     */
    public void load(Map<String, String> source) {
        properties.putAll(source);
        for (String key : REQUIRED_KEYS) {
            if (!properties.containsKey(key)) {
                throw new IllegalArgumentException("Missing required config key: " + key);
            }
        }
    }

    /**
     * Returns the trimmed value associated with the given key.
     *
     * @param key the configuration key
     * @return the trimmed value, or null if the key is not present
     */
    public String get(String key) {
        // BUG 1 — NP_NULL_ON_SOME_PATH
        // Map.get() returns null when the key is absent. Calling .trim() on null
        // throws NullPointerException.
        //
        // Fix option A: null check before calling .trim()
        // String value = properties.get(key);
        // return (value != null) ? value.trim() : null;
        //
        // Fix option B: use getOrDefault() to return null explicitly
        // return properties.getOrDefault(key, null); // still needs trim check
        //
        // Fix option C (best): getOrDefault with a null-safe trim
        // String value = properties.getOrDefault(key, null);
        // return (value != null) ? value.trim() : null;
        return properties.get(key).trim();
    }

    /**
     * Returns the internal properties map.
     *
     * @return the properties map
     */
    public Map<String, String> getProperties() {
        // BUG 2 — EI_EXPOSE_REP
        // Returning the internal Map directly allows the caller to mutate it:
        // config.getProperties().clear(); // deletes all config!
        // config.getProperties().put("host", "attacker.example.com"); // overwrite!
        //
        // Fix: return Collections.unmodifiableMap(properties)
        // or return a defensive copy: new HashMap<>(properties)
        return properties;
    }

    /**
     * Stores an integer value for the given key, converting it to a string.
     *
     * @param key   the configuration key
     * @param value the integer value to store
     */
    public void putInt(String key, int value) {
        // BUG 4 — DM_NUMBER_CTOR
        // 'new Integer(int)' uses the deprecated Integer constructor.
        // It was deprecated in Java 9 and removed in Java 17.
        // Integer.valueOf(int) is the idiomatic replacement. It also reuses
        // cached Integer instances for values in [-128, 127], which is
        // faster and uses less memory for common config values like port numbers.
        //
        // Fix: Integer.valueOf(value).toString()
        // Or even simpler: String.valueOf(value)
        properties.put(key, new Integer(value).toString());
    }
}