You are viewing a plain text version of this content. The canonical link for it is here.
Posted to issues@solr.apache.org by "HoustonPutman (via GitHub)" <gi...@apache.org> on 2023/08/23 14:52:20 UTC
[GitHub] [solr] HoustonPutman opened a new pull request, #1863: SOLR-16933: Include full query response in API tool
HoustonPutman opened a new pull request, #1863:
URL: https://github.com/apache/solr/pull/1863
https://issues.apache.org/jira/browse/SOLR-16933
So the initial issue here is that the SolrDocumentList serialization used to only be really handled by the `SolrRequestWriter` (solr core only), and if you tried to use a general JSON serializer (`JSONWriter`), it would try to serialize the `SolrDocumentList` as an ArrayList of SolrDocuments, because it extends `ArrayList<SolrDocument>`. This meant that the `numFound`, `numFoundExact`, `start`, etc. fields were lost.
Two solutions here:
1. `SolrDocumentList` now extends `MapWriter`, so that it will serialize correctly even when used with a "dumb" serializer.
2. The CLI tools that just print output from the APIs they call (`ApiTool`, `CreateTool`, `DeleteTool`), should use the `NoOpResponseParser("json")`, and just plainly echo the output of the API, without de-serializing and re-serializing.
In the end # 2 is what actually fixes this, but # 1 is a good thing to include so that in the future people don't hit snags trying to serialize a Solr Query response with `JSONWriter`.
(The indentation of JSONWriter with the SolrDocumentList is not ideal, but it works. And it doesn't really matter as we aren't reserializing anymore.)
--
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
[GitHub] [solr] epugh commented on a diff in pull request #1863: SOLR-16933: Include full query response in API tool
Posted by "epugh (via GitHub)" <gi...@apache.org>.
epugh commented on code in PR #1863:
URL: https://github.com/apache/solr/pull/1863#discussion_r1303193644
##########
solr/core/src/test/org/apache/solr/cli/ApiToolTest.java:
##########
@@ -42,4 +58,48 @@ public void testParsingGetUrl() throws URISyntaxException {
assertEquals("select id from COLL_NAME limit 10", params.get("stmt"));
}
}
+
+ @Test
+ public void testQueryResponse() throws Exception {
Review Comment:
Nice to see more test coverage!
--
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
[GitHub] [solr] HoustonPutman commented on a diff in pull request #1863: SOLR-16933: Include full query response in API tool
Posted by "HoustonPutman (via GitHub)" <gi...@apache.org>.
HoustonPutman commented on code in PR #1863:
URL: https://github.com/apache/solr/pull/1863#discussion_r1303398270
##########
solr/core/src/test/org/apache/solr/cli/ApiToolTest.java:
##########
@@ -42,4 +58,48 @@ public void testParsingGetUrl() throws URISyntaxException {
assertEquals("select id from COLL_NAME limit 10", params.get("stmt"));
}
}
+
+ @Test
+ public void testQueryResponse() throws Exception {
+ int docCount = 1000;
+ CollectionAdminRequest.createCollection(COLLECTION_NAME, "config", 2, 1)
+ .process(cluster.getSolrClient());
+ cluster.waitForActiveCollection(COLLECTION_NAME, 2, 2);
+
+ String tmpFileLoc =
+ new File(cluster.getBaseDir().toFile().getAbsolutePath() + File.separator).getPath();
+
+ UpdateRequest ur = new UpdateRequest();
+ ur.setAction(AbstractUpdateRequest.ACTION.COMMIT, true, true);
+
+ for (int i = 0; i < docCount; i++) {
+ ur.add(
+ "id",
+ String.valueOf(i),
+ "desc_s",
+ TestUtil.randomSimpleString(random(), 10, 50),
+ "a_dt",
+ "2019-09-30T05:58:03Z");
+ }
+ cluster.getSolrClient().request(ur, COLLECTION_NAME);
+
+ ApiTool tool = new ApiTool();
+
+ String response =
+ tool.callGet(
+ cluster.getJettySolrRunner(0).getBaseUrl()
+ + "/"
+ + COLLECTION_NAME
+ + "/select?q=*:*&rows=1&fl=id&sort=id+asc");
+ // Fields that could be missed because of serialization
+ assertFindInJson(response, "\"numFound\":1000,");
+ // Correct formatting
+ assertFindInJson(response, "\"docs\":[{");
+ }
+
+ private void assertFindInJson(String json, String find) {
Review Comment:
It's basically an assertStringContains, which I can't find elsewhere...
--
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
[GitHub] [solr] HoustonPutman commented on pull request #1863: SOLR-16933: Include full query response in API tool
Posted by "HoustonPutman (via GitHub)" <gi...@apache.org>.
HoustonPutman commented on PR #1863:
URL: https://github.com/apache/solr/pull/1863#issuecomment-1690609516
Unfortunately the Http2SolrClient does not play nicely with the NoOpResponseParser for errors, so I switched to using the JSONMapResponseParser. Though I did make the error handling a little better for NoOpResponseParser usage... Will issue another commit.
--
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
[GitHub] [solr] HoustonPutman commented on a diff in pull request #1863: SOLR-16933: Include full query response in API tool
Posted by "HoustonPutman (via GitHub)" <gi...@apache.org>.
HoustonPutman commented on code in PR #1863:
URL: https://github.com/apache/solr/pull/1863#discussion_r1303259400
##########
solr/core/src/java/org/apache/solr/cli/ApiTool.java:
##########
@@ -61,29 +60,39 @@ public List<Option> getOptions() {
@Override
public void runImpl(CommandLine cli) throws Exception {
+ String response = null;
String getUrl = cli.getOptionValue("get");
if (getUrl != null) {
- getUrl = getUrl.replace("+", "%20");
- URI uri = new URI(getUrl);
- String solrUrl = getSolrUrlFromUri(uri);
- String path = uri.getPath();
- try (var solrClient = SolrCLI.getSolrClient(solrUrl)) {
- NamedList<Object> response =
- solrClient.request(
- // For path parameter we need the path without the root so from the second / char
- // (because root can be configured)
- // E.g URL is http://localhost:8983/solr/admin/info/system path is
- // /solr/admin/info/system and the path without root is /admin/info/system
- new GenericSolrRequest(
- SolrRequest.METHOD.GET,
- path.substring(path.indexOf("/", path.indexOf("/") + 1)),
- getSolrParamsFromUri(uri)));
+ response = callGet(getUrl);
+ }
+ if (response != null) {
+ // pretty-print the response to stdout
+ echo(response);
+ }
+ }
- // pretty-print the response to stdout
- CharArr arr = new CharArr();
- new JSONWriter(arr, 2).write(response.asMap());
- echo(arr.toString());
- }
+ protected String callGet(String url) throws Exception {
Review Comment:
Probably, but also probably for another day 🙂
##########
solr/core/src/java/org/apache/solr/cli/ApiTool.java:
##########
@@ -61,29 +60,39 @@ public List<Option> getOptions() {
@Override
public void runImpl(CommandLine cli) throws Exception {
+ String response = null;
String getUrl = cli.getOptionValue("get");
if (getUrl != null) {
- getUrl = getUrl.replace("+", "%20");
- URI uri = new URI(getUrl);
- String solrUrl = getSolrUrlFromUri(uri);
- String path = uri.getPath();
- try (var solrClient = SolrCLI.getSolrClient(solrUrl)) {
- NamedList<Object> response =
- solrClient.request(
- // For path parameter we need the path without the root so from the second / char
- // (because root can be configured)
- // E.g URL is http://localhost:8983/solr/admin/info/system path is
- // /solr/admin/info/system and the path without root is /admin/info/system
- new GenericSolrRequest(
- SolrRequest.METHOD.GET,
- path.substring(path.indexOf("/", path.indexOf("/") + 1)),
- getSolrParamsFromUri(uri)));
+ response = callGet(getUrl);
+ }
+ if (response != null) {
+ // pretty-print the response to stdout
+ echo(response);
+ }
+ }
- // pretty-print the response to stdout
- CharArr arr = new CharArr();
- new JSONWriter(arr, 2).write(response.asMap());
- echo(arr.toString());
- }
+ protected String callGet(String url) throws Exception {
Review Comment:
Probably, but also probably for another day 🙂
--
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
[GitHub] [solr] HoustonPutman merged pull request #1863: SOLR-16933: Include full query response in API tool
Posted by "HoustonPutman (via GitHub)" <gi...@apache.org>.
HoustonPutman merged PR #1863:
URL: https://github.com/apache/solr/pull/1863
--
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
[GitHub] [solr] epugh commented on a diff in pull request #1863: SOLR-16933: Include full query response in API tool
Posted by "epugh (via GitHub)" <gi...@apache.org>.
epugh commented on code in PR #1863:
URL: https://github.com/apache/solr/pull/1863#discussion_r1303190761
##########
solr/core/src/java/org/apache/solr/cli/ApiTool.java:
##########
@@ -61,29 +60,39 @@ public List<Option> getOptions() {
@Override
public void runImpl(CommandLine cli) throws Exception {
+ String response = null;
String getUrl = cli.getOptionValue("get");
if (getUrl != null) {
- getUrl = getUrl.replace("+", "%20");
- URI uri = new URI(getUrl);
- String solrUrl = getSolrUrlFromUri(uri);
- String path = uri.getPath();
- try (var solrClient = SolrCLI.getSolrClient(solrUrl)) {
- NamedList<Object> response =
- solrClient.request(
- // For path parameter we need the path without the root so from the second / char
- // (because root can be configured)
- // E.g URL is http://localhost:8983/solr/admin/info/system path is
- // /solr/admin/info/system and the path without root is /admin/info/system
- new GenericSolrRequest(
- SolrRequest.METHOD.GET,
- path.substring(path.indexOf("/", path.indexOf("/") + 1)),
- getSolrParamsFromUri(uri)));
+ response = callGet(getUrl);
+ }
+ if (response != null) {
+ // pretty-print the response to stdout
+ echo(response);
+ }
+ }
- // pretty-print the response to stdout
- CharArr arr = new CharArr();
- new JSONWriter(arr, 2).write(response.asMap());
- echo(arr.toString());
- }
+ protected String callGet(String url) throws Exception {
Review Comment:
I suspect that all the logic here in callGet is duplicated in other places around our codebase..... I wish we had a single place to do these types of calls that we could reuse. Maybe for another day. ;-).
##########
solr/core/src/test/org/apache/solr/cli/ApiToolTest.java:
##########
@@ -42,4 +58,48 @@ public void testParsingGetUrl() throws URISyntaxException {
assertEquals("select id from COLL_NAME limit 10", params.get("stmt"));
}
}
+
+ @Test
+ public void testQueryResponse() throws Exception {
+ int docCount = 1000;
+ CollectionAdminRequest.createCollection(COLLECTION_NAME, "config", 2, 1)
+ .process(cluster.getSolrClient());
+ cluster.waitForActiveCollection(COLLECTION_NAME, 2, 2);
+
+ String tmpFileLoc =
+ new File(cluster.getBaseDir().toFile().getAbsolutePath() + File.separator).getPath();
+
+ UpdateRequest ur = new UpdateRequest();
+ ur.setAction(AbstractUpdateRequest.ACTION.COMMIT, true, true);
+
+ for (int i = 0; i < docCount; i++) {
+ ur.add(
+ "id",
+ String.valueOf(i),
+ "desc_s",
+ TestUtil.randomSimpleString(random(), 10, 50),
+ "a_dt",
+ "2019-09-30T05:58:03Z");
+ }
+ cluster.getSolrClient().request(ur, COLLECTION_NAME);
+
+ ApiTool tool = new ApiTool();
+
+ String response =
+ tool.callGet(
+ cluster.getJettySolrRunner(0).getBaseUrl()
+ + "/"
+ + COLLECTION_NAME
+ + "/select?q=*:*&rows=1&fl=id&sort=id+asc");
+ // Fields that could be missed because of serialization
+ assertFindInJson(response, "\"numFound\":1000,");
+ // Correct formatting
+ assertFindInJson(response, "\"docs\":[{");
+ }
+
+ private void assertFindInJson(String json, String find) {
Review Comment:
sigh... Don't we have this elsewhere defined ;-).
--
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