Copilot commented on code in PR #4464:
URL: https://github.com/apache/solr/pull/4464#discussion_r3298390879
##########
solr/core/src/java/org/apache/solr/core/PluginInfo.java:
##########
@@ -192,27 +194,19 @@ public PluginInfo getChild(String type) {
}
@Override
- @SuppressWarnings({"unchecked", "rawtypes"})
- public Map<String, Object> toMap(Map<String, Object> map) {
- map.putAll(attributes);
- Map m = map;
- if (initArgs != null) m.putAll(initArgs.asMap(3));
- if (children != null) {
- for (PluginInfo child : children) {
- Object old = m.get(child.name);
- if (old == null) {
- m.put(child.name, child.toMap(new LinkedHashMap<>()));
- } else if (old instanceof List list) {
- list.add(child.toMap(new LinkedHashMap<>()));
- } else {
- ArrayList l = new ArrayList();
- l.add(old);
- l.add(child.toMap(new LinkedHashMap<>()));
- m.put(child.name, l);
- }
- }
+ @SuppressWarnings("unchecked")
+ public void writeMap(EntryWriter ew) throws IOException {
+ new NamedList<>(attributes).writeMap(ew);
+ if (initArgs != null) {
+ new SimpleOrderedMap<>(initArgs).writeMap(ew);
+ }
+ if (children == null || children.isEmpty()) {
+ return;
+ }
+
+ for (PluginInfo child : children) {
+ child.writeMap(ew);
}
Review Comment:
This `writeMap` implementation does not preserve the previous `toMap(...)`
structure for children. Previously, each child was nested under `child.name`
(and multiple children with the same name were grouped into a `List`). The new
code flattens all child entries directly into the parent writer via
`child.writeMap(ew)`, which can cause key collisions, dropped grouping, and an
incompatible serialized shape. Additionally, `initArgs` previously used
`initArgs.asMap(3)` semantics (which can preserve multi-valued keys by
collecting them), but `new SimpleOrderedMap<>(initArgs).writeMap(ew)` may not
replicate that behavior. Recommended fix: (1) write `attributes` and `initArgs`
using the same multi-value handling as before (e.g., iterate
`initArgs.asMap(3)` and `ew.put(...)`), and (2) group `children` by
`child.name` and `ew.put(childName, childMapOrList)` where each child value is
materialized (e.g., via `Utils.convertToMap(child, new LinkedHashMap<>())`) or
represented as a `MapWriter`
in a list, matching the old output.
##########
solr/core/src/java/org/apache/solr/handler/admin/BaseHandlerApiSupport.java:
##########
@@ -195,26 +196,27 @@ public Map<String, Object> toMap(Map<String, Object>
suppliedMap) {
: map.get(key);
if (o == null) o = pathValues.get(key);
if (o == null && useRequestParams) o = origParams.getParams(key);
+ if (o == null) continue;
// make strings out of as many things as we can now to minimize
differences from
// the standard impls that pass through a
NamedList/SimpleOrderedMap...
Class<?> oClass = o.getClass();
if (oClass.isPrimitive()
|| Number.class.isAssignableFrom(oClass)
|| Character.class.isAssignableFrom(oClass)
|| Boolean.class.isAssignableFrom(oClass)) {
- suppliedMap.put(param, String.valueOf(o));
+ ew.put(param, String.valueOf(o));
} else if (List.class.isAssignableFrom(oClass)
+ && !((List) o).isEmpty()
&& ((List) o).get(0) instanceof String) {
Review Comment:
This changes the previously unconditional `List -> String[]` conversion
behavior: an empty `List<String>` will now fall through to the generic branch
and be written as an empty `List` instead of `new String[0]`. If downstream
consumers rely on the prior invariant that multi-valued params are always
serialized as `String[]`, this will introduce a type mismatch. Suggested fix:
treat an empty list as a `String[]` as well (i.e., allow `list.isEmpty()` to
still convert to `new String[0]`), while still keeping the safer element-type
check for non-empty lists.
--
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]
