Copilot commented on code in PR #4463:
URL: https://github.com/apache/solr/pull/4463#discussion_r3298387895
##########
solr/core/src/test/org/apache/solr/handler/designer/TestSchemaDesignerAPI.java:
##########
@@ -845,7 +844,7 @@ public void testSchemaDiffEndpoint() throws Exception {
idFieldMapUpdated.indexOf("omitTermFreqAndPositions", 0),
Boolean.FALSE);
SolrParams solrParams = idFieldMapUpdated.toSolrParams();
- Map<String, Object> mapParams = solrParams.toMap(new HashMap<>());
+ Map<String, Object> mapParams = new SimpleOrderedMap<>(solrParams);
Review Comment:
`SimpleOrderedMap` is a `NamedList`-style structure (not a `java.util.Map`),
so assigning it to `Map<String,Object>` is very likely to fail compilation or
change the runtime type in ways the test doesn't intend. If you need a real
`Map`, keep using `solrParams.toMap(new HashMap<>())` or switch to
`Utils.convertToMap(solrParams, new HashMap<>())` (whichever is appropriate for
`SolrParams`).
##########
solr/core/src/java/org/apache/solr/handler/admin/IndexSizeEstimator.java:
##########
@@ -277,12 +278,10 @@ private void convert(Map<String, Object> result) {
for (Map.Entry<String, Object> entry : result.entrySet()) {
Object value = entry.getValue();
if (value instanceof ItemPriorityQueue queue) {
- Map<String, Object> map = new LinkedHashMap<>();
- queue.toMap(map);
+ Map<String, Object> map = new SimpleOrderedMap<>(queue);
entry.setValue(map);
} else if (value instanceof MapWriterSummaryStatistics stats) {
- Map<String, Object> map = new LinkedHashMap<>();
- stats.toMap(map);
+ Map<String, Object> map = new SimpleOrderedMap<>(stats);
Review Comment:
Both `map` variables are declared as `Map<String,Object>` but are
initialized with `SimpleOrderedMap`, which is a `NamedList`-like type rather
than a `Map`. If callers truly need a `Map`, materialize one (e.g., via
`Utils.convertToMap(...)` into a `LinkedHashMap`) or change the variable/type
expectations to `SimpleOrderedMap`/`NamedList` consistently.
##########
solr/core/src/java/org/apache/solr/handler/admin/api/OverseerOperationAPI.java:
##########
@@ -59,7 +59,7 @@ public OverseerOperationAPI(CoreAdminHandler
coreAdminHandler) {
@Command(name = OVERSEER_OP_CMD)
public void joinOverseerLeaderElection(PayloadObj<OverseerOperationPayload>
payload)
throws Exception {
- final Map<String, Object> v1Params = payload.get().toMap(new HashMap<>());
+ final Map<String, Object> v1Params = new SimpleOrderedMap<>(payload.get());
Review Comment:
Here `v1Params` is declared as `Map<String,Object>` but is initialized with
`new SimpleOrderedMap<>(...)`, which is not a `Map` and can break compilation
or downstream code expecting a `Map` (e.g., param wrapping/serialization).
Prefer producing an actual `Map` via `Utils.convertToMap(payload.get(), new
HashMap<>())` (or keep the prior `toMap(...)` approach) unless the rest of the
call chain is explicitly designed to accept `NamedList`/`SimpleOrderedMap`.
##########
solr/core/src/java/org/apache/solr/handler/SolrConfigHandler.java:
##########
@@ -283,16 +284,16 @@ private void handleGET() {
Map pluginNameVsPluginInfo = (Map) val.get(parts.get(1));
if (pluginNameVsPluginInfo != null) {
Object o =
- pluginNameVsPluginInfo instanceof MapSerializable
+ pluginNameVsPluginInfo instanceof MapWriter
? pluginNameVsPluginInfo
: pluginNameVsPluginInfo.get(componentName);
Map<String, Object> pluginInfo =
- o instanceof MapSerializable
- ? ((MapSerializable) o).toMap(new LinkedHashMap<>())
+ o instanceof MapWriter
+ ? new SimpleOrderedMap<>((MapWriter) o)
Review Comment:
This introduces multiple places where `SimpleOrderedMap` instances are being
treated as `Map<String,Object>` values (either by assignment or by embedding
into `Map.of(...)`). If the response building/JSON serialization path expects
actual `Map` instances, this can change output shape or fail at runtime.
Consider converting `MapWriter`/`PluginInfo`/`SolrConfig` to a concrete `Map`
(e.g., via `Utils.convertToMap(..., new LinkedHashMap<>())`) in the spots where
the surrounding code’s static types are `Map`.
##########
solr/core/src/java/org/apache/solr/handler/SolrConfigHandler.java:
##########
@@ -356,7 +367,7 @@ private Map<String, Object>
expandUseParams(SolrQueryRequest req, Object plugin)
if (plugin instanceof Map) {
pluginInfo = (Map) plugin;
} else if (plugin instanceof PluginInfo) {
- pluginInfo = ((PluginInfo) plugin).toMap(new LinkedHashMap<>());
+ pluginInfo = new SimpleOrderedMap<>((PluginInfo) plugin);
Review Comment:
This introduces multiple places where `SimpleOrderedMap` instances are being
treated as `Map<String,Object>` values (either by assignment or by embedding
into `Map.of(...)`). If the response building/JSON serialization path expects
actual `Map` instances, this can change output shape or fail at runtime.
Consider converting `MapWriter`/`PluginInfo`/`SolrConfig` to a concrete `Map`
(e.g., via `Utils.convertToMap(..., new LinkedHashMap<>())`) in the spots where
the surrounding code’s static types are `Map`.
##########
solr/core/src/java/org/apache/solr/handler/SolrConfigHandler.java:
##########
@@ -320,10 +323,18 @@ private void handleGET() {
}
}
+ @SuppressWarnings({"unchecked"})
+ private Map<String, Object> getComponentInfo(
+ Map<String, Object> pluginInfo, String componentName) {
+ return pluginInfo.containsKey("class")
+ ? pluginInfo
+ : (Map<String, Object>) pluginInfo.getOrDefault(componentName, new
HashMap<>());
+ }
+
private Map<String, Object> getConfigDetails(String componentType,
SolrQueryRequest req) {
String componentName = componentType == null ? null :
req.getParams().get("componentName");
boolean showParams = req.getParams().getBool("expandParams", false);
- Map<String, Object> map = this.req.getCore().getSolrConfig().toMap(new
LinkedHashMap<>());
+ Map<String, Object> map = new
SimpleOrderedMap<>(this.req.getCore().getSolrConfig());
Review Comment:
This introduces multiple places where `SimpleOrderedMap` instances are being
treated as `Map<String,Object>` values (either by assignment or by embedding
into `Map.of(...)`). If the response building/JSON serialization path expects
actual `Map` instances, this can change output shape or fail at runtime.
Consider converting `MapWriter`/`PluginInfo`/`SolrConfig` to a concrete `Map`
(e.g., via `Utils.convertToMap(..., new LinkedHashMap<>())`) in the spots where
the surrounding code’s static types are `Map`.
##########
solr/core/src/java/org/apache/solr/handler/SolrConfigHandler.java:
##########
@@ -320,10 +323,18 @@ private void handleGET() {
}
}
+ @SuppressWarnings({"unchecked"})
+ private Map<String, Object> getComponentInfo(
+ Map<String, Object> pluginInfo, String componentName) {
+ return pluginInfo.containsKey("class")
+ ? pluginInfo
+ : (Map<String, Object>) pluginInfo.getOrDefault(componentName, new
HashMap<>());
Review Comment:
Using `getOrDefault(componentName, new HashMap<>())` returns a new ephemeral
map when the key is missing; subsequent writes to the returned map (e.g.,
adding `_packageinfo_`) won’t be reflected in `pluginInfo`. If the intent is to
ensure the nested component map exists and is updated, use a storing strategy
(e.g., `computeIfAbsent`) or explicitly `put` the newly created map into
`pluginInfo` before returning it.
##########
solr/core/src/java/org/apache/solr/handler/SolrConfigHandler.java:
##########
@@ -204,7 +205,7 @@ private void handleGET() {
Map<String, Object> m = new LinkedHashMap<>();
m.put(ZNODEVER, params.getZnodeVersion());
if (p != null) {
- m.put(RequestParams.NAME, Map.of(parts.get(2), p.toMap(new
LinkedHashMap<>())));
+ m.put(RequestParams.NAME, Map.of(parts.get(2), new
SimpleOrderedMap<>(p)));
Review Comment:
This introduces multiple places where `SimpleOrderedMap` instances are being
treated as `Map<String,Object>` values (either by assignment or by embedding
into `Map.of(...)`). If the response building/JSON serialization path expects
actual `Map` instances, this can change output shape or fail at runtime.
Consider converting `MapWriter`/`PluginInfo`/`SolrConfig` to a concrete `Map`
(e.g., via `Utils.convertToMap(..., new LinkedHashMap<>())`) in the spots where
the surrounding code’s static types are `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: [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]
