Copilot commented on code in PR #4124:
URL: https://github.com/apache/solr/pull/4124#discussion_r2784445800
##########
solr/core/src/java/org/apache/solr/handler/admin/ZookeeperInfoHandler.java:
##########
@@ -360,71 +353,160 @@ 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");
req.setParams(SolrParams.wrapDefaults(new MapSolrParams(map), params));
+
+ // Ensure paging support is initialized
+ ensurePagingSupportInitialized();
+
+ // Validate parameters
+ validateParameters(params);
+
+ // Determine request type and handle accordingly
+ boolean isGraphView = "graph".equals(params.get("view"));
+ ZkBasePrinter printer =
+ isGraphView ? handleGraphViewRequest(params) :
handlePathViewRequest(params);
+
+ try {
+ printer.print();
+ } finally {
+ printer.close();
+ }
+
+ addJsonToResponse(printer.getJsonString(), rsp);
+ }
+
+ /** Ensures the paging support is initialized (thread-safe lazy
initialization). */
+ private void ensurePagingSupportInitialized() {
synchronized (this) {
if (pagingSupport == null) {
pagingSupport = new PagedCollectionSupport();
ZkController zkController = cores.getZkController();
if (zkController != null) {
- // get notified when the ZK session expires (so we can clear the
cached collections and
- // rebuild)
+ // Get notified when the ZK session expires (so we can clear cached
collections)
zkController.addOnReconnectListener(pagingSupport);
}
}
}
+ }
- String path = params.get(PATH);
-
+ /**
+ * Validates incoming parameters.
+ *
+ * @param params Request parameters to validate
+ * @throws SolrException if validation fails
+ */
+ private void validateParameters(SolrParams params) {
if (params.get("addr") != null) {
throw new SolrException(ErrorCode.BAD_REQUEST, "Illegal parameter
\"addr\"");
}
+ }
- String detailS = params.get(PARAM_DETAIL);
- boolean detail = detailS != null && detailS.equals("true");
+ /**
+ * Handles the graph view request with paginated collections.
+ *
+ * @param params Request parameters including pagination settings
+ * @return JSON string representing paginated collection data
+ */
+ private ZkBasePrinter handleGraphViewRequest(SolrParams params) {
+ // Extract pagination parameters
+ int start = params.getInt("start", 0);
+ int rows = params.getInt("rows", -1);
- String dumpS = params.get("dump");
- boolean dump = dumpS != null && dumpS.equals("true");
+ // Extract filter parameters
+ FilterType filterType = extractFilterType(params);
+ String filter = extractFilter(params, filterType);
+
+ // Extract display options (applicable to graph view)
+ boolean detail = params.getBool(PARAM_DETAIL, false);
+ boolean dump = params.getBool("dump", false);
+
+ // Create printer for paginated collections
+ return new ZkGraphPrinter(
+ cores.getZkController(),
+ new PageOfCollections(start, rows, filterType, filter),
+ pagingSupport,
+ detail,
+ dump);
+ }
- int start = params.getInt("start", 0); // Note start ignored if rows not
specified
- int rows = params.getInt("rows", -1);
+ /**
+ * Handles the path view request for a specific ZooKeeper path.
+ *
+ * @param params Request parameters including the path to display
+ * @return JSON string representing the ZooKeeper path data
Review Comment:
Javadoc mismatch: handlePathViewRequest returns a ZkBasePrinter, but the
@return description says it returns a JSON string. Please update the Javadoc to
reflect the actual return type/behavior.
```suggestion
* @return a ZkBasePrinter that writes the ZooKeeper path data
```
##########
solr/core/src/java/org/apache/solr/handler/admin/ZookeeperInfoHandler.java:
##########
@@ -360,71 +353,160 @@ 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");
req.setParams(SolrParams.wrapDefaults(new MapSolrParams(map), params));
+
+ // Ensure paging support is initialized
+ ensurePagingSupportInitialized();
+
+ // Validate parameters
+ validateParameters(params);
+
+ // Determine request type and handle accordingly
+ boolean isGraphView = "graph".equals(params.get("view"));
Review Comment:
handleRequestBody now treats any request with view=graph as a graph request
and ignores PATH entirely. Previously, graph-mode pagination was only enabled
for the legacy "/clusterstate.json" pseudo-path; other paths could still be
viewed even if view=graph was present. If this is intentional, consider
validating/rejecting incompatible params (e.g., view=graph with a non-empty
PATH) or documenting the precedence to avoid surprising API consumers.
```suggestion
boolean isGraphView = "graph".equals(params.get("view"));
String path = params.get(PATH);
if (isGraphView
&& StringUtils.isNotEmpty(path)
&& !"/clusterstate.json".equals(path)) {
throw new SolrException(
ErrorCode.BAD_REQUEST,
"Parameter combination not supported: view=graph cannot be used
with PATH='"
+ path
+ "'. Use an empty PATH or '/clusterstate.json' with
view=graph.");
}
```
##########
solr/core/src/java/org/apache/solr/handler/admin/ZookeeperInfoHandler.java:
##########
@@ -360,71 +353,160 @@ 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");
req.setParams(SolrParams.wrapDefaults(new MapSolrParams(map), params));
+
+ // Ensure paging support is initialized
+ ensurePagingSupportInitialized();
+
+ // Validate parameters
+ validateParameters(params);
+
+ // Determine request type and handle accordingly
+ boolean isGraphView = "graph".equals(params.get("view"));
+ ZkBasePrinter printer =
+ isGraphView ? handleGraphViewRequest(params) :
handlePathViewRequest(params);
+
+ try {
+ printer.print();
+ } finally {
+ printer.close();
+ }
+
+ addJsonToResponse(printer.getJsonString(), rsp);
+ }
+
+ /** Ensures the paging support is initialized (thread-safe lazy
initialization). */
+ private void ensurePagingSupportInitialized() {
synchronized (this) {
if (pagingSupport == null) {
pagingSupport = new PagedCollectionSupport();
ZkController zkController = cores.getZkController();
if (zkController != null) {
- // get notified when the ZK session expires (so we can clear the
cached collections and
- // rebuild)
+ // Get notified when the ZK session expires (so we can clear cached
collections)
zkController.addOnReconnectListener(pagingSupport);
}
}
}
+ }
- String path = params.get(PATH);
-
+ /**
+ * Validates incoming parameters.
+ *
+ * @param params Request parameters to validate
+ * @throws SolrException if validation fails
+ */
+ private void validateParameters(SolrParams params) {
if (params.get("addr") != null) {
throw new SolrException(ErrorCode.BAD_REQUEST, "Illegal parameter
\"addr\"");
}
+ }
- String detailS = params.get(PARAM_DETAIL);
- boolean detail = detailS != null && detailS.equals("true");
+ /**
+ * Handles the graph view request with paginated collections.
+ *
+ * @param params Request parameters including pagination settings
+ * @return JSON string representing paginated collection data
Review Comment:
Javadoc mismatch: handleGraphViewRequest returns a ZkBasePrinter, but the
@return description says it returns a JSON string. Please update the Javadoc to
reflect the actual return type/behavior.
```suggestion
* @return a {@link ZkBasePrinter} that will render the graph view for the
requested page of collections
```
##########
solr/core/src/java/org/apache/solr/handler/admin/ZookeeperInfoHandler.java:
##########
@@ -436,30 +518,30 @@ private String normalizePath(String path) {
//
//
--------------------------------------------------------------------------------------
- static class ZKPrinter implements ContentStream {
- static boolean FULLPATH_DEFAULT = false;
-
- boolean indent = true;
- boolean fullpath = FULLPATH_DEFAULT;
- boolean detail = false;
- boolean dump = false;
-
- String keeperAddr; // the address we're connected to
-
- final Utils.BAOS baos = new Utils.BAOS();
- final Writer out = new OutputStreamWriter(baos, StandardCharsets.UTF_8);
- SolrZkClient zkClient;
+ /**
+ * Base class for ZooKeeper JSON printers. Provides common functionality for
building JSON from
+ * ZooKeeper data.
+ */
+ abstract static class ZkBasePrinter {
+ protected boolean detail;
+ protected boolean dump;
- PageOfCollections page;
- PagedCollectionSupport pagingSupport;
- ZkController zkController;
+ protected final Utils.BAOS baos = new Utils.BAOS();
+ protected final Writer out = new OutputStreamWriter(baos,
StandardCharsets.UTF_8);
+ protected final SolrZkClient zkClient;
+ protected final ZkController zkController;
+ protected final String keeperAddr;
- public ZKPrinter(ZkController controller) throws IOException {
+ public ZkBasePrinter(ZkController controller, boolean detail, boolean
dump) {
this.zkController = controller;
- keeperAddr = controller.getZkServerAddress();
- zkClient = controller.getZkClient();
+ this.detail = detail;
+ this.dump = dump;
+ this.keeperAddr = controller.getZkServerAddress();
+ this.zkClient = controller.getZkClient();
Review Comment:
ZkBasePrinter dereferences the passed ZkController without a null check.
CoreContainer registers this handler even when not ZooKeeper-aware, so
/admin/zookeeper in standalone mode can trigger a NullPointerException here.
Please guard against cores.getZkController()==null (e.g., throw a
BAD_REQUEST/SERVICE_UNAVAILABLE with a clear message) or make ZkBasePrinter
tolerate a null controller and emit an error response.
##########
solr/core/src/java/org/apache/solr/handler/admin/ZookeeperInfoHandler.java:
##########
@@ -360,71 +353,160 @@ 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");
req.setParams(SolrParams.wrapDefaults(new MapSolrParams(map), params));
+
+ // Ensure paging support is initialized
+ ensurePagingSupportInitialized();
+
+ // Validate parameters
+ validateParameters(params);
+
+ // Determine request type and handle accordingly
+ boolean isGraphView = "graph".equals(params.get("view"));
+ ZkBasePrinter printer =
+ isGraphView ? handleGraphViewRequest(params) :
handlePathViewRequest(params);
+
+ try {
+ printer.print();
+ } finally {
+ printer.close();
+ }
+
+ addJsonToResponse(printer.getJsonString(), rsp);
+ }
+
+ /** Ensures the paging support is initialized (thread-safe lazy
initialization). */
+ private void ensurePagingSupportInitialized() {
synchronized (this) {
if (pagingSupport == null) {
pagingSupport = new PagedCollectionSupport();
ZkController zkController = cores.getZkController();
if (zkController != null) {
- // get notified when the ZK session expires (so we can clear the
cached collections and
- // rebuild)
+ // Get notified when the ZK session expires (so we can clear cached
collections)
zkController.addOnReconnectListener(pagingSupport);
}
}
}
+ }
- String path = params.get(PATH);
-
+ /**
+ * Validates incoming parameters.
+ *
+ * @param params Request parameters to validate
+ * @throws SolrException if validation fails
+ */
+ private void validateParameters(SolrParams params) {
if (params.get("addr") != null) {
throw new SolrException(ErrorCode.BAD_REQUEST, "Illegal parameter
\"addr\"");
}
+ }
- String detailS = params.get(PARAM_DETAIL);
- boolean detail = detailS != null && detailS.equals("true");
+ /**
+ * Handles the graph view request with paginated collections.
+ *
+ * @param params Request parameters including pagination settings
+ * @return JSON string representing paginated collection data
+ */
+ private ZkBasePrinter handleGraphViewRequest(SolrParams params) {
+ // Extract pagination parameters
+ int start = params.getInt("start", 0);
+ int rows = params.getInt("rows", -1);
- String dumpS = params.get("dump");
- boolean dump = dumpS != null && dumpS.equals("true");
+ // Extract filter parameters
+ FilterType filterType = extractFilterType(params);
+ String filter = extractFilter(params, filterType);
+
+ // Extract display options (applicable to graph view)
+ boolean detail = params.getBool(PARAM_DETAIL, false);
+ boolean dump = params.getBool("dump", false);
+
+ // Create printer for paginated collections
+ return new ZkGraphPrinter(
+ cores.getZkController(),
+ new PageOfCollections(start, rows, filterType, filter),
+ pagingSupport,
+ detail,
+ dump);
+ }
- int start = params.getInt("start", 0); // Note start ignored if rows not
specified
- int rows = params.getInt("rows", -1);
+ /**
+ * Handles the path view request for a specific ZooKeeper path.
+ *
+ * @param params Request parameters including the path to display
+ * @return JSON string representing the ZooKeeper path data
+ */
+ private ZkBasePrinter handlePathViewRequest(SolrParams params) {
+ // Extract path parameter
+ String path = params.get(PATH);
+
+ // Extract display options
+ boolean detail = params.getBool(PARAM_DETAIL, false);
+ boolean dump = params.getBool("dump", false);
+ // Create printer for specific path
+ return new ZkPathPrinter(cores.getZkController(), path, detail, dump);
+ }
+
+ /**
+ * Extracts and normalizes the filter type from request parameters.
+ *
+ * @param params Request parameters
+ * @return The filter type (defaults to FilterType.none if not specified)
+ */
+ private FilterType extractFilterType(SolrParams params) {
String filterType = params.get("filterType");
if (filterType != null) {
filterType = filterType.trim().toLowerCase(Locale.ROOT);
- if (filterType.length() == 0) filterType = null;
+ if (filterType.length() == 0) {
+ return FilterType.none;
+ }
+ return FilterType.valueOf(filterType);
+ }
+ return FilterType.none;
+ }
+
+ /**
+ * Extracts and normalizes the filter value from request parameters.
+ *
+ * @param params Request parameters
+ * @param filterType The filter type being used
+ * @return The filter string, or null if not applicable
+ */
+ private String extractFilter(SolrParams params, FilterType filterType) {
+ if (filterType == FilterType.none) {
+ return null;
}
- FilterType type = (filterType != null) ? FilterType.valueOf(filterType) :
FilterType.none;
- String filter = (type != FilterType.none) ? params.get("filter") : null;
+ String filter = params.get("filter");
if (filter != null) {
filter = filter.trim();
- if (filter.length() == 0) filter = null;
+ if (filter.length() > 0) {
+ return filter;
+ }
}
+ return null;
+ }
- ZKPrinter printer = new ZKPrinter(cores.getZkController());
- printer.detail = detail;
- printer.dump = dump;
- boolean isGraphView = "graph".equals(params.get("view"));
- // There is no znode /clusterstate.json (removed in Solr 9), but we do as
if there's one and
- // return collection listing. Need to change services.js if cleaning up
here, collection list is
- // used from Admin UI Cloud - Graph
- boolean paginateCollections = (isGraphView &&
"/clusterstate.json".equals(path));
- printer.page = paginateCollections ? new PageOfCollections(start, rows,
type, filter) : null;
- printer.pagingSupport = pagingSupport;
+ /**
+ * Converts JSON string to structured response objects and adds to
SolrQueryResponse.
+ *
+ * @param jsonString The JSON string to parse
+ * @param rsp The response object to populate
+ */
+ private void addJsonToResponse(String jsonString, SolrQueryResponse rsp) {
+ // Parse the JSON we built and return as structured data
+ // This allows any response writer (json, xml, etc.) to serialize it
properly
+ // The JSON is always a Map since both printPath() and
printPaginatedCollections()
+ // start with json.startObject() and end with json.endObject()
+ @SuppressWarnings("unchecked")
+ Map<String, Object> jsonMap = (Map<String, Object>)
Utils.fromJSONString(jsonString);
- try {
- if (paginateCollections) {
- // List collections and allow pagination, but no specific znode info
like when looking at a
- // normal ZK path
- printer.printPaginatedCollections();
- } else {
- printer.print(path);
- }
- } finally {
- printer.close();
+ for (Map.Entry<String, Object> entry : jsonMap.entrySet()) {
Review Comment:
This implementation stringifies JSON in the printer and then immediately
parses it back into a Map (Utils.fromJSONString) before writing the response.
That round-trip adds CPU + heap overhead proportional to response size
(potentially large for deep ZK paths). Consider building structured objects
directly (Map/NamedList) and adding them to rsp, or keep streaming output, to
avoid the stringify->parse overhead.
##########
solr/core/src/java/org/apache/solr/handler/admin/ZookeeperInfoHandler.java:
##########
@@ -360,71 +353,160 @@ 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");
req.setParams(SolrParams.wrapDefaults(new MapSolrParams(map), params));
+
+ // Ensure paging support is initialized
+ ensurePagingSupportInitialized();
+
+ // Validate parameters
+ validateParameters(params);
+
+ // Determine request type and handle accordingly
+ boolean isGraphView = "graph".equals(params.get("view"));
+ ZkBasePrinter printer =
+ isGraphView ? handleGraphViewRequest(params) :
handlePathViewRequest(params);
+
+ try {
+ printer.print();
+ } finally {
+ printer.close();
+ }
+
+ addJsonToResponse(printer.getJsonString(), rsp);
+ }
+
+ /** Ensures the paging support is initialized (thread-safe lazy
initialization). */
+ private void ensurePagingSupportInitialized() {
synchronized (this) {
if (pagingSupport == null) {
pagingSupport = new PagedCollectionSupport();
ZkController zkController = cores.getZkController();
if (zkController != null) {
- // get notified when the ZK session expires (so we can clear the
cached collections and
- // rebuild)
+ // Get notified when the ZK session expires (so we can clear cached
collections)
zkController.addOnReconnectListener(pagingSupport);
}
}
}
+ }
- String path = params.get(PATH);
-
+ /**
+ * Validates incoming parameters.
+ *
+ * @param params Request parameters to validate
+ * @throws SolrException if validation fails
+ */
+ private void validateParameters(SolrParams params) {
if (params.get("addr") != null) {
throw new SolrException(ErrorCode.BAD_REQUEST, "Illegal parameter
\"addr\"");
}
+ }
- String detailS = params.get(PARAM_DETAIL);
- boolean detail = detailS != null && detailS.equals("true");
+ /**
+ * Handles the graph view request with paginated collections.
+ *
+ * @param params Request parameters including pagination settings
+ * @return JSON string representing paginated collection data
+ */
+ private ZkBasePrinter handleGraphViewRequest(SolrParams params) {
+ // Extract pagination parameters
+ int start = params.getInt("start", 0);
+ int rows = params.getInt("rows", -1);
- String dumpS = params.get("dump");
- boolean dump = dumpS != null && dumpS.equals("true");
+ // Extract filter parameters
+ FilterType filterType = extractFilterType(params);
+ String filter = extractFilter(params, filterType);
+
+ // Extract display options (applicable to graph view)
+ boolean detail = params.getBool(PARAM_DETAIL, false);
+ boolean dump = params.getBool("dump", false);
+
+ // Create printer for paginated collections
+ return new ZkGraphPrinter(
+ cores.getZkController(),
+ new PageOfCollections(start, rows, filterType, filter),
+ pagingSupport,
+ detail,
+ dump);
+ }
- int start = params.getInt("start", 0); // Note start ignored if rows not
specified
- int rows = params.getInt("rows", -1);
+ /**
+ * Handles the path view request for a specific ZooKeeper path.
+ *
+ * @param params Request parameters including the path to display
+ * @return JSON string representing the ZooKeeper path data
+ */
+ private ZkBasePrinter handlePathViewRequest(SolrParams params) {
+ // Extract path parameter
+ String path = params.get(PATH);
+
+ // Extract display options
+ boolean detail = params.getBool(PARAM_DETAIL, false);
+ boolean dump = params.getBool("dump", false);
+ // Create printer for specific path
+ return new ZkPathPrinter(cores.getZkController(), path, detail, dump);
+ }
+
+ /**
+ * Extracts and normalizes the filter type from request parameters.
+ *
+ * @param params Request parameters
+ * @return The filter type (defaults to FilterType.none if not specified)
+ */
+ private FilterType extractFilterType(SolrParams params) {
String filterType = params.get("filterType");
if (filterType != null) {
filterType = filterType.trim().toLowerCase(Locale.ROOT);
- if (filterType.length() == 0) filterType = null;
+ if (filterType.length() == 0) {
+ return FilterType.none;
+ }
+ return FilterType.valueOf(filterType);
Review Comment:
FilterType.valueOf(filterType) will throw IllegalArgumentException for
unexpected values, which likely becomes a 500. Please catch this and return a
400 (BAD_REQUEST) with a message listing allowed values (none/name/status).
```suggestion
switch (filterType) {
case "none":
return FilterType.none;
case "name":
return FilterType.name;
case "status":
return FilterType.status;
default:
throw new SolrException(
ErrorCode.BAD_REQUEST,
"Invalid filterType '" + filterType + "'. Allowed values are:
none, name, status");
}
```
--
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]
