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.

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.