dsmiley commented on code in PR #4640:
URL: https://github.com/apache/solr/pull/4640#discussion_r3708683324
##########
solr/solrj/src/java/org/apache/solr/client/solrj/response/json/JsonMapResponseParser.java:
##########
@@ -64,8 +68,23 @@ public Set<String> getContentTypes() {
return CONTENT_TYPES;
}
+ private static final SolrParams REQUEST_PARAMS =
+ new MapSolrParams(Map.of(JsonTextWriter.JSON_NL_STYLE,
JsonTextWriter.JSON_NL_MAP));
Review Comment:
see `SolrParams.of(...`
##########
solr/solrj/src/test/org/apache/solr/client/solrj/response/ResponseParserCanonicalResponseTest.java:
##########
@@ -0,0 +1,103 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one or more
+ * contributor license agreements. See the NOTICE file distributed with
+ * this work for additional information regarding copyright ownership.
+ * The ASF licenses this file to You under the Apache License, Version 2.0
+ * (the "License"); you may not use this file except in compliance with
+ * the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an "AS IS" BASIS,
+ * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ * See the License for the specific language governing permissions and
+ * limitations under the License.
+ */
+package org.apache.solr.client.solrj.response;
+
+import static java.nio.charset.StandardCharsets.UTF_8;
+
+import java.io.ByteArrayInputStream;
+import java.io.InputStream;
+import java.util.Map;
+import org.apache.solr.SolrTestCase;
+import org.apache.solr.client.solrj.response.json.JsonMapResponseParser;
+import org.apache.solr.common.SolrDocumentList;
+import org.apache.solr.common.params.SolrParams;
+import org.apache.solr.common.util.JsonTextWriter;
+import org.apache.solr.common.util.NamedList;
+import org.junit.Test;
+
+/**
+ * Pins the {@link ResponseParser#processCanonicalResponse} contract: whatever
a parser's natural
+ * output looks like, this method returns the canonical shape the SolrJ
response classes read — a
+ * NamedList tree with SolrDocumentList for document sections. The conversion
belongs to the parser,
+ * so a client does not need to know which parsers require it.
+ */
+public class ResponseParserCanonicalResponseTest extends SolrTestCase {
+
+ private static final String JSON =
+ """
+ {"responseHeader":{"status":0,"QTime":1},\
+
"response":{"numFound":1,"start":0,"numFoundExact":true,"docs":[{"id":"1"}]}}""";
+
+ private static InputStream json() {
+ return new ByteArrayInputStream(JSON.getBytes(UTF_8));
+ }
+
+ /** The JSON map parser's own output is raw: Maps where the response classes
expect NamedLists. */
+ @Test
+ public void testJsonMapParserRawOutputIsNotCanonical() throws Exception {
+ NamedList<Object> raw = new
JsonMapResponseParser().processResponse(json(), null);
+ assertTrue("raw header should be a Map", raw.get("responseHeader")
instanceof Map);
+ assertFalse(
+ "raw header should not be a NamedList", raw.get("responseHeader")
instanceof NamedList);
+ assertFalse(
+ "raw response should not be a SolrDocumentList",
+ raw.get("response") instanceof SolrDocumentList);
+ }
+
+ /** ... and processCanonicalResponse converts it, without the caller asking.
*/
+ @Test
+ public void testJsonMapParserCanonicalResponseIsConverted() throws Exception
{
+ NamedList<Object> out = new
JsonMapResponseParser().processCanonicalResponse(json(), null);
+ assertTrue("header must be a NamedList", out.get("responseHeader")
instanceof NamedList);
+ assertTrue(
+ "response must be a SolrDocumentList", out.get("response") instanceof
SolrDocumentList);
+ assertEquals(1, ((SolrDocumentList) out.get("response")).getNumFound());
+ }
+
+ /** Parsers that are canonical already inherit the default and are unchanged
by it. */
+ @Test
+ public void testCanonicalParsersPassThrough() throws Exception {
+ String xml =
+ """
+ <?xml version="1.0" encoding="UTF-8"?>
+ <response><lst name="responseHeader"><int
name="status">0</int></lst></response>""";
+ NamedList<Object> out =
+ new XMLResponseParser()
+ .processCanonicalResponse(new
ByteArrayInputStream(xml.getBytes(UTF_8)), null);
+ assertTrue("header must be a NamedList", out.get("responseHeader")
instanceof NamedList);
+ }
+
+ /**
+ * A parser that needs the response written a particular way supplies that
param itself, rather
+ * than relying on every caller to know it. The JSON map parser needs {@code
json.nl=map}: under
+ * the default {@code flat} a NamedList arrives as an array of alternating
names and values, whose
+ * structure cannot be recovered.
+ */
+ @Test
+ public void testJsonMapParserRequestsNlMap() {
+ SolrParams params = new JsonMapResponseParser().getRequestParams();
+ assertNotNull("the JSON map parser must ask for a recoverable NamedList
form", params);
+ assertEquals(JsonTextWriter.JSON_NL_MAP,
params.get(JsonTextWriter.JSON_NL_STYLE));
+ }
Review Comment:
IMO this test doesn't add value. It unit tests a fine detail using more
code than the non-test. Instead we want to see an integration test showing the
correct behavior that this underlying detail here helps arrange for -- i.e. the
circumstance that led to the need to have this.
##########
solr/solrj/src/java/org/apache/solr/client/solrj/response/ResponseParser.java:
##########
@@ -50,6 +51,17 @@ private boolean validateContentTypes() {
/** The writer type placed onto the request as the {@code wt} param. */
public abstract String getWriterType(); // for example: wt=XML, JSON, etc
+ /**
+ * Params this parser needs on the request for its own reads, applied
alongside {@code wt}.
+ *
+ * <p>A parser that needs the response written a particular way returns
those params here rather
+ * than relying on callers to set them. Anything the caller set explicitly
wins, so this only
+ * supplies defaults. Returns null when the parser needs nothing beyond
{@code wt}.
+ */
+ public SolrParams getRequestParams() {
Review Comment:
Lets name this `getAdditionalRequestParams`.
##########
solr/solrj/src/java/org/apache/solr/client/solrj/impl/HttpSolrClient.java:
##########
@@ -139,6 +140,16 @@ protected ModifiableSolrParams initializeSolrParams(
// The parser 'wt=' param is used instead of the original params
ModifiableSolrParams wparams = new
ModifiableSolrParams(solrRequest.getParams());
wparams.set(CommonParams.WT, parserToUse.getWriterType());
+ // Params the parser needs for its own reads, without overriding what the
request already set.
+ SolrParams parserParams = parserToUse.getRequestParams();
+ if (parserParams != null) {
+ for (Iterator<String> it = parserParams.getParameterNamesIterator();
it.hasNext(); ) {
+ String name = it.next();
+ if (wparams.get(name) == null) {
+ wparams.set(name, parserParams.getParams(name));
+ }
+ }
Review Comment:
why loop the names when you could just call
`parserParams.get(CommonParams.WT)` ?
##########
changelog/unreleased/SOLR-17316-response-parsers.yml:
##########
@@ -3,7 +3,9 @@
title: >
SolrJ's QueryResponse and other response objects now work when the client is
configured with a
non-binary response parser (such as the JSON parser); previously their
accessors could throw a
- ClassCastException.
+ ClassCastException, and nested documents were unreachable. A ResponseParser
can now declare the
+ request params it needs via getRequestParams(); JsonMapResponseParser uses
this to ask for
Review Comment:
Not worth putting here; it's an implementation detail. What you wrote
originally was very good.
##########
solr/solrj/src/test/org/apache/solr/client/solrj/response/TestSuggesterResponse.java:
##########
Review Comment:
thanks for this. It's good.
However, note that most of Lucene & Solr's randomized testing is done at a
deeper level such that an individual test generally doesn't even have to do
anything to get the randomization -- it just happens at a deeper test
framework/infra level. For example... imagine if the default was a settable
static supplier... and imagine if SolrTestCase were to set it. Then the useful
test coverage would go through the roof (thousands of Solr tests) and we'd
probably toss aside more of your tests as redundant. I'm not sure I want to
say we should do precisely this... but I'm at least informing you of the rather
unique randomized testing philosophy that permeates the Lucene & Solr projects.
##########
solr/solrj/src/test/org/apache/solr/client/solrj/SolrExampleJsonMapTest.java:
##########
@@ -14,28 +14,19 @@
* See the License for the specific language governing permissions and
* limitations under the License.
*/
-package org.apache.solr.client.solrj.response;
+package org.apache.solr.client.solrj;
-import org.apache.solr.SolrTestCase;
+import org.apache.solr.SolrTestCaseJ4.SuppressSSL;
import org.apache.solr.client.solrj.response.json.JsonMapResponseParser;
-import org.junit.Test;
-/**
- * Pins the producesCanonicalForm() contract that gates response
normalization: only the JSON map
- * parser (which yields raw Maps/Lists) needs normalizing; the parsers that
already produce the
- * canonical NamedList/SolrDocumentList shape must report true so they pass
through untouched.
- */
-public class ResponseParserCanonicalFormTest extends SolrTestCase {
-
- @Test
- public void testCanonicalParsersReportTrue() {
- assertTrue(new JavaBinResponseParser().producesCanonicalForm());
- assertTrue(new XMLResponseParser().producesCanonicalForm());
- assertTrue(new InputStreamResponseParser("json").producesCanonicalForm());
- }
-
- @Test
- public void testJsonMapParserReportsFalse() {
- assertFalse(new JsonMapResponseParser().producesCanonicalForm());
+/** Runs the example tests over {@link JsonMapResponseParser}. */
Review Comment:
Niiiiice.... :-) thank you
##########
solr/solrj/src/test/org/apache/solr/client/solrj/response/ResponseParserCanonicalResponseTest.java:
##########
@@ -0,0 +1,103 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one or more
+ * contributor license agreements. See the NOTICE file distributed with
+ * this work for additional information regarding copyright ownership.
+ * The ASF licenses this file to You under the Apache License, Version 2.0
+ * (the "License"); you may not use this file except in compliance with
+ * the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an "AS IS" BASIS,
+ * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ * See the License for the specific language governing permissions and
+ * limitations under the License.
+ */
+package org.apache.solr.client.solrj.response;
+
+import static java.nio.charset.StandardCharsets.UTF_8;
+
+import java.io.ByteArrayInputStream;
+import java.io.InputStream;
+import java.util.Map;
+import org.apache.solr.SolrTestCase;
+import org.apache.solr.client.solrj.response.json.JsonMapResponseParser;
+import org.apache.solr.common.SolrDocumentList;
+import org.apache.solr.common.params.SolrParams;
+import org.apache.solr.common.util.JsonTextWriter;
+import org.apache.solr.common.util.NamedList;
+import org.junit.Test;
+
+/**
+ * Pins the {@link ResponseParser#processCanonicalResponse} contract: whatever
a parser's natural
+ * output looks like, this method returns the canonical shape the SolrJ
response classes read — a
+ * NamedList tree with SolrDocumentList for document sections. The conversion
belongs to the parser,
+ * so a client does not need to know which parsers require it.
+ */
+public class ResponseParserCanonicalResponseTest extends SolrTestCase {
+
+ private static final String JSON =
+ """
+ {"responseHeader":{"status":0,"QTime":1},\
+
"response":{"numFound":1,"start":0,"numFoundExact":true,"docs":[{"id":"1"}]}}""";
+
+ private static InputStream json() {
+ return new ByteArrayInputStream(JSON.getBytes(UTF_8));
+ }
+
+ /** The JSON map parser's own output is raw: Maps where the response classes
expect NamedLists. */
+ @Test
+ public void testJsonMapParserRawOutputIsNotCanonical() throws Exception {
+ NamedList<Object> raw = new
JsonMapResponseParser().processResponse(json(), null);
+ assertTrue("raw header should be a Map", raw.get("responseHeader")
instanceof Map);
+ assertFalse(
+ "raw header should not be a NamedList", raw.get("responseHeader")
instanceof NamedList);
+ assertFalse(
+ "raw response should not be a SolrDocumentList",
+ raw.get("response") instanceof SolrDocumentList);
+ }
+
+ /** ... and processCanonicalResponse converts it, without the caller asking.
*/
+ @Test
+ public void testJsonMapParserCanonicalResponseIsConverted() throws Exception
{
+ NamedList<Object> out = new
JsonMapResponseParser().processCanonicalResponse(json(), null);
+ assertTrue("header must be a NamedList", out.get("responseHeader")
instanceof NamedList);
+ assertTrue(
+ "response must be a SolrDocumentList", out.get("response") instanceof
SolrDocumentList);
+ assertEquals(1, ((SolrDocumentList) out.get("response")).getNumFound());
+ }
+
+ /** Parsers that are canonical already inherit the default and are unchanged
by it. */
+ @Test
+ public void testCanonicalParsersPassThrough() throws Exception {
+ String xml =
+ """
+ <?xml version="1.0" encoding="UTF-8"?>
+ <response><lst name="responseHeader"><int
name="status">0</int></lst></response>""";
+ NamedList<Object> out =
+ new XMLResponseParser()
+ .processCanonicalResponse(new
ByteArrayInputStream(xml.getBytes(UTF_8)), null);
+ assertTrue("header must be a NamedList", out.get("responseHeader")
instanceof NamedList);
+ }
+
+ /**
+ * A parser that needs the response written a particular way supplies that
param itself, rather
+ * than relying on every caller to know it. The JSON map parser needs {@code
json.nl=map}: under
+ * the default {@code flat} a NamedList arrives as an array of alternating
names and values, whose
+ * structure cannot be recovered.
+ */
+ @Test
+ public void testJsonMapParserRequestsNlMap() {
+ SolrParams params = new JsonMapResponseParser().getRequestParams();
+ assertNotNull("the JSON map parser must ask for a recoverable NamedList
form", params);
+ assertEquals(JsonTextWriter.JSON_NL_MAP,
params.get(JsonTextWriter.JSON_NL_STYLE));
+ }
+
+ /** Parsers that need nothing beyond wt contribute no params. */
Review Comment:
again; no value
##########
solr/solrj/src/test/org/apache/solr/client/solrj/response/ResponseNormalizerTest.java:
##########
Review Comment:
I see you are supporting anonymous child documents -- which is the original
thing and I've been meaning to deprecate it. Nowadays, we do "nested
documents", which have named relationships from parent to child. Neither is
reflected in the schema, but anyway you can fetch children (named **or**
anonymous) via `fl=*,[child fl=*]` if I recall off the top of my head.
##########
solr/solrj/src/java/org/apache/solr/client/solrj/impl/HttpSolrClient.java:
##########
@@ -139,6 +140,16 @@ protected ModifiableSolrParams initializeSolrParams(
// The parser 'wt=' param is used instead of the original params
ModifiableSolrParams wparams = new
ModifiableSolrParams(solrRequest.getParams());
wparams.set(CommonParams.WT, parserToUse.getWriterType());
+ // Params the parser needs for its own reads, without overriding what the
request already set.
+ SolrParams parserParams = parserToUse.getRequestParams();
+ if (parserParams != null) {
+ for (Iterator<String> it = parserParams.getParameterNamesIterator();
it.hasNext(); ) {
+ String name = it.next();
+ if (wparams.get(name) == null) {
+ wparams.set(name, parserParams.getParams(name));
+ }
+ }
Review Comment:
I can see you (really the LLMs you use :-) ) are unfamiliar with
`SolrParams.wrapDefaults` use that. Even has the null check.
--
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]
