Copilot commented on code in PR #4177:
URL: https://github.com/apache/solr/pull/4177#discussion_r2874001999
##########
solr/core/src/java/org/apache/solr/jersey/container/ContainerRequestUtils.java:
##########
@@ -140,4 +144,20 @@ private static String getServerAddress(URI baseUri) {
}
return serverAddress;
}
+
+ /**
+ * Normalizes the {@code /c/} collection alias to {@code /collections/} so
that JAX-RS path
+ * matching works correctly. The V2 API supports {@code /c/} as a shorthand
for {@code
+ * /collections/}, but JAX-RS path templates use the regex {@code
cores|collections} to match the
+ * index-type segment.
+ */
+ static String normalizeCAlias(String uri) {
+ if (uri.startsWith("/c/")) {
+ return "/collections/" + uri.substring(3);
+ }
+ if (uri.equals("/c")) {
+ return "/collections";
+ }
+ return uri;
Review Comment:
`normalizeCAlias` changes URI normalization behavior for all Jersey v2
requests, but there’s no unit test asserting the expected transformations
(e.g., `/c/<name>/...` → `/collections/<name>/...`, and `/c` → `/collections`).
Adding a small focused test would help prevent regressions in v2 path matching
(especially since this runs before all JAX-RS routing).
##########
solr/solr-ref-guide/modules/indexing-guide/pages/indexing-with-v2-apis.adoc:
##########
@@ -0,0 +1,183 @@
+= Indexing with the V2 Update API
+// 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.
+
+Solr's xref:configuration-guide:v2-api.adoc[v2 API] provides update endpoints
under the `/api` path prefix.
+These endpoints accept the same document formats as the
xref:indexing-with-update-handlers.adoc[v1 update handlers], with some
important differences described below.
+
+NOTE: The v2 API is classified as "experimental" and may change in
backwards-incompatible ways.
+
+== V2 Update Endpoint Paths
+
+For a SolrCloud collection the v2 update base path is:
+
+----
+/api/collections/{collection}/update
+----
+
+For a standalone core the v2 update base path is:
+
+----
+/api/cores/{core}/update
+----
+
+The following sub-paths are available:
+
+[width="100%",options="header",]
+|===
+|Path |Accepted Format |Notes
+|`/update` |JSON (array of documents) |Equivalent to v1 `/update/json/docs`;
document arrays only
+|`/update/json` |JSON (array of documents) |Same behavior as `/update`;
document arrays only
+|`/update/xml` |XML |Supports full XML update syntax (add, delete, commit,
optimize)
+|`/update/csv` |CSV |Equivalent to v1 `/update/csv`
+|`/update/bin` |Javabin |Equivalent to v1 `/update` with `Content-Type:
application/javabin`
+|===
+
+IMPORTANT: The v2 `/update` and `/update/json` endpoints are document-only:
they process a JSON array of documents (like the v1 `/update/json/docs` path)
and do *not* support the full
xref:indexing-with-update-handlers.adoc#sending-json-update-commands[JSON
update command syntax] (commit, delete, optimize within the request body).
+Use `/update/xml` for those operations, or issue commit/rollback via separate
API calls.
+
+== JSON Document Indexing
+
+The v2 `/update` and `/update/json` endpoints both accept a JSON array of
documents:
Review Comment:
The endpoint table says v2 `/update` and `/update/json` accept a “JSON
(array of documents)” and are “document arrays only”, but the same page later
states a single document can be posted as a JSON object. Please reconcile these
statements (e.g., document that both a single JSON object and an array are
accepted) to avoid contradictory guidance.
```suggestion
|`/update` |JSON document or array of documents |Equivalent to v1
`/update/json/docs`; documents only (no JSON command syntax)
|`/update/json` |JSON document or array of documents |Same behavior as
`/update`; documents only (no JSON command syntax)
|`/update/xml` |XML |Supports full XML update syntax (add, delete, commit,
optimize)
|`/update/csv` |CSV |Equivalent to v1 `/update/csv`
|`/update/bin` |Javabin |Equivalent to v1 `/update` with `Content-Type:
application/javabin`
|===
IMPORTANT: The v2 `/update` and `/update/json` endpoints are document-only:
they process one or more JSON documents (either a single JSON object or an
array of objects, like the v1 `/update/json/docs` path) and do *not* support
the full
xref:indexing-with-update-handlers.adoc#sending-json-update-commands[JSON
update command syntax] (commit, delete, optimize within the request body).
Use `/update/xml` for those operations, or issue commit/rollback via
separate API calls.
== JSON Document Indexing
The v2 `/update` and `/update/json` endpoints both accept JSON document(s).
The example below shows an array of documents:
```
##########
solr/core/src/java/org/apache/solr/handler/admin/api/UpdateAPI.java:
##########
@@ -17,52 +17,93 @@
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 jakarta.inject.Inject;
+import org.apache.solr.api.JerseyResource;
+import org.apache.solr.client.api.endpoint.UpdateApi;
+import org.apache.solr.client.api.model.SolrJerseyResponse;
+import org.apache.solr.common.SolrException;
+import org.apache.solr.core.SolrCore;
import org.apache.solr.handler.UpdateRequestHandler;
+import org.apache.solr.jersey.APIConfigProvider;
+import org.apache.solr.jersey.PermissionName;
import org.apache.solr.request.SolrQueryRequest;
import org.apache.solr.response.SolrQueryResponse;
/**
- * All v2 APIs that share a prefix of /update
+ * V2 API implementation for indexing documents.
*
- * <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.
+ * <p>These APIs delegate to the v1 {@link UpdateRequestHandler}. The {@code
/update} and {@code
+ * /update/json} paths are rewritten to {@code /update/json/docs} so that JSON
arrays of documents
+ * are processed by the JSON loader rather than the update-command loader.
*/
-public class UpdateAPI {
+public class UpdateAPI extends JerseyResource implements UpdateApi {
+
private final UpdateRequestHandler updateRequestHandler;
+ private final SolrQueryRequest solrQueryRequest;
+ private final SolrQueryResponse solrQueryResponse;
+
+ @Inject
+ public UpdateAPI(
+ UpdateRequestHandlerConfig handlerConfig,
+ SolrQueryRequest solrQueryRequest,
+ SolrQueryResponse solrQueryResponse) {
+ this.updateRequestHandler = handlerConfig.updateRequestHandler;
+ this.solrQueryRequest = solrQueryRequest;
+ this.solrQueryResponse = solrQueryResponse;
+ }
Review Comment:
The PR title/description frame this as a docs-only change, but this file
introduces a functional change to the v2 update implementation (JAX-RS resource
+ rewrite/response handling). Please update the PR title/description to reflect
the included production code/test changes so reviewers can assess scope and
risk appropriately.
##########
solr/api/src/java/org/apache/solr/client/api/endpoint/UpdateApi.java:
##########
@@ -0,0 +1,69 @@
+/*
+ * 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.endpoint;
+
+import static org.apache.solr.client.api.util.Constants.INDEX_PATH_PREFIX;
+
+import io.swagger.v3.oas.annotations.Operation;
+import jakarta.ws.rs.POST;
+import jakarta.ws.rs.Path;
+import org.apache.solr.client.api.model.SolrJerseyResponse;
+import org.apache.solr.client.api.util.StoreApiParameters;
+
+/** V2 API definitions for indexing documents via the update handler. */
+@Path(INDEX_PATH_PREFIX + "/update")
+public interface UpdateApi {
+
+ @POST
+ @StoreApiParameters
+ @Operation(
+ summary = "Add, delete, or update documents using any supported content
type",
+ tags = {"update"})
+ SolrJerseyResponse update() throws Exception;
+
+ @POST
+ @Path("/json")
+ @StoreApiParameters
+ @Operation(
+ summary = "Add, delete, or update documents in JSON format",
+ tags = {"update"})
+ SolrJerseyResponse updateJson() throws Exception;
+
+ @POST
+ @Path("/xml")
+ @StoreApiParameters
+ @Operation(
+ summary = "Add, delete, or update documents in XML format",
+ tags = {"update"})
+ SolrJerseyResponse updateXml() throws Exception;
+
+ @POST
+ @Path("/csv")
+ @StoreApiParameters
+ @Operation(
+ summary = "Add, delete, or update documents in CSV format",
+ tags = {"update"})
+ SolrJerseyResponse updateCsv() throws Exception;
+
+ @POST
+ @Path("/bin")
+ @StoreApiParameters
+ @Operation(
+ summary = "Add, delete, or update documents in JavaBin format",
+ tags = {"update"})
+ SolrJerseyResponse updateBin() throws Exception;
Review Comment:
Similarly, the summaries for `/update/csv` and `/update/bin` say “Add,
delete, or update documents…”, but these loaders are document-ingest only (no
delete/commit commands in-body). Please tighten the wording (e.g., “index
documents”) to avoid implying full update-command support for these content
types.
##########
solr/api/src/java/org/apache/solr/client/api/endpoint/UpdateApi.java:
##########
@@ -0,0 +1,69 @@
+/*
+ * 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.endpoint;
+
+import static org.apache.solr.client.api.util.Constants.INDEX_PATH_PREFIX;
+
+import io.swagger.v3.oas.annotations.Operation;
+import jakarta.ws.rs.POST;
+import jakarta.ws.rs.Path;
+import org.apache.solr.client.api.model.SolrJerseyResponse;
+import org.apache.solr.client.api.util.StoreApiParameters;
+
+/** V2 API definitions for indexing documents via the update handler. */
+@Path(INDEX_PATH_PREFIX + "/update")
+public interface UpdateApi {
+
+ @POST
+ @StoreApiParameters
+ @Operation(
+ summary = "Add, delete, or update documents using any supported content
type",
+ tags = {"update"})
+ SolrJerseyResponse update() throws Exception;
+
+ @POST
+ @Path("/json")
+ @StoreApiParameters
+ @Operation(
+ summary = "Add, delete, or update documents in JSON format",
+ tags = {"update"})
+ SolrJerseyResponse updateJson() throws Exception;
Review Comment:
The OpenAPI summaries for `/update` and `/update/json` currently say they
can “Add, delete, or update” and (for `/update`) accept “any supported content
type”. In this PR’s implementation these endpoints are rewritten to
`/update/json/docs` (docs-only JSON loader) and do not support JSON update
commands in the request body, nor non-JSON bodies. Please update the summaries
to match actual behavior so generated docs/clients don’t advertise unsupported
operations.
##########
solr/core/src/java/org/apache/solr/handler/admin/api/UpdateAPI.java:
##########
@@ -17,52 +17,93 @@
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 jakarta.inject.Inject;
+import org.apache.solr.api.JerseyResource;
+import org.apache.solr.client.api.endpoint.UpdateApi;
+import org.apache.solr.client.api.model.SolrJerseyResponse;
+import org.apache.solr.common.SolrException;
+import org.apache.solr.core.SolrCore;
import org.apache.solr.handler.UpdateRequestHandler;
+import org.apache.solr.jersey.APIConfigProvider;
+import org.apache.solr.jersey.PermissionName;
import org.apache.solr.request.SolrQueryRequest;
import org.apache.solr.response.SolrQueryResponse;
/**
- * All v2 APIs that share a prefix of /update
+ * V2 API implementation for indexing documents.
*
- * <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.
+ * <p>These APIs delegate to the v1 {@link UpdateRequestHandler}. The {@code
/update} and {@code
+ * /update/json} paths are rewritten to {@code /update/json/docs} so that JSON
arrays of documents
+ * are processed by the JSON loader rather than the update-command loader.
*/
-public class UpdateAPI {
+public class UpdateAPI extends JerseyResource implements UpdateApi {
+
private final UpdateRequestHandler updateRequestHandler;
+ private final SolrQueryRequest solrQueryRequest;
+ private final SolrQueryResponse solrQueryResponse;
+
+ @Inject
+ public UpdateAPI(
+ UpdateRequestHandlerConfig handlerConfig,
+ SolrQueryRequest solrQueryRequest,
+ SolrQueryResponse solrQueryResponse) {
+ this.updateRequestHandler = handlerConfig.updateRequestHandler;
+ this.solrQueryRequest = solrQueryRequest;
+ this.solrQueryResponse = solrQueryResponse;
+ }
- public UpdateAPI(UpdateRequestHandler updateRequestHandler) {
- this.updateRequestHandler = updateRequestHandler;
+ @Override
+ @PermissionName(UPDATE_PERM)
+ public SolrJerseyResponse update() throws Exception {
+ return handleUpdate(UpdateRequestHandler.DOC_PATH);
}
- @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);
+ @Override
+ @PermissionName(UPDATE_PERM)
+ public SolrJerseyResponse updateJson() {
+ return handleUpdate(UpdateRequestHandler.DOC_PATH);
}
- @EndPoint(method = POST, path = "/update/xml", permission = UPDATE_PERM)
- public void updateXml(SolrQueryRequest req, SolrQueryResponse rsp) throws
Exception {
- updateRequestHandler.handleRequest(req, rsp);
+ @Override
+ @PermissionName(UPDATE_PERM)
+ public SolrJerseyResponse updateXml() {
+ return handleUpdate(null);
}
- @EndPoint(method = POST, path = "/update/csv", permission = UPDATE_PERM)
- public void updateCsv(SolrQueryRequest req, SolrQueryResponse rsp) throws
Exception {
- updateRequestHandler.handleRequest(req, rsp);
+ @Override
+ @PermissionName(UPDATE_PERM)
+ public SolrJerseyResponse updateCsv() {
+ return handleUpdate(null);
}
- @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);
+ @Override
+ @PermissionName(UPDATE_PERM)
+ public SolrJerseyResponse updateBin() {
+ return handleUpdate(null);
}
- @EndPoint(method = POST, path = "/update/bin", permission = UPDATE_PERM)
- public void updateJavabin(SolrQueryRequest req, SolrQueryResponse rsp)
throws Exception {
- updateRequestHandler.handleRequest(req, rsp);
+ private SolrJerseyResponse handleUpdate(String pathOverride) {
+ final SolrJerseyResponse response =
instantiateJerseyResponse(SolrJerseyResponse.class);
+ if (pathOverride != null) {
+ solrQueryRequest.getContext().put(PATH, pathOverride);
+ }
+ SolrCore.preDecorateResponse(solrQueryRequest, solrQueryResponse);
+ updateRequestHandler.handleRequest(solrQueryRequest, solrQueryResponse);
+ SolrCore.postDecorateResponse(updateRequestHandler, solrQueryRequest,
solrQueryResponse);
Review Comment:
`handleUpdate` calls `SolrCore.postDecorateResponse(...)`, which adds a
`responseHeader` to `solrQueryResponse`. The Jersey layer then serializes the
returned `SolrJerseyResponse` by squashing it into the same `SolrQueryResponse`
(also including its own `responseHeader`), which can result in duplicate
`responseHeader` entries in the output. Consider removing the explicit pre/post
decoration here (to align with other Jersey resources) or otherwise ensure only
one responseHeader is serialized (e.g., clear the existing header before
returning, or use a squash-without-header approach for this endpoint).
```suggestion
// Do not call SolrCore.preDecorateResponse/postDecorateResponse here:
// Jersey will handle response decoration (including responseHeader) to
avoid duplicates.
updateRequestHandler.handleRequest(solrQueryRequest, solrQueryResponse);
```
##########
solr/core/src/test/org/apache/solr/handler.admin.api/UpdateAPITest.java:
##########
@@ -0,0 +1,125 @@
+/*
+ * 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.core.CoreContainer.ALLOW_PATHS_SYSPROP;
+
+import org.apache.solr.SolrTestCaseJ4;
Review Comment:
This new test is in `solr/core/src/test/org/apache/solr/handler.admin.api/`
but declares package `org.apache.solr.handler.admin.api`. Nearly all other
tests for this package live under
`solr/core/src/test/org/apache/solr/handler/admin/api/`. Moving this file to
the conventional directory would keep the test layout consistent and avoid
surprises with IDE/test discovery tooling.
--
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]
