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.

Coding Exercises

This lab already produces a fix and a test; these exercises drill the pattern — locate a weak message, change only the text, prove it with a focused test — across several real sites so it becomes reflex. Use rg/find to locate every site; never cite a line number you did not just read.

1. (warm-up) Inventory the validation-message surface. Write a script that rg -n "addValidationError\(" server/src/main/java and prints, for each hit, the file and the literal message string. Verify: the output is a real list of candidate weak messages (skim for ones that name no field or remedy). This is Stretch Goal 1, made into a reproducible inventory you can re-run on any branch.

2. (warm-up) Write the test before the fix (red-green). Pick the ResizeRequest.validate() site (find server -name "ResizeRequest.java" -path "*/main/*") or another validate() you found. In its test class (find server -name "ResizeRequestTests.java", or create one with the SPDX header), add a method that builds an invalid request, calls validate(), and asserts the current message via containsString. Verify: it passes against the unchanged message — you now have a guard that will go red the instant you change the text, which is exactly what you want before editing.

3. (core) Make the message-only fix and update the test. Improve the message to name the field and the remedy (per Step 3's two rules), then update your Exercise-2 assertion to the new fragment. Verify: the scoped run is green (./gradlew :server:test --tests "*ResizeRequestTests.testValidationMessage*"), and a git diff shows you changed only message strings and the test — no condition, no exception type, no control flow. Prove the behavior is unchanged by also asserting the exception is still non-null and the same type.

4. (core) A second, different fix in another class. Find a RestHandler that throws IllegalArgumentException for a bad parameter with an unhelpful message (rg -n "throw new IllegalArgumentException" server/src/main/java/org/opensearch/rest | head). Improve the message to name the bad value and the allowed values, and write an expectThrows test asserting the new text. Verify: green scoped run. Doing the pattern in two shapes (a validate() returning an exception vs. a handler that throws) is what makes it transferable.

5. (core) A parameterized multi-error assertion. If a validate() you touched accumulates multiple errors (Stretch Goal 3), write a test that triggers two of them at once and asserts both fragments appear in the combined message via two containsString checks. Verify: the test fails if either fragment is missing — proving the accumulation order/format is what you expect.

6. (advanced) Advanced challenge — generalize the pattern into a reusable test helper, then apply it. Notice the repetition: build invalid request → validate()/throw → assert fragment. Write a small private helper in your test class — e.g. assertValidationMessageContains(ActionRequest req, String fragment) — that runs the validation and asserts the fragment, and refactor your Exercises 2–3 to use it. Then apply your end-to-end loop (reproduce with curl from Lab 1.3, locate, message-only diff, helper-based test, CHANGELOG, signed commit) to one more weak message from your Exercise-1 inventory, in a different package, and open it as a real (draft) PR. Verify: both test classes are green under :server:test, precommit is clean, and each PR body states "message-only, no behavior change" and links an issue. You have now turned a single fix into a repeatable contribution engine — the goal of this whole level.

Issues to Practice On

This lab's bread and butter is the good first issue label on opensearch-project/OpenSearch — and specifically the error-message and validation-clarity issues within it.

# The canonical source for this lab:
gh issue list --repo opensearch-project/OpenSearch --label "good first issue" --state open

# Often overlaps with these (labels move; confirm on the tracker):
gh issue list --repo opensearch-project/OpenSearch --label "enhancement" --label "good first issue" --state open
gh issue list --repo opensearch-project/OpenSearch --label "bug" --label "good first issue" --state open

# Search issue text for message/validation clarity work:
gh issue list --repo opensearch-project/OpenSearch --search "error message OR validation message in:title,body" --state open
gh label list --repo opensearch-project/OpenSearch | grep -iE "good first|enhancement|bug"

Representative issue patterns for this subsystem:

• An unclear error or validation message. Exactly this lab. A user pastes a confusing reason. Reproduce with curl, locate the literal with rg, improve it (field + remedy), add an assertThrows/containsString test, ship with CHANGELOG + DCO. Keep the blast radius to the message string.
• A small, well-specified enhancement. A good first issue enhancement (a new default, a clearer log line, an additional _cat column) where the scope is fully pinned in the issue. Resist scope creep — implement exactly what the issue asks, test it, and file follow-ups for anything else you notice (per the Pitfalls table).

Planted-bug drill. Plant a behavior change disguised as a message fix and let your test catch the overreach:

1. In a scratch branch, edit a validate() so you also relax the condition (e.g. change if (x == null) to if (false)) while "improving" the message. Run the request's tests (./gradlew :server:test --tests "*ResizeRequestTests*") and watch a behavior test go red — proof you left "good first issue" territory. Revert the condition; keep only the message change; confirm green. Add an assertion that the validation still fires (exception non-null) so a future accidental relaxation is caught.
2. Or plant a brittle test: assertEquals the entire long message, then make a one-word wording tweak and watch the test break needlessly. Switch to containsString on a stable fragment — the lesson from the Pitfalls table, now felt.

Etiquette: claim the issue (comment to be assigned) before working it, reproduce first, and ship the message-only diff with a test + CHANGELOG + DCO Signed-off-by (git commit -s). The next lab, Lab 2.4, puts you on the reviewing side of exactly these PRs; norms live in community interaction.

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.