Lab 8.2: Implement the Fix, Write the Test, Open the PR

Background

You have a deterministic reproducer from Lab 8.1 that fails on main. This lab takes you from that red test to a merge-quality Pull Request: a minimal diff, the SPDX header, spotlessApply formatting, the test that fails-then-passes, a CHANGELOG.md entry, DCO-signed commits, a clean ./gradlew precommit, and a PR description a reviewer can act on.

The discipline here is what separates a contribution that merges in days from one that rots for months. The fix itself is often the smallest part of the work.

Why This Lab Matters for Contributors

• A perfect fix with a sloppy PR gets bounced; a small fix with a clean PR merges fast.
• The fails-then-passes test is the proof maintainers trust. Without it, your fix is an assertion.
• precommit and DCO are hard gates — failing them wastes CI minutes and reviewer goodwill.

Prerequisites

• A reproducer that fails on main (Lab 8.1). You will reuse it verbatim as the regression test.
• A fork of opensearch-project/OpenSearch and a feature branch:

  git checkout -b fix/terms-size-validation
  

• Git configured for DCO sign-off:

  git config user.name "Your Name"
  git config user.email "salbat2022@gmail.com"
  

• Read PR quality and responding to feedback.

Step-by-Step Tasks

Step 1 — Root-cause to the exact line (15 min)

Your repro told you what; now find where. From Lab 8.1 the trigger is "terms with size <= 0". Locate the setter/parser that should reject it:

grep -rn "public TermsAggregationBuilder size" \
  server/src/main/java/org/opensearch/search/aggregations/bucket/terms/TermsAggregationBuilder.java
grep -rn "shardSize\|requiredSize\|bucketCountThresholds" \
  server/src/main/java/org/opensearch/search/aggregations/bucket/terms/TermsAggregationBuilder.java | head

Read the setter. On main it likely stores the value without validating it (that absence is the bug). Confirm by reading the method body — do not guess line numbers, read the code the grep points to.

Step 2 — Write the minimal fix (15 min)

The fix is a single guard in the setter. Minimal diff — change only what the bug requires, no reformatting of surrounding code:

--- a/server/src/main/java/org/opensearch/search/aggregations/bucket/terms/TermsAggregationBuilder.java
+++ b/server/src/main/java/org/opensearch/search/aggregations/bucket/terms/TermsAggregationBuilder.java
@@ public TermsAggregationBuilder size(int size) {
-    public TermsAggregationBuilder size(int size) {
-        bucketCountThresholds.setRequiredSize(size);
-        return this;
-    }
+    public TermsAggregationBuilder size(int size) {
+        if (size <= 0) {
+            throw new IllegalArgumentException("[size] must be greater than 0. Found [" + size + "] in [" + name + "]");
+        }
+        bucketCountThresholds.setRequiredSize(size);
+        return this;
+    }

Three properties of a good message (you will reuse these in Lab 8.3): it names the parameter ([size]), states the constraint (must be greater than 0), and echoes the offending value and location (Found [-1] in [g]).

Note: If the real validation belongs at parse time (XContent) rather than the setter, put it where the value first becomes known so the error surfaces at the REST boundary with a 400, not deep in execution. For this bug the setter is the single choke point both REST and the Java client pass through, so it is the right place.

Step 3 — Verify the test now passes (fails-then-passes) (10 min)

This is the proof. Your Lab 8.1 test was red on main. With the fix applied it must go green. Then revert the fix and confirm it goes red again. That round trip is the whole point.

# With the fix applied:
./gradlew :server:test --tests "*.TermsAggregationBuilderReproTests"
# Expect: BUILD SUCCESSFUL

# Prove the test actually guards the bug — stash the fix and re-run:
git stash
./gradlew :server:test --tests "*.TermsAggregationBuilderReproTests"   # expect FAILED
git stash pop

A test that is green both with and without your fix proves nothing. If that happens, the test is asserting something the fix did not change — rewrite it against the bug.

Step 4 — Add the CHANGELOG entry (5 min)

Every PR adds one line to CHANGELOG.md under the ## [Unreleased ...] section, in the right category (Added/Changed/Fixed/Deprecated/Removed). precommit enforces this.

--- a/CHANGELOG.md
+++ b/CHANGELOG.md
@@ ## [Unreleased 3.x]
 ### Fixed
+- Reject non-positive `size` in the `terms` aggregation with a clear error ([#NNNNN](https://github.com/opensearch-project/OpenSearch/pull/NNNNN))

grep -n "## \[Unreleased" CHANGELOG.md | head    # find the section to edit

Step 5 — Format and run the local gate (15 min)

Never let CI find a formatting nit you could have caught locally.

./gradlew spotlessApply                  # auto-format (Spotless)
./gradlew spotlessJavaCheck              # verify formatting is clean
./gradlew :server:precommit              # checkstyle, forbidden APIs, license headers, CHANGELOG check
./gradlew :server:test --tests "*.TermsAggregationBuilderReproTests"

precommit is the gate that most first PRs trip on. Common failures: missing SPDX header on a new file, a line over the checkstyle limit, a forbidden API (System.out, Math.random), or a missing CHANGELOG entry. Fix them all before pushing.

Step 6 — Commit with DCO sign-off (5 min)

OpenSearch requires a DCO sign-off, not a CLA. Every commit needs a Signed-off-by: line, which -s adds:

git add server/src/main/java/org/opensearch/search/aggregations/bucket/terms/TermsAggregationBuilder.java \
        server/src/test/java/org/opensearch/search/aggregations/bucket/terms/TermsAggregationBuilderReproTests.java \
        CHANGELOG.md
git commit -s -m "Reject non-positive size in terms aggregation

The terms aggregation accepted size <= 0 silently, surfacing as a
confusing failure later. Validate in the size() setter and throw a
clear IllegalArgumentException. Adds a unit test.

Closes #NNNNN"

Confirm the sign-off landed:

git log -1 | grep "Signed-off-by"
# Signed-off-by: Your Name <salbat2022@gmail.com>

Warning: If you forget -s, the DCO bot blocks the PR. Fix with git commit --amend -s (single commit) or git rebase --signoff main (multiple).

Step 7 — Push and open the PR (15 min)

git push -u origin fix/terms-size-validation
gh pr create --repo opensearch-project/OpenSearch \
  --title "Reject non-positive size in terms aggregation" \
  --body-file /tmp/pr-body.md

Fill in the repo's PR template (.github/pull_request_template.md). A complete description:

### Description
The `terms` aggregation accepted a non-positive `size` (e.g. `size: 0` or `-1`)
without validation, leading to a confusing downstream failure instead of a clean
400. This adds validation in `TermsAggregationBuilder.size(int)` that throws an
`IllegalArgumentException` with the parameter, constraint, and offending value.

### Related Issues
Closes #NNNNN

### Reproduction
Fails on `main` (commit <hash>):
`./gradlew :server:test --tests "*.TermsAggregationBuilderReproTests"`

### Testing
- Added `TermsAggregationBuilderReproTests` (red before the fix, green after).
- `./gradlew :server:precommit` passes; `spotlessApply` applied.

### Check List
- [x] New functionality includes testing.
- [x] Commits are signed per the DCO using `--signoff`.
- [x] Public documentation issue/PR created (n/a — internal validation).
- [x] CHANGELOG entry added.

Step 8 — Read CI and respond to review (10 min)

GitHub Actions runs precommit, unit tests, and broader gates. When it goes red:

• Open the failing job, read the first error (later errors are often cascades).
• Distinguish your failure from a flaky unrelated test (a known flaky-test). If unrelated, say so and (politely) ask a maintainer to re-run, or reference the tracking issue.

Respond to review by pushing follow-up commits (the PR updates in place); squash only if a maintainer asks. Address every comment, even just to say "done" or to explain a disagreement civilly. See responding to feedback.

Implementation Requirements

• A minimal diff: only the lines the fix needs, no drive-by reformatting.
• SPDX header on any new file; precommit green.
• The Lab 8.1 reproducer is the regression test and is fails-then-passes verified.
• A CHANGELOG.md entry under [Unreleased] in the correct category.
• DCO sign-off on every commit (git commit -s).
• A PR (or PR-ready branch) with the template fully filled in.

Expected Output

# With the fix:
TermsAggregationBuilderReproTests > testNegativeSizeIsRejected PASSED
TermsAggregationBuilderReproTests > testZeroSizeIsRejected PASSED
BUILD SUCCESSFUL

# Without the fix (git stash):
TermsAggregationBuilderReproTests > testNegativeSizeIsRejected FAILED

# precommit:
> Task :server:precommit
BUILD SUCCESSFUL

A PR with green CI, a filled template, a one-line CHANGELOG, and a signed-off commit.

Troubleshooting

Stretch Goals

1. Add a parallel shard_size <= 0 validation guard and a test, in the same PR (it is the same logical change). Decide whether shard_size < size should also be rejected or clamped — read how bucketCountThresholds.ensureValidity() already handles it.
2. Add a REST-YAML test asserting the 400 + error type, so the contract is covered end-to-end, not just the Java setter.
3. Trace the backport path: if the fix should land on 2.x, what label triggers the backport bot? (backport 2.x). Read compatibility.

Coding Exercises

The fix is the smallest part; the shape of the change is the lab. Each exercise produces a piece of a real, mergeable PR — a fails-then-passes test, a minimal diff, a CHANGELOG line. Locate every line you touch with rg (the iron rule), and verify the round trip every time.

1. (warm-up) The fails-then-passes proof. Apply the single-guard fix from Step 2 to TermsAggregationBuilder.size(int), run your Lab 8.1 test green, then git stash the fix and prove it goes red again. Capture both outputs. A test green with and without the fix proves nothing — if that happens, your test asserts the wrong thing.

   ./gradlew :server:test --tests "*.TermsAggregationBuilderReproTests"   # green with fix
   git stash && ./gradlew :server:test --tests "*.TermsAggregationBuilderReproTests"; git stash pop  # red without
   

2. (core) A parallel guard in the same PR. Add the matching shard_size <= 0 validation (it is the same logical change), and a test for it. Decide whether shard_size < size should be rejected or clamped — read how the existing logic already handles it, don't guess:

   grep -rn "ensureValidity\|shardSize\|setShardSize" \
     server/src/main/java/org/opensearch/search/aggregations/bucket/terms/TermsAggregationBuilder.java \
     server/src/main/java/org/opensearch/search/aggregations/bucket/terms/TermsAggregator.java | head
   

3. (core) A REST-YAML contract test. Encode the fix at the boundary an operator hits: a yamlRestTest asserting the bad request now returns a clean 400 with the right error type, not a confusing downstream failure. Copy an existing structure under rest-api-spec/test. This proves the guard surfaces at the REST edge, not just in the Java client.

4. (core) A real PR-shaped change. Produce the full artifact on a feature branch: minimal diff, SPDX header on any new file, spotlessApply, a CHANGELOG.md line in the correct category under [Unreleased], and DCO-signed commits (git commit -s). Run the local gate and make it green:

   ./gradlew spotlessApply :server:precommit
   git log -1 | grep "Signed-off-by"
   

   The deliverable is a branch a maintainer could merge — diff, test, CHANGELOG, sign-off, green precommit.

5. (advanced) Advanced challenge — fix a real open issue end to end. Take the failing test you wrote in Lab 8.1 against a live bug issue, root-cause it to the exact line, write the minimal fix, prove fails-then-passes, add the CHANGELOG entry and a test, and open (or fully prepare) the PR with the template filled in and the issue linked via Closes #NNNNN. Then trace the backport path: if it should land on 2.x, which label triggers the bot? (backport 2.x — read compatibility.) This is the complete contribution arc; everything else in this level rehearses pieces of it.

Issues to Practice On

Pick a small, concrete bug in opensearch-project/OpenSearch server core that you can reproduce, fix, and test in one sitting (labels move; confirm with gh label list --repo opensearch-project/OpenSearch).

Representative patterns. (1) A "missing validation → confusing downstream failure" bug (the worked example) — the fix is a single guard at the choke point both REST and the Java client pass through; ship it with a fails-then-passes test. (2) A "wrong behavior on a boundary value" bug (off-by-one, 0/negative inputs) — reproduce the boundary, fix the comparison, and add tests for the boundary and the value either side of it. Loop: reproduce → locate via rg → fix → test → PR with CHANGELOG + DCO sign-off.

Planted-bug drill. Apply your fix, confirm green, then deliberately weaken it — change size <= 0 to size < 0 so size: 0 slips through again. Run ./gradlew :server:test --tests "*TermsAggregationBuilderReproTests" and confirm testZeroSizeIsRejected goes red while testNegativeSizeIsRejected stays green. That split shows you exactly which assertion guards which boundary — restore the correct condition. This is how you learn that one test per boundary is not redundancy, it is coverage.

Etiquette: Claim the issue before working it, reproduce on main first, keep the diff minimal, and every PR needs a test + a CHANGELOG.md entry + a DCO Signed-off-by (git commit -s). See community interaction and the level-2 PR lab.

Validation / Self-check

1. Show the test red without the fix and green with it. Why does that round trip matter?
2. Is your diff minimal? Point to any line that is not strictly required.
3. Where is the validation placed, and why is that the right choke point?
4. Does every commit have a Signed-off-by: line?
5. Which precommit checks did you run, and what did each catch (if anything)?
6. Is the CHANGELOG entry in the correct category and section?
7. Could a reviewer reproduce, understand, and verify your fix from the PR description alone?

Cross-references: Lab 8.1: Reproduce an Issue, Lab 8.3: Error Messages, PR quality, responding to feedback, Capstone.