FrankChen021 commented on code in PR #20151:
URL: https://github.com/apache/druid/pull/20151#discussion_r3979207275
##########
server/src/main/java/org/apache/druid/client/DirectDruidClient.java:
##########
@@ -238,11 +249,139 @@ private InputStream dequeue() throws InterruptedException
return holder.getStream();
}
+ /**
+ * Scans past leading whitespace in {@code buffer} looking for the
first content byte, without consuming
+ * (advancing the reader index of) the buffer, and returns it. Once a
non-whitespace byte is found, the prefix
+ * is considered resolved (see {@link #bodyPrefixResolved}) and later
calls return null. If {@code buffer} is
+ * empty or entirely whitespace, the prefix remains unresolved (and
null is returned) so a later call, from a
+ * subsequent chunk, can retry the check; this matters for chunked
responses, where the initial
+ * {@link HttpResponse} can carry an empty body and the real content,
HTML or otherwise, only arrives via
+ * {@link #handleChunk}.
+ */
+ @Nullable
+ private Byte bodyPrefixByte(ChannelBuffer buffer)
+ {
+ if (bodyPrefixResolved.get()) {
+ return null;
+ }
+ final int readerIndex = buffer.readerIndex();
+ final int readable = buffer.readableBytes();
+ for (int i = 0; i < readable; i++) {
+ byte b = buffer.getByte(readerIndex + i);
+ if (b == ' ' || b == '\n' || b == '\r' || b == '\t') {
+ continue;
+ }
+ bodyPrefixResolved.set(true);
+ return b;
+ }
+ return null;
+ }
+
+ /**
+ * Classifies the body prefix in {@code buffer} (see {@link
#bodyPrefixByte}) and fails the query if it is not
+ * JSON. HTML always fails; any other non-JSON body fails only when
the status is 429/503, since Druid itself
+ * never sends a non-JSON body with those statuses but proxies
routinely do (an HTML error page from nginx or
+ * a load balancer, a plain-text "upstream connect error" from Envoy).
A JSON body, whatever the status, is
+ * left alone so that the normal parse path can surface the server's
own structured error.
+ *
+ * @param contentType Content-Type header of the initial response,
possibly null; a text/html value fails the
+ * query regardless of the body prefix
+ * @param chunkNum 0 for the initial response body, else the chunk
number
+ */
+ private void failIfNonJsonBody(String contentType, ChannelBuffer
buffer, long chunkNum)
+ {
+ final boolean isHtmlContentType =
+ contentType != null &&
StringUtils.toLowerCase(contentType).contains("text/html");
+ final Byte prefix = bodyPrefixByte(buffer);
+ final boolean isHtml = isHtmlContentType || (prefix != null &&
prefix == '<');
+ final boolean isNonJson = prefix != null && prefix != '{' && prefix
!= '[';
+ final int statusCode = responseStatusCode;
+ if (isHtml || (isNonJson && (statusCode == 429 || statusCode ==
503))) {
+ throwForNonJsonBody(statusCode, contentType, buffer, chunkNum,
isHtml);
+ }
+ }
+
+ /**
+ * Returns up to 256 characters of {@code buffer} (read from at most
its first 512 bytes) for inclusion in an
+ * error message, without consuming the buffer.
+ */
+ private String bodyPreview(ChannelBuffer buffer)
+ {
+ final int len = Math.min(buffer.readableBytes(), 512);
+ if (len == 0) {
+ return "";
+ }
+ final byte[] previewBytes = new byte[len];
+ buffer.getBytes(buffer.readerIndex(), previewBytes);
+ final String preview = StringUtils.fromUtf8(previewBytes);
+ return preview.substring(0, Math.min(preview.length(), 256));
+ }
+
+ /**
+ * Fails the query because the response body is not JSON, typically an
error page produced by a load balancer
+ * or reverse proxy sitting in front of the data server. A 429/503
status is reported as
+ * {@link QueryCapacityExceededException} since that is what such
intermediaries return when the server is
+ * over capacity; any other status is reported as a {@link
QueryInterruptedException}. Either way, the caller
+ * gets a message that says what actually came back instead of a
{@code JsonParseException} on {@code '<'}.
+ *
+ * @param statusCode HTTP status of the initial response
+ * @param contentType Content-Type header of the initial response,
possibly null
+ * @param buffer the buffer in which the non-JSON body was
detected (the initial response body or a chunk)
+ * @param chunkNum 0 if detected in the initial response body, else
the chunk number
+ * @param isHtml whether the body was identified as HTML
specifically (vs. some other non-JSON content)
+ */
+ private void throwForNonJsonBody(
+ int statusCode,
+ String contentType,
+ ChannelBuffer buffer,
+ long chunkNum,
+ boolean isHtml
+ )
+ {
+ final String preview = bodyPreview(buffer);
Review Comment:
## Follow-up assessment
The printable-character filtering fixes the CR/LF/log-forging part of my
earlier comment. I still consider the arbitrary-content aspect actionable: the
preview is copied into an exception message, logged by `JsonParserIterator`,
and can be put into the query error/trailer; Druid's logging guidance says
exception messages must not leak data, so I retained this as a P2 inline
finding. I also found a separate publication-order race in `failCause`/`fail`,
posted as a P1 inline finding. Reviewed all 5 changed files at `1730b3c3`.
<!-- mergelens:review -->
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
---------------------------------------------------------------------
To unsubscribe, e-mail: [email protected]
For additional commands, e-mail: [email protected]
