SOLR-18373: remove NamedList.asShallowMap/get(String,int) and SolrParams.toNamedList - #4761
Conversation
github-actions
Bot
added
client:solrj
module:ltr
tests
cat:search
cat:index
cat:api
cat:packagemanager
labels
|
@dsmiley this removes three things you deprecated ( AI-assisted (Claude Sonnet 5) |
Same shape as apache#4763/apache#4761 -- a narrow, single-purpose ClusterState helper, no observable behavior change.
…ams.toNamedList Four deprecated members, 30 call sites over five compile rounds. The replacement for asShallowMap is the SimpleOrderedMap(MapWriter) constructor, which both deprecation notes point at: NamedList implements MapWriter, and SimpleOrderedMap extends NamedList and implements Map, so it can stand in wherever a Map was wanted. get(String, int) becomes indexOf(name, start) with getVal(idx), which is the same loop - indexOf's body is character-for-character the removed method's except that it returns the index. And SolrParams.toNamedList becomes new SimpleOrderedMap<>(params), whose writeMap applies the identical String-versus-String[] rule. What the deprecation notes do not say, and what had to be read out of the deleted code. asShallowMap was a hybrid, not a view. get, put, remove, clear and containsKey were live against the backing NamedList, but entrySet(), keySet() and values() all returned asMap(1) - a depth-1 COPY that collapsed duplicate keys into a List and converted nested NamedList values to Maps. So the migration is behaviour-preserving for the read-only sites and changes duplicate handling for the ones that stream over entrySet: SolrXmlConfig would now let Collectors.toMap throw on duplicate coreAdminHandlerActions entries rather than stringify a collapsed List, and LTRThreadModule would remove both copies of a duplicated threadModule key rather than one. Both are arguably fixes; neither is reachable with the flat scalar values those sites actually see. containsKey is the other axis: the removed view's was get(key) != null, SimpleOrderedMap's is indexOf(key) >= 0, and they differ for a key present with a null value. Reached only at PackageManager, where a top-level null params cannot occur. Two sites were not read-only at all - QueryComponent and CombinedQueryComponent both did getResponseHeader().asShallowMap().put(...), i.e. a write-through. They now do the same thing explicitly, indexOf then add-or-setVal, which is what the deleted put did. Neither setPartialResults (add-only-if-absent, and the key may already hold "omitted") nor the file's neighbouring remove-then-add idiom is equivalent, the latter because it also moves the key to the end of the header and changes serialized order. Two things that looked like format risks and were not, both settled by reading rather than assuming. JavaBinCodec writes ORDERED_MAP for a SimpleOrderedMap and NAMED_LST for a plain NamedList, so swapping the type inside an update request or a response header looks like a wire change - but the removed toNamedList() already constructed a SimpleOrderedMap, so the tag was already ORDERED_MAP. And SolrParams.writeMap is the canonical serialisation used everywhere else; it differs from the removed method only in skipping a parameter whose value array is empty, which toNamedList() emitted as an empty array. One site where the documented replacement would have been a bug. SolrJacksonMapper registers a StdSerializer<NamedList> and did writeObject(value.asShallowMap()); handing it a SimpleOrderedMap, which IS a NamedList, would dispatch straight back into the same serializer forever. It uses asMap(0) instead - a plain LinkedHashMap that leaves nested NamedLists for Jackson to dispatch one level at a time, which is what the anonymous Map did. Left better than found. NamedListTest.testShallowMap tested only the removed method's write-through and is deleted; in its place SimpleOrderedMapTest gains the invariant every migrated call site now depends on - that the MapWriter constructor copies, so mutating the copy does not reach the source and adding to the source does not reach the copy. A trap confirms it is not vacuous: asserting view semantics instead fails exactly that test, 1 of 17, with zero compile errors. DefaultSchemaSuggester needed no wrapper at all: fieldProps is already a SimpleOrderedMap, so dropping .asShallowMap() passes the same instance and even preserves the write-through. Verified: compileJava and compileTestJava for the whole build, spotlessCheck, ecjLintMain and ecjLintTest on solrj and core, renderJavadoc on both, and every changed test class - 8 classes, 71 tests, 0 failures, 1 skipped. Two findings parked rather than touched, both pre-existing: SolrQueryResponse.getResponseHeader declares NamedList<Object> while its body casts to SimpleOrderedMap<Object>, so widening that return type would collapse both write-through hunks to one line each - but it is a public and binary-incompatible API change, so it belongs to its own ticket. And PackageManager tests a top-level "params" key while SolrConfigHandler puts the paramset under "response", so packageParamsExist appears to be permanently false. AI-assisted (Claude Sonnet 5)
Same shape as apache#4763 (David: not changelog-worthy) -- narrow, rarely-used NamedList/SolrParams methods, no observable behavior change.
bad3f17 to
a6e287b
Compare
| .entrySet() | ||
| .stream() | ||
| .collect(Collectors.toMap(Entry::getKey, item -> item.getValue().toString())); | ||
| new SimpleOrderedMap<>( |
There was a problem hiding this comment.
no; do not use SimpleOrderedMap as a general purpose Map. It's not documented well but we should only be creating new ones when we are writing response data structures for efficiency reasons.
There was a problem hiding this comment.
Switched to a plain loop over the NamedList into a LinkedHashMap, no SimpleOrderedMap involved.
| // Not SimpleOrderedMap: it IS a NamedList, so this serializer would recurse on it. | ||
| gen.writeObject(value.asMap(0)); |
There was a problem hiding this comment.
can you elaborate with more words here?
There was a problem hiding this comment.
Expanded: SimpleOrderedMap extends NamedList, so this serializer would recurse into it infinitely if used here; asMap(0) returns a plain LinkedHashMap at the top level while leaving any nested NamedList values untouched.
There was a problem hiding this comment.
Then it seems there is a lost opportunity here to be more efficient, as we're creating a new data structure merely to write it out, when Jackson surely knows how to serialize a Map. I'm not sure if there's a way for us to get Jackson to write it as a Map, bypassing the NamedList detection (avoid infinite recursion).
CC @gerlowskija
There was a problem hiding this comment.
If we can't resolve this right now, the comment should recognize this sad situation so a future reader sees the opportunity / issue.
There was a problem hiding this comment.
Implemented it: registered a second, more specific serializer for SimpleOrderedMap that writes it directly via entrySet(), no copy, no recursion (verified with a standalone Jackson probe before touching this code). Nested plain NamedLists still fall through to the existing asMap(0) path.
Added SolrJacksonMapperTest.
…p SimpleOrderedMap misuse, clarify comments - QueryComponent/CombinedQueryComponent: the indexOf+if/else+setVal upsert is just remove(key)+add(key,val) -- same effect, no branch. - SolrXmlConfig/PackageManager: stop using SimpleOrderedMap as a generic Map adapter (that's not what it's for). SolrXmlConfig builds the map directly off NamedList's own Iterable<Map.Entry>; PackageManager reads the NamedList value/key directly instead of wrapping it first. - SolrJacksonMapper: expanded the comment explaining why SimpleOrderedMap specifically (not just "a NamedList") would recurse here. - JavaBinUpdateRequestCodec: reworded a comment that referenced "as before" (PR-review language) to instead state the actual reason -- JavaBinCodec picks the wire tag from the runtime type, and receivers expect ORDERED_MAP here.
… upsert getResponseHeader() is always a SimpleOrderedMap at runtime, which already has an in-place put(): indexOf + setVal/add. That replaces the remove()+add() pair without reordering the entry to the end of the header.
|
note: precommit failed but for a reason I believe that should go away if you sync from main. |
…edlist-asshallowmap
…lper getResponseHeader()'s documented contract is NamedList<Object>, not SimpleOrderedMap -- the previous commit's cast broke QueryComponentPartialResultsTest, whose MockResponseBuilder stubs getResponseHeader() to return a plain NamedList via Mockito, fully within that contract. Reverted to remove()+add(), and extracted it into a shared updateResponseHeader() on QueryComponent, reused by CombinedQueryComponent (which extends it) for both the partialResults site and the segmentTerminatedEarly upsert that already used the same pattern.
… copy SimpleOrderedMap already implements Map, so the copy-to-LinkedHashMap step asMap(0) does before handing it to Jackson is unnecessary for that case -- it was only there to dodge the infinite recursion a SimpleOrderedMap would otherwise cause in NamedListSerializer (SimpleOrderedMap extends NamedList). Registered a second, more specific serializer for SimpleOrderedMap that writes it out via its own entrySet(), delegating each value back through the provider so nested NamedLists/SimpleOrderedMaps still get whichever serializer actually matches their runtime type. defaultSerializeField() doesn't honor the mapper's NON_NULL inclusion on its own, so null values are skipped explicitly to match how the NamedListSerializer path already behaves. Verified with a real end-to-end test (SolrJacksonMapperTest) covering the direct SimpleOrderedMap case, a plain NamedList nested inside it (which still needs the asMap(0) path), and null-value omission on both paths.
| header.remove(key); | ||
| header.add(key, value); | ||
| } | ||
|
|
There was a problem hiding this comment.
Why did you do this? I much prefer it as it was (assuming it worked)
There was a problem hiding this comment.
It didn't work, that's why -- I reverted it. getResponseHeader()'s documented contract is NamedList<Object>, not SimpleOrderedMap. QueryComponentPartialResultsTest's mock stubs getResponseHeader() to return a plain NamedList via Mockito, fully within that contract, and the cast threw ClassCastException there. remove()/add() works regardless of the actual runtime type.
There was a problem hiding this comment.
Sigh... that really sucks because we're IMO making the code worse on the account of a test matter. I wonder how easy it might be to "just" change the return types of SolrQueryResponse in its own PR separately from this.
There was a problem hiding this comment.
Opened #4809 -- tightens getResponseHeader()/addResponseHeader() to SimpleOrderedMap<Object>, which lets these call sites use put() directly. It's binary-incompatible for external callers on the old NamedList signature (verified with a NoSuchMethodError repro), flagged in the PR description since this is long-standing public API -- your call there.
There was a problem hiding this comment.
Thank you :-)
In the mean time: Couldn't we update our mocks to return a SimpleOrderedMap? Just because the "documented contract" is a NamedList, doesn't mean Solr truly needs to support a plain NamedList from these methods on SolrQueryResponse. SQR is the base implementation; subclasses add to the instances returned by the base; don't replace / override (I think).
There was a problem hiding this comment.
Done -- fixed MockResponseBuilder to return a real SimpleOrderedMap, restored the cast+put() (verified: QueryComponentPartialResultsTest passes without needing the return-type change from #4809 at all). That PR can stand on its own merits now rather than being a blocker here.
…tract Per dsmiley: MockResponseBuilder's mock returning a plain NamedList was the actual bug, not the documented contract. Made it return a real SimpleOrderedMap and restored the cast+put() in updateResponseHeader. Also: unused hamcrest assertThat import in SolrJacksonMapperTest was already failing ecjLint on this branch (unrelated to this change).
3 participants
https://issues.apache.org/jira/browse/SOLR-18373
Removes
NamedList.get(String,int),asShallowMap()/asShallowMap(boolean), andSolrParams.toNamedList()— 30 call sites, migrated to the documentedSimpleOrderedMap(MapWriter)constructor /indexOf+getVal.Where to look:
asShallowMap()was a hybrid, not a view —get/put/removewere live against the backingNamedList, butentrySet()/keySet()/values()returned a depth-1 copy that collapsed duplicate keys. The migration preserves the live-write sites (QueryComponent,CombinedQueryComponent— explicitindexOfthenadd-or-setVal) and changes duplicate-key handling only where nothing reachable actually has duplicates (SolrXmlConfig,LTRThreadModule).One place where the documented replacement would have been a bug:
SolrJacksonMapper'sNamedListserializer usedasShallowMap(); handing it aSimpleOrderedMap(which IS aNamedList) would recurse into itself. UsesasMap(0)instead.Left better than found: the deleted test covered only
asShallowMap's write-through;SimpleOrderedMapTestgains the copy-not-view invariant every migrated site now depends on, confirmed by a trap.Verified: full compile, ecjLint/renderJavadoc on solrj+core, 8 changed test classes — 71 tests, 0 failures.
SOLR-18374, SOLR-18380, SOLR-18386 and SOLR-18389 touch files this PR also touches — merging this one first should make those cleaner to extract.
AI-assisted (Claude Sonnet 5)