Lab 2.3: Fix It — A Good First Issue

Background

This is a fix-it lab: a realistic, end-to-end walk of a beginner-appropriate OpenSearch fix, from the user-visible symptom to a merged-quality PR with a unit test. The class of bug is the sweet spot for a first real contribution — an unclear validation message: a request is correctly rejected, but the error text leaves the user guessing what they did wrong. The fix is surgical (a better message), the blast radius is tiny, and a clean unit test is easy to write with assertThrows/assertEquals.

The specific symptom, code site, and class names below are illustrative of the pattern — the exact file and message on your branch will differ, and you will grep to find the real one. What is not illustrative is the method: how you locate the code, scope the change, write the test, and keep the PR focused. That transfers to every good first issue you will ever take.

Why This Lab Matters for Contributors

• Validation-message fixes are the highest-value beginner contribution: they improve real user experience, require touching only one method, and force you to write a focused test.
• You practice the full discipline: reproduce → locate → minimal diff → unit test → CHANGELOG → signed commit → PR — the loop you set up in Lab 2.2.
• The pitfalls here (scope creep, missing CHANGELOG, missing DCO, untested message) are the exact reasons first PRs stall.

Prerequisites

• Lab 2.2 complete; fork + branch + DCO + CHANGELOG mechanics are second nature.
• A running node from Lab 1.3 to reproduce the symptom.

Step 1: The Symptom

A user reports (in a good first issue) that a clearly-invalid request returns an unhelpful error. Two common, real flavors of this:

Flavor A — a validation method with a vague message. Many actions implement ActionRequest.validate(), accumulating problems into an ActionRequestValidationException via addValidationError(...). When the message is terse, the user cannot tell which field or what constraint failed. Example shape of a weak message:

{ "error": { "type": "action_request_validation_exception",
             "reason": "Validation Failed: 1: index is missing;" } }

Flavor B — a REST parser rejecting a parameter with no guidance. A RestHandler reads a query parameter and throws IllegalArgumentException with a message that names neither the bad value nor the allowed values.

Reproduce a concrete one against your running node. For instance, the _shrink/_split resize API requires a target index name; omitting it (or other resize preconditions) trips a validate():

# Symptom reproduction (illustrative — the exact endpoint/message varies by branch):
curl -s -X POST "localhost:9200/source/_shrink/" \
  -H 'Content-Type: application/json' -d '{}' | jq '.error | {type, reason}'

Read the reason. If it does not tell the user what is wrong and how to fix it, that is your bug.

Note: Pick a message that is genuinely unclear, not merely terse-but-correct. "index is missing" might be fine; "request is invalid" is not. The reviewer's bar is: does the new message help a confused user more than the old one, without changing behavior?

Step 2: Locate the Code

Find the message string, then the method that emits it. Start from the literal text:

# Search for the offending message text (use a distinctive fragment):
grep -rn "request is invalid\|Validation Failed" server/src/main/java | head

# More reliably, find the validate() method and its addValidationError calls
# for the request type in question (e.g. ResizeRequest):
grep -rn "addValidationError" server/src/main/java/org/opensearch/action/admin/indices/shrink/ResizeRequest.java
find server -name "ResizeRequest.java" -path "*/main/*"

Open the method. A weak validate() looks like this (illustrative):

@Override
public ActionRequestValidationException validate() {
    ActionRequestValidationException validationException = null;
    if (targetIndexRequest == null) {
        validationException = addValidationError("no target index request", validationException);
    }
    if (targetIndexRequest != null && targetIndexRequest.index() == null) {
        validationException = addValidationError("the target index name is not set", validationException);
    }
    // ...
    return validationException;
}

The first message — "no target index request" — is the kind of internal-jargon string a user cannot act on. That is your target.

Tip: Confirm there is an existing test for this validate() before you write a new one. A test class usually sits at the mirror path under src/test: find server -name "ResizeRequestTests.java". If it exists, you will add a method; if not, you may create one (with the SPDX header — see Lab 2.1).

Step 3: The Diff

Improve the message so it states the field and the fix. Keep it to the message string(s) — do not change when the error fires, only what it says.

diff --git a/server/src/main/java/org/opensearch/action/admin/indices/shrink/ResizeRequest.java b/server/src/main/java/org/opensearch/action/admin/indices/shrink/ResizeRequest.java
index 1234567..89abcde 100644
--- a/server/src/main/java/org/opensearch/action/admin/indices/shrink/ResizeRequest.java
+++ b/server/src/main/java/org/opensearch/action/admin/indices/shrink/ResizeRequest.java
@@ public ActionRequestValidationException validate() {
         ActionRequestValidationException validationException = null;
         if (targetIndexRequest == null) {
-            validationException = addValidationError("no target index request", validationException);
+            validationException = addValidationError(
+                "target index request is missing; specify the target index name in the request path, "
+                    + "e.g. POST /<source>/_shrink/<target>",
+                validationException
+            );
         }
         if (targetIndexRequest != null && targetIndexRequest.index() == null) {
-            validationException = addValidationError("the target index name is not set", validationException);
+            validationException = addValidationError(
+                "the target index name is not set; it must be provided in the request path "
+                    + "(POST /<source>/_shrink/<target>)",
+                validationException
+            );
         }
         return validationException;

Two rules for the message:

1. Name the field and the remedy. A good validation message answers "what is wrong" and "what do I type to fix it."
2. Do not change behavior. Same condition, same exception type, same fire-or-not — only clearer words. If you find yourself adding or removing a check, you have left "good first issue" territory; file a follow-up issue instead (see Pitfalls).

Step 4: The Unit Test

A message change is testable and must be tested — assert the new text so a future refactor cannot silently regress it. Extend (or create) the request's test, using assertThrows + assertEquals on the validation output. OpenSearchTestCase gives you the base class and expectThrows.

public class ResizeRequestTests extends OpenSearchTestCase {

    public void testValidationMessageWhenTargetIndexMissing() {
        ResizeRequest request = new ResizeRequest();   // no target set -> should fail validate()

        ActionRequestValidationException e = request.validate();

        assertNotNull("expected a validation error when the target index is missing", e);
        assertThat(
            e.getMessage(),
            containsString("target index request is missing; specify the target index name in the request path")
        );
    }
}

If you prefer the throw-style assertion (for an action whose constructor or REST parser throws rather than returning a validation exception), the pattern is:

public void testRejectsUnknownValueWithHelpfulMessage() {
    IllegalArgumentException e = expectThrows(
        IllegalArgumentException.class,
        () -> SomeParser.parseMode("nonsense")
    );
    assertEquals(
        "unknown mode [nonsense]; allowed values are [a, b, c]",
        e.getMessage()
    );
}

Note the conventions: test methods start with test, no @Test annotation is required (the runner discovers test* methods), and you use the framework's expectThrows/assertThat with Hamcrest matchers (containsString). New test files need the SPDX header.

Run only your test (fast loop from Lab 1.2):

./gradlew :server:test --tests "org.opensearch.action.admin.indices.shrink.ResizeRequestTests.testValidationMessageWhenTargetIndexMissing"

Expected:

org.opensearch.action.admin.indices.shrink.ResizeRequestTests > testValidationMessageWhenTargetIndexMissing PASSED

BUILD SUCCESSFUL

Step 5: CHANGELOG, Format, Sign, Push

The same pipeline as Lab 2.2:

# 1. CHANGELOG entry under [Unreleased] -> Fixed:
#    - Clarify the resize (_shrink/_split) validation message when the target index is missing ([#NNNNN](...))

# 2. Format and gate:
./gradlew spotlessApply
./gradlew :server:test --tests "*ResizeRequestTests*"
./gradlew precommit

# 3. Signed commit (DCO):
git add server/src/main/java/.../ResizeRequest.java \
        server/src/test/java/.../ResizeRequestTests.java \
        CHANGELOG.md
git commit -s -m "Clarify resize validation messages when the target index is missing"

# 4. Push and open the PR (fill the template; link the issue):
git push origin fix/resize-validation-message

Confirm the sign-off:

git log -1 --format='%B' | tail -2
# Clarify resize validation messages when the target index is missing
#
# Signed-off-by: Your Name <your.email@example.com>

In the PR body, link the issue (Resolves #12345) and state plainly: "This changes only the validation message text; no behavior changes. Added a unit test asserting the new message." That one sentence preempts the reviewer's first question.

Where This Goes Wrong (Pitfalls)

Implementation Requirements

Deliverables:

• A reproduced symptom: the original, unclear error captured from a real curl.
• A minimal diff that changes only the message text (no behavior change).
• A unit test (OpenSearchTestCase subclass) asserting the new message via assertThrows/expectThrows + assertEquals/containsString, run green.
• A CHANGELOG.md entry under ## [Unreleased] → Fixed.
• A clean ./gradlew spotlessApply and ./gradlew precommit.
• A signed commit and an opened PR whose body states "message-only, no behavior change" and links the issue.

Troubleshooting

Your grep for the message text finds nothing

The message may be assembled from parts (string concatenation, String.format, a constant). Search for a distinctive word, or search by the method (grep -rn "addValidationError" server/src/main/java) and read the candidates.

The test passes but the real curl still shows the old message

You changed a different code path than the one the request hits. Re-reproduce, then trace from the REST handler: the resize REST handler → the action → the request's validate(). Make sure the message you edited is the one on the path your curl exercises.

assertEquals on the message is brittle across runs

If randomized inputs make the exact message vary, assert a stable fragment with containsString instead of the full string. Reserve exact-match assertEquals for messages whose precise text is a deliberate contract.

precommit flags your new test file

Almost always a missing SPDX header or an unused import. Copy the header from a sibling test; run ./gradlew spotlessApply to drop unused imports.

Expected Output

After: the same invalid request returns a message a user can act on:

curl -s -X POST "localhost:9200/source/_shrink/" -H 'Content-Type: application/json' -d '{}' \
  | jq -r '.error.reason'
# Validation Failed: 1: target index request is missing; specify the target index name
#   in the request path, e.g. POST /<source>/_shrink/<target>;

And the test asserting it:

org.opensearch.action.admin.indices.shrink.ResizeRequestTests > testValidationMessageWhenTargetIndexMissing PASSED
BUILD SUCCESSFUL

Stretch Goals

1. Find three more weak messages. grep -rn "addValidationError" server/src/main/java and skim for messages that name no field or remedy. Each is a candidate good first issue — file one (do not fix all of them in one PR).

2. Trace the message to the wire. Confirm where the ActionRequestValidationException is turned into the JSON error the user sees — follow it from validate() up through the REST response path (rest-layer deep dive).

3. Add a parameterized assertion. If the validate() accumulates multiple errors, write a test that triggers two of them and asserts both fragments appear in the combined message.

4. Compare to upstream wording. Look at how a few existing high-quality validation messages in the same package are phrased (grep -rn "addValidationError" server/src/main/java/org/opensearch/action/admin/indices) and match that house style.

Validation / Self-check

You are done when you can answer these without notes:

1. What distinguishes a "good first issue" message fix from a behavior change — and what do you do the moment you realize you are changing behavior?
2. Why must a message-only change still have a test, and which assertion (assertEquals vs containsString) is appropriate when?
3. How do you locate the exact code site that emits a message a user reported?
4. Which three CI checks would block this PR if you skipped the corresponding step (test, CHANGELOG, sign-off)?
5. What single sentence in the PR body preempts the reviewer's first question on a message change?
6. Name two pitfalls that most often stall a first PR and how each is avoided.

Next: Lab 2.4 — Review It: Spot the Flaws in a PR, where you sit on the other side of the table.