[GitHub] [solr] dsmiley commented on a diff in pull request #975: Allow JAX-RS v2 API definition

[GitBox <gi...@apache.org>]

dsmiley commented on code in PR #975:
URL: https://github.com/apache/solr/pull/975#discussion_r952142394


##########
solr/core/src/java/org/apache/solr/api/V2HttpCall.java:
##########
@@ -157,20 +168,16 @@ public void call(SolrQueryRequest req, SolrQueryResponse rsp) {
         origCorename = pathSegments.get(1);
         core = cores.getCore(origCorename);
       }
+
+      // We didn't find a core, so we're either an error or a Jersey 'ADMIN' api
       if (core == null) {
-        log.error(">> path: '{}'", path);
-        if (path.endsWith(CommonParams.INTROSPECT)) {
-          initAdminRequest(path);
-          return;
-        } else {
-          throw new SolrException(
-              SolrException.ErrorCode.NOT_FOUND,
-              "no core retrieved for core name: " + origCorename + ". Path: " + path);
-        }
-      } else {
-        Thread.currentThread().setContextClassLoader(core.getResourceLoader().getClassLoader());
+        // This codepath needs to cover custom plugin Jersey APIs, as well as in-built ones
+        log.info("Hoping this path is covered by the Jersey-admin case: {}", path);

Review Comment:
   I recommend use of "nocommit" for this and anything else that's not supposed to be merged



##########
solr/core/src/java/org/apache/solr/handler/configsets/ListConfigSetsAPI.java:
##########
@@ -16,34 +16,54 @@
  */
 package org.apache.solr.handler.configsets;
 
-import static org.apache.solr.client.solrj.SolrRequest.METHOD.GET;
-import static org.apache.solr.security.PermissionNameProvider.Name.CONFIG_READ_PERM;
-
-import java.util.List;
-import org.apache.solr.api.EndPoint;
-import org.apache.solr.client.solrj.SolrResponse;
-import org.apache.solr.cloud.OverseerSolrResponse;
-import org.apache.solr.common.util.NamedList;
+import org.apache.solr.api.JerseyResource;
 import org.apache.solr.core.CoreContainer;
-import org.apache.solr.request.SolrQueryRequest;
-import org.apache.solr.response.SolrQueryResponse;
+import org.apache.solr.jersey.PermissionName;
+import org.slf4j.Logger;
+import org.slf4j.LoggerFactory;
+
+import javax.inject.Inject;
+import javax.ws.rs.GET;
+import javax.ws.rs.Path;
+import javax.ws.rs.Produces;
+import javax.ws.rs.core.Context;
+import javax.ws.rs.core.HttpHeaders;
+import java.lang.invoke.MethodHandles;
+
+import static org.apache.solr.security.PermissionNameProvider.Name.CONFIG_READ_PERM;
 
 /**
  * V2 API for adding or updating a single file within a configset.
  *
  * <p>This API (GET /v2/cluster/configs) is analogous to the v1 /admin/configs?action=LIST command.
  */
-public class ListConfigSetsAPI extends ConfigSetAPIBase {
+
+@Path("/cluster/configs")

Review Comment:
   I love how this makes it clear how this handler is accessed.  Formerly, you'd have to cross your fingers that it might be documented (it was here but is maybe outdated soon).



##########
solr/core/src/java/org/apache/solr/handler/admin/ConfigSetsHandler.java:
##########
@@ -75,6 +77,18 @@ public static boolean isAutoGeneratedConfigSet(String configName) {
     return configName != null && configName.endsWith(AUTOCREATED_CONFIGSET_SUFFIX);
   }
 
+  private void squashIntoSolrResponse(SolrQueryResponse rsp, ReflectMapWriter mw) {
+    Map<String, Object> myMap = new HashMap<>();
+    myMap = mw.toMap(myMap);
+    if (myMap.isEmpty()) {
+      log.info("Hmm, map is empty after writing in values from {}", mw);
+    }
+    for (String key : myMap.keySet()) {
+      log.info("Adding key={}, value={} to rsp", key, myMap.get(key));

Review Comment:
   Good feedback



##########
solr/core/src/java/org/apache/solr/handler/configsets/ListConfigsetsResponse.java:
##########
@@ -0,0 +1,32 @@
+/*
+ * 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.configsets;
+
+import com.fasterxml.jackson.annotation.JsonProperty;
+
+import java.util.List;
+
+public class ListConfigsetsResponse extends SolrJerseyResponse {
+
+    //TODO We use both JsonProperty annotations here becaues the v2 serialization code expects Jackson, but
+    // ReflectMapWriter (which we use to squash the typed v2 response into a SolrQueryResponse for v1 code) relies on

Review Comment:
   Maybe it could use reflection to support both.  ReflectMapWriter is in SolrJ so it can't reference Jackson.  `CreateAliasPayload` and lots of other DTOs are in SolrJ with annotations.



##########
solr/core/src/java/org/apache/solr/handler/api/SomeCoreResource.java:
##########
@@ -0,0 +1,44 @@
+/*
+ * 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.api;
+
+import org.apache.solr.api.JerseyResource;
+import org.apache.solr.jersey.PermissionName;
+import org.apache.solr.security.PermissionNameProvider;
+import org.slf4j.Logger;
+import org.slf4j.LoggerFactory;
+
+import javax.ws.rs.GET;
+import javax.ws.rs.Path;
+import javax.ws.rs.PathParam;
+import javax.ws.rs.Produces;
+import javax.ws.rs.core.MediaType;
+import java.lang.invoke.MethodHandles;
+
+@Path("/collections/{collectionName}/somecorepath")
+public class SomeCoreResource extends JerseyResource {
+    private static final Logger log = LoggerFactory.getLogger(MethodHandles.lookup().lookupClass());
+
+    @GET
+    @Produces(MediaType.TEXT_PLAIN)
+    @PermissionName(PermissionNameProvider.Name.READ_PERM)
+    public String helloPlainText(@PathParam("collectionName") String collectionName) {
+        log.info("Made it into SomeCoreResource.helloPlainText with collName {}", collectionName);

Review Comment:
   This category of detection is not worth it IMO; I'll configure Lift to ignore it.



##########
solr/core/src/java/org/apache/solr/handler/configsets/ListConfigSetsAPI.java:
##########
@@ -16,34 +16,54 @@
  */
 package org.apache.solr.handler.configsets;
 
-import static org.apache.solr.client.solrj.SolrRequest.METHOD.GET;
-import static org.apache.solr.security.PermissionNameProvider.Name.CONFIG_READ_PERM;
-
-import java.util.List;
-import org.apache.solr.api.EndPoint;
-import org.apache.solr.client.solrj.SolrResponse;
-import org.apache.solr.cloud.OverseerSolrResponse;
-import org.apache.solr.common.util.NamedList;
+import org.apache.solr.api.JerseyResource;
 import org.apache.solr.core.CoreContainer;
-import org.apache.solr.request.SolrQueryRequest;
-import org.apache.solr.response.SolrQueryResponse;
+import org.apache.solr.jersey.PermissionName;
+import org.slf4j.Logger;
+import org.slf4j.LoggerFactory;
+
+import javax.inject.Inject;
+import javax.ws.rs.GET;
+import javax.ws.rs.Path;
+import javax.ws.rs.Produces;
+import javax.ws.rs.core.Context;
+import javax.ws.rs.core.HttpHeaders;
+import java.lang.invoke.MethodHandles;
+
+import static org.apache.solr.security.PermissionNameProvider.Name.CONFIG_READ_PERM;
 
 /**
  * V2 API for adding or updating a single file within a configset.
  *
  * <p>This API (GET /v2/cluster/configs) is analogous to the v1 /admin/configs?action=LIST command.
  */
-public class ListConfigSetsAPI extends ConfigSetAPIBase {
+
+@Path("/cluster/configs")
+public class ListConfigSetsAPI extends JerseyResource {
+
+  private static final Logger log = LoggerFactory.getLogger(MethodHandles.lookup().lookupClass());
+
+  @Context
+  public HttpHeaders headers;
+
+  private final CoreContainer coreContainer;
+
+  @Inject
   public ListConfigSetsAPI(CoreContainer coreContainer) {
-    super(coreContainer);
+    this.coreContainer = coreContainer;
   }
 
-  @EndPoint(method = GET, path = "/cluster/configs", permission = CONFIG_READ_PERM)
-  public void listConfigSet(SolrQueryRequest req, SolrQueryResponse rsp) throws Exception {
-    final NamedList<Object> results = new NamedList<>();
-    List<String> configSetsList = configSetService.listConfigs();
-    results.add("configSets", configSetsList);
-    SolrResponse response = new OverseerSolrResponse(results);
-    rsp.getValues().addAll(response.getResponse());
+
+  @GET
+  @Produces({"application/json", "application/javabin"})
+  @PermissionName(CONFIG_READ_PERM)
+  public ListConfigsetsResponse listConfigSet() throws Exception {
+    log.info("CoreContainer={}, HttpHeaders.accept={}", coreContainer, (headers != null) ? headers.getAcceptableMediaTypes() : "null");

Review Comment:
   I'm doubtful we need to log for each of our API entry-points when we already have a catch-all approach.



-- 
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: issues-unsubscribe@solr.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org


---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@solr.apache.org
For additional commands, e-mail: issues-help@solr.apache.org