SOLR-16458: Migrate node properties API to JAX-RS by iprithv · Pull Request #4775 · apache/solr

iprithv · 2026-08-20T23:04:36Z

Converts GET /api/node/properties from the homegrown @EndPoint wrapper to JAX-RS, matching the existing node health / logging / system APIs.

JAX-RS interface and typed NodePropertiesResponse in solr/api
GetNodeProperties owns redaction / collection logic
v1 /admin/info/properties delegates to that class; response key system.properties is unchanged
SolrJ generates NodeApi.GetNodeProperties
HTTP coverage via SolrJettyTestRule for named lookup, full list, and hidden-property redaction

https://issues.apache.org/jira/browse/SOLR-16458

epugh

one qeustion about how we get a single property format.. I think we are getting away in the v2 api from a name=myproperty parameter and using a path to be more restful? Also, do we need a Ref Guide update? That often get's missed.

epugh · 2026-08-20T23:12:06Z

+
+  @Schema(description = "JRE system properties for the Solr node. Secret values are redacted.")
+  @JsonProperty(SYSTEM_PROPERTIES)
+  public Map<String, String> systemProperties;


is it really as simple as a string/string? Great.

Copilot

Pull request overview

Migrates the node-properties API to JAX-RS with typed responses and SolrJ support.

Changes:

Adds JAX-RS endpoints and response model.
Delegates v1 property collection and redaction to the new implementation.
Adds HTTP tests and user documentation.

Reviewed changes

Copilot reviewed 12 out of 12 changed files in this pull request and generated 2 comments.

Show a summary per file

File	Description
`jvm-settings.adoc`	Documents node-properties endpoints.
`implicit-requesthandlers.adoc`	Updates handler and API references.
`PropertiesRequestHandlerTest.java`	Tests v1 named-property lookup.
`InfoHandlerTest.java`	Updates overridden exception signature.
`V2NodeAPIMappingTest.java`	Removes legacy endpoint mapping tests.
`GetNodePropertiesTest.java`	Adds v2 HTTP coverage.
`PropertiesRequestHandler.java`	Delegates v1 requests to JAX-RS logic.
`NodePropertiesAPI.java`	Removes the legacy v2 adapter.
`GetNodeProperties.java`	Implements property retrieval and redaction.
`NodePropertiesResponse.java`	Defines the typed response model.
`NodePropertiesApi.java`	Defines the JAX-RS API contract.
`SOLR-16458-migrate-node-properties-api.yml`	Adds the changelog entry.

💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.

epugh · 2026-08-25T23:45:27Z

+  private NodePropertiesResponse buildResponse(String name) {
+    final NodePropertiesResponse response = instantiateJerseyResponse(NodePropertiesResponse.class);
+    response.systemProperties = collectProperties(name);
+    return response;


i think this "disable http caching" is actually a red herring, and we don't need it. @gerlowskija I would be curious if you have any sense that we "need" this caching? Claude, when I dug into it loclaly, didn't think so, and I can't figure out why it would be needed. In fact, curious if GetNodeSystemInfo does...?

This is something that ended up in some of our JAX-RS APIs because someone decided on it at some point on the v1 codepath. For example, see us disabling caching in the CollectionsHandler here.

So in general I think the AI is probably right here that giving hints about cacheability is valid and appropriate. Whether that should be done in individual APIs or as some sort of "ContainerResponseFilter" (see PostRequestLoggingFilter), I'm not really sure.

Interesting, so I think I read things backwards... I thought it was enforcing caching (when I talked to you @gerlowskija) but the recommendation is to prevent caching. I will open a seperate JIRA for saying "lets look at cahcing for admin v2 apis".. I don't really want to try to one off solve it in this one PR since hoenstly we haven' teven seen this as an issue! So let's solve it indepdent of this one.. also, so I can merge thisn ;-).

If you want to influence the caching in this class, the way to do it is to have the constructor inject a "SolrQueryResponse" object and then call the appropriate caching method on that instance.

I will open a seperate JIRA for saying "lets look at cahcing for admin v2 apis".

IMO we're not really consistent on the v1 side either, so I'd use a broader scope of: "Re-evaluate use of caching headers in all Solr APIs". We may decide to rip them out across the board, who knows 🤷

thank you, I poked at this a bit more, GetNodeSystemInfo does call setHttpCaching(false), but it doesn’t actually do anything.. that flag only turns into Cache-Control headers on the core request path, not admin/node APIs. same for CollectionsHandler. so we aren’t dropping any headers that used to be there.

If we do want real no-cache on admin GETs, I think a filter or handleAdminRequest would be a better place for it than this API.. happy to leave that for a follow-up unless you’d rather we do it here?

https://issues.apache.org/jira/browse/SOLR-18404

epugh · 2026-08-25T23:47:42Z

I am going to merge this tomorrow pending the build completing. I think it's ready...

gerlowskija

Left a few comments in line, only 1 really needs addressed though (the PropertiesRequestHandler one).

This is awesome work!

gerlowskija · 2026-08-26T14:04:21Z

@@ -0,0 +1,8 @@
+title: "v2 GET /api/node/properties is now a JAX-RS API; a single property is fetched at /api/node/properties/{propertyName} (SolrJ: NodeApi.GetNodeProperties / GetNodeProperty)"


[-0] users probably don't care what framework the API is implemented in. What they likely care about is that (1) the form of the API has changed and (2) by virtue of being in our OpenAPI spec Solr now generates a handy SolrRequest/SolrResponse class for these APIs.

I'd recommend rewording the changelog entry to highlight those aspects, which a user is more likely to care about.

understood, changed it to mention the new path and the SolrJ classes, not JAX-RS :)

gerlowskija · 2026-08-26T14:09:49Z

+ */
+public class NodePropertiesResponse extends SolrJerseyResponse {
+
+  public static final String SYSTEM_PROPERTIES = "system.properties";


[Q] Does this field name pre-exist this PR? In general we prefer camelCase, so if this is "new" then we should standardize on that. But if it's pre-existing,let's not worry about it...

yes, v1 already used system.properties (Admin UI, NodeValueFetcher, etc.)

so, now is a good time to make these changes... how hard would it be to keep system.properties in v1, but have systemProperties in v2? And, are there a whole bunch of other weridly named fields like this? Or is this the only one?

gerlowskija · 2026-08-26T14:15:01Z

+  public NodePropertiesResponse getNodeProperty(String propertyName) {
+    final NodeConfig nodeConfig = coreContainer.getNodeConfig();
+    if (!System.getProperties().containsKey(propertyName)
+        && !nodeConfig.isSysPropHidden(propertyName)) {


[Q] Why does the redacted-ness factor into whether we throw a 404 here or not? Might be missing it, but I don't see this logic in the original API...

404 + hidden, original API never 404’d. On the v2 path we 404 unknown names, but hidden names always return --REDACTED-- so you can’t tell whether a secret is set. I added a comment and a test for that.

gerlowskija · 2026-08-26T14:23:33Z

+  }
+
+  private NodePropertiesResponse buildResponse(String name) {
+    final NodePropertiesResponse response = instantiateJerseyResponse(NodePropertiesResponse.class);


[0] It's nbd but I don't think we strictly need instantiateJerseyResponse here. That's primarily useful on APIs with many sub-steps where we may want the response to contain information about both the steps that succeeded and those that failed (think collection creation)

Having it here won't do any harm though afaik, so purely a preference thing 🤷

left it for now, mostly to match the other node APIs. happy to drop it if you’d rather :)

gerlowskija · 2026-08-26T14:24:24Z

+  private NodePropertiesResponse buildResponse(String name) {
+    final NodePropertiesResponse response = instantiateJerseyResponse(NodePropertiesResponse.class);
+    response.systemProperties = collectProperties(name);
+    return response;


If you want to influence the caching in this class, the way to do it is to have the constructor inject a "SolrQueryResponse" object and then call the appropriate caching method on that instance.

gerlowskija · 2026-08-26T14:25:31Z

+  private NodePropertiesResponse buildResponse(String name) {
+    final NodePropertiesResponse response = instantiateJerseyResponse(NodePropertiesResponse.class);
+    response.systemProperties = collectProperties(name);
+    return response;


I will open a seperate JIRA for saying "lets look at cahcing for admin v2 apis".

IMO we're not really consistent on the v1 side either, so I'd use a broader scope of: "Re-evaluate use of caching headers in all Solr APIs". We may decide to rip them out across the board, who knows 🤷

gerlowskija · 2026-08-26T14:28:12Z

    rsp.setHttpCaching(false);
+    String name = req.getParams().get(NAME);
+    Map<String, String> props =
+        new GetNodeProperties(getCoreContainer(req)).collectProperties(name);


[-1] Unless I'm missing something, you shouldn't need this. See V2ApiUtils's "squash" methods instead - they're already built specifically to convert between our typed POJOs and the "NamedList" that v1 APIs unfortunately require.

gerlowskija · 2026-08-26T14:30:07Z

+
+    NodePropertiesResponse rsp = fetchProperties(null);
+
+    assertTrue("expected more than one system property", rsp.systemProperties.size() > 1);


[0] Feel free to ignore, but IMO assertEquals should be used where possible. Even when you give assertTrue a custom message, like you've done here, it loses some information that assertEqual would print out (e.g. the actual value of systemProperties.size()

switched it to assertEquals on the property count :)

gerlowskija · 2026-08-26T14:39:50Z


 The Java Properties screen, however, provides easy access to all the properties of the JVM running Solr, including the classpaths, file encodings, JVM memory settings, operating system, and more.

+The same information is available from the Node Properties API: `GET /solr/admin/info/properties` (v1) or `GET /api/node/properties` (v2).


L72 above mentions /admin/info/system as a way to get related JVM information. Maybe this content would fit in a bit better up there, instead of in this section focused mainly on the Admin UI screen?

moved the API note up next to /admin/info/system. thanks!

Signed-off-by: prithvi <prithvisivasankar@gmail.com>

Migrate node properties API to JAX-RS

f73ea38

github-actions Bot added tests cat:api labels Aug 20, 2026

epugh reviewed Aug 20, 2026

View reviewed changes

epugh self-assigned this Aug 20, 2026

review chnages

6cad9ea

github-actions Bot added the documentation Improvements or additions to documentation label Aug 21, 2026

iprithv requested a review from epugh August 21, 2026 16:55

epugh requested a balanced review from Copilot August 25, 2026 21:23

Copilot started reviewing on behalf of epugh August 25, 2026 21:24 View session

Merge remote-tracking branch 'upstream/main' into pr/4775

5f24087

Copilot AI reviewed Aug 25, 2026

View reviewed changes

epugh added 3 commits August 25, 2026 17:40

handle if a unknown property is requsested

9a8a7f2

narrow the excpetion

96ecc06

Merge remote-tracking branch 'upstream/main' into pr/4775

d76f265

epugh requested review from gerlowskija and removed request for epugh August 25, 2026 23:47

lint check fix

fa4aeb5

gerlowskija requested changes Aug 26, 2026

View reviewed changes

review changes

b7af094

Signed-off-by: prithvi <prithvisivasankar@gmail.com>

iprithv requested a review from gerlowskija August 26, 2026 18:00

		@@ -0,0 +1,8 @@
		title: "v2 GET /api/node/properties is now a JAX-RS API; a single property is fetched at /api/node/properties/{propertyName} (SolrJ: NodeApi.GetNodeProperties / GetNodeProperty)"


		NodePropertiesResponse rsp = fetchProperties(null);

		assertTrue("expected more than one system property", rsp.systemProperties.size() > 1);


		The Java Properties screen, however, provides easy access to all the properties of the JVM running Solr, including the classpaths, file encodings, JVM memory settings, operating system, and more.

		The same information is available from the Node Properties API: `GET /solr/admin/info/properties` (v1) or `GET /api/node/properties` (v2).

Uh oh!

Conversation

iprithv commented Aug 20, 2026

Uh oh!

epugh left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Pull request overview

Reviewed changes

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

epugh commented Aug 25, 2026

Uh oh!

gerlowskija left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

gerlowskija Aug 26, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

gerlowskija Aug 26, 2026 •

edited

Loading