test(es): Validate spec-conformant NDJSON in the request snapshot har… - #9111
Merged
jkowall merged 7 commits intoAug 1, 2026
Merged
Conversation
…ness
The harness canonicalizes _bulk/_msearch bodies into a JSON array under an
"ndjson" key, which proves the shape but not that the bytes are NDJSON the
backend accepts. Reviewers only ever see the canonical form, so three classes
of regression were invisible in a diff:
- the trailing newline was stripped by TrimRight before analysis, so a body
missing the final newline the _bulk API requires snapshotted identically to
a correct one;
- parseNDJSON skipped blank lines, so "{a}\n\n{b}" snapshotted the same as
"{a}\n{b}";
- CapturedRequest recorded no headers, so the request's Content-Type was never
checked at all.
Capture the Content-Type, and validate NDJSON endpoints against a pure
validateNDJSON helper: an accepted media type, a non-empty body, and exactly
one trailing newline. parseNDJSON now rejects blank lines instead of skipping
them. Keeping the rules in a function that returns an error makes them
unit-testable directly, without a stub testing.TB.
The media type is checked against both application/x-ndjson and
application/json: esapi.BulkRequest in the official go-elasticsearch client
sets application/json when the caller leaves the header unset, so the async
bulk indexer emits it, and requiring x-ndjson alone would reject a request the
official client itself produces.
No snapshot fixtures change: the new rules are assertions, not additions to
the canonical output.
Signed-off-by: hharshhsaini <sainiharsh3311@gmail.com>
jkowall
added
the
changelog:test
jkowall
requested changes
Jul 26, 2026
jkowall
left a comment
jkowall
left a comment
Contributor
There was a problem hiding this comment.
One edge case remains in the NDJSON validity guarantee; details inline.
A body of only "\n" passed validateNDJSON: it is non-empty, ends with exactly one newline, and is not "\n\n". toSnapshot then trims it to empty and returns before parseNDJSON runs, so the blank line was never rejected and validation returned nil for a body carrying no document — which the backend would reject. Reject a body with no non-whitespace content (len(TrimSpace(body)) == 0) and add newline-only and whitespace-only cases to TestValidateNDJSON. Signed-off-by: hharshhsaini <sainiharsh3311@gmail.com>
jkowall
requested changes
Jul 28, 2026
jkowall
left a comment
jkowall
left a comment
Contributor
There was a problem hiding this comment.
The newline-only body fix is correct, but the Content-Type validation still has one spec-conformance gap; details inline.
validateNDJSON split the Content-Type on the first ";" with strings.Cut, which
accepted a header with a malformed parameter ("application/x-ndjson; garbage")
and rejected a valid mixed-case one ("Application/X-NDJSON") because the compare
was case-sensitive.
Use mime.ParseMediaType: it validates the parameters (rejecting the malformed
case) and normalizes the media type to lower case (accepting the mixed-case
one). Propagate its parse error, and add malformed-parameter and mixed-case
cases to TestValidateNDJSON.
Signed-off-by: hharshhsaini <sainiharsh3311@gmail.com>
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## main #9111 +/- ##
==========================================
- Coverage 97.84% 97.84% -0.01%
==========================================
Files 377 377
Lines 19812 19833 +21
==========================================
+ Hits 19386 19405 +19
- Misses 289 291 +2
Partials 137 137
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
Contributor
Author
|
@jkowall Thanks for the re-review. As you noted, the only remaining red (elasticsearch 9.x direct) is unrelated to this test-only change, and the codecov / Coverage Gate reds are downstream of codecov not reporting. Whenever you have a moment, could you re-run those jobs or merge through, happy to rebase again if it helps. |
CI Summary ReportMetrics ComparisonView changed metricsFor label-level diff details, open the CI run and expand the "Compare metrics and generate summary" step logs. metrics_snapshot_kafka_v2 — ⬇️ download diff
Code Coverage✅ Coverage 98.4% (baseline 98.4%) ➡️ View CI run | View publish logs |
Merged
via the queue into
jaegertracing:main
with commit Aug 1, 2026
835d71b
200 of 204 checks passed
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Which problem is this PR solving?
Description of the changes
The harness renders
_bulk/_msearchbodies as a canonicalized JSON array under an"ndjson"key, which proves the shape but not that the bytes are NDJSON the backend will accept. Since reviewers only ever see the canonical form, threeregressions were invisible in a diff:
bytes.TrimRight(r.Body, "\n")ran before analysis, so a body missing the final newline the_bulkAPI requires snapshotted identically to a correct one;parseNDJSONskipped blank lines, so{a}\n\n{b}snapshotted the same as{a}\n{b};CapturedRequestrecorded no headers, soContent-Typewas never checked.Changes:
CapturedRequestnow recordsContentType, captured inServeHTTP.validateNDJSON(contentType, body) errorchecks the media type, a non-empty body, and exactly one trailing newline.toSnapshotapplies it to_bulk/_msearchpaths. Returning an error (rather than asserting inside the helper)keeps the rules unit-testable directly, with no stub
testing.TB.parseNDJSONrejects blank lines instead of skipping them.One deviation worth review. The issue asks to validate
Content-Typeisapplication/x-ndjson. Implemented strictly, that failsTestWriterRequestSnapshots:esapi.BulkRequestin the official go-elasticsearch client setsapplication/jsonwhen the caller leaves the header unset, so our async bulk indexer emits it. I therefore accept both
application/x-ndjsonandapplication/json. If you'd rather the async path be normalized tox-ndjsonand the check tightened, happyto do that instead, it's a client change rather than a harness one, so I kept it out of this PR.
No snapshot fixtures change: the new rules are assertions, not additions to the canonical output.
How was this change tested?
TestValidateNDJSONtable-covers accepted media types (incl. acharsetparameter), missing/wrong types, empty body, missing trailing newline, and an extra trailing newline.TestParseNDJSONRejectsBlankLinescovers leading, interior, and whitespace-only blank lines, plus the valid two-document case.TestIsNDJSONEndpointcovers_bulk/_msearchvs_search/_doc.TestRecorderCapturesNDJSONnow asserts theContent-Typeis captured.make fmt,make lint,make testall pass.Checklist
AI Usage in this PR (choose one)
See AI Usage Policy.