You are viewing a plain text version of this content. The canonical link for it is here.
Posted to commits@solr.apache.org by ge...@apache.org on 2022/05/20 13:42:15 UTC

[solr] branch main updated: SOLR-15742: Convert v2 /update APIs to annotations (#868)

This is an automated email from the ASF dual-hosted git repository.

gerlowskija pushed a commit to branch main
in repository https://gitbox.apache.org/repos/asf/solr.git


The following commit(s) were added to refs/heads/main by this push:
     new ba4cc63c607 SOLR-15742: Convert v2 /update APIs to annotations (#868)
ba4cc63c607 is described below

commit ba4cc63c607551dc5fe8ee5b4124855ba319c295
Author: Jason Gerlowski <ge...@apache.org>
AuthorDate: Fri May 20 09:42:10 2022 -0400

    SOLR-15742: Convert v2 /update APIs to annotations (#868)
    
    Solr's been in the slow process of moving its v2 APIs away from the
    existing apispec/mapping framework towards one that relies on more
    explicit annotations to specify API properties.
    
    This commit converts the APIs from the core.Update apispec file to
    the "new" framework.
---
 .../apache/solr/handler/UpdateRequestHandler.java  |   4 +-
 .../solr/handler/UpdateRequestHandlerApi.java      |  71 ------------
 .../solr/handler/V2UpdateRequestHandler.java       |  52 +++++++++
 .../apache/solr/handler/admin/api/UpdateAPI.java   |  68 +++++++++++
 solr/core/src/resources/ImplicitPlugins.json       |   2 +-
 .../test/org/apache/solr/core/SolrCoreTest.java    |   2 +-
 .../solr/handler/V2UpdateAPIMappingTest.java       | 124 +++++++++++++++++++++
 .../solr/util/tracing/TestDistributedTracing.java  |   7 +-
 solr/solrj/src/resources/apispec/core.Update.json  |  17 ---
 9 files changed, 253 insertions(+), 94 deletions(-)

diff --git a/solr/core/src/java/org/apache/solr/handler/UpdateRequestHandler.java b/solr/core/src/java/org/apache/solr/handler/UpdateRequestHandler.java
index 22f97952ef7..4adf9afd8d3 100644
--- a/solr/core/src/java/org/apache/solr/handler/UpdateRequestHandler.java
+++ b/solr/core/src/java/org/apache/solr/handler/UpdateRequestHandler.java
@@ -19,9 +19,7 @@ package org.apache.solr.handler;
 import static org.apache.solr.common.params.CommonParams.PATH;
 import static org.apache.solr.security.PermissionNameProvider.Name.UPDATE_PERM;
 
-import java.util.Collections;
-import java.util.HashMap;
-import java.util.Map;
+import java.util.*;
 import org.apache.solr.common.SolrException;
 import org.apache.solr.common.SolrException.ErrorCode;
 import org.apache.solr.common.params.CommonParams;
diff --git a/solr/core/src/java/org/apache/solr/handler/UpdateRequestHandlerApi.java b/solr/core/src/java/org/apache/solr/handler/UpdateRequestHandlerApi.java
deleted file mode 100644
index 9cae43c3dbf..00000000000
--- a/solr/core/src/java/org/apache/solr/handler/UpdateRequestHandlerApi.java
+++ /dev/null
@@ -1,71 +0,0 @@
-/*
- * 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;
-
-import com.google.common.collect.ImmutableMap;
-import java.util.Collection;
-import java.util.Collections;
-import java.util.Map;
-import org.apache.solr.api.Api;
-import org.apache.solr.common.SolrException;
-import org.apache.solr.common.util.Utils;
-import org.apache.solr.request.SolrQueryRequest;
-import org.apache.solr.response.SolrQueryResponse;
-
-public class UpdateRequestHandlerApi extends UpdateRequestHandler {
-
-  @Override
-  public Collection<Api> getApis() {
-    return Collections.singleton(getApiImpl());
-  }
-
-  private Api getApiImpl() {
-    return new Api(Utils.getSpec("core.Update")) {
-      @Override
-      public void call(SolrQueryRequest req, SolrQueryResponse rsp) {
-        String path = req.getPath();
-        String target = mapping.get(path);
-        if (target != null) req.getContext().put("path", target);
-        try {
-          handleRequest(req, rsp);
-        } catch (RuntimeException e) {
-          throw e;
-        } catch (Exception e) {
-          throw new SolrException(SolrException.ErrorCode.BAD_REQUEST, e);
-        }
-      }
-    };
-  }
-
-  @Override
-  public Boolean registerV1() {
-    return Boolean.FALSE;
-  }
-
-  @Override
-  public Boolean registerV2() {
-    return Boolean.TRUE;
-  }
-
-  private static final Map<String, String> mapping =
-      ImmutableMap.<String, String>builder()
-          .put("/update", DOC_PATH)
-          .put(JSON_PATH, DOC_PATH)
-          .put("/update/json/commands", JSON_PATH)
-          .build();
-}
diff --git a/solr/core/src/java/org/apache/solr/handler/V2UpdateRequestHandler.java b/solr/core/src/java/org/apache/solr/handler/V2UpdateRequestHandler.java
new file mode 100644
index 00000000000..d4bbae9b7b6
--- /dev/null
+++ b/solr/core/src/java/org/apache/solr/handler/V2UpdateRequestHandler.java
@@ -0,0 +1,52 @@
+/*
+ * 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;
+
+import java.util.Collection;
+import org.apache.solr.api.AnnotatedApi;
+import org.apache.solr.api.Api;
+import org.apache.solr.handler.admin.api.UpdateAPI;
+
+/**
+ * An extension of {@link UpdateRequestHandler} used solely to register the v2 /update APIs
+ *
+ * <p>At core-load time, Solr looks at each 'plugin' in ImplicitPlugins.json, fetches the v2 {@link
+ * Api} implementations associated with each RequestHandler, and registers them in an {@link
+ * org.apache.solr.api.ApiBag}. Since UpdateRequestHandler is mentioned multiple times in
+ * ImplicitPlugins.json (once for each update API: /update, /update/json, etc.), this would cause
+ * the v2 APIs to be registered in duplicate. To avoid this, Solr has this RequestHandler, whose
+ * only purpose is to register the v2 APIs that conceptually should be associated with
+ * UpdateRequestHandler.
+ */
+public class V2UpdateRequestHandler extends UpdateRequestHandler {
+
+  @Override
+  public Collection<Api> getApis() {
+    return AnnotatedApi.getApis(new UpdateAPI(this));
+  }
+
+  @Override
+  public Boolean registerV1() {
+    return Boolean.FALSE;
+  }
+
+  @Override
+  public Boolean registerV2() {
+    return Boolean.TRUE;
+  }
+}
diff --git a/solr/core/src/java/org/apache/solr/handler/admin/api/UpdateAPI.java b/solr/core/src/java/org/apache/solr/handler/admin/api/UpdateAPI.java
new file mode 100644
index 00000000000..e97198dec08
--- /dev/null
+++ b/solr/core/src/java/org/apache/solr/handler/admin/api/UpdateAPI.java
@@ -0,0 +1,68 @@
+/*
+ * 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 static org.apache.solr.client.solrj.SolrRequest.METHOD.POST;
+import static org.apache.solr.common.params.CommonParams.PATH;
+import static org.apache.solr.security.PermissionNameProvider.Name.UPDATE_PERM;
+
+import org.apache.solr.api.EndPoint;
+import org.apache.solr.handler.UpdateRequestHandler;
+import org.apache.solr.request.SolrQueryRequest;
+import org.apache.solr.response.SolrQueryResponse;
+
+/**
+ * All v2 APIs that share a prefix of /update
+ *
+ * <p>Most of these v2 APIs are implemented as pure "pass-throughs" to the v1 code paths, but there
+ * are a few exceptions: /update and /update/json are both rewritten to /update/json/docs.
+ */
+public class UpdateAPI {
+  private final UpdateRequestHandler updateRequestHandler;
+
+  public UpdateAPI(UpdateRequestHandler updateRequestHandler) {
+    this.updateRequestHandler = updateRequestHandler;
+  }
+
+  @EndPoint(method = POST, path = "/update", permission = UPDATE_PERM)
+  public void update(SolrQueryRequest req, SolrQueryResponse rsp) throws Exception {
+    req.getContext().put(PATH, "/update/json/docs");
+    updateRequestHandler.handleRequest(req, rsp);
+  }
+
+  @EndPoint(method = POST, path = "/update/xml", permission = UPDATE_PERM)
+  public void updateXml(SolrQueryRequest req, SolrQueryResponse rsp) throws Exception {
+    updateRequestHandler.handleRequest(req, rsp);
+  }
+
+  @EndPoint(method = POST, path = "/update/csv", permission = UPDATE_PERM)
+  public void updateCsv(SolrQueryRequest req, SolrQueryResponse rsp) throws Exception {
+    updateRequestHandler.handleRequest(req, rsp);
+  }
+
+  @EndPoint(method = POST, path = "/update/json", permission = UPDATE_PERM)
+  public void updateJson(SolrQueryRequest req, SolrQueryResponse rsp) throws Exception {
+    req.getContext().put(PATH, "/update/json/docs");
+    updateRequestHandler.handleRequest(req, rsp);
+  }
+
+  @EndPoint(method = POST, path = "/update/bin", permission = UPDATE_PERM)
+  public void updateJavabin(SolrQueryRequest req, SolrQueryResponse rsp) throws Exception {
+    updateRequestHandler.handleRequest(req, rsp);
+  }
+}
diff --git a/solr/core/src/resources/ImplicitPlugins.json b/solr/core/src/resources/ImplicitPlugins.json
index 8e8bd6de31f..b0ee1fec3ca 100644
--- a/solr/core/src/resources/ImplicitPlugins.json
+++ b/solr/core/src/resources/ImplicitPlugins.json
@@ -27,7 +27,7 @@
       }
     },
     "update":{
-      "class":"solr.UpdateRequestHandlerApi",
+      "class":"solr.V2UpdateRequestHandler",
       "useParams": "_UPDATE_JSON_DOCS"
     },
     "/config": {
diff --git a/solr/core/src/test/org/apache/solr/core/SolrCoreTest.java b/solr/core/src/test/org/apache/solr/core/SolrCoreTest.java
index aa07f0e91fc..c0f130e21bc 100644
--- a/solr/core/src/test/org/apache/solr/core/SolrCoreTest.java
+++ b/solr/core/src/test/org/apache/solr/core/SolrCoreTest.java
@@ -136,7 +136,7 @@ public class SolrCoreTest extends SolrTestCaseJ4 {
       ++ihCount;
       assertEquals(pathToClassMap.get("/debug/dump"), "solr.DumpRequestHandler");
       ++ihCount;
-      assertEquals(pathToClassMap.get("update"), "solr.UpdateRequestHandlerApi");
+      assertEquals(pathToClassMap.get("update"), "solr.V2UpdateRequestHandler");
       ++ihCount;
       assertEquals(pathToClassMap.get("/tasks/cancel"), "solr.QueryCancellationHandler");
       ++ihCount;
diff --git a/solr/core/src/test/org/apache/solr/handler/V2UpdateAPIMappingTest.java b/solr/core/src/test/org/apache/solr/handler/V2UpdateAPIMappingTest.java
new file mode 100644
index 00000000000..a7955a6b400
--- /dev/null
+++ b/solr/core/src/test/org/apache/solr/handler/V2UpdateAPIMappingTest.java
@@ -0,0 +1,124 @@
+/*
+ * 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;
+
+import static org.apache.solr.SolrTestCaseJ4.assumeWorkingMockito;
+import static org.apache.solr.common.params.CommonParams.PATH;
+import static org.junit.Assert.assertEquals;
+import static org.mockito.Mockito.mock;
+
+import com.google.common.collect.Maps;
+import java.util.Collections;
+import java.util.HashMap;
+import java.util.List;
+import java.util.Map;
+import org.apache.solr.api.Api;
+import org.apache.solr.api.ApiBag;
+import org.apache.solr.common.util.CommandOperation;
+import org.apache.solr.handler.admin.ConfigSetsHandler;
+import org.apache.solr.handler.admin.api.UpdateAPI;
+import org.apache.solr.request.LocalSolrQueryRequest;
+import org.apache.solr.request.SolrQueryRequest;
+import org.apache.solr.response.SolrQueryResponse;
+import org.junit.Before;
+import org.junit.BeforeClass;
+import org.junit.Test;
+import org.mockito.ArgumentCaptor;
+
+/** Unit tests for the v2 to v1 mapping logic in {@link UpdateAPI} */
+public class V2UpdateAPIMappingTest {
+  private ApiBag apiBag;
+  private ArgumentCaptor<SolrQueryRequest> queryRequestCaptor;
+  private UpdateRequestHandler mockUpdateHandler;
+  private ConfigSetsHandler mockConfigSetHandler;
+
+  @BeforeClass
+  public static void ensureWorkingMockito() {
+    assumeWorkingMockito();
+  }
+
+  @Before
+  public void setupApiBag() throws Exception {
+    mockUpdateHandler = mock(UpdateRequestHandler.class);
+    mockConfigSetHandler = mock(ConfigSetsHandler.class);
+    queryRequestCaptor = ArgumentCaptor.forClass(SolrQueryRequest.class);
+
+    apiBag = new ApiBag(false);
+    final UpdateAPI updateAPI = new UpdateAPI(mockUpdateHandler);
+
+    apiBag.registerObject(updateAPI);
+  }
+
+  @Test
+  public void testUpdateApiRewriting() {
+
+    // Assume JSON in path if no specific format specified
+    {
+      final SolrQueryRequest req = runUpdateApi("/update");
+      assertEquals("/update/json/docs", req.getContext().get(PATH));
+    }
+
+    // Rewrite v2 /update/json to v1's /update/json/docs
+    {
+      final SolrQueryRequest req = runUpdateApi("/update/json");
+      assertEquals("/update/json/docs", req.getContext().get(PATH));
+    }
+
+    // No rewriting for /update/xml, /update/csv, or /update/bin
+    {
+      final SolrQueryRequest req = runUpdateApi("/update/xml");
+      assertEquals("/update/xml", req.getContext().get(PATH));
+    }
+    {
+      final SolrQueryRequest req = runUpdateApi("/update/csv");
+      assertEquals("/update/csv", req.getContext().get(PATH));
+    }
+    {
+      final SolrQueryRequest req = runUpdateApi("/update/bin");
+      assertEquals("/update/bin", req.getContext().get(PATH));
+    }
+  }
+
+  private SolrQueryRequest runUpdateApi(String path) {
+    final HashMap<String, String> parts = new HashMap<>();
+    final Api api = apiBag.lookup(path, "POST", parts);
+    final SolrQueryResponse rsp = new SolrQueryResponse();
+    final LocalSolrQueryRequest req =
+        new LocalSolrQueryRequest(null, Maps.newHashMap()) {
+          @Override
+          public List<CommandOperation> getCommands(boolean validateInput) {
+            return Collections.emptyList();
+          }
+
+          @Override
+          public Map<String, String> getPathTemplateValues() {
+            return parts;
+          }
+
+          @Override
+          public String getHttpMethod() {
+            return "POST";
+          }
+        };
+    req.getContext().put(PATH, path);
+
+    api.call(req, rsp);
+
+    return req;
+  }
+}
diff --git a/solr/core/src/test/org/apache/solr/util/tracing/TestDistributedTracing.java b/solr/core/src/test/org/apache/solr/util/tracing/TestDistributedTracing.java
index 624c9d162ef..d4b8c232b46 100644
--- a/solr/core/src/test/org/apache/solr/util/tracing/TestDistributedTracing.java
+++ b/solr/core/src/test/org/apache/solr/util/tracing/TestDistributedTracing.java
@@ -152,7 +152,12 @@ public class TestDistributedTracing extends SolrCloudTestCase {
         .build()
         .process(cloudClient);
     finishedSpans = getAndClearSpans();
-    assertEquals("post:/c/{collection}/update/json", finishedSpans.get(0).operationName());
+    // TODO See SOLR-16205.  A quirk in the way that span operationNames are generated causes
+    // core-level v2 endpoints
+    //  implemented using AnnotatedApi to produce spans lacking the full url (ie.
+    // /c/{collection}/update/json).  This test
+    //  will need updated to assert against the "full url" when SOLR-16205 is fixed.
+    assertEquals("post:/update/json", finishedSpans.get(0).operationName());
     assertDbInstanceColl(finishedSpans.get(0));
 
     final V2Response v2Response =
diff --git a/solr/solrj/src/resources/apispec/core.Update.json b/solr/solrj/src/resources/apispec/core.Update.json
deleted file mode 100644
index e58164fa54b..00000000000
--- a/solr/solrj/src/resources/apispec/core.Update.json
+++ /dev/null
@@ -1,17 +0,0 @@
-{
-  "documentation": "https://solr.apache.org/guide/indexing-with-update-handlers.html",
-  "methods": [
-    "POST"
-  ],
-  "url": {
-    "paths": [
-      "/update",
-      "/update/xml",
-      "/update/csv",
-      "/update/json",
-      "/update/bin",
-      "/update/json/commands"
-    ]
-
-  }
-}