You are viewing a plain text version of this content. The canonical link for it is here.
Posted to issues@solr.apache.org by "serhiy-bzhezytskyy (via GitHub)" <gi...@apache.org> on 2026/08/19 15:05:58 UTC
[PR] SOLR-18373: remove NamedList.asShallowMap/get(String,int) and SolrParams.toNamedList [solr]
serhiy-bzhezytskyy opened a new pull request, #4761:
URL: https://github.com/apache/solr/pull/4761
Removes `NamedList.get(String,int)`, `asShallowMap()`/`asShallowMap(boolean)`, and `SolrParams.toNamedList()` — 30 call sites, migrated to the documented `SimpleOrderedMap(MapWriter)` constructor / `indexOf`+`getVal`.
Where to look: `asShallowMap()` was a hybrid, not a view — `get`/`put`/`remove` were live against the backing `NamedList`, but `entrySet()`/`keySet()`/`values()` returned a depth-1 copy that collapsed duplicate keys. The migration preserves the live-write sites (`QueryComponent`, `CombinedQueryComponent` — explicit `indexOf` then `add`-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`'s `NamedList` serializer used `asShallowMap()`; handing it a `SimpleOrderedMap` (which IS a `NamedList`) would recurse into itself. Uses `asMap(0)` instead.
Left better than found: the deleted test covered only `asShallowMap`'s write-through; `SimpleOrderedMapTest` gains 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)
--
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
Re: [PR] SOLR-18373: remove NamedList.asShallowMap/get(String,int) and SolrParams.toNamedList [solr]
Posted by "serhiy-bzhezytskyy (via GitHub)" <gi...@apache.org>.
serhiy-bzhezytskyy commented on code in PR #4761:
URL: https://github.com/apache/solr/pull/4761#discussion_r3841016377
##########
solr/core/src/java/org/apache/solr/jersey/SolrJacksonMapper.java:
##########
@@ -70,7 +70,8 @@ public NamedListSerializer(Class<NamedList> nlClazz) {
@Override
public void serialize(NamedList value, JsonGenerator gen, SerializerProvider provider)
throws IOException {
- gen.writeObject(value.asShallowMap());
+ // Not SimpleOrderedMap: it IS a NamedList, so this serializer would recurse on it.
+ gen.writeObject(value.asMap(0));
Review 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.
--
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
Re: [PR] SOLR-18373: remove NamedList.asShallowMap/get(String,int) and SolrParams.toNamedList [solr]
Posted by "serhiy-bzhezytskyy (via GitHub)" <gi...@apache.org>.
serhiy-bzhezytskyy commented on code in PR #4761:
URL: https://github.com/apache/solr/pull/4761#discussion_r3838130211
##########
solr/core/src/java/org/apache/solr/handler/component/CombinedQueryComponent.java:
##########
Review Comment:
Applied the same fix here ahead of SOLR-18398: `((SimpleOrderedMap<Object>) rb.rsp.getResponseHeader()).put(...)`. Once SOLR-18398 is merged and `getResponseHeader()` returns `SimpleOrderedMap` directly, the cast can be dropped.
##########
solr/core/src/java/org/apache/solr/core/SolrXmlConfig.java:
##########
@@ -127,11 +128,11 @@ public static NodeConfig fromConfig(
// It should go inside the fillSolrSection method but
// since it is arranged as a separate section it is placed here
Map<String, String> coreAdminHandlerActions =
- readNodeListAsNamedList(root.get("coreAdminHandlerActions"), "<coreAdminHandlerActions>")
- .asShallowMap()
- .entrySet()
- .stream()
- .collect(Collectors.toMap(Entry::getKey, item -> item.getValue().toString()));
+ new SimpleOrderedMap<>(
Review Comment:
Switched to a plain loop over the NamedList into a `LinkedHashMap`, no `SimpleOrderedMap` involved.
##########
solr/core/src/java/org/apache/solr/jersey/SolrJacksonMapper.java:
##########
@@ -70,7 +70,8 @@ public NamedListSerializer(Class<NamedList> nlClazz) {
@Override
public void serialize(NamedList value, JsonGenerator gen, SerializerProvider provider)
throws IOException {
- gen.writeObject(value.asShallowMap());
+ // Not SimpleOrderedMap: it IS a NamedList, so this serializer would recurse on it.
+ gen.writeObject(value.asMap(0));
Review 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.
##########
solr/core/src/java/org/apache/solr/packagemanager/PackageManager.java:
##########
@@ -421,16 +422,14 @@ private Pair<List<String>, List<String>> deployCollectionPackage(
// Get package params
try {
- boolean packageParamsExist =
- solrClient
- .request(
- new GenericV2SolrRequest(
- SolrRequest.METHOD.GET,
- PackageUtils.getCollectionParamsPath(collection) + "/packages")
- .setRequiresCollection(
- false) /* Making a collection-request, but already baked into path */)
- .asShallowMap()
- .containsKey("params");
+ NamedList<Object> collectionParams =
+ solrClient.request(
+ new GenericV2SolrRequest(
+ SolrRequest.METHOD.GET,
+ PackageUtils.getCollectionParamsPath(collection) + "/packages")
+ .setRequiresCollection(
+ false) /* Making a collection-request, but already baked into path */);
+ boolean packageParamsExist = new SimpleOrderedMap<>(collectionParams).containsKey("params");
Review Comment:
Dropped the `SimpleOrderedMap` wrapper: `collectionParams` is already a `NamedList`, so `get("params") != null` works directly on it -- went with this per your second suggestion.
##########
solr/solrj/src/java/org/apache/solr/client/solrj/request/JavaBinUpdateRequestCodec.java:
##########
@@ -56,7 +57,8 @@ public class JavaBinUpdateRequestCodec {
public void marshal(UpdateRequest updateRequest, OutputStream os) throws IOException {
NamedList<Object> nl = new NamedList<>();
- NamedList<Object> params = updateRequest.getParams().toNamedList();
+ // SimpleOrderedMap serializes with the same JavaBin ORDERED_MAP tag as before.
Review Comment:
Reworded: `JavaBinCodec` picks the wire tag (`ORDERED_MAP` vs `NAMED_LST`) from the runtime type, and receivers expect `ORDERED_MAP` here -- so it has to stay a `SimpleOrderedMap`, not a plain `NamedList`.
--
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
Re: [PR] SOLR-18373: remove NamedList.asShallowMap/get(String,int) and SolrParams.toNamedList [solr]
Posted by "dsmiley (via GitHub)" <gi...@apache.org>.
dsmiley commented on code in PR #4761:
URL: https://github.com/apache/solr/pull/4761#discussion_r3846164108
##########
solr/core/src/java/org/apache/solr/handler/component/QueryComponent.java:
##########
@@ -1282,6 +1280,11 @@ protected void mergeIds(ResponseBuilder rb, ShardRequest sreq) {
}
}
+ protected static void updateResponseHeader(NamedList<Object> header, String key, Object value) {
+ header.remove(key);
+ header.add(key, value);
+ }
+
Review 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.
--
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
Re: [PR] SOLR-18373: remove NamedList.asShallowMap/get(String,int) and SolrParams.toNamedList [solr]
Posted by "dsmiley (via GitHub)" <gi...@apache.org>.
dsmiley merged PR #4761:
URL: https://github.com/apache/solr/pull/4761
--
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
Re: [PR] SOLR-18373: remove NamedList.asShallowMap/get(String,int) and SolrParams.toNamedList [solr]
Posted by "serhiy-bzhezytskyy (via GitHub)" <gi...@apache.org>.
serhiy-bzhezytskyy commented on code in PR #4761:
URL: https://github.com/apache/solr/pull/4761#discussion_r3844603237
##########
solr/core/src/java/org/apache/solr/handler/component/QueryComponent.java:
##########
@@ -1282,6 +1280,11 @@ protected void mergeIds(ResponseBuilder rb, ShardRequest sreq) {
}
}
+ protected static void updateResponseHeader(NamedList<Object> header, String key, Object value) {
+ header.remove(key);
+ header.add(key, value);
+ }
+
Review 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.
--
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
Re: [PR] SOLR-18373: remove NamedList.asShallowMap/get(String,int) and SolrParams.toNamedList [solr]
Posted by "dsmiley (via GitHub)" <gi...@apache.org>.
dsmiley commented on code in PR #4761:
URL: https://github.com/apache/solr/pull/4761#discussion_r3839207742
##########
solr/core/src/java/org/apache/solr/jersey/SolrJacksonMapper.java:
##########
@@ -70,7 +70,8 @@ public NamedListSerializer(Class<NamedList> nlClazz) {
@Override
public void serialize(NamedList value, JsonGenerator gen, SerializerProvider provider)
throws IOException {
- gen.writeObject(value.asShallowMap());
+ // Not SimpleOrderedMap: it IS a NamedList, so this serializer would recurse on it.
+ gen.writeObject(value.asMap(0));
Review 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
--
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
Re: [PR] SOLR-18373: remove NamedList.asShallowMap/get(String,int) and SolrParams.toNamedList [solr]
Posted by "epugh (via GitHub)" <gi...@apache.org>.
epugh commented on code in PR #4761:
URL: https://github.com/apache/solr/pull/4761#discussion_r3835829819
##########
solr/core/src/java/org/apache/solr/handler/component/QueryComponent.java:
##########
@@ -1237,10 +1237,13 @@ protected void mergeIds(ResponseBuilder rb, ShardRequest sreq) {
populateNextCursorMarkFromMergedShards(rb);
if (thereArePartialResults) {
- rb.rsp
- .getResponseHeader()
- .asShallowMap()
- .put(SolrQueryResponse.RESPONSE_HEADER_PARTIAL_RESULTS_KEY, Boolean.TRUE);
+ NamedList<Object> header = rb.rsp.getResponseHeader();
Review Comment:
this pattern just seems awkward...
--
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
Re: [PR] SOLR-18373: remove NamedList.asShallowMap/get(String,int) and SolrParams.toNamedList [solr]
Posted by "dsmiley (via GitHub)" <gi...@apache.org>.
dsmiley commented on code in PR #4761:
URL: https://github.com/apache/solr/pull/4761#discussion_r3837490732
##########
solr/core/src/java/org/apache/solr/core/SolrXmlConfig.java:
##########
@@ -127,11 +128,11 @@ public static NodeConfig fromConfig(
// It should go inside the fillSolrSection method but
// since it is arranged as a separate section it is placed here
Map<String, String> coreAdminHandlerActions =
- readNodeListAsNamedList(root.get("coreAdminHandlerActions"), "<coreAdminHandlerActions>")
- .asShallowMap()
- .entrySet()
- .stream()
- .collect(Collectors.toMap(Entry::getKey, item -> item.getValue().toString()));
+ new SimpleOrderedMap<>(
Review 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.
##########
solr/core/src/java/org/apache/solr/handler/component/CombinedQueryComponent.java:
##########
Review Comment:
I filed https://issues.apache.org/jira/browse/SOLR-18398 for Solr 11 and assigned to me.
Related to that -- I see that a number of SolrQueryResponse getter's return a NamedList that are in fact known to be a SimpleOrderedMap. I think it's fair to return the correct type. Granted that'd be a big enough change that it'd deserve it's own PR first. So what that in place, the logic here is basically one line -- call `put`. It will apply to more places in this PR likewise.
##########
solr/solrj/src/java/org/apache/solr/client/solrj/request/JavaBinUpdateRequestCodec.java:
##########
@@ -56,7 +57,8 @@ public class JavaBinUpdateRequestCodec {
public void marshal(UpdateRequest updateRequest, OutputStream os) throws IOException {
NamedList<Object> nl = new NamedList<>();
- NamedList<Object> params = updateRequest.getParams().toNamedList();
+ // SimpleOrderedMap serializes with the same JavaBin ORDERED_MAP tag as before.
Review Comment:
"as before" is PR language to communicate a change but is confusing / inappropriate to actually commit
##########
solr/core/src/java/org/apache/solr/packagemanager/PackageManager.java:
##########
@@ -421,16 +422,14 @@ private Pair<List<String>, List<String>> deployCollectionPackage(
// Get package params
try {
- boolean packageParamsExist =
- solrClient
- .request(
- new GenericV2SolrRequest(
- SolrRequest.METHOD.GET,
- PackageUtils.getCollectionParamsPath(collection) + "/packages")
- .setRequiresCollection(
- false) /* Making a collection-request, but already baked into path */)
- .asShallowMap()
- .containsKey("params");
+ NamedList<Object> collectionParams =
+ solrClient.request(
+ new GenericV2SolrRequest(
+ SolrRequest.METHOD.GET,
+ PackageUtils.getCollectionParamsPath(collection) + "/packages")
+ .setRequiresCollection(
+ false) /* Making a collection-request, but already baked into path */);
+ boolean packageParamsExist = new SimpleOrderedMap<>(collectionParams).containsKey("params");
Review Comment:
this is very sad BTW; just call `collectionParams.indexOf("params") >= 0`
##########
solr/core/src/java/org/apache/solr/jersey/SolrJacksonMapper.java:
##########
@@ -70,7 +70,8 @@ public NamedListSerializer(Class<NamedList> nlClazz) {
@Override
public void serialize(NamedList value, JsonGenerator gen, SerializerProvider provider)
throws IOException {
- gen.writeObject(value.asShallowMap());
+ // Not SimpleOrderedMap: it IS a NamedList, so this serializer would recurse on it.
+ gen.writeObject(value.asMap(0));
Review Comment:
can you elaborate with more words 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: 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
Re: [PR] SOLR-18373: remove NamedList.asShallowMap/get(String,int) and SolrParams.toNamedList [solr]
Posted by "serhiy-bzhezytskyy (via GitHub)" <gi...@apache.org>.
serhiy-bzhezytskyy commented on code in PR #4761:
URL: https://github.com/apache/solr/pull/4761#discussion_r3851571978
##########
solr/core/src/java/org/apache/solr/handler/component/QueryComponent.java:
##########
@@ -1282,6 +1280,11 @@ protected void mergeIds(ResponseBuilder rb, ShardRequest sreq) {
}
}
+ protected static void updateResponseHeader(NamedList<Object> header, String key, Object value) {
+ header.remove(key);
+ header.add(key, value);
+ }
+
Review 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.
@dsmiley
--
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
Re: [PR] SOLR-18373: remove NamedList.asShallowMap/get(String,int) and SolrParams.toNamedList [solr]
Posted by "serhiy-bzhezytskyy (via GitHub)" <gi...@apache.org>.
serhiy-bzhezytskyy commented on code in PR #4761:
URL: https://github.com/apache/solr/pull/4761#discussion_r3853900043
##########
solr/core/src/java/org/apache/solr/handler/component/QueryComponent.java:
##########
@@ -1282,6 +1280,11 @@ protected void mergeIds(ResponseBuilder rb, ShardRequest sreq) {
}
}
+ protected static void updateResponseHeader(NamedList<Object> header, String key, Object value) {
+ header.remove(key);
+ header.add(key, value);
+ }
+
Review 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.
--
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
Re: [PR] SOLR-18373: remove NamedList.asShallowMap/get(String,int) and SolrParams.toNamedList [solr]
Posted by "dsmiley (via GitHub)" <gi...@apache.org>.
dsmiley commented on code in PR #4761:
URL: https://github.com/apache/solr/pull/4761#discussion_r3837619421
##########
solr/core/src/java/org/apache/solr/packagemanager/PackageManager.java:
##########
@@ -421,16 +422,14 @@ private Pair<List<String>, List<String>> deployCollectionPackage(
// Get package params
try {
- boolean packageParamsExist =
- solrClient
- .request(
- new GenericV2SolrRequest(
- SolrRequest.METHOD.GET,
- PackageUtils.getCollectionParamsPath(collection) + "/packages")
- .setRequiresCollection(
- false) /* Making a collection-request, but already baked into path */)
- .asShallowMap()
- .containsKey("params");
+ NamedList<Object> collectionParams =
+ solrClient.request(
+ new GenericV2SolrRequest(
+ SolrRequest.METHOD.GET,
+ PackageUtils.getCollectionParamsPath(collection) + "/packages")
+ .setRequiresCollection(
+ false) /* Making a collection-request, but already baked into path */);
+ boolean packageParamsExist = new SimpleOrderedMap<>(collectionParams).containsKey("params");
Review Comment:
or `get` and do null check... which is Map-friendly in support of the eventual transition to Map.
--
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
Re: [PR] SOLR-18373: remove NamedList.asShallowMap/get(String,int) and SolrParams.toNamedList [solr]
Posted by "serhiy-bzhezytskyy (via GitHub)" <gi...@apache.org>.
serhiy-bzhezytskyy commented on PR #4761:
URL: https://github.com/apache/solr/pull/4761#issuecomment-5346813765
@dsmiley this removes three things you deprecated (`NamedList.get(String,int)`, `asShallowMap()`, `SolrParams.toNamedList()`). Worth your eyes specifically: `asShallowMap()` turned out to be a hybrid, not a plain view -- `get`/`put`/`remove` were live against the backing list but `entrySet()`/`keySet()`/`values()` returned copies. You know this class best, so a review here would help more than most.
AI-assisted (Claude Sonnet 5)
--
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
Re: [PR] SOLR-18373: remove NamedList.asShallowMap/get(String,int) and SolrParams.toNamedList [solr]
Posted by "dsmiley (via GitHub)" <gi...@apache.org>.
dsmiley commented on code in PR #4761:
URL: https://github.com/apache/solr/pull/4761#discussion_r3853295410
##########
solr/core/src/java/org/apache/solr/handler/component/QueryComponent.java:
##########
@@ -1282,6 +1280,11 @@ protected void mergeIds(ResponseBuilder rb, ShardRequest sreq) {
}
}
+ protected static void updateResponseHeader(NamedList<Object> header, String key, Object value) {
+ header.remove(key);
+ header.add(key, value);
+ }
+
Review 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).
--
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
Re: [PR] SOLR-18373: remove NamedList.asShallowMap/get(String,int) and SolrParams.toNamedList [solr]
Posted by "dsmiley (via GitHub)" <gi...@apache.org>.
dsmiley commented on code in PR #4761:
URL: https://github.com/apache/solr/pull/4761#discussion_r3839212582
##########
solr/core/src/java/org/apache/solr/jersey/SolrJacksonMapper.java:
##########
@@ -70,7 +70,8 @@ public NamedListSerializer(Class<NamedList> nlClazz) {
@Override
public void serialize(NamedList value, JsonGenerator gen, SerializerProvider provider)
throws IOException {
- gen.writeObject(value.asShallowMap());
+ // Not SimpleOrderedMap: it IS a NamedList, so this serializer would recurse on it.
+ gen.writeObject(value.asMap(0));
Review Comment:
If we can't resolve this right now, the comment should recognize this sad situation so a future reader sees the opportunity / issue.
--
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
Re: [PR] SOLR-18373: remove NamedList.asShallowMap/get(String,int) and SolrParams.toNamedList [solr]
Posted by "serhiy-bzhezytskyy (via GitHub)" <gi...@apache.org>.
serhiy-bzhezytskyy commented on code in PR #4761:
URL: https://github.com/apache/solr/pull/4761#discussion_r3838129869
##########
solr/core/src/java/org/apache/solr/handler/component/QueryComponent.java:
##########
@@ -1237,10 +1237,13 @@ protected void mergeIds(ResponseBuilder rb, ShardRequest sreq) {
populateNextCursorMarkFromMergedShards(rb);
if (thereArePartialResults) {
- rb.rsp
- .getResponseHeader()
- .asShallowMap()
- .put(SolrQueryResponse.RESPONSE_HEADER_PARTIAL_RESULTS_KEY, Boolean.TRUE);
+ NamedList<Object> header = rb.rsp.getResponseHeader();
Review Comment:
Replaced with `((SimpleOrderedMap<Object>) rb.rsp.getResponseHeader()).put(...)`. `getResponseHeader()` is always a `SimpleOrderedMap` at runtime, and its `put()` already does indexOf+setVal-or-add in one call, so this collapses to a single statement without reordering the entry.
--
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
Re: [PR] SOLR-18373: remove NamedList.asShallowMap/get(String,int) and SolrParams.toNamedList [solr]
Posted by "dsmiley (via GitHub)" <gi...@apache.org>.
dsmiley commented on PR #4761:
URL: https://github.com/apache/solr/pull/4761#issuecomment-5387626232
note: precommit failed but for a reason I believe that should go away if you sync from main.
--
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
Re: [PR] SOLR-18373: remove NamedList.asShallowMap/get(String,int) and SolrParams.toNamedList [solr]
Posted by "dsmiley (via GitHub)" <gi...@apache.org>.
dsmiley commented on code in PR #4761:
URL: https://github.com/apache/solr/pull/4761#discussion_r3844232992
##########
solr/core/src/java/org/apache/solr/handler/component/QueryComponent.java:
##########
@@ -1282,6 +1280,11 @@ protected void mergeIds(ResponseBuilder rb, ShardRequest sreq) {
}
}
+ protected static void updateResponseHeader(NamedList<Object> header, String key, Object value) {
+ header.remove(key);
+ header.add(key, value);
+ }
+
Review Comment:
Why did you do this? I much prefer it as it was (assuming it worked)
##########
solr/core/src/test/org/apache/solr/jersey/SolrJacksonMapperTest.java:
##########
Review Comment:
nice
--
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