Jackie-Jiang commented on code in PR #19723:
URL: https://github.com/apache/pinot/pull/19723#discussion_r4200941773
##########
pinot-common/src/main/java/org/apache/pinot/common/audit/AuditRequestProcessor.java:
##########
@@ -170,9 +182,17 @@ private AuditEvent.AuditRequestPayload
captureRequestPayload(ContainerRequestCon
}
if (config.isCaptureRequestPayload() && requestContext.hasEntity()) {
- String requestBody = readRequestBody(requestContext,
config.getMaxPayloadSize());
- if (StringUtils.isNotBlank(requestBody)) {
- payload.setBody(requestBody);
+ final MediaType mediaType = requestContext.getMediaType();
+ if (isTextualMediaType(mediaType)) {
+ String requestBody = readRequestBody(requestContext,
config.getMaxPayloadSize());
+ if (StringUtils.isNotBlank(requestBody)) {
+ payload.setBody(requestBody);
+ }
+ } else {
+ // Segment uploads arrive as multipart/form-data wrapping a gzipped
tarball. Copying
+ // those bytes into the audit record adds no auditable information
-- the record cannot
+ // be read back as text -- while making each record orders of
magnitude larger.
+ payload.setBody(String.format(NON_TEXT_BODY_MARKER, mediaType));
Review Comment:
[P2, non-blocking] Preserve textual multipart schema uploads
With payload capture enabled, this also suppresses schema JSON:
`AddSchemaCommand` uses `FileUploadDownloadClient.addSchema()`, which sends
`multipart/form-data`, and the controller accepts multipart schema updates too.
Previously these readable bodies were captured; now only the omission marker
remains, losing the schema contents from the audit trail.
Please preserve capture for textual schema uploads while excluding binary
segment uploads, and cover this through `processRequest()`.
##########
pinot-common/src/main/java/org/apache/pinot/common/audit/AuditRequestProcessor.java:
##########
@@ -257,4 +288,33 @@ String readRequestBody(ContainerRequestContext
requestContext, int maxPayloadSiz
}
return null;
}
+
+ /// Only text-shaped bodies are worth copying into an audit record. Anything
else (multipart
+ /// segment uploads, octet-stream) is recorded by type and size instead.
+ /// A missing media type is treated as textual so that behaviour is
unchanged for clients that
+ /// do not set Content-Type.
+ @VisibleForTesting
+ static boolean isTextualMediaType(@Nullable MediaType mediaType) {
+ if (mediaType == null) {
+ return true;
+ }
+ if ("text".equalsIgnoreCase(mediaType.getType())) {
+ return true;
+ }
+ if (!"application".equalsIgnoreCase(mediaType.getType())) {
+ return false;
+ }
+ final String subtype = mediaType.getSubtype().toLowerCase(Locale.ROOT);
+ return subtype.equals("json") || subtype.equals("xml") ||
subtype.equals("x-www-form-urlencoded")
+ || subtype.endsWith("+json") || subtype.endsWith("+xml");
+ }
+
+ @VisibleForTesting
+ static boolean isMostlyReplacementChars(String decoded) {
+ if (decoded.isEmpty()) {
+ return false;
+ }
+ long replacements = decoded.chars().filter(c -> c == 0xFFFD).count();
+ return replacements * 100 > (long) decoded.length() *
MAX_REPLACEMENT_PERCENT;
Review Comment:
[P2, non-blocking] Distinguish decoding errors from literal U+FFFD
Counting U+FFFD characters does not establish invalid UTF-8: the character
itself has a valid UTF-8 encoding. For example, the valid UTF-8 JSON
`{"a":"��"}` has two literal U+FFFD characters out of ten decoded characters,
giving a 20% ratio and causing the entire audit body to be discarded.
Please detect malformed byte sequences during decoding instead, retaining
the incomplete-tail allowance, and add a valid UTF-8 case containing literal
replacement characters.
--
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]
