gerlowskija commented on code in PR #4775:
URL: https://github.com/apache/solr/pull/4775#discussion_r3863525990
##########
changelog/unreleased/SOLR-16458-migrate-node-properties-api.yml:
##########
@@ -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)"
Review Comment:
[-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.
##########
solr/api/src/java/org/apache/solr/client/api/model/NodePropertiesResponse.java:
##########
@@ -0,0 +1,34 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one or more
+ * contributor license agreements. See the NOTICE file distributed with
+ * this work for additional information regarding copyright ownership.
+ * The ASF licenses this file to You under the Apache License, Version 2.0
+ * (the "License"); you may not use this file except in compliance with
+ * the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an "AS IS" BASIS,
+ * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ * See the License for the specific language governing permissions and
+ * limitations under the License.
+ */
+package org.apache.solr.client.api.model;
+
+import com.fasterxml.jackson.annotation.JsonProperty;
+import io.swagger.v3.oas.annotations.media.Schema;
+import java.util.Map;
+
+/**
+ * Response body for {@code GET /api/node/properties} and {@code GET
+ * /api/node/properties/{propertyName}}.
+ */
+public class NodePropertiesResponse extends SolrJerseyResponse {
+
+ public static final String SYSTEM_PROPERTIES = "system.properties";
Review Comment:
[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...
##########
solr/core/src/java/org/apache/solr/handler/admin/PropertiesRequestHandler.java:
##########
@@ -51,21 +55,13 @@ public PropertiesRequestHandler(CoreContainer cc) {
@Override
public void handleRequestBody(SolrQueryRequest req, SolrQueryResponse rsp)
throws IOException {
- NamedList<String> props = new SimpleOrderedMap<>();
- String name = req.getParams().get(NAME);
- NodeConfig nodeConfig = getCoreContainer(req).getNodeConfig();
- if (name != null) {
- String property = nodeConfig.getRedactedSysPropValue(name);
- props.add(name, property);
- } else {
- Enumeration<?> enumeration = System.getProperties().propertyNames();
- while (enumeration.hasMoreElements()) {
- name = (String) enumeration.nextElement();
- props.add(name, nodeConfig.getRedactedSysPropValue(name));
- }
- }
- rsp.add("system.properties", props);
rsp.setHttpCaching(false);
+ String name = req.getParams().get(NAME);
+ Map<String, String> props =
+ new GetNodeProperties(getCoreContainer(req)).collectProperties(name);
Review Comment:
[-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.
##########
solr/core/src/java/org/apache/solr/handler/admin/api/GetNodeProperties.java:
##########
@@ -0,0 +1,82 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one or more
+ * contributor license agreements. See the NOTICE file distributed with
+ * this work for additional information regarding copyright ownership.
+ * The ASF licenses this file to You under the Apache License, Version 2.0
+ * (the "License"); you may not use this file except in compliance with
+ * the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an "AS IS" BASIS,
+ * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ * See the License for the specific language governing permissions and
+ * limitations under the License.
+ */
+package org.apache.solr.handler.admin.api;
+
+import jakarta.inject.Inject;
+import java.util.Enumeration;
+import java.util.LinkedHashMap;
+import java.util.Map;
+import org.apache.solr.api.JerseyResource;
+import org.apache.solr.client.api.endpoint.NodePropertiesApi;
+import org.apache.solr.client.api.model.NodePropertiesResponse;
+import org.apache.solr.core.CoreContainer;
+import org.apache.solr.core.NodeConfig;
+import org.apache.solr.jersey.PermissionName;
+import org.apache.solr.security.PermissionNameProvider;
+
+/**
+ * V2 API for listing system properties on the receiving node.
+ *
+ * <p>GET /api/node/properties lists all properties. GET
/api/node/properties/{propertyName} returns
+ * a single property. Both are analogous to v1 /admin/info/properties, which
still uses a {@code
+ * name} query parameter for the single-property form.
+ *
+ * <p>The v1 {@link org.apache.solr.handler.admin.PropertiesRequestHandler}
delegates to this class.
+ */
+public class GetNodeProperties extends JerseyResource implements
NodePropertiesApi {
+
+ private final CoreContainer coreContainer;
+
+ @Inject
+ public GetNodeProperties(CoreContainer coreContainer) {
+ this.coreContainer = coreContainer;
+ }
+
+ @Override
+ @PermissionName(PermissionNameProvider.Name.CONFIG_READ_PERM)
+ public NodePropertiesResponse getNodeProperties() {
+ return buildResponse(null);
+ }
+
+ @Override
+ @PermissionName(PermissionNameProvider.Name.CONFIG_READ_PERM)
+ public NodePropertiesResponse getNodeProperty(String propertyName) {
+ return buildResponse(propertyName);
+ }
+
+ private NodePropertiesResponse buildResponse(String name) {
+ final NodePropertiesResponse response =
instantiateJerseyResponse(NodePropertiesResponse.class);
+ response.systemProperties = collectProperties(name);
+ return response;
Review Comment:
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.
##########
solr/core/src/java/org/apache/solr/handler/admin/api/GetNodeProperties.java:
##########
@@ -0,0 +1,82 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one or more
+ * contributor license agreements. See the NOTICE file distributed with
+ * this work for additional information regarding copyright ownership.
+ * The ASF licenses this file to You under the Apache License, Version 2.0
+ * (the "License"); you may not use this file except in compliance with
+ * the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an "AS IS" BASIS,
+ * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ * See the License for the specific language governing permissions and
+ * limitations under the License.
+ */
+package org.apache.solr.handler.admin.api;
+
+import jakarta.inject.Inject;
+import java.util.Enumeration;
+import java.util.LinkedHashMap;
+import java.util.Map;
+import org.apache.solr.api.JerseyResource;
+import org.apache.solr.client.api.endpoint.NodePropertiesApi;
+import org.apache.solr.client.api.model.NodePropertiesResponse;
+import org.apache.solr.core.CoreContainer;
+import org.apache.solr.core.NodeConfig;
+import org.apache.solr.jersey.PermissionName;
+import org.apache.solr.security.PermissionNameProvider;
+
+/**
+ * V2 API for listing system properties on the receiving node.
+ *
+ * <p>GET /api/node/properties lists all properties. GET
/api/node/properties/{propertyName} returns
+ * a single property. Both are analogous to v1 /admin/info/properties, which
still uses a {@code
+ * name} query parameter for the single-property form.
+ *
+ * <p>The v1 {@link org.apache.solr.handler.admin.PropertiesRequestHandler}
delegates to this class.
+ */
+public class GetNodeProperties extends JerseyResource implements
NodePropertiesApi {
+
+ private final CoreContainer coreContainer;
+
+ @Inject
+ public GetNodeProperties(CoreContainer coreContainer) {
+ this.coreContainer = coreContainer;
+ }
+
+ @Override
+ @PermissionName(PermissionNameProvider.Name.CONFIG_READ_PERM)
+ public NodePropertiesResponse getNodeProperties() {
+ return buildResponse(null);
+ }
+
+ @Override
+ @PermissionName(PermissionNameProvider.Name.CONFIG_READ_PERM)
+ public NodePropertiesResponse getNodeProperty(String propertyName) {
+ return buildResponse(propertyName);
+ }
+
+ private NodePropertiesResponse buildResponse(String name) {
+ final NodePropertiesResponse response =
instantiateJerseyResponse(NodePropertiesResponse.class);
+ response.systemProperties = collectProperties(name);
+ return response;
Review Comment:
> 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 🤷
##########
solr/core/src/test/org/apache/solr/handler/admin/api/GetNodePropertiesTest.java:
##########
@@ -0,0 +1,107 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one or more
+ * contributor license agreements. See the NOTICE file distributed with
+ * this work for additional information regarding copyright ownership.
+ * The ASF licenses this file to You under the Apache License, Version 2.0
+ * (the "License"); you may not use this file except in compliance with
+ * the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an "AS IS" BASIS,
+ * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ * See the License for the specific language governing permissions and
+ * limitations under the License.
+ */
+package org.apache.solr.handler.admin.api;
+
+import org.apache.solr.SolrTestCase;
+import org.apache.solr.client.api.model.NodePropertiesResponse;
+import org.apache.solr.client.solrj.RemoteSolrException;
+import org.apache.solr.client.solrj.request.NodeApi;
+import org.apache.solr.core.NodeConfig;
+import org.apache.solr.util.SolrJettyTestRule;
+import org.junit.After;
+import org.junit.BeforeClass;
+import org.junit.ClassRule;
+import org.junit.Test;
+
+/**
+ * HTTP tests for {@code GET /api/node/properties} and {@code GET
+ * /api/node/properties/{propertyName}} via the generated SolrJ client classes.
+ */
+public class GetNodePropertiesTest extends SolrTestCase {
+
+ private static final String VISIBLE_PROP = "GetNodePropertiesTest.visible";
+ private static final String SECRET_PROP = "GetNodePropertiesTest.password";
+ private static final String PASSWORD = "secret123";
+
+ @ClassRule public static final SolrJettyTestRule solrTestRule = new
SolrJettyTestRule();
+
+ @BeforeClass
+ public static void setupSolr() throws Exception {
+ solrTestRule.startSolr(createTempDir());
+ }
+
+ @After
+ public void clearTestProperties() {
+ System.clearProperty(VISIBLE_PROP);
+ System.clearProperty(SECRET_PROP);
+ }
+
+ @Test
+ public void testNamedProperty() throws Exception {
+ var req = new NodeApi.GetNodeProperty("java.version");
+ var rsp = req.process(solrTestRule.getAdminClient());
+
+ assertNotNull(rsp);
+ assertNull(rsp.error);
+ assertEquals(1, rsp.systemProperties.size());
+ assertEquals(System.getProperty("java.version"),
rsp.systemProperties.get("java.version"));
+ }
+
+ @Test
+ public void testAllProperties() throws Exception {
+ System.setProperty(VISIBLE_PROP, "hello");
+
+ NodePropertiesResponse rsp = fetchProperties(null);
+
+ assertTrue("expected more than one system property",
rsp.systemProperties.size() > 1);
Review Comment:
[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()`
##########
solr/core/src/java/org/apache/solr/handler/admin/api/GetNodeProperties.java:
##########
@@ -0,0 +1,90 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one or more
+ * contributor license agreements. See the NOTICE file distributed with
+ * this work for additional information regarding copyright ownership.
+ * The ASF licenses this file to You under the Apache License, Version 2.0
+ * (the "License"); you may not use this file except in compliance with
+ * the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an "AS IS" BASIS,
+ * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ * See the License for the specific language governing permissions and
+ * limitations under the License.
+ */
+package org.apache.solr.handler.admin.api;
+
+import jakarta.inject.Inject;
+import java.util.Enumeration;
+import java.util.LinkedHashMap;
+import java.util.Map;
+import org.apache.solr.api.JerseyResource;
+import org.apache.solr.client.api.endpoint.NodePropertiesApi;
+import org.apache.solr.client.api.model.NodePropertiesResponse;
+import org.apache.solr.common.SolrException;
+import org.apache.solr.core.CoreContainer;
+import org.apache.solr.core.NodeConfig;
+import org.apache.solr.jersey.PermissionName;
+import org.apache.solr.security.PermissionNameProvider;
+
+/**
+ * V2 API for listing system properties on the receiving node.
+ *
+ * <p>GET /api/node/properties lists all properties. GET
/api/node/properties/{propertyName} returns
+ * a single property. Both are analogous to v1 /admin/info/properties, which
still uses a {@code
+ * name} query parameter for the single-property form.
+ *
+ * <p>The v1 {@link org.apache.solr.handler.admin.PropertiesRequestHandler}
delegates to this class.
+ */
+public class GetNodeProperties extends JerseyResource implements
NodePropertiesApi {
+
+ private final CoreContainer coreContainer;
+
+ @Inject
+ public GetNodeProperties(CoreContainer coreContainer) {
+ this.coreContainer = coreContainer;
+ }
+
+ @Override
+ @PermissionName(PermissionNameProvider.Name.CONFIG_READ_PERM)
+ public NodePropertiesResponse getNodeProperties() {
+ return buildResponse(null);
+ }
+
+ @Override
+ @PermissionName(PermissionNameProvider.Name.CONFIG_READ_PERM)
+ public NodePropertiesResponse getNodeProperty(String propertyName) {
+ final NodeConfig nodeConfig = coreContainer.getNodeConfig();
+ if (!System.getProperties().containsKey(propertyName)
+ && !nodeConfig.isSysPropHidden(propertyName)) {
Review Comment:
[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...
##########
solr/core/src/java/org/apache/solr/handler/admin/api/GetNodeProperties.java:
##########
@@ -0,0 +1,90 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one or more
+ * contributor license agreements. See the NOTICE file distributed with
+ * this work for additional information regarding copyright ownership.
+ * The ASF licenses this file to You under the Apache License, Version 2.0
+ * (the "License"); you may not use this file except in compliance with
+ * the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an "AS IS" BASIS,
+ * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ * See the License for the specific language governing permissions and
+ * limitations under the License.
+ */
+package org.apache.solr.handler.admin.api;
+
+import jakarta.inject.Inject;
+import java.util.Enumeration;
+import java.util.LinkedHashMap;
+import java.util.Map;
+import org.apache.solr.api.JerseyResource;
+import org.apache.solr.client.api.endpoint.NodePropertiesApi;
+import org.apache.solr.client.api.model.NodePropertiesResponse;
+import org.apache.solr.common.SolrException;
+import org.apache.solr.core.CoreContainer;
+import org.apache.solr.core.NodeConfig;
+import org.apache.solr.jersey.PermissionName;
+import org.apache.solr.security.PermissionNameProvider;
+
+/**
+ * V2 API for listing system properties on the receiving node.
+ *
+ * <p>GET /api/node/properties lists all properties. GET
/api/node/properties/{propertyName} returns
+ * a single property. Both are analogous to v1 /admin/info/properties, which
still uses a {@code
+ * name} query parameter for the single-property form.
+ *
+ * <p>The v1 {@link org.apache.solr.handler.admin.PropertiesRequestHandler}
delegates to this class.
+ */
+public class GetNodeProperties extends JerseyResource implements
NodePropertiesApi {
+
+ private final CoreContainer coreContainer;
+
+ @Inject
+ public GetNodeProperties(CoreContainer coreContainer) {
+ this.coreContainer = coreContainer;
+ }
+
+ @Override
+ @PermissionName(PermissionNameProvider.Name.CONFIG_READ_PERM)
+ public NodePropertiesResponse getNodeProperties() {
+ return buildResponse(null);
+ }
+
+ @Override
+ @PermissionName(PermissionNameProvider.Name.CONFIG_READ_PERM)
+ public NodePropertiesResponse getNodeProperty(String propertyName) {
+ final NodeConfig nodeConfig = coreContainer.getNodeConfig();
+ if (!System.getProperties().containsKey(propertyName)
+ && !nodeConfig.isSysPropHidden(propertyName)) {
+ throw new SolrException(
+ SolrException.ErrorCode.NOT_FOUND,
+ "No system property named '" + propertyName + "' exists on this
node.");
+ }
+ return buildResponse(propertyName);
+ }
+
+ private NodePropertiesResponse buildResponse(String name) {
+ final NodePropertiesResponse response =
instantiateJerseyResponse(NodePropertiesResponse.class);
Review Comment:
[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
🤷
##########
solr/solr-ref-guide/modules/deployment-guide/pages/jvm-settings.adoc:
##########
@@ -78,6 +78,10 @@ Many of the system environment variables include Java
settings, and these can be
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).
Review Comment:
L72 above mentiones /admin/info/system as a way to get related JVM
information. Maybe this mention is a better fit for up there, instead of in
this section focused mainly on the Admin UI screen?
--
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]
