SOLR-17316: make SolrJ response objects work with non-binary ResponseParsers - #4640
SOLR-17316: make SolrJ response objects work with non-binary ResponseParsers#4640serhiy-bzhezytskyy wants to merge 11 commits into
Conversation
…arsers getStatus() and getQTime() cast the header value to Integer, which threw a ClassCastException under a parser that yields a different numeric type (the JSON parser yields Long). Widen via Number.intValue() instead.
The SolrJ response classes assume the Java types the binary parser produces, so
reading a response parsed by a non-binary parser (e.g. the JSON map parser) threw
ClassCastException: JSON yields raw Map/List where the code expects
NamedList/SolrDocumentList, and Long where it casts to Integer.
- ResponseNormalizer converts a parsed response into the canonical shape (nested
objects -> NamedList/SimpleOrderedMap, a {numFound,docs} object ->
SolrDocumentList); it is a no-op for already-canonical binary/XML responses.
- ResponseParser.producesCanonicalForm() gates it; JsonMapResponseParser returns
false. HttpSolrClient normalizes at the shared response boundary, covering both
the JDK and Jetty transports while binary/XML pay nothing.
- Remaining numeric reads that cast to Integer/Float are widened via Number
(grouping, interval and pivot facet counts, spellcheck, analysis token offsets,
Luke, schema version).
Tests cover the normalizer, cross-format parity (binary/XML/json-map), each
affected section, and an end-to-end HTTP query with the JSON parser on both
transports.
|
Are there any objections to its merger? I am ready to resolve them, just let me know. Thanks |
There was a problem hiding this comment.
TBH I'd rather see an integration test, showing something working realisitcally that previously didn't. I'm not a fan of unit tests like this.
I can't tell yet as I'm coming to understand this PR still but it's possible there's existting tests that just need to randomly pick the "wt". We use randomization a lot, and it can reduce the testing maintenance & increase net tested exposure IMO.
There was a problem hiding this 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.
There was a problem hiding this comment.
Randomized wt in TestSuggesterResponse — it already randomized javabin/xml for SOLR-15070, so this adds the JSON parser to that. Removed QueryResponseCrossFormatTest.
SolrExampleTests has a subclass per parser and JSON was missing, so SolrExampleJsonMapTest now runs all 42 against JsonMapResponseParser.
| // 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) { |
There was a problem hiding this comment.
We should not (on principle) replace every NamedList with a SimpleOrderedMap. Some NL's are a SOM; some are not. A SOM conveys unique keys that may not be true with a general NL.
There was a problem hiding this comment.
Fixed — the concrete type is preserved now:
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.
There was a problem hiding this comment.
Two things I checked ...
Yes this is by design. It's why SOM exists. It's for efficient response formulation -- no safety check, and conveys a Map and thus it renders different with JSON.
| NamedList<Object> rsp; | ||
| try { | ||
| rsp = processor.processResponse(is, encoding); | ||
| if (!processor.producesCanonicalForm()) { |
There was a problem hiding this comment.
hmm; this seems bolted on. Could we instead have processor.processCanonicalResponse that internally has this similar logic? This puts the processor in charge. No processor.producesCanonicalForm predicate would be needed.
There was a problem hiding this comment.
Agreed, and done as you described:
// 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 Maps, its processCanonicalResponse yields NamedLists and a SolrDocumentList, and a canonical parser passes through unchanged. Mutation-checked — dropping the override fails both that test and the integration test.
There was a problem hiding this comment.
I'd like to make a claim/observation, at the risk of being wrong and a little embarrassed. Your PR comment responses are AI generated, at least this and some others were. Yes? I think it's strongly preferable for AI generated responses to humans to be somehow marked as such. FWIW at work we have a footer and/or marker emoji. I went to know the difference between Serhiy, in the flesh whom I'm getting to know, and a bot.
There was a problem hiding this comment.
hey David, deal :) (not bot)
…e/style fixes Review feedback on apache#4640. The client no longer knows which parsers need normalizing. ResponseParser gains processCanonicalResponse(), which defaults to processResponse() — most parsers are canonical already and inherit it unchanged — and JsonMapResponseParser overrides it to convert. The producesCanonicalForm() predicate is gone, and HttpSolrClient just calls the one method: rsp = processor.processCanonicalResponse(is, encoding); The existing processResponse() is untouched, since 20 call sites use it directly, including the error path in ConcurrentUpdateBaseSolrClient which only reads resp.get("error") and needs no conversion. ResponseNormalizer moves from org.apache.solr.common.util to org.apache.solr.client.solrj.response: it is not a common utility. A plain NamedList is no longer promoted to SimpleOrderedMap. SimpleOrderedMap implements Map and is written differently by the response writers -- a JSON writer renders it as {"foo":10} and a NamedList as ["foo",10] -- so widening the type changes the contract of the value. Only the concrete type is preserved now; a JSON object still becomes a SimpleOrderedMap, since its keys are unique by construction. Worth noting the mutation check: with "always promote" in place, all 27 existing tests passed, so two tests were added for the distinction and they do fail on it. Also: pattern-matching instanceof throughout ResponseNormalizer, text blocks for the JSON literals in the parity test, no FQNs, and no try-with-resources around solrTestRule.getSolrClient() -- its javadoc says "The caller doesn't need to close it". ResponseParserCanonicalFormTest becomes ResponseParserCanonicalResponseTest and pins behaviour rather than the removed predicate: the JSON parser's raw output is Maps, its canonical output is NamedLists and a SolrDocumentList, and a canonical parser passes through unchanged. 200 solrj response/client tests pass; :solr:solrj:check clean.
… map parser Extends the randomization SOLR-15070 introduced in that test — javabin or xml — to pick among three parsers, so the JSON map parser goes through the same suggester assertions as the other two. It is a regression test for this PR rather than added coverage: on the commit before the normalizer, forcing that parser in fails all three of the test's methods with ClassCastException: class java.util.LinkedHashMap cannot be cast to class org.apache.solr.common.util.NamedList and with the conversion in place they pass. Mutation-checked — removing the conversion from JsonMapResponseParser#processCanonicalResponse brings the ClassCastException back under -Ptests.iters=10.
The test fed the JSON parser a literal whose facet_fields was written in json.nl=map form, which JsonMapResponseParser never requests, so the input shape does not occur on a live request — running the response classes end-to-end over a real server surfaced that same section failing as an array. Two of its three methods also asserted existing javabin and xml behaviour rather than anything this change introduces. Coverage of the numeric widening in QueryResponse is kept by QueryResponseSectionParityTest, and of a real request by QueryResponseJsonParserIntegrationTest.
…and have the JSON map parser ask for json.nl=map JsonMapResponseParser could not read the response it was getting. Under the default json.nl=flat a NamedList is written as an array of alternating names and values, so facet_fields arrived as a List where the response classes expect a NamedList, and the structure cannot be recovered after the fact. The style had to be set by hand on every request, which is not something a caller should have to know per parser. ResponseParser#getRequestParams supplies params alongside wt. Anything the request set explicitly wins, so this only provides defaults. Applied in HttpSolrClient#initializeSolrParams, which every HTTP client routes through. QueryResponseJsonParserIntegrationTest no longer sets json.nl itself, which is what the change is for, and asserts that an explicit value survives.
JSON has no document type, so the JSON writer emits nested documents as a _childDocuments_ field holding a list of maps. The binary and XML parsers hand them back as child documents; this one left them as a plain field, so SolrDocument#hasChildDocuments was false and the children were unreachable through the documented accessors. Nesting is recursive, so grandchildren are covered too.
facet_queries, a range facet's counts and a pivot's query counts were cast to NamedList<Integer> and iterated as Integer entries, so a response whose numbers arrive as Long threw ClassCastException. Same defect as the accessors already widened here, in three places the earlier commits did not reach; the values are still narrowed to int, so nothing about the public types changes.
SolrExampleTests has a subclass per parser and JSON was missing, so the whole 42-test suite now runs against JsonMapResponseParser. That is what found the three defects fixed in the preceding commits; on the commit before them it fails seven ways. Nine assertions in SolrExampleTests pinned the boxed type of a number rather than its value -- (Integer) getFieldValue(..), assertEquals(1.0f, ..), RangeFacet<Float, Float> -- and are relaxed to Number where the value is the point. Nothing is ignored: 42 of 42 pass, and the binary, XML, CBOR and HTTP/2 subclasses are unaffected.
| for (Iterator<String> it = parserParams.getParameterNamesIterator(); it.hasNext(); ) { | ||
| String name = it.next(); | ||
| if (wparams.get(name) == null) { | ||
| wparams.set(name, parserParams.getParams(name)); | ||
| } | ||
| } |
There was a problem hiding this comment.
why loop the names when you could just call parserParams.get(CommonParams.WT) ?
There was a problem hiding this comment.
I can see you (really the LLMs you use :-) ) are unfamiliar with SolrParams.wrapDefaults use that. Even has the null check.
| } | ||
|
|
||
| private static final SolrParams REQUEST_PARAMS = | ||
| new MapSolrParams(Map.of(JsonTextWriter.JSON_NL_STYLE, JsonTextWriter.JSON_NL_MAP)); |
| * 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() { |
There was a problem hiding this comment.
Lets name this getAdditionalRequestParams.
There was a problem hiding this 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.
| /** | ||
| * 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)); | ||
| } |
There was a problem hiding this 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.
| assertEquals(JsonTextWriter.JSON_NL_MAP, params.get(JsonTextWriter.JSON_NL_STYLE)); | ||
| } | ||
|
|
||
| /** Parsers that need nothing beyond wt contribute no params. */ |
There was a problem hiding this 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 hesitant to truly recommend precisely this... but I'm at least taking the educational opportunity of sharing the rather unique randomized testing philosophy that permeates the Lucene & Solr projects.
There was a problem hiding this comment.
David, thanks a lot for sharing it. i will dive deeper into it, it's very interesting approach...
| @Test | ||
| public void testJsonMapParserReportsFalse() { | ||
| assertFalse(new JsonMapResponseParser().producesCanonicalForm()); | ||
| /** Runs the example tests over {@link JsonMapResponseParser}. */ |
There was a problem hiding this comment.
Niiiiice.... :-) thank you. Should be getting lots more coverage here.
| 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 |
There was a problem hiding this comment.
Not worth putting here; it's an implementation detail. What you wrote originally was very good.
Uses SolrParams.wrapDefaults and SolrParams.of instead of hand-rolled merging, and renames getRequestParams to getAdditionalRequestParams. Parser-required params now take precedence over the request's own, as wt already did — a parser that can't read the form the caller asked for would fail rather than honour it. EmbeddedSolrServer called processResponse directly, so a JSON parser there still threw ClassCastException. It now applies the parser's params and reads the response canonically. Named nested documents are reconstructed too, keyed on _nest_path_.
3 participants
https://issues.apache.org/jira/browse/SOLR-17316
SolrJ's response classes assume binary-parser types, so reading a response parsed by a non-binary parser (the JSON parser) throws ClassCastException. Two commits so the small safe fix can go in on its own if you'd rather.
Commit 1 is the narrow fix: getStatus/getQTime cast to Integer, which CCEs when JSON gives Long. Widened via Number. That's it — resolves the getters the issue names.
Commit 2 goes further. It's not just the header — getResults(), facets, grouping all break under JSON too, because the parser hands back raw Map/List where the code wants NamedList/SolrDocumentList. So there's a ResponseNormalizer that converts a parsed response to the canonical shape at the client boundary (no-op for binary/XML), gated by a new ResponseParser.producesCanonicalForm() so only the JSON map parser triggers it. Both transports covered, binary pays nothing. Plus the remaining Integer/Float casts widened.
Heads up: commit 2 basically does what SOLR-3451 asked for in 2012, which was closed Won't Fix ("solr does not have a way to write a JSON response and read the same value"). Still true for json.nl=flat since it's lossy, but json.nl=map round-trips and the normalizer does the rest. So I'd treat commit 2 as reopening that discussion — fine to split it out or take it to dev@ if you'd prefer, commit 1 stands alone either way.
Tests: the normalizer + edge cases, binary/xml/json-map parity, the affected sections (grouping, facets, stats, spellcheck, highlighting, terms, moreLikeThis, analysis, Luke, schema), and an end-to-end JSON query on both the Jetty and JDK clients.