Repository navigation
Conversation
Use JDK UTF-8 conversion to replace malformed surrogates in parser error descriptions and isolated low surrogates in string output. Match the legacy writer's question-mark replacement while preserving valid Unicode and parsing behavior. Cover identifier, ABI escape, numeric errors, and low-surrogate output with regression tests, including strict UTF-8 checks through native Jetty responses.
|
LGTM. |
| return builder.toString(); | ||
| } | ||
|
|
||
| private static String replaceMalformedSurrogates(String value) { |
There was a problem hiding this comment.
[NIT] PR description does not disclose the JsonFormat.java changes
Cross-cutting concern — documentation gap, not a defect in this line; anchored here because this hunk is representative of the undisclosed change.
The head commit modifies four spots in JsonFormat.java (isolated low-surrogate replacement in escapeText, the InvalidEscapeSequence message in unescapeText, and two parseException descriptions via the new replaceMalformedSurrogates helper), accompanied by three regression test classes. The PR description's "What does this PR do?" and "Why are these changes required?" sections only cover the wrapper removal and never mention JsonFormat, so reviewers cannot anticipate this file in the changed scope, and changelog/release notes derived from the description would omit it. The user-observable net diff vs base is zero (the commit preserves the legacy '?' replacement behavior), so this is about disclosure completeness only.
Suggestion: Add one bullet to "What does this PR do?" noting that JsonFormat preserves the legacy Unicode replacement behavior, along with the three accompanying regression test classes.
| if (c >= 0x0000 && c <= 0x001F) { | ||
| appendEscapedUnicode(builder, c); | ||
| } else if (Character.isLowSurrogate(c)) { | ||
| builder.append(replaceMalformedSurrogates(String.valueOf(c))); |
There was a problem hiding this comment.
[NIT] Declare the surrogate byte-compatibility boundary in the PR description
Cross-cutting concern — documentation gap, not a defect in this line; anchored here because this hunk defines the compatibility boundary.
Removing CharResponseWrapper changes the actual response encoder for ALL responses: from the wrapper's OutputStreamWriter(UTF-8), which replaces an unpaired surrogate with '?', to Jetty's Utf8HttpWriter, which emits an isolated low surrogate as 3-byte CESU-8 (invalid UTF-8). This PR preserves the legacy bytes for the JsonFormat write/parse paths via replaceMalformedSurrogates, but pass-through paths outside JsonFormat (typically contract-layer exception messages carried by Util.processError) are not covered: if such a message contains an unpaired surrogate, on-the-wire bytes change from '?' to CESU-8. The trigger surface is tiny (protobuf string fields are UTF-8-validated; the main source of unpaired surrogates is error messages echoing raw input, and the JSON parse path is already covered here), so no code change is requested — but a user-observable byte-level change should be stated explicitly.
Suggestion: Add one boundary sentence to the PR description stating that exception-message pass-through paths outside JsonFormat are not surrogate-compatible and the trigger surface is assessed as negligible (or record it as known technical debt in the module docs).
What does this PR do?
Replaces response-body copying for HTTP traffic metrics with Jetty's native
Response.getContentCount(). RemovesCharResponseWrapper,ServletOutputStreamCopy, and the unused response wrapping inHttpApiAccessFilter.Why are these changes required?
The old wrappers retained response bodies in memory solely to measure their size. Full-node wallet requests could pass through two copies, and bulk writes fell back to a per-byte loop. Jetty already maintains a byte count, including content still in its output buffer, so these copies are unnecessary.
HttpApiAccessFilterbuilt a wrapper it never read, on four HTTP services — three of which register no interceptor at all.Using Jetty's native response writer also fixes incomplete responses caused by bytes remaining in the old wrapper's unflushed
OutputStreamWriterbuffer. Unflushedprint()calls could lose an entire small response or the trailing bytes of a larger response. This is latent rather than live: every servlet underservices/httpends withprintln, and the one non-printlnwriter call runs upstream of any wrapper.Before/after checks found matching response bodies and byte counts for the tested synchronous production write patterns, including
println, JSON-RPC output-stream writes, and UTF-8 responses.Metrics are still recorded after the filter chain returns. They measure response-body bytes written to Jetty, excluding HTTP headers and chunk framing; they do not guarantee delivery to the client. Existing exclusions for disabled-API and lite-node responses remain. Requests without a Jetty base request record zero bytes, and the new lookup introduces no cast. Production filter registration and request parsing are unchanged.
This PR has been tested by:
println, unflushedprint, output-stream writes withContent-Length,UTF-8, a body past the output buffer, and HTTP 400, using the access-filter-before-interceptor
ordering of full-node wallet endpoints. An outer completion latch keeps the metric assertions
deterministic. Unit tests cover the non-Jetty fallback and propagation to both traffic meters.
printand large-body tests fail; mutation testing confirms theremaining assertions are not vacuous.
Follow up
Heads-up for whoever lands #6923: it removes the Codahale
MetricsUtil.meterMarkcalls from thissame method, so the two will conflict textually. The byte count itself is still needed afterwards
for the Prometheus
tron:http_byteshistogram.