dsmiley commented on code in PR #882:
URL: https://github.com/apache/solr/pull/882#discussion_r883947375
##########
solr/core/src/java/org/apache/solr/response/SolrQueryResponse.java:
##########
@@ -217,17 +219,15 @@ public String getToLogAsString() {
/** Returns a string of the form "prefix name1=value1 name2=value2 ..." */
public String getToLogAsString(String prefix) {
- StringBuilder sb = new StringBuilder(prefix);
- for (int i = 0; i < toLog.size(); i++) {
- if (sb.length() > 0) {
- sb.append(' ');
- }
- String name = toLog.getName(i);
- Object val = toLog.getVal(i);
- if (name != null) {
- sb.append(name).append('=');
- }
- sb.append(val);
+ StringBuilder sb = new StringBuilder();
+ if (prefix != null && !prefix.isEmpty()) {
+ // TODO: remove "prefix"
+ Map<String, Object> keyValPairs = new LinkedHashMap<>();
+ keyValPairs.put("prefix", prefix);
+ keyValPairs.putAll(toLog);
+ sb.append(JSONUtil.toJSON(keyValPairs, -1));
+ } else {
+ sb.append(JSONUtil.toJSON(toLog, -1));
Review Comment:
Beautiful. @gus-asf with the move to JSON here, generating JSON is now a
one-liner and not custom code. Note the "prefix" thing ought to go away
(separate issue).
##########
solr/core/src/java/org/apache/solr/response/SolrQueryResponse.java:
##########
@@ -217,17 +219,15 @@ public String getToLogAsString() {
/** Returns a string of the form "prefix name1=value1 name2=value2 ..." */
public String getToLogAsString(String prefix) {
- StringBuilder sb = new StringBuilder(prefix);
- for (int i = 0; i < toLog.size(); i++) {
- if (sb.length() > 0) {
- sb.append(' ');
- }
- String name = toLog.getName(i);
- Object val = toLog.getVal(i);
- if (name != null) {
- sb.append(name).append('=');
- }
- sb.append(val);
+ StringBuilder sb = new StringBuilder();
Review Comment:
Why do we have a StringBuilder here at all anymore?
##########
solr/core/src/java/org/apache/solr/rest/BaseSolrResource.java:
##########
@@ -147,7 +147,7 @@ protected void handleException(Logger log) {
getSolrResponse().add("error", info);
String message = (String) info.get("msg");
if (null != message && !message.trim().isEmpty()) {
- getSolrResponse().getToLog().add("msg", "{" + message.trim() + "}");
+ getSolrResponse().addToLog("msg", "{" + message.trim() + "}");
Review Comment:
Why curly brackets? This is a symptom of code that tried to make the log
parseable but it should not be the job of this class here (the caller of
addToLog) to concern itself with that.
##########
solr/core/src/java/org/apache/solr/core/SolrCore.java:
##########
@@ -2922,7 +2919,7 @@ public String[] getParams(String param) { // assume param
is in lpSet
} // assume in lpSet
};
- toLog.add("params", "{" + filteredParams + "}");
Review Comment:
Removal of needless curly brackets is good!
##########
solr/core/src/java/org/apache/solr/response/SolrQueryResponse.java:
##########
@@ -217,17 +219,15 @@ public String getToLogAsString() {
/** Returns a string of the form "prefix name1=value1 name2=value2 ..." */
Review Comment:
This Javadoc is now wrong.
##########
solr/core/src/java/org/apache/solr/servlet/HttpSolrCall.java:
##########
@@ -893,7 +893,7 @@ private void handleAdminRequest() throws IOException {
handler != null
? MarkerFactory.getMarker(handler.getClass().getName())
: MarkerFactory.getMarker(HttpSolrCall.class.getName()),
- solrResp.getToLogAsString("[admin]"));
+ solrResp.getToLogAsString("admin"));
Review Comment:
Instead, could `addToLog("admin", true)` be called at some early point to
ensure it shows up (first)?
##########
solr/core/src/test/org/apache/solr/handler/component/ResponseLogComponentTest.java:
##########
@@ -54,7 +54,7 @@ public void testToLogIds() throws Exception {
"responseLog",
"true");
SolrQueryResponse qr = h.queryAndResponse(handler, req);
- NamedList<Object> entries = qr.getToLog();
+ Map<String, Object> entries = qr.getToLog();
Review Comment:
Could declare as "var" and future-proof :-). This is a test after-all.
##########
solr/core/src/test/org/apache/solr/response/TestSolrQueryResponse.java:
##########
@@ -90,35 +90,30 @@ public void testResponse() throws Exception {
public void testToLog() throws Exception {
final SolrQueryResponse response = new SolrQueryResponse();
assertEquals("toLog initially not empty", 0, response.getToLog().size());
- assertEquals("logid_only", response.getToLogAsString("logid_only"));
+ assertEquals("{\"prefix\":\"prefix_only\"}",
response.getToLogAsString("prefix_only"));
// initially empty, then add something
response.addToLog("key1", "value1");
- {
- final Iterator<Map.Entry<String, Object>> it =
response.getToLog().iterator();
- assertTrue(it.hasNext());
- final Map.Entry<String, Object> entry1 = it.next();
- assertEquals("key1", entry1.getKey());
- assertEquals("value1", entry1.getValue());
- assertFalse(it.hasNext());
- }
- assertEquals("key1=value1", response.getToLogAsString(""));
- assertEquals("abc123 key1=value1", response.getToLogAsString("abc123"));
+
+ Map<String, Object> toLog = response.getToLog();
Review Comment:
I recommend doing assertEquals with a Map you create yourself like
{{Map.of("key1", "value1")}}. Same with further below -- way more concise and
easy to read/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: issues-unsubscr...@solr.apache.org
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscr...@solr.apache.org
For additional commands, e-mail: issues-h...@solr.apache.org
