Copilot commented on code in PR #4181:
URL: https://github.com/apache/solr/pull/4181#discussion_r2878120805
##########
solr/core/src/java/org/apache/solr/jersey/JerseyApplications.java:
##########
@@ -44,6 +44,7 @@ public CoreContainerApp() {
// Request and response serialization/deserialization
// TODO: could these be singletons to save per-request object creations?
register(JacksonJsonProvider.class, 1);
+ register(MessageBodyWriters.JsonMessageBodyWriter.class, 5);
Review Comment:
This migration relies on `JsonMessageBodyWriter` being selected over
Jersey’s `JacksonJsonProvider` for `application/json` so dynamic fields written
into `SolrQueryResponse` are preserved. Please ensure the registration
rank/priority here actually causes `JsonMessageBodyWriter` to win provider
selection for JSON; if not, responses will be serialized by Jackson directly
and the issue this PR is fixing will regress. A tangible fix is to adjust the
registration rank (or add an explicit `@Priority`) so the Solr JSON writer is
preferred for `application/json`.
```suggestion
register(JacksonJsonProvider.class, 5);
register(MessageBodyWriters.JsonMessageBodyWriter.class, 1);
```
##########
solr/core/src/test/org/apache/solr/handler/admin/api/RealTimeGetAPITest.java:
##########
@@ -0,0 +1,101 @@
+/*
+ * 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 java.util.List;
+import org.apache.solr.SolrTestCaseJ4;
+import org.apache.solr.client.api.model.FlexibleSolrJerseyResponse;
+import org.apache.solr.client.api.model.IndexType;
+import org.apache.solr.client.solrj.SolrClient;
+import org.apache.solr.client.solrj.request.DocumentsApi;
+import org.apache.solr.client.solrj.request.V2Request;
+import org.apache.solr.client.solrj.response.V2Response;
+import org.apache.solr.common.SolrInputDocument;
+import org.apache.solr.util.SolrJettyTestRule;
+import org.junit.BeforeClass;
+import org.junit.ClassRule;
+import org.junit.Test;
+
+/** Integration test for the {@link RealTimeGetAPI} JAX-RS endpoint. */
+public class RealTimeGetAPITest extends SolrTestCaseJ4 {
+
+ private static final String COLLECTION = "rtgTestCollection";
+
+ @ClassRule public static SolrJettyTestRule solrTestRule = new
SolrJettyTestRule();
+
+ @BeforeClass
+ public static void beforeTest() throws Exception {
+ System.setProperty(
+ ALLOW_PATHS_SYSPROP,
configset("cloud-minimal").getParent().toAbsolutePath().toString());
+ solrTestRule.startSolr(createTempDir());
Review Comment:
This test mutates a global system property (`ALLOW_PATHS_SYSPROP`) but
doesn’t restore the previous value afterward, which can leak into other tests
in the same JVM. Capture the previous value before setting it and restore/clear
it in an `@AfterClass` method to prevent cross-test contamination.
##########
solr/core/src/java/org/apache/solr/handler/admin/api/RealTimeGetAPI.java:
##########
@@ -30,19 +34,25 @@
* <p>This API (GET /v2/collections/collectionName/get) is analogous to the v1
* /solr/collectionName/get API.
*/
-public class RealTimeGetAPI {
+public class RealTimeGetAPI extends JerseyResource implements RealTimeGetApi {
private final RealTimeGetHandler rtgHandler;
+ private final SolrQueryRequest solrQueryRequest;
+ private final SolrQueryResponse solrQueryResponse;
- public RealTimeGetAPI(RealTimeGetHandler rtgHandler) {
- this.rtgHandler = rtgHandler;
+ @Inject
+ public RealTimeGetAPI(
+ SolrCore solrCore, SolrQueryRequest solrQueryRequest, SolrQueryResponse
solrQueryResponse) {
+ this.rtgHandler = (RealTimeGetHandler) solrCore.getRequestHandler("/get");
+ this.solrQueryRequest = solrQueryRequest;
+ this.solrQueryResponse = solrQueryResponse;
}
- @EndPoint(
- path = {"/get"},
- method = GET,
- permission = PermissionNameProvider.Name.READ_PERM)
- public void getDocuments(SolrQueryRequest req, SolrQueryResponse rsp) throws
Exception {
- rtgHandler.handleRequestBody(req, rsp);
+ @Override
+ @PermissionName(PermissionNameProvider.Name.READ_PERM)
+ public FlexibleSolrJerseyResponse getDocuments(String id, List<String> ids)
throws Exception {
+ final var response =
instantiateJerseyResponse(FlexibleSolrJerseyResponse.class);
+ rtgHandler.handleRequestBody(solrQueryRequest, solrQueryResponse);
+ return response;
Review Comment:
`id` and `ids` are part of the public JAX-RS/OpenAPI contract but are
currently ignored by the implementation. This makes the API behavior dependent
on how/if query params are propagated into `solrQueryRequest`, and risks
mismatches with the generated SolrJ client’s encoding for `ids` (multi-valued
vs comma-separated). Consider explicitly applying `id`/`ids` to the
`SolrQueryRequest` params (e.g., via `ModifiableSolrParams` + `setParams`) or
adding a clarifying comment explaining why these arguments are intentionally
unused because the request already contains them.
##########
changelog/unreleased/migrate-realtimegetapi-to-jax-rs.yml:
##########
@@ -0,0 +1,7 @@
+title: Migrate RealtimeGetAPI to JAX-RS. Realtime Get API now has OpenAPI and
SolrJ support.
Review Comment:
The changelog title uses inconsistent casing (`RealtimeGetAPI` / `Realtime
Get`) compared to the class name `RealTimeGetAPI`, and contains a double space
after the period. Consider standardizing to `RealTimeGetAPI` and using a single
space for readability/consistency.
```suggestion
title: Migrate RealTimeGetAPI to JAX-RS. RealTimeGetAPI now has OpenAPI and
SolrJ support.
```
--
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]
