You are viewing a plain text version of this content. The canonical link for it is here.
Posted to issues@solr.apache.org by "sonatype-lift[bot] (via GitHub)" <gi...@apache.org> on 2023/03/03 16:59:04 UTC
[GitHub] [solr] sonatype-lift[bot] commented on a diff in pull request #1182: SOLR-16504 Convert CLI tools to Jetty HTTP 2 client.
sonatype-lift[bot] commented on code in PR #1182:
URL: https://github.com/apache/solr/pull/1182#discussion_r1124750911
##########
solr/core/src/java/org/apache/solr/util/SolrCLI.java:
##########
@@ -648,164 +619,30 @@ public static NamedList<Object> postJsonToSolr(
return solrClient.request(req);
}
- /** Useful when a tool just needs to send one request to Solr. */
- public static Map<String, Object> getJson(String getUrl) throws Exception {
- Map<String, Object> json = null;
- CloseableHttpClient httpClient = getHttpClient();
- try {
- json = getJson(httpClient, getUrl, 2, true);
- } finally {
- closeHttpClient(httpClient);
- }
- return json;
- }
-
- /** Utility function for sending HTTP GET request to Solr with built-in retry support. */
- public static Map<String, Object> getJson(
- HttpClient httpClient, String getUrl, int attempts, boolean isFirstAttempt) throws Exception {
- Map<String, Object> json = null;
- if (attempts >= 1) {
- try {
- json = getJson(httpClient, getUrl);
- } catch (Exception exc) {
- if (exceptionIsAuthRelated(exc)) {
- throw exc;
- }
- if (--attempts > 0 && checkCommunicationError(exc)) {
- if (!isFirstAttempt) // only show the log warning after the second attempt fails
- log.warn(
- "Request to {} failed, sleeping for 5 seconds before re-trying the request ...",
- getUrl,
- exc);
- try {
- Thread.sleep(5000);
- } catch (InterruptedException ie) {
- Thread.interrupted();
- }
-
- // retry using recursion with one-less attempt available
- json = getJson(httpClient, getUrl, attempts, false);
- } else {
- // no more attempts or error is not retry-able
- throw exc;
- }
- }
- }
-
- return json;
- }
-
- @SuppressWarnings("unchecked")
- private static class SolrResponseHandler implements ResponseHandler<Map<String, Object>> {
- @Override
- public Map<String, Object> handleResponse(HttpResponse response)
- throws ClientProtocolException, IOException {
- HttpEntity entity = response.getEntity();
- if (entity != null) {
-
- String respBody = EntityUtils.toString(entity);
- Object resp = null;
- try {
- resp = fromJSONString(respBody);
- } catch (JSONParser.ParseException pe) {
- throw new ClientProtocolException(
- "Expected JSON response from server but received: "
- + respBody
- + "\nTypically, this indicates a problem with the Solr server; check the Solr server logs for more information.");
- }
- if (resp instanceof Map) {
- return (Map<String, Object>) resp;
- } else {
- throw new ClientProtocolException(
- "Expected JSON object in response but received " + resp);
- }
- } else {
- StatusLine statusLine = response.getStatusLine();
- throw new HttpResponseException(statusLine.getStatusCode(), statusLine.getReasonPhrase());
- }
- }
- }
-
- /**
- * Utility function for sending HTTP GET request to Solr and then doing some validation of the
- * response.
- */
- public static Map<String, Object> getJson(HttpClient httpClient, String getUrl) throws Exception {
- try {
- // ensure we're requesting JSON back from Solr
- HttpGet httpGet =
- new HttpGet(
- new URIBuilder(getUrl).setParameter(CommonParams.WT, CommonParams.JSON).build());
-
- // make the request and get back a parsed JSON object
- Map<String, Object> json =
- httpClient.execute(
- httpGet,
- new SolrResponseHandler(),
- HttpClientUtil.createNewHttpClientRequestContext());
- // check the response JSON from Solr to see if it is an error
- Long statusCode = asLong("/responseHeader/status", json);
- if (statusCode != null && statusCode == -1) {
- throw new SolrServerException(
- "Unable to determine outcome of GET request to: " + getUrl + "! Response: " + json);
- } else if (statusCode != null && statusCode != 0) {
- String errMsg = asString("/error/msg", json);
- if (errMsg == null) errMsg = String.valueOf(json);
- throw new SolrServerException(errMsg);
- } else {
- // make sure no "failure" object in there either
- Object failureObj = json.get("failure");
- if (failureObj != null) {
- if (failureObj instanceof Map) {
- Object err = ((Map) failureObj).get("");
- if (err != null) throw new SolrServerException(err.toString());
- }
- throw new SolrServerException(failureObj.toString());
- }
- }
- return json;
- } catch (ClientProtocolException cpe) {
- // Currently detecting authentication by string-matching the HTTP response
- // Perhaps SolrClient should have thrown an exception itself??
- if (cpe.getMessage().contains("HTTP ERROR 401")
- || cpe.getMessage().contentEquals("HTTP ERROR 403")) {
- int code = cpe.getMessage().contains("HTTP ERROR 401") ? 401 : 403;
- throw new SolrException(
- SolrException.ErrorCode.getErrorCode(code),
- "Solr requires authentication for "
- + getUrl
- + ". Please supply valid credentials. HTTP code="
- + code);
- } else {
- throw cpe;
- }
- }
- }
-
/** Helper function for reading a String value from a JSON Object tree. */
- public static String asString(String jsonPath, Map<String, Object> json) {
+ public static String asString(String jsonPath, NamedList<Object> json) {
return pathAs(String.class, jsonPath, json);
}
/** Helper function for reading a Long value from a JSON Object tree. */
- public static Long asLong(String jsonPath, Map<String, Object> json) {
+ public static Long asLong(String jsonPath, NamedList<Object> json) {
return pathAs(Long.class, jsonPath, json);
}
/** Helper function for reading a List of Strings from a JSON Object tree. */
@SuppressWarnings("unchecked")
- public static List<String> asList(String jsonPath, Map<String, Object> json) {
+ public static List<String> asList(String jsonPath, NamedList<Object> json) {
return pathAs(List.class, jsonPath, json);
}
/** Helper function for reading a Map from a JSON Object tree. */
@SuppressWarnings("unchecked")
- public static Map<String, Object> asMap(String jsonPath, Map<String, Object> json) {
- return pathAs(Map.class, jsonPath, json);
+ public static Map<String, Object> asMap(String jsonPath, NamedList<Object> json) {
+ return pathAs(SimpleOrderedMap.class, jsonPath, json).asMap();
Review Comment:
<picture><img alt="16% of developers fix this issue" src="https://lift.sonatype.com/api/commentimage/fixrate/16/display.svg"></picture>
<b>*NULL_DEREFERENCE:</b>* object returned by `pathAs(org.apache.solr.common.util.SimpleOrderedMap,jsonPath,json)` could be null and is dereferenced at line 641.
ââ <b>3 similar findings have been found in this PR</b>
<details><summary>đ Expand here to view all instances of this finding</summary><br/>
<div align=\"center\">
| **File Path** | **Line Number** |
| ------------- | ------------- |
| solr/core/src/java/org/apache/solr/util/SolrCLI.java | [830](https://github.com/apache/solr/blob/6baff1ad921f3a3b81783f49e35678005b69734c/solr/core/src/java/org/apache/solr/util/SolrCLI.java#L830) |
| solr/core/src/java/org/apache/solr/packagemanager/PackageUtils.java | [177](https://github.com/apache/solr/blob/6baff1ad921f3a3b81783f49e35678005b69734c/solr/core/src/java/org/apache/solr/packagemanager/PackageUtils.java#L177) |
| solr/core/src/java/org/apache/solr/util/SolrCLI.java | [829](https://github.com/apache/solr/blob/6baff1ad921f3a3b81783f49e35678005b69734c/solr/core/src/java/org/apache/solr/util/SolrCLI.java#L829) |
<p><a href="https://lift.sonatype.com/results/github.com/apache/solr/01GTM3XJXXFSMNMCW4MT9QFXZG?t=Infer|NULL_DEREFERENCE" target="_blank">Visit the Lift Web Console</a> to find more details in your report.</p></div></details>
---
<details><summary>âšī¸ Expand to see all <b>@sonatype-lift</b> commands</summary>
You can reply with the following commands. For example, reply with ***@sonatype-lift ignoreall*** to leave out all findings.
| **Command** | **Usage** |
| ------------- | ------------- |
| `@sonatype-lift ignore` | Leave out the above finding from this PR |
| `@sonatype-lift ignoreall` | Leave out all the existing findings from this PR |
| `@sonatype-lift exclude <file\|issue\|path\|tool>` | Exclude specified `file\|issue\|path\|tool` from Lift findings by updating your config.toml file |
**Note:** When talking to LiftBot, you need to **refresh** the page to see its response.
<sub>[Click here](https://github.com/apps/sonatype-lift/installations/new) to add LiftBot to another repo.</sub></details>
---
<b>Help us improve LIFT! (<i>Sonatype LiftBot external survey</i>)</b>
Was this a good recommendation for you? <sub><small>Answering this survey will not impact your Lift settings.</small></sub>
[â[đ Not relevant](https://www.sonatype.com/lift-comment-rating?comment=421839049&lift_comment_rating=1)â] - [â[đ Won't fix](https://www.sonatype.com/lift-comment-rating?comment=421839049&lift_comment_rating=2)â] - [â[đ Not critical, will fix](https://www.sonatype.com/lift-comment-rating?comment=421839049&lift_comment_rating=3)â] - [â[đ Critical, will fix](https://www.sonatype.com/lift-comment-rating?comment=421839049&lift_comment_rating=4)â] - [â[đ Critical, fixing now](https://www.sonatype.com/lift-comment-rating?comment=421839049&lift_comment_rating=5)â]
##########
solr/core/src/java/org/apache/solr/packagemanager/PackageManager.java:
##########
@@ -268,19 +270,21 @@ public Map<String, SolrPackageInstance> getPackagesDeployedAsClusterLevelPlugins
MultiValuedMap<String, PluginMeta> packagePlugins = new HashSetValuedHashMap<>();
Map<String, Object> result;
try {
- result =
- (Map<String, Object>)
- Utils.executeGET(
- solrClient.getHttpClient(),
- solrBaseUrl + PackageUtils.CLUSTERPROPS_PATH,
- Utils.JSONCONSUMER);
- } catch (SolrException ex) {
- if (ex.code() == ErrorCode.NOT_FOUND.code) {
+ NamedList<Object> response =
+ solrClient.request(
+ new GenericSolrRequest(
+ SolrRequest.METHOD.GET,
+ PackageUtils.CLUSTERPROPS_PATH,
+ new ModifiableSolrParams()));
+ Integer statusCode = (Integer) ((NamedList) response.get("responseHeader")).get("status");
+ if (statusCode == ErrorCode.NOT_FOUND.code) {
Review Comment:
<picture><img alt="6% of developers fix this issue" src="https://lift.sonatype.com/api/commentimage/fixrate/6/display.svg"></picture>
<b>*NULLPTR_DEREFERENCE:</b>* `statusCode` could be null (from the call to `NamedList.get(...)` on line 279) and is dereferenced.
ââ <b>2 similar findings have been found in this PR</b>
<details><summary>đ Expand here to view all instances of this finding</summary><br/>
<div align=\"center\">
| **File Path** | **Line Number** |
| ------------- | ------------- |
| solr/core/src/java/org/apache/solr/packagemanager/PackageManager.java | [279](https://github.com/apache/solr/blob/6baff1ad921f3a3b81783f49e35678005b69734c/solr/core/src/java/org/apache/solr/packagemanager/PackageManager.java#L279) |
| solr/core/src/java/org/apache/solr/packagemanager/PackageUtils.java | [177](https://github.com/apache/solr/blob/6baff1ad921f3a3b81783f49e35678005b69734c/solr/core/src/java/org/apache/solr/packagemanager/PackageUtils.java#L177) |
<p><a href="https://lift.sonatype.com/results/github.com/apache/solr/01GTM3XJXXFSMNMCW4MT9QFXZG?t=Infer|NULLPTR_DEREFERENCE" target="_blank">Visit the Lift Web Console</a> to find more details in your report.</p></div></details>
---
<details><summary>âšī¸ Expand to see all <b>@sonatype-lift</b> commands</summary>
You can reply with the following commands. For example, reply with ***@sonatype-lift ignoreall*** to leave out all findings.
| **Command** | **Usage** |
| ------------- | ------------- |
| `@sonatype-lift ignore` | Leave out the above finding from this PR |
| `@sonatype-lift ignoreall` | Leave out all the existing findings from this PR |
| `@sonatype-lift exclude <file\|issue\|path\|tool>` | Exclude specified `file\|issue\|path\|tool` from Lift findings by updating your config.toml file |
**Note:** When talking to LiftBot, you need to **refresh** the page to see its response.
<sub>[Click here](https://github.com/apps/sonatype-lift/installations/new) to add LiftBot to another repo.</sub></details>
---
<b>Help us improve LIFT! (<i>Sonatype LiftBot external survey</i>)</b>
Was this a good recommendation for you? <sub><small>Answering this survey will not impact your Lift settings.</small></sub>
[â[đ Not relevant](https://www.sonatype.com/lift-comment-rating?comment=421839576&lift_comment_rating=1)â] - [â[đ Won't fix](https://www.sonatype.com/lift-comment-rating?comment=421839576&lift_comment_rating=2)â] - [â[đ Not critical, will fix](https://www.sonatype.com/lift-comment-rating?comment=421839576&lift_comment_rating=3)â] - [â[đ Critical, will fix](https://www.sonatype.com/lift-comment-rating?comment=421839576&lift_comment_rating=4)â] - [â[đ Critical, fixing now](https://www.sonatype.com/lift-comment-rating?comment=421839576&lift_comment_rating=5)â]
--
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-unsubscribe@solr.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@solr.apache.org
For additional commands, e-mail: issues-help@solr.apache.org