Lab 01 — FlawedPatch: Review MessageBatcher

Phase: 06 — Review Culture and Patch Workflow
Difficulty: ⭐⭐⭐☆☆ | Estimated Time: 2.5–3 hours
Primary Artifact: MessageBatcher.java with 6 planted review issues
Verify: mvn test passes all 4 tests after your fixes

Scenario

A contributor submitted this PR to the dev@ list:

[PATCH] PROJ-4471 — MessageBatcher: return flushed messages for downstream processing

Previously flush() returned void and callers had no way to process the flushed batch without maintaining their own state. This patch changes flush() to return the flushed list. Also adds autoFlush support so batchers can flush automatically on close.

Tests are passing locally. Please review.

The CI is green. Checkstyle is green. SpotBugs is green.

Your job: find the 6 issues before this merges.

Setup

cd phase-06-review-culture/lab-01-flawed-patch
mvn test

Expected: 2 tests PASS, 2 tests FAIL. Read the failure messages — they tell you which of the 6 issues are caught by tests and which are not.

Step 1 — Read the code and write your review

Before running anything: read MessageBatcher.java and write a structured review comment for each issue you find. Use the format:

[SEVERITY] Method/field: Problem statement.

Why it matters: ...
Evidence: (quote the line)
Fix: ...

There are 6 issues. Find all 6 before looking at the answer table below.

Step 2 — Fix the correctness issues

Fix the issues in order of severity. Issues #1, #2, #3, #4 require code changes. Issue #5 is a cleanup. Issue #6 requires a new test.

For issue #1 (thread safety): Use synchronized on the this monitor for both add() and flush(). This is the simplest correct fix; a production system might use ReentrantLock or a ConcurrentLinkedQueue for better throughput.

For issue #2 (API regression): The fix is context-dependent. In a library, you would NOT change the return type in a minor release. Document your finding as: "This is a binary-incompatible change. If required, it must ship in a major version with a deprecation notice for flush() returning void, or be introduced as a new method flushAndGet()."

For this lab: change the signature back to public synchronized void flush() and add a separate public synchronized List<String> drainBatch() method.

For issue #3 (null guard): throw IllegalArgumentException("message must not be null").

For issue #4 (wrong exception type): throw IllegalStateException("batcher is closed").

For issue #5 (dead code): simplify size > maxSize || size == maxSize to size >= maxSize.

For issue #6 (missing test): add testAutoFlush_closeTriggersDrain() to MessageBatcherTest.java.

Step 3 — Verify all 4 tests pass

mvn test && echo "ALL PASS"

Step 4 — Write the review summary

Write a final paragraph (3–5 sentences) as if you were posting to dev@:

Thanks for the patch. I've identified 2 BLOCKERs, 1 MAJOR, and 3 lower-priority issues
before we can merge this:

1. [BLOCKER] Thread safety: ...
2. [BLOCKER] API regression: ...
...

Marking -0 pending resolution of the BLOCKERs. Happy to re-review once those are addressed.

A -0 means "I have concerns but am not vetoing" — common for patches that are close but need work.

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.review</groupId>
    <artifactId>flawed-patch-lab</artifactId>
    <version>1.0.0-SNAPSHOT</version>
    <packaging>jar</packaging>
    <name>Review Culture Lab 01 — FlawedPatch</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>

    <dependencies>
        <dependency>
            <groupId>org.junit.jupiter</groupId>
            <artifactId>junit-jupiter</artifactId>
            <version>5.10.2</version>
            <scope>test</scope>
        </dependency>
    </dependencies>

    <build>
        <plugins>
            <plugin>
                <groupId>org.apache.maven.plugins</groupId>
                <artifactId>maven-surefire-plugin</artifactId>
                <version>3.2.5</version>
            </plugin>
        </plugins>
    </build>

</project>

MessageBatcher.java

package org.example.review;

import java.util.ArrayList;
import java.util.Collections;
import java.util.List;

/**
 * Collects messages into a batch and flushes them when the batch reaches
 * {@code maxSize}, or when {@link #flush()} is called explicitly.
 *
 * <p>
 * This class has 6 review issues. Find them all before fixing.
 *
 * <p>
 * PR description: "Changed flush() to return the flushed messages for
 * downstream processing. Added autoFlush support so batchers flush on close."
 */
public class MessageBatcher {

    // ISSUE 1 — Thread safety
    // ArrayList is not thread-safe. If two threads call add() simultaneously:
    // - Thread A may see a partial write from Thread B during ArrayList.grow()
    // - Result: ArrayIndexOutOfBoundsException OR silently lost messages
    // The class is documented as usable from multiple threads (see the test),
    // but the implementation has no synchronization.
    //
    // Fix: synchronize add() and flush() on 'this', or use CopyOnWriteArrayList,
    // or restructure to use ConcurrentLinkedQueue + atomic drain.
    private List<String> batch = new ArrayList<>();

    private final int maxSize;
    private final boolean autoFlush;
    private boolean closed = false;

    /**
     * Creates a new batcher.
     *
     * @param maxSize   the maximum number of messages before an automatic flush
     * @param autoFlush if true, the batcher flushes remaining messages when closed
     */
    public MessageBatcher(int maxSize, boolean autoFlush) {
        this.maxSize = maxSize;
        this.autoFlush = autoFlush;
    }

    /**
     * Adds a message to the batch. If the batch reaches {@code maxSize},
     * an automatic flush is triggered.
     *
     * @param message the message to add
     * @throws RuntimeException if the batcher has been closed
     */
    public void add(String message) {
        if (closed) {
            // ISSUE 4 — Wrong exception type
            // The closed-state contract violation should use IllegalStateException,
            // not the raw RuntimeException. IllegalStateException is the standard
            // Java idiom for "object is in a state that does not allow this operation"
            // (see: Iterator.remove() after end, Queue.remove() on empty queue).
            // Callers catching IllegalStateException specifically will miss this.
            //
            // Fix: throw new IllegalStateException("batcher is closed");
            throw new RuntimeException("batcher is closed");
        }

        // ISSUE 3 — No null guard
        // If message is null, it is silently added to the batch.
        // Downstream code iterating the flushed list and calling message.length()
        // will get a NullPointerException with no indication of where the null
        // was introduced.
        //
        // Fix: if (message == null) throw new IllegalArgumentException("message must
        // not be null");
        batch.add(message);

        // ISSUE 5 — Dead code
        // The condition 'size > maxSize || size == maxSize' simplifies to 'size >=
        // maxSize'.
        // The '== maxSize' branch can never execute when '> maxSize' is already true,
        // and both branches do the same thing (flush). This is a NIT — no correctness
        // impact — but it signals that the author was not thinking clearly about the
        // condition.
        //
        // Fix: if (batch.size() >= maxSize) { flush(); }
        if (batch.size() > maxSize || batch.size() == maxSize) {
            flush();
        }
    }

    /**
     * Flushes the current batch and returns the flushed messages.
     *
     * <p>
     * <strong>WARNING — ISSUE 2 (API regression):</strong>
     * The previous version of this method had signature
     * {@code public void flush()}.
     * Changing the return type to {@code List<String>} is a
     * <em>binary-incompatible</em>
     * change: any caller compiled against the old void signature will get:
     * 
     * <pre>
     *   java.lang.NoSuchMethodError: flush()V
     * </pre>
     * 
     * at runtime, because the JVM resolves method descriptors including the return
     * type.
     * <p>
     * This is source-compatible (existing code that discards the return value still
     * compiles),
     * but binary-incompatible. In Apache library policy, this requires a major
     * version bump
     * or introduction as a new method name.
     *
     * @return an unmodifiable snapshot of the flushed messages (never null, may be
     *         empty)
     */
    public List<String> flush() {
        List<String> flushed = Collections.unmodifiableList(new ArrayList<>(batch));
        batch.clear();
        return flushed;
    }

    /**
     * Closes the batcher. If {@code autoFlush} was set at construction time,
     * any remaining messages are flushed before closing.
     *
     * <p>
     * <strong>ISSUE 6 — Missing test:</strong>
     * The {@code autoFlush} feature added in this PR has no test. There is no test
     * that verifies messages added before {@code close()} are flushed when
     * {@code autoFlush=true}, nor that they are NOT flushed when
     * {@code autoFlush=false}.
     * The feature could be completely broken and CI would not catch it.
     */
    public void close() {
        if (autoFlush && !batch.isEmpty()) {
            flush();
        }
        closed = true;
    }
}

MessageBatcherTest.java

package org.example.review;

import org.junit.jupiter.api.BeforeEach;
import org.junit.jupiter.api.Test;

import java.util.List;
import java.util.concurrent.CountDownLatch;
import java.util.concurrent.ExecutorService;
import java.util.concurrent.Executors;
import java.util.concurrent.TimeUnit;
import java.util.concurrent.atomic.AtomicInteger;

import static org.junit.jupiter.api.Assertions.*;

/**
 * Tests for {@link MessageBatcher}.
 *
 * Before fixes: 2 tests PASS, 2 tests FAIL.
 * After all fixes: all 4 tests PASS.
 */
class MessageBatcherTest {

    private MessageBatcher batcher;

    @BeforeEach
    void setUp() {
        batcher = new MessageBatcher(3, false);
    }

    /**
     * Test 1 — PASSES before fixes.
     * Basic add-and-flush contract: messages added before flush() are returned.
     */
    @Test
    void testAddAndFlush() {
        batcher.add("hello");
        batcher.add("world");
        List<String> flushed = batcher.flush();
        assertEquals(2, flushed.size());
        assertTrue(flushed.contains("hello"));
        assertTrue(flushed.contains("world"));
    }

    /**
     * Test 2 — PASSES before fixes.
     * When batch reaches maxSize, flush() is triggered automatically.
     * After auto-flush the batch is empty; a subsequent flush() returns an empty
     * list.
     */
    @Test
    void testMaxSizeTrigger_batchClearedAfterAutoFlush() {
        batcher.add("a");
        batcher.add("b");
        batcher.add("c"); // triggers auto-flush at size == maxSize
        List<String> afterAutoFlush = batcher.flush();
        assertEquals(0, afterAutoFlush.size(),
                "Batch should be empty after auto-flush triggered at maxSize");
    }

    /**
     * Test 3 — FAILS before fixes.
     *
     * Issue #4: close() on a closed batcher throws RuntimeException.
     * The correct exception is IllegalStateException.
     *
     * Before fix: RuntimeException is thrown → assertThrows(IllegalStateException)
     * FAILS.
     * After fix: IllegalStateException is thrown → test PASSES.
     */
    @Test
    void testAddAfterClose_throwsIllegalState() {
        batcher.close();
        assertThrows(IllegalStateException.class,
                () -> batcher.add("late message"),
                "Expected IllegalStateException when adding to a closed batcher");
    }

    /**
     * Test 4 — FAILS before fixes.
     *
     * Issue #3: add(null) silently succeeds instead of throwing
     * IllegalArgumentException.
     *
     * Before fix: no exception is thrown → assertThrows() FAILS.
     * After fix: IllegalArgumentException is thrown → test PASSES.
     */
    @Test
    void testNullMessage_throwsIllegalArgument() {
        assertThrows(IllegalArgumentException.class,
                () -> batcher.add(null),
                "Expected IllegalArgumentException when adding a null message");
    }

    // -------------------------------------------------------------------------
    // Bonus tests: add these as part of fixing Issue #6 (missing autoFlush test)
    // and Issue #1 (thread-safety) after you fix the implementation.
    // -------------------------------------------------------------------------

    /**
     * Bonus — add this test when fixing Issue #6 (autoFlush missing test).
     *
     * When autoFlush=true, close() must flush remaining messages.
     * When autoFlush=false, close() must NOT flush remaining messages.
     */
    // @Test
    // void testAutoFlush_closeFlushesWhenEnabled() { ... }

    /**
     * Bonus — add this test when fixing Issue #1 (thread safety).
     *
     * 10 threads each add 100 messages concurrently.
     * After all threads complete, flush() must return exactly 1000 messages
     * (no lost updates).
     *
     * Before fixing thread safety: this test fails or throws
     * ArrayIndexOutOfBoundsException due to unsynchronized ArrayList growth.
     */
    // @Test
    // void testConcurrentAdd_noLostUpdates() throws InterruptedException { ... }
}