SOLR-18318: Fix NPE in GetNodeSystemInfo when nodes param is set

[Yashgoswami-ds]

Description

Fixes an issue where the v2 /api/node/system endpoint throws a NullPointerException when the nodes parameter is provided in SolrCloud mode.

The issue occurs when GetNodeSystemInfo proxies requests to other nodes and tries to store the proxied responses in NodeSystemResponse.remoteNodeData. Since remoteNodeData was not initialized, adding node responses caused an NPE and returned an HTTP 500 error instead of the expected aggregated node information.

Solution

Initialized NodeSystemResponse.remoteNodeData with a LinkedHashMap so that proxied node responses can be safely stored.

Added a new SolrCloud test GetNodeSystemInfoSolrCloudTest that covers the failing scenario by requesting node system information with the nodes parameter and verifies that the response contains the expected node data.

Tests

Ran:

./gradlew :solr:core:test --tests GetNodeSystemInfoSolrCloudTest

[epugh]

triggered CI. any chance you can look at the comment as well?? Be nice to know.

 // TODO The typing here is kindof wonky - can I tighten 'Object' here to be NodeSystemResponse or
  // will Jackson choke on that?

[Yashgoswami-ds]

Thanks for the suggestion. I’ll take a look at the remoteNodeData usage and verify whether we can safely tighten the type to NodeSystemResponse without affecting Jackson serialization. I’ll update it if it is safe.

[epugh]

looks like some precommit faiolures... I believe since this is a bugfix we need a changelog too.

[Yashgoswami-ds]

triggered CI. any chance you can look at the comment as well?? Be nice to know.

 // TODO The typing here is kindof wonky - can I tighten 'Object' here to be NodeSystemResponse or
  // will Jackson choke on that?

Thanks for pointing this out. I looked into the usage of remoteNodeData.

The proxied response that we add in GetNodeSystemInfo is indeed a NodeSystemResponse, so I checked whether we could tighten the type to Map<String, NodeSystemResponse>. However, remoteNodeData is also populated through @JsonAnySetter, where Jackson provides dynamic fields with Object values.

Changing the type would require changing the @JsonAnySetter handling as well, and it could affect the current JSON serialization/deserialization behavior of this response model. Since this PR is focused on fixing the NPE caused by remoteNodeData being uninitialized, I have kept the existing Object type and only initialized the map with a LinkedHashMap so proxied node responses can be safely stored.

Thanks again for the suggestion.

[Yashgoswami-ds]

please review

[Yashgoswami-ds]

Spotless formatting fixes and pushed the updated commit. Waiting for CI results.

[epugh]

Okay, I've manually tested the fix, looks great! Since I can't push to your branch, can you update the TODO comment with the suggested text, and then I'll merge?

[Yashgoswami-ds]

Okay, I've manually tested the fix, looks great! Since I can't push to your branch, can you update the TODO comment with the suggested text, and then I'll merge?

Updated the TODO comment as suggested. Kept the type as Object because @JsonAnySetter also handles dynamic values through this map.

Thanks for the review!

[epugh]

Thanks for this!

[Yashgoswami-ds]

Thanks for this!

Thanks @epugh for the review and guidance! Appreciate you taking the time to look into this.

[epugh]

if you are looking for other places to contribute... Please either look at the list of CLI tools that we want to migrate to Picocli: #3254 or you can look at the list of tasks related to migrating APIs to V2: https://issues.apache.org/jira/browse/SOLR-15781. Also worth joining the dev@solr.apache.org mailing list.

[Yashgoswami-ds]

if you are looking for other places to contribute... Please either look at the list of CLI tools that we want to migrate to Picocli: #3254 or you can look at the list of tasks related to migrating APIs to V2: https://issues.apache.org/jira/browse/SOLR-15781. Also worth joining the dev@solr.apache.org mailing list.

Thanks for the suggestions! I'll definitely look into these areas and pick up another issue to contribute.

I'll also join the dev@solr.apache.org mailing list to stay updated with the community discussions.

Thanks again for the guidance!

[dsmiley]

This should not have passed the precommit check. solr/core/src/test/org/apache/solr/handler/admin/api/GetNodeSystemInfoSolrCloudTest.java is missing an ASF license, which is enforced by RAT which runs on ./gradlew check

[dsmiley]

Looking at the Checks tab, precommit was in a failed state. Did you see it @epugh? We ought to gate PR merging on that particular check. There was another PR you merged a couple days ago that failed similarly -- it was a tidy issue in that case.

[Yashgoswami-ds]

This should not have passed the precommit check. solr/core/src/test/org/apache/solr/handler/admin/api/GetNodeSystemInfoSolrCloudTest.java is missing an ASF license, which is enforced by RAT which runs on ./gradlew check

@dsmiley Thanks for pointing this out. Just to clarify, the missing ASF license header was noticed after the initial CI run, and I added it along with the formatting fixes before the final merge.

I'll make sure to check the full precommit/RAT results more carefully in future contributions so these issues are caught earlier.