epugh commented on code in PR #1151:
URL: https://github.com/apache/solr/pull/1151#discussion_r3682802466
##########
changelog/unreleased/solr16155.yml:
##########
@@ -0,0 +1,12 @@
+title: >
+ Document field values are no longer included in indexing error messages,
since they may leak
+ sensitive data into logs and API responses. To aid debugging, enable TRACE
level logging for
+ org.apache.solr.update.DocumentBuilder or
org.apache.solr.handler.loader.JavabinLoader
Review Comment:
Seems like this deserves some docs in the Ref Guide? So that folks actually
know how to use it!
##########
solr/core/src/java/org/apache/solr/update/DocumentBuilder.java:
##########
@@ -163,21 +168,20 @@ public static Document toDocument(
&& field.getValueCount() > 1
&& !(sfield.getType() instanceof DenseVectorField)) {
- // Ensure we do not flood the logs with extremely long values
- String fieldValue = field.getValue().toString();
- if (fieldValue.length() > MAX_VALUES_AS_STRING_LENGTH) {
- assert fieldValue.endsWith("]");
- fieldValue = fieldValue.substring(0, MAX_VALUES_AS_STRING_LENGTH -
4) + "...]";
+ if (log.isTraceEnabled()) {
+ log.trace(
+ "{}multiple values encountered for non multiValued field {}: {}",
Review Comment:
is this `{}` quite right? I think we are getting an id, so the opening
sentence on line 173 seems like it needs to tweak?
##########
solr/core/src/java/org/apache/solr/update/DocumentBuilder.java:
##########
@@ -363,6 +369,27 @@ private static boolean addCopyFields(
return used;
}
+ private static void traceFieldError(
+ SolrInputDocument doc, IndexSchema schema, SolrInputField field,
Exception ex) {
+ if (log.isTraceEnabled()) {
+ log.trace(
+ "{}Error adding field '{}'='{}'",
+ getID(doc, schema),
+ field.getName(),
+ truncatedValue(field.getValue()),
+ ex);
+ }
+ }
+
+ /** Renders a field value as a string, truncated to avoid flooding the logs
*/
+ private static String truncatedValue(Object value) {
Review Comment:
likewise surprised this isn't in many places a need?
##########
solr/core/src/test/org/apache/solr/schema/DenseVectorFieldTest.java:
##########
@@ -718,7 +718,7 @@ public void
denseVectorByteEncoding_shouldRaiseExceptionWithValuesOutsideBoundar
assertThat(
thrown.getCause().getMessage(),
is(
- "Error while creating field
'vector_byte_encoding{type=knn_vector_byte_encoding,properties=indexed,stored,omitNorms,omitTermFreqAndPositions,useDocValuesAsStored}'
from value '[128, 6, 7, 8]'"));
+ "Error while creating field
'vector_byte_encoding{type=knn_vector_byte_encoding,properties=indexed,stored,omitNorms,omitTermFreqAndPositions,useDocValuesAsStored}'"));
Review Comment:
I will put a more general comment on the thrust of this whole feature, but
while this is good, it also makes things harder to read too when you are going
through the code...
##########
solr/core/src/java/org/apache/solr/update/DocumentBuilder.java:
##########
@@ -363,6 +369,27 @@ private static boolean addCopyFields(
return used;
}
+ private static void traceFieldError(
Review Comment:
I wonder if this method needs to be promoted to being a widely used
method....? Centralize this tracing capability?
--
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]
