Copilot commented on code in PR #4124:
URL: https://github.com/apache/solr/pull/4124#discussion_r2782986687
##########
solr/core/src/java/org/apache/solr/handler/admin/ZookeeperInfoHandler.java:
##########
@@ -424,7 +416,22 @@ public void handleRequestBody(SolrQueryRequest req,
SolrQueryResponse rsp) throw
} finally {
printer.close();
}
- rsp.getValues().add(RawResponseWriter.CONTENT, printer);
+
+ // Parse the JSON we built and return as structured data
+ // This allows any response writer (json, xml, etc.) to serialize it
properly
+ Object parsedJson = Utils.fromJSONString(printer.getJsonString());
+
+ // If it's a Map, add its contents directly to the response
+ if (parsedJson instanceof Map) {
+ @SuppressWarnings("unchecked")
+ Map<String, Object> jsonMap = (Map<String, Object>) parsedJson;
+ for (Map.Entry<String, Object> entry : jsonMap.entrySet()) {
+ rsp.add(entry.getKey(), entry.getValue());
+ }
+ } else {
+ // Fallback: add as single value
+ rsp.add("zk_data", parsedJson);
+ }
Review Comment:
The handler now generates JSON into a buffer, parses it back with
`Utils.fromJSONString(...)`, and then re-serializes it via the response writer.
This adds avoidable CPU/memory overhead and GC pressure, especially for large
ZK trees. Consider building the response structure directly (Maps/Lists on
`rsp`) instead of round-tripping through a JSON string.
```suggestion
// Add the generated JSON directly to the response without re-parsing it
// This avoids an unnecessary JSON parse/serialize round-trip.
rsp.add("zk_data", printer.getJsonString());
```
##########
solr/core/src/java/org/apache/solr/handler/admin/ZookeeperInfoHandler.java:
##########
@@ -762,50 +749,19 @@ boolean printZnode(JSONWriter json, String path) throws
IOException {
}
json.endObject();
- } catch (KeeperException e) {
- writeError(500, e.toString());
- return false;
- } catch (InterruptedException e) {
+ } catch (KeeperException | InterruptedException e) {
writeError(500, e.toString());
return false;
}
return true;
}
- /* @Override
- public void write(OutputStream os) throws IOException {
- ByteBuffer bytes = baos.getByteBuffer();
- os.write(bytes.array(),0,bytes.limit());
- }
- */
- @Override
- public String getName() {
- return null;
- }
-
- @Override
- public String getSourceInfo() {
- return null;
- }
-
- @Override
- public String getContentType() {
- return JSONResponseWriter.CONTENT_TYPE_JSON_UTF8;
- }
-
- @Override
- public Long getSize() {
- return null;
- }
-
- @Override
- public InputStream getStream() throws IOException {
- return new ByteBufferInputStream(baos.getByteBuffer());
- }
-
- @Override
- public Reader getReader() throws IOException {
- return null;
+ /**
+ * Returns the JSON content as a string. This will be parsed back into
objects for proper
+ * serialization by response writers.
+ */
+ public String getJsonString() {
+ return baos.toString();
Review Comment:
`getJsonString()` returns `baos.toString()`, but `baos` is a
`ByteArrayOutputStream` and `toString()` decodes using the platform default
charset. Since the JSON bytes are written as UTF-8, this can corrupt non-ASCII
data and even break `Utils.fromJSONString(...)` on non-UTF-8 default platforms.
Decode explicitly as UTF-8 (e.g., use `toString(StandardCharsets.UTF_8)` or
equivalent).
```suggestion
return baos.toString(StandardCharsets.UTF_8);
```
##########
solr/core/src/java/org/apache/solr/handler/admin/ZookeeperInfoHandler.java:
##########
@@ -360,7 +353,8 @@ public void onReconnect() {
@SuppressWarnings({"unchecked"})
public void handleRequestBody(SolrQueryRequest req, SolrQueryResponse rsp)
throws Exception {
final SolrParams params = req.getParams();
- Map<String, String> map = Map.of(WT, "raw", OMIT_HEADER, "true");
+ // Force JSON response and omit header for cleaner output
+ Map<String, String> map = Map.of(WT, "json", OMIT_HEADER, "true");
Review Comment:
The comment "allows any response writer (json, xml, etc.)" conflicts with
the earlier `wrapDefaults(new MapSolrParams(map), params)` call, which forces
`wt=json` because the forced params are the primary params. Either update the
comment to reflect that JSON is enforced, or stop forcing `wt` if other writers
are intended to work.
```suggestion
// Force omitting the response header for cleaner output; allow client
to choose response writer
Map<String, String> map = Map.of(OMIT_HEADER, "true");
```
--
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]
