serhiy-bzhezytskyy commented on code in PR #4640:
URL: https://github.com/apache/solr/pull/4640#discussion_r3699950662
##########
solr/solrj/src/java/org/apache/solr/client/solrj/impl/HttpSolrClient.java:
##########
@@ -229,6 +230,9 @@ protected NamedList<Object> processErrorsAndResponse(
NamedList<Object> rsp;
try {
rsp = processor.processResponse(is, encoding);
+ if (!processor.producesCanonicalForm()) {
Review Comment:
Agreed, and done as you described:
```java
// ResponseParser — the default, inherited by every parser that is canonical
already
public NamedList<Object> processCanonicalResponse(InputStream body, String
encoding)
throws IOException {
return processResponse(body, encoding);
}
// JsonMapResponseParser — the only override
@Override
public NamedList<Object> processCanonicalResponse(InputStream body, String
encoding)
throws IOException {
return ResponseNormalizer.normalize(processResponse(body, encoding));
}
```
`HttpSolrClient` is now one line with no predicate, and
`producesCanonicalForm()` is gone entirely.
One thing I deliberately did not do: `processResponse` is left as it is. It
has 20 call sites, and at least one of them wants the raw form — the error path
in `ConcurrentUpdateBaseSolrClient` reads only `resp.get("error")`, so
converting there would be work for nothing.
The test that pinned the predicate is now
`ResponseParserCanonicalResponseTest` and pins behaviour instead: the JSON
parser's `processResponse` yields `Map`s, its `processCanonicalResponse` yields
`NamedList`s and a `SolrDocumentList`, and a canonical parser passes through
unchanged. Mutation-checked — dropping the override fails both that test and
the integration test.
##########
solr/solrj/src/java/org/apache/solr/common/util/ResponseNormalizer.java:
##########
@@ -0,0 +1,123 @@
+/*
+ * 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.common.util;
+
+import java.util.ArrayList;
+import java.util.List;
+import java.util.Map;
+import org.apache.solr.common.SolrDocument;
+import org.apache.solr.common.SolrDocumentList;
+
+/**
+ * Converts a parsed response into the canonical shape the SolrJ response
classes expect (the shape
+ * the binary and XML parsers produce): nested JSON objects become {@link
NamedList}s and a {@code
+ * {numFound, docs}} object becomes a {@link SolrDocumentList}.
+ *
+ * <p>Only unambiguous, self-describing conversions are performed. It is a
no-op for values already
+ * in canonical form (so binary/XML responses pass through unchanged). It does
not attempt to
+ * interpret the ambiguous flat arrays produced by {@code json.nl=flat}; a
typed JSON parser should
+ * request {@code json.nl=map} for its own reads.
+ */
+public final class ResponseNormalizer {
+
+ private ResponseNormalizer() {}
+
+ /** Returns a normalized copy of the given response NamedList. */
+ public static NamedList<Object> normalize(NamedList<Object> response) {
+ if (response == null) {
+ return null;
+ }
+ SimpleOrderedMap<Object> out = new SimpleOrderedMap<>(response.size());
+ for (Map.Entry<String, Object> e : response) {
+ out.add(e.getKey(), normalizeValue(e.getValue()));
+ }
+ return out;
+ }
+
+ @SuppressWarnings("unchecked")
+ private static Object normalizeValue(Object val) {
+ if (val instanceof SolrDocumentList || val instanceof SolrDocument) {
+ // Already canonical (binary/XML produce these directly); leave
untouched. Must precede the
+ // List/Map branches since SolrDocumentList is a List and SolrDocument
is a Map.
+ return val;
+ } else if (val instanceof NamedList) {
Review Comment:
Fixed — the concrete type is preserved now:
```java
NamedList<Object> out =
in instanceof SimpleOrderedMap<?>
? new SimpleOrderedMap<>(in.size())
: new NamedList<>(in.size());
```
A JSON object still becomes a `SimpleOrderedMap`, since its keys are unique
by construction, but nothing widens a plain `NamedList` into one any more.
Two things I checked while fixing it, in case either is useful.
`SimpleOrderedMap` does not enforce uniqueness — its javadoc says "It's
normally not a good idea to repeat keys… but this is not enforced" — so the
concrete harm is elsewhere: it `implements Map`, and the response writers
render the two differently ("a JSON response writer may choose to write a
SimpleOrderedMap as {"foo":10,"bar":20} and may choose to write a NamedList as
["foo",10,"bar",20]"). Widening the type changes both of those.
And a mutation check was worth running: with "always promote" restored, all
27 existing tests still passed. So the distinction had no coverage at all.
There are two tests for it now — one asserting a plain `NamedList` with a
repeated key survives as a `NamedList`, one asserting a `SimpleOrderedMap`
stays one — and they do fail on the mutation.
##########
solr/solrj/src/test/org/apache/solr/client/solrj/response/QueryResponseJsonParserIntegrationTest.java:
##########
@@ -0,0 +1,95 @@
+/*
+ * 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 org.apache.solr.SolrTestCaseJ4.sdoc;
+
+import org.apache.solr.SolrTestCase;
+import org.apache.solr.client.solrj.SolrClient;
+import org.apache.solr.client.solrj.request.SolrQuery;
+import org.apache.solr.client.solrj.response.json.JsonMapResponseParser;
+import org.apache.solr.util.ExternalPaths;
+import org.apache.solr.util.SolrJettyTestRule;
+import org.junit.BeforeClass;
+import org.junit.ClassRule;
+import org.junit.Test;
+
+/**
+ * End-to-end: a real HTTP query with the JSON map response parser must return
a fully typed
+ * QueryResponse, proving SolrRequest.process() normalizes the non-canonical
JSON response at the
+ * boundary before the response classes read it (SOLR-17316).
+ */
+public class QueryResponseJsonParserIntegrationTest extends SolrTestCase {
+
+ @ClassRule public static SolrJettyTestRule solrTestRule = new
SolrJettyTestRule();
+
+ @BeforeClass
+ public static void beforeClass() throws Exception {
+ System.setProperty("solr.security.allow.paths", "*");
Review Comment:
It is necessary, though not for a good reason — I measured it rather than
guess. Removing the line fails the test at collection creation:
```
Path .../server/solr/configsets/sample_techproducts_configs must be relative
to
SOLR_HOME, SOLR_DATA_HOME coreRootDirectory.
Set system property 'solr.security.allow.paths' to add other allowed paths.
```
So it is `TECHPRODUCTS_CONFIGSET` that requires it, being outside the test
SOLR_HOME.
I tried to avoid it and could not, at least not cheaply. `SolrPingTest` uses
the `solrj/solr` test home with no such property, but it gets away with it
because it uses `EmbeddedSolrServerTestRule` — this test needs a real HTTP
round trip, so `SolrJettyTestRule`, and Jetty scans the whole home and picks up
`solrj/solr/shared/core0/core.properties`, which has an unsubstituted
`dataDir=${dataDir1}`. The seven configsets under
`solrj/src/test-files/.../configsets` each carry 4–15 unsubstituted
placeholders, and only three have a `*_s` dynamic field.
If you know a configset that works for this without the property, I will
switch to it. Otherwise, when SOLR-18123 lands and the property goes away, this
test will need whatever replaces it — I'd rather it be adjusted then than have
me invent a workaround now.
##########
solr/solrj/src/test/org/apache/solr/client/solrj/response/AdminResponseNumericTypeTest.java:
##########
Review Comment:
Taking this one separately — it is the largest of your notes and I have not
acted on it yet.
`QueryResponseJsonParserIntegrationTest` (in this PR) is the integration
test: a real Jetty round trip over `wt=json` that reads documents, facets and
grouping through `QueryResponse`. `AdminResponseNumericTypeTest` and
`QueryResponseSectionParityTest` are the unit tests you are reacting to.
On randomizing `wt` in existing tests: that is the better shape, and it
would cover far more than these tests do. Before I rewrite it that way I want
to check what it costs — a good number of tests assert on the binary form
specifically, so the randomization would have to be scoped rather than global.
I'll report back with what I find rather than guessing at it here.
--
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]
