From 17db73e77be4506790ffde539ddfbfb16a2a23b8 Mon Sep 17 00:00:00 2001 From: Jason Gerlowski Date: Thu, 9 Jan 2025 12:24:34 -0500 Subject: [PATCH 1/7] Introduce InputStream-exposing SolrResponse Creates InputStreamResponse and corresponding tests to wrap the NamedList produced by InputStreamResponseParser. Modifies the codegen mustache template to use this response type for any v2 APIs that are tagged as producing 'rawOutput'. --- .../solr/client/api/util/Constants.java | 5 + .../solrj/embedded/EmbeddedSolrServer.java | 5 +- .../solrj/io/stream/JSONTupleStream.java | 2 +- .../client/solrj/io/stream/SolrStream.java | 2 +- .../solrj/io/graph/GraphExpressionTest.java | 2 +- .../client/solrj/InputStreamResponse.java | 132 ++++++++++++++++++ .../client/solrj/impl/HttpSolrClient.java | 7 +- .../client/solrj/impl/HttpSolrClientBase.java | 7 +- .../solrj/impl/InputStreamResponseParser.java | 17 ++- .../src/resources/java-template/api.mustache | 14 +- .../client/solrj/InputStreamResponseTest.java | 79 +++++++++++ .../solrj/impl/HttpSolrClientTestBase.java | 2 +- 12 files changed, 252 insertions(+), 22 deletions(-) create mode 100644 solr/solrj/src/java/org/apache/solr/client/solrj/InputStreamResponse.java create mode 100644 solr/solrj/src/test/org/apache/solr/client/solrj/InputStreamResponseTest.java diff --git a/solr/api/src/java/org/apache/solr/client/api/util/Constants.java b/solr/api/src/java/org/apache/solr/client/api/util/Constants.java index 49e69d37fac..eab0b6f1020 100644 --- a/solr/api/src/java/org/apache/solr/client/api/util/Constants.java +++ b/solr/api/src/java/org/apache/solr/client/api/util/Constants.java @@ -29,7 +29,12 @@ private Constants() { public static final String CORE_NAME_PATH_PARAMETER = "coreName"; + // Annotation used on endpoints that should be skipped by code-generation public static final String OMIT_FROM_CODEGEN_PROPERTY = "omitFromCodegen"; + // Annotation used to indicate that the specified API can return arbitrary, unparseable content + // such as ZK or filestore files + public static final String RAW_OUTPUT_PROPERTY = "rawOutput"; + public static final String GENERIC_ENTITY_PROPERTY = "genericEntity"; public static final String BINARY_CONTENT_TYPE_V2 = "application/vnd.apache.solr.javabin"; diff --git a/solr/core/src/java/org/apache/solr/client/solrj/embedded/EmbeddedSolrServer.java b/solr/core/src/java/org/apache/solr/client/solrj/embedded/EmbeddedSolrServer.java index a15195f6228..c1bb7116d72 100644 --- a/solr/core/src/java/org/apache/solr/client/solrj/embedded/EmbeddedSolrServer.java +++ b/solr/core/src/java/org/apache/solr/client/solrj/embedded/EmbeddedSolrServer.java @@ -308,10 +308,7 @@ public void writeResults(ResultContext ctx, JavaBinCodec codec) throws IOExcepti if (responseParser instanceof InputStreamResponseParser) { // SPECIAL CASE - NamedList namedList = new NamedList<>(); - namedList.add("stream", byteBuffer.toInputStream()); - namedList.add("responseStatus", 200); // always by this point - return namedList; + return InputStreamResponseParser.createInputStreamNamedList(200, byteBuffer.toInputStream()); } // note: don't bother using the Reader variant; it often throws UnsupportedOperationException diff --git a/solr/solrj-streaming/src/java/org/apache/solr/client/solrj/io/stream/JSONTupleStream.java b/solr/solrj-streaming/src/java/org/apache/solr/client/solrj/io/stream/JSONTupleStream.java index 35a7b021d64..f03ca147e75 100644 --- a/solr/solrj-streaming/src/java/org/apache/solr/client/solrj/io/stream/JSONTupleStream.java +++ b/solr/solrj-streaming/src/java/org/apache/solr/client/solrj/io/stream/JSONTupleStream.java @@ -64,7 +64,7 @@ public static JSONTupleStream create(SolrClient server, SolrParams requestParams query.setResponseParser(new InputStreamResponseParser("json")); query.setMethod(SolrRequest.METHOD.POST); NamedList genericResponse = server.request(query); - InputStream stream = (InputStream) genericResponse.get("stream"); + InputStream stream = (InputStream) genericResponse.get(InputStreamResponseParser.STREAM_KEY); InputStreamReader reader = new InputStreamReader(stream, StandardCharsets.UTF_8); return new JSONTupleStream(reader); } diff --git a/solr/solrj-streaming/src/java/org/apache/solr/client/solrj/io/stream/SolrStream.java b/solr/solrj-streaming/src/java/org/apache/solr/client/solrj/io/stream/SolrStream.java index 1f06c852fd1..72425040b18 100644 --- a/solr/solrj-streaming/src/java/org/apache/solr/client/solrj/io/stream/SolrStream.java +++ b/solr/solrj-streaming/src/java/org/apache/solr/client/solrj/io/stream/SolrStream.java @@ -302,7 +302,7 @@ private TupleStreamParser constructParser(SolrParams requestParams) var client = clientCache.getHttpSolrClient(baseUrl); NamedList genericResponse = client.request(query); - InputStream stream = (InputStream) genericResponse.get("stream"); + InputStream stream = (InputStream) genericResponse.get(InputStreamResponseParser.STREAM_KEY); CloseableHttpResponse httpResponse = (CloseableHttpResponse) genericResponse.get("closeableResponse"); diff --git a/solr/solrj-streaming/src/test/org/apache/solr/client/solrj/io/graph/GraphExpressionTest.java b/solr/solrj-streaming/src/test/org/apache/solr/client/solrj/io/graph/GraphExpressionTest.java index 487dec09b6f..90a47751395 100644 --- a/solr/solrj-streaming/src/test/org/apache/solr/client/solrj/io/graph/GraphExpressionTest.java +++ b/solr/solrj-streaming/src/test/org/apache/solr/client/solrj/io/graph/GraphExpressionTest.java @@ -1250,7 +1250,7 @@ public void testGraphHandler() throws Exception { NamedList genericResponse = client.request(query); - InputStream stream = (InputStream) genericResponse.get("stream"); + InputStream stream = (InputStream) genericResponse.get(InputStreamResponseParser.STREAM_KEY); InputStreamReader reader = new InputStreamReader(stream, StandardCharsets.UTF_8); String xml = readString(reader); // Validate the nodes diff --git a/solr/solrj/src/java/org/apache/solr/client/solrj/InputStreamResponse.java b/solr/solrj/src/java/org/apache/solr/client/solrj/InputStreamResponse.java new file mode 100644 index 00000000000..b86e76f13ac --- /dev/null +++ b/solr/solrj/src/java/org/apache/solr/client/solrj/InputStreamResponse.java @@ -0,0 +1,132 @@ +/* + * 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.solrj; + +import static org.apache.solr.client.solrj.impl.InputStreamResponseParser.HTTP_STATUS_KEY; +import static org.apache.solr.client.solrj.impl.InputStreamResponseParser.STREAM_KEY; + +import java.io.ByteArrayOutputStream; +import java.io.IOException; +import java.io.InputStream; +import java.lang.invoke.MethodHandles; +import java.nio.charset.Charset; +import java.util.function.Function; +import org.apache.solr.client.solrj.impl.InputStreamResponseParser; +import org.apache.solr.client.solrj.response.SimpleSolrResponse; +import org.apache.solr.common.SolrException; +import org.apache.solr.common.util.IOUtils; +import org.apache.solr.common.util.NamedList; +import org.apache.solr.common.util.ObjectReleaseTracker; +import org.slf4j.Logger; +import org.slf4j.LoggerFactory; + +/** + * Represents the NamedList response format created by {@link InputStreamResponseParser}. + * + *

Particularly useful when targeting APIs that return arbitrary or binary data (e.g. replication + * APIs for fetching index files) + */ +public class InputStreamResponse extends SimpleSolrResponse { + private static final Logger log = LoggerFactory.getLogger(MethodHandles.lookup().lookupClass()); + + // True if the HTTP status is '200 OK', false otherwise + public static final Function HTTP_OK_VALIDATOR = (status) -> 200 == status; + // True if the HTTP status is neither a 4xx or a 5xx error. + public static final Function NON_ERROR_CODE_VALIDATOR = + (status) -> status < 399; + + @Override + public void setResponse(NamedList rsp) { + if (rsp.get(STREAM_KEY) == null) { + throw new IllegalArgumentException( + "Missing key '" + + STREAM_KEY + + "'; " + + getClass().getSimpleName() + + " can only be used with requests or clients configured to use " + + InputStreamResponseParser.class.getSimpleName()); + } + super.setResponse(rsp); + } + + public int getHttpStatus() { + return (int) getResponse().get(HTTP_STATUS_KEY); + } + + /** + * Access the server response as an {@link InputStream}, regardless of the HTTP status code + * + *

Caller is responsible for consuming and closing the stream, and releasing it from the + * tracking done by {@link ObjectReleaseTracker}. No validation is done on the HTTP status code. + */ + public InputStream getResponseStream() { + final NamedList resp = getResponse(); + + return (InputStream) resp.get(STREAM_KEY); + } + + /** + * Access the server response as an {@link InputStream}, after ensuring that the HTTP status code + * is 200 ('OK') + * + *

Caller is responsible for consuming and closing the stream, and releasing it from the + * tracking done by {@link ObjectReleaseTracker}. + */ + public InputStream getResponseStreamIfSuccessful() { + return getResponseStreamIfSuccessful(HTTP_OK_VALIDATOR); + } + + /** + * Access the server response as an {@link InputStream}, after ensuring the HTTP status code + * passes a provided validator. + * + * @param statusValidator a function that returns true iff the response body should be returned + */ + public InputStream getResponseStreamIfSuccessful(Function statusValidator) { + validateExpectedStatus(statusValidator); + return getResponseStream(); + } + + private void validateExpectedStatus(Function statusChecker) { + final var httpStatus = getHttpStatus(); + if (!statusChecker.apply(httpStatus)) { + try { + log.error( + "Request returned unexpected HTTP status code {}; response content: {}", + httpStatus, + consumeAndStringifyForLogging(getResponseStream())); + } catch (IOException e) { + log.error("could not print error", e); + } + throw new SolrException( + SolrException.ErrorCode.getErrorCode(httpStatus), + "Unexpected status code [" + httpStatus + "] on response."); + } + } + + private String consumeAndStringifyForLogging(InputStream inputStream) throws IOException { + final var baos = new ByteArrayOutputStream(); + try { + inputStream.transferTo(baos); + return baos.toString(Charset.defaultCharset()); + } finally { + ObjectReleaseTracker.release(inputStream); + IOUtils.closeQuietly(baos); + IOUtils.closeQuietly(inputStream); + } + } +} diff --git a/solr/solrj/src/java/org/apache/solr/client/solrj/impl/HttpSolrClient.java b/solr/solrj/src/java/org/apache/solr/client/solrj/impl/HttpSolrClient.java index bff1a819407..2fa58a9f6c1 100644 --- a/solr/solrj/src/java/org/apache/solr/client/solrj/impl/HttpSolrClient.java +++ b/solr/solrj/src/java/org/apache/solr/client/solrj/impl/HttpSolrClient.java @@ -615,12 +615,11 @@ protected NamedList executeMethod( } } if (processor == null || processor instanceof InputStreamResponseParser) { - // no processor specified, return raw stream - NamedList rsp = new NamedList<>(); - rsp.add("stream", respBody); + final var rsp = + InputStreamResponseParser.createInputStreamNamedList( + response.getStatusLine().getStatusCode(), respBody); rsp.add("closeableResponse", response); - rsp.add("responseStatus", response.getStatusLine().getStatusCode()); // Only case where stream should not be closed shouldClose = false; return rsp; diff --git a/solr/solrj/src/java/org/apache/solr/client/solrj/impl/HttpSolrClientBase.java b/solr/solrj/src/java/org/apache/solr/client/solrj/impl/HttpSolrClientBase.java index 8683b965ad0..adf655cb2c6 100644 --- a/solr/solrj/src/java/org/apache/solr/client/solrj/impl/HttpSolrClientBase.java +++ b/solr/solrj/src/java/org/apache/solr/client/solrj/impl/HttpSolrClientBase.java @@ -217,13 +217,10 @@ protected NamedList processErrorsAndResponse( } if (wantStream(processor)) { - // no processor specified, return raw stream - NamedList rsp = new NamedList<>(); - rsp.add("stream", is); - rsp.add("responseStatus", httpStatus); // Only case where stream should not be closed shouldClose = false; - return rsp; + // no processor specified, return raw stream + return InputStreamResponseParser.createInputStreamNamedList(httpStatus, is); } checkContentType(processor, is, mimeType, encoding, httpStatus, urlExceptionMessage); diff --git a/solr/solrj/src/java/org/apache/solr/client/solrj/impl/InputStreamResponseParser.java b/solr/solrj/src/java/org/apache/solr/client/solrj/impl/InputStreamResponseParser.java index 530851c3fa8..06b52626ef4 100644 --- a/solr/solrj/src/java/org/apache/solr/client/solrj/impl/InputStreamResponseParser.java +++ b/solr/solrj/src/java/org/apache/solr/client/solrj/impl/InputStreamResponseParser.java @@ -21,9 +21,16 @@ import org.apache.solr.client.solrj.ResponseParser; import org.apache.solr.common.util.NamedList; -/** Simply puts the InputStream into an entry in a NamedList named "stream". */ +/** + * Simply puts the InputStream into an entry in a NamedList named "stream". + * + * @see org.apache.solr.client.solrj.InputStreamResponse + */ public class InputStreamResponseParser extends ResponseParser { + public static String STREAM_KEY = "stream"; + public static String HTTP_STATUS_KEY = "responseStatus"; + private final String writerType; public InputStreamResponseParser(String writerType) { @@ -44,4 +51,12 @@ public NamedList processResponse(Reader reader) { public NamedList processResponse(InputStream body, String encoding) { throw new UnsupportedOperationException(); } + + public static NamedList createInputStreamNamedList( + int httpStatus, InputStream inputStream) { + final var nl = new NamedList(); + nl.add(STREAM_KEY, inputStream); + nl.add(HTTP_STATUS_KEY, httpStatus); + return nl; + } } diff --git a/solr/solrj/src/resources/java-template/api.mustache b/solr/solrj/src/resources/java-template/api.mustache index c4ef7f9c59b..f276f3f0a72 100644 --- a/solr/solrj/src/resources/java-template/api.mustache +++ b/solr/solrj/src/resources/java-template/api.mustache @@ -36,6 +36,7 @@ import org.apache.solr.client.solrj.JacksonParsingResponse; import org.apache.solr.client.solrj.JacksonContentWriter; import org.apache.solr.client.solrj.request.RequestWriter.ContentWriter; import org.apache.solr.client.solrj.impl.InputStreamResponseParser; +import org.apache.solr.client.solrj.InputStreamResponse; import org.apache.solr.client.solrj.ResponseParser; {{! Covers all top-level request/response model classes, but not necessarily any types nested in those classes }} @@ -86,11 +87,16 @@ public class {{classname}} { {{#operation}} {{^vendorExtensions.x-omitFromCodegen}} - public static class {{operationIdCamelCase}}Response extends JacksonParsingResponse<{{modelPackage}}.{{returnType}}> { - public {{operationIdCamelCase}}Response() { - super({{modelPackage}}.{{returnType}}.class); - } + {{#vendorExtensions.x-rawOutput}} + public static class {{operationIdCamelCase}}Response extends InputStreamResponse {} + {{/vendorExtensions.x-rawOutput}} + {{^vendorExtensions.x-rawOutput}} + public static class {{operationIdCamelCase}}Response extends JacksonParsingResponse<{{modelPackage}}.{{returnType}}> { + public {{operationIdCamelCase}}Response() { + super({{modelPackage}}.{{returnType}}.class); + } } + {{/vendorExtensions.x-rawOutput}} public static class {{operationIdCamelCase}} extends SolrRequest<{{operationIdCamelCase}}Response> { {{#bodyParam}} diff --git a/solr/solrj/src/test/org/apache/solr/client/solrj/InputStreamResponseTest.java b/solr/solrj/src/test/org/apache/solr/client/solrj/InputStreamResponseTest.java new file mode 100644 index 00000000000..83fa3d48abf --- /dev/null +++ b/solr/solrj/src/test/org/apache/solr/client/solrj/InputStreamResponseTest.java @@ -0,0 +1,79 @@ +/* + * 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.solrj; + +import static org.hamcrest.Matchers.containsString; + +import java.io.ByteArrayInputStream; +import java.io.IOException; +import org.apache.solr.SolrTestCase; +import org.apache.solr.client.solrj.impl.InputStreamResponseParser; +import org.apache.solr.common.SolrException; +import org.apache.solr.common.util.NamedList; +import org.junit.Test; + +/** Unit tests for {@link InputStreamResponse} */ +public class InputStreamResponseTest extends SolrTestCase { + + @Test + public void testDetectsWhenResponseDoesntMatchExpectedFormat() { + final var inputStreamResponse = new InputStreamResponse(); + final var rawNl = new NamedList(); + rawNl.add("someKey", "someVal"); + + final var thrown = + expectThrows( + IllegalArgumentException.class, + () -> { + inputStreamResponse.setResponse(rawNl); + }); + assertThat(thrown.getMessage(), containsString("Missing key 'stream'")); + } + + @Test + public void testAllowsAccessToStatusAndStream() throws IOException { + final var inputStreamResponse = new InputStreamResponse(); + final var rawNl = + InputStreamResponseParser.createInputStreamNamedList( + 200, new ByteArrayInputStream(new byte[] {'h', 'e', 'l', 'l', 'o'})); + inputStreamResponse.setResponse(rawNl); + + assertEquals(200, inputStreamResponse.getHttpStatus()); + try (final var is = inputStreamResponse.getResponseStream()) { + final var streamVal = new String(is.readAllBytes()); + assertEquals("hello", streamVal); + } + } + + @Test + public void testThrowsErrorIfUnexpectedResponseEncountered() { + final var inputStreamResponse = new InputStreamResponse(); + final var rawNl = + InputStreamResponseParser.createInputStreamNamedList( + 500, new ByteArrayInputStream(new byte[] {'h', 'e', 'l', 'l', 'o'})); + inputStreamResponse.setResponse(rawNl); + + final var thrown = + expectThrows( + SolrException.class, + () -> { + inputStreamResponse.getResponseStreamIfSuccessful(); + }); + assertEquals(500, thrown.code()); // Matches that status of the HTTP response + assertThat(thrown.getMessage(), containsString("Unexpected status code")); + } +} diff --git a/solr/solrj/src/test/org/apache/solr/client/solrj/impl/HttpSolrClientTestBase.java b/solr/solrj/src/test/org/apache/solr/client/solrj/impl/HttpSolrClientTestBase.java index 1c8525b74b6..a5c6967ddd7 100644 --- a/solr/solrj/src/test/org/apache/solr/client/solrj/impl/HttpSolrClientTestBase.java +++ b/solr/solrj/src/test/org/apache/solr/client/solrj/impl/HttpSolrClientTestBase.java @@ -427,7 +427,7 @@ protected void testGetRawStream(HttpSolrClientBase client) throws Exception { final var req = new QueryRequest(params("q", "*:*")); req.setResponseParser(new InputStreamResponseParser("xml")); final var rsp = req.process(client); - Object stream = rsp.getResponse().get("stream"); + Object stream = rsp.getResponse().get(InputStreamResponseParser.STREAM_KEY); assertNotNull(stream); assertThat(stream, instanceOf(InputStream.class)); InputStream is = (InputStream) stream; From 61180850476298fa39847ec7aeab399b043d7e24 Mon Sep 17 00:00:00 2001 From: Jason Gerlowski Date: Tue, 14 Jan 2025 08:15:53 -0500 Subject: [PATCH 2/7] Switch ZK-Read and Replication to StreamingOutput This also switches the endpoint-definitions to be codegen compatible using the new "rawOutput" property. --- solr/api/build.gradle | 2 ++ .../client/api/endpoint/ConfigsetsApi.java | 1 + .../client/api/endpoint/ReplicationApis.java | 9 +++--- .../api/endpoint/ZooKeeperReadApis.java | 15 +++++++--- .../api/model/ZooKeeperFileResponse.java | 28 ------------------- .../solr/handler/admin/ZookeeperRead.java | 27 ++++++++++-------- 6 files changed, 33 insertions(+), 49 deletions(-) delete mode 100644 solr/api/src/java/org/apache/solr/client/api/model/ZooKeeperFileResponse.java diff --git a/solr/api/build.gradle b/solr/api/build.gradle index adad0302602..a5878bb06ac 100644 --- a/solr/api/build.gradle +++ b/solr/api/build.gradle @@ -55,6 +55,8 @@ resolve { outputDir = file(project.openApiSpecDir) outputFileName = "solr-openapi-${version}" prettyPrint = true +// Ignore resources not annotated with 'Operation', useful for omitting endpoints from OAS + readAllResources = false } dependencies { diff --git a/solr/api/src/java/org/apache/solr/client/api/endpoint/ConfigsetsApi.java b/solr/api/src/java/org/apache/solr/client/api/endpoint/ConfigsetsApi.java index 9961b4c9f28..4bc812043e9 100644 --- a/solr/api/src/java/org/apache/solr/client/api/endpoint/ConfigsetsApi.java +++ b/solr/api/src/java/org/apache/solr/client/api/endpoint/ConfigsetsApi.java @@ -89,6 +89,7 @@ SolrJerseyResponse uploadConfigSet( @PUT @Path("{filePath:.+}") + @Operation(summary = "Create a new configset.", tags = "configsets") SolrJerseyResponse uploadConfigSetFile( @PathParam("configSetName") String configSetName, @PathParam("filePath") String filePath, diff --git a/solr/api/src/java/org/apache/solr/client/api/endpoint/ReplicationApis.java b/solr/api/src/java/org/apache/solr/client/api/endpoint/ReplicationApis.java index 3fe5ac14f45..1f0476f8661 100644 --- a/solr/api/src/java/org/apache/solr/client/api/endpoint/ReplicationApis.java +++ b/solr/api/src/java/org/apache/solr/client/api/endpoint/ReplicationApis.java @@ -16,7 +16,7 @@ */ package org.apache.solr.client.api.endpoint; -import static org.apache.solr.client.api.util.Constants.OMIT_FROM_CODEGEN_PROPERTY; +import static org.apache.solr.client.api.util.Constants.RAW_OUTPUT_PROPERTY; import io.swagger.v3.oas.annotations.Operation; import io.swagger.v3.oas.annotations.Parameter; @@ -59,10 +59,9 @@ FileListResponse fetchFileList( @CoreApiParameters @Operation( summary = "Get a stream of a specific file path of a core", - tags = {"core-replication"}, - extensions = { // TODO Remove as a part of SOLR-17562 - @Extension( - properties = {@ExtensionProperty(name = OMIT_FROM_CODEGEN_PROPERTY, value = "true")}) + tags = {"replication"}, + extensions = { + @Extension(properties = {@ExtensionProperty(name = RAW_OUTPUT_PROPERTY, value = "true")}) }) @Path("/files/{filePath}") StreamingOutput fetchFile( diff --git a/solr/api/src/java/org/apache/solr/client/api/endpoint/ZooKeeperReadApis.java b/solr/api/src/java/org/apache/solr/client/api/endpoint/ZooKeeperReadApis.java index f41e8de3d63..58c5091fe42 100644 --- a/solr/api/src/java/org/apache/solr/client/api/endpoint/ZooKeeperReadApis.java +++ b/solr/api/src/java/org/apache/solr/client/api/endpoint/ZooKeeperReadApis.java @@ -16,15 +16,19 @@ */ package org.apache.solr.client.api.endpoint; +import static org.apache.solr.client.api.util.Constants.RAW_OUTPUT_PROPERTY; + import io.swagger.v3.oas.annotations.Operation; import io.swagger.v3.oas.annotations.Parameter; +import io.swagger.v3.oas.annotations.extensions.Extension; +import io.swagger.v3.oas.annotations.extensions.ExtensionProperty; import jakarta.ws.rs.GET; import jakarta.ws.rs.Path; import jakarta.ws.rs.PathParam; import jakarta.ws.rs.Produces; import jakarta.ws.rs.QueryParam; import jakarta.ws.rs.core.MediaType; -import org.apache.solr.client.api.model.ZooKeeperFileResponse; +import jakarta.ws.rs.core.StreamingOutput; import org.apache.solr.client.api.model.ZooKeeperListChildrenResponse; /** V2 API definitions for Solr's ZooKeeper ready-proxy endpoint */ @@ -35,9 +39,12 @@ public interface ZooKeeperReadApis { @Path("/data{zkPath:.+}") @Operation( summary = "Return the data stored in a specified ZooKeeper node", - tags = {"zookeeper-read"}) + tags = {"zookeeper-read"}, + extensions = { + @Extension(properties = {@ExtensionProperty(name = RAW_OUTPUT_PROPERTY, value = "true")}) + }) @Produces({"application/vnd.apache.solr.raw", MediaType.APPLICATION_JSON}) - ZooKeeperFileResponse readNode( + StreamingOutput readNode( @Parameter(description = "The path of the node to read from ZooKeeper") @PathParam("zkPath") String zkPath); @@ -48,7 +55,7 @@ ZooKeeperFileResponse readNode( @GET @Path("/data/security.json") @Produces({"application/vnd.apache.solr.raw", MediaType.APPLICATION_JSON}) - ZooKeeperFileResponse readSecurityJsonNode(); + StreamingOutput readSecurityJsonNode(); @GET @Path("/children{zkPath:.*}") diff --git a/solr/api/src/java/org/apache/solr/client/api/model/ZooKeeperFileResponse.java b/solr/api/src/java/org/apache/solr/client/api/model/ZooKeeperFileResponse.java deleted file mode 100644 index d09302fa168..00000000000 --- a/solr/api/src/java/org/apache/solr/client/api/model/ZooKeeperFileResponse.java +++ /dev/null @@ -1,28 +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.client.api.model; - -import com.fasterxml.jackson.annotation.JsonProperty; - -public class ZooKeeperFileResponse extends SolrJerseyResponse { - // TODO Should be switched over to using StreamingOutput as a part of SOLR-17562 - @JsonProperty("content") // A flag value that RawResponseWriter handles specially - public Object output; - - @JsonProperty("zkData") - public String zkData; -} diff --git a/solr/core/src/java/org/apache/solr/handler/admin/ZookeeperRead.java b/solr/core/src/java/org/apache/solr/handler/admin/ZookeeperRead.java index 8ded06a7a01..bf0db85fcdb 100644 --- a/solr/core/src/java/org/apache/solr/handler/admin/ZookeeperRead.java +++ b/solr/core/src/java/org/apache/solr/handler/admin/ZookeeperRead.java @@ -21,19 +21,21 @@ import static org.apache.solr.security.PermissionNameProvider.Name.ZK_READ_PERM; import jakarta.inject.Inject; +import jakarta.ws.rs.WebApplicationException; +import jakarta.ws.rs.core.StreamingOutput; +import java.io.IOException; +import java.io.OutputStream; import java.util.HashMap; import java.util.LinkedHashMap; import java.util.List; import java.util.Map; import org.apache.solr.client.api.endpoint.ZooKeeperReadApis; -import org.apache.solr.client.api.model.ZooKeeperFileResponse; import org.apache.solr.client.api.model.ZooKeeperListChildrenResponse; import org.apache.solr.client.api.model.ZooKeeperStat; import org.apache.solr.client.solrj.impl.BinaryResponseParser; import org.apache.solr.client.solrj.impl.XMLResponseParser; import org.apache.solr.common.SolrException; import org.apache.solr.common.params.CommonParams; -import org.apache.solr.common.util.ContentStreamBase; import org.apache.solr.core.CoreContainer; import org.apache.solr.handler.admin.api.AdminAPIBase; import org.apache.solr.jersey.PermissionName; @@ -64,7 +66,7 @@ public ZookeeperRead(CoreContainer coreContainer, SolrQueryRequest req, SolrQuer /** Request contents of a znode, except security.json */ @Override @PermissionName(ZK_READ_PERM) - public ZooKeeperFileResponse readNode(String zkPath) { + public StreamingOutput readNode(String zkPath) { zkPath = sanitizeZkPath(zkPath); return readNodeAndAddToResponse(zkPath); } @@ -72,7 +74,7 @@ public ZooKeeperFileResponse readNode(String zkPath) { /** Request contents of the security.json node */ @Override @PermissionName(SECURITY_READ_PERM) - public ZooKeeperFileResponse readSecurityJsonNode() { + public StreamingOutput readSecurityJsonNode() { return readNodeAndAddToResponse("/security.json"); } @@ -141,18 +143,19 @@ private String guessMime(byte firstByte) { } /** Reads content of a znode */ - private ZooKeeperFileResponse readNodeAndAddToResponse(String zkPath) { - final ZooKeeperFileResponse zkFileResponse = - instantiateJerseyResponse(ZooKeeperFileResponse.class); - + private StreamingOutput readNodeAndAddToResponse(String zkPath) { byte[] d = readPathFromZookeeper(zkPath); if (d == null || d.length == 0) { - zkFileResponse.zkData = EMPTY; - return zkFileResponse; + d = new byte[0]; } - zkFileResponse.output = new ContentStreamBase.ByteArrayStream(d, null, guessMime(d[0])); - return zkFileResponse; + final var bytesToWrite = d; + return new StreamingOutput() { + @Override + public void write(OutputStream output) throws IOException, WebApplicationException { + output.write(bytesToWrite); + } + }; } /** Reads a single node from zookeeper and return as byte array */ From 410be13b9401bc1b7832e0494e8cbbbec389da65 Mon Sep 17 00:00:00 2001 From: Jason Gerlowski Date: Tue, 14 Jan 2025 09:27:03 -0500 Subject: [PATCH 3/7] Add dev-docs on StreamingOutput usage --- dev-docs/v2-api-conventions.adoc | 25 +++++++++++++++++++++++-- 1 file changed, 23 insertions(+), 2 deletions(-) diff --git a/dev-docs/v2-api-conventions.adoc b/dev-docs/v2-api-conventions.adoc index 74a760c2bfd..75e2896560b 100644 --- a/dev-docs/v2-api-conventions.adoc +++ b/dev-docs/v2-api-conventions.adoc @@ -1,3 +1,4 @@ += API Design == HTTP Paths Where possible, each v2 API is given an HTTP path that reflects the resource type and/or name most relevant to its functionality. @@ -69,8 +70,8 @@ For use within the v2 API, the four "popular" HTTP methods have the following se == Errors v2 APIs should be consistent in how they report errors. Throwing a `SolrException` will convey -1.the error code as the HTTP response status code, as `responseHeader.status` and as `error.code`, and -1.the error message as `error.msg`. +1. The error code as the HTTP response status code, as `responseHeader.status` and as `error.code`, and +2. The error message as `error.msg`. API calls that reference a specific resource (e.g. `specificCollName`, `specificAliasName`, `specificPropertyName` and others per the above list) that do not exist should return `SolrException.ErrorCode.NOT_FOUND` (HTTP 404). @@ -82,3 +83,23 @@ Often these operations were initially conceived of as procedural "commands" and Solr's v2 API currently accommodates these "command" APIs by appending the command name (often a verb like "unload", "reload", or "split") onto the otherwise "resource"-based path. For example: Solr's core "unload" command uses the API `POST /api/cores/specificCoreName/unload`. + += JAX-RS Implementation Conventions + +== Streaming + +Solr has a number of APIs that return binary file data or other arbitrary content, such as the "replication" APIs used to pull index files from other cores. +Please use the following conventions when implementing similar endpoints: +1. `@Operation` annotations use an "extension property" to indicate to codegen tools that the API output is "raw" or untyped. For example: ++ +``` + @Operation( + summary = "Return the data stored in a specified ZooKeeper node", + tags = {"zookeeper-read"}, + extensions = { + @Extension(properties = {@ExtensionProperty(name = RAW_OUTPUT_PROPERTY, value = "true")}) + }) +``` +2. Interface methods should return a type that implements the JAX-RS `StreamingOutput` interface. + +See the `fetchFile()` method in `ReplicationApis.java` for a concrete example. From 4bf732b4a96f4e8ea934263ca25e5a7313fa41ca Mon Sep 17 00:00:00 2001 From: Jason Gerlowski Date: Tue, 14 Jan 2025 10:14:02 -0500 Subject: [PATCH 4/7] Fix check --- .../org/apache/solr/client/solrj/InputStreamResponseTest.java | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) diff --git a/solr/solrj/src/test/org/apache/solr/client/solrj/InputStreamResponseTest.java b/solr/solrj/src/test/org/apache/solr/client/solrj/InputStreamResponseTest.java index 83fa3d48abf..6737c310832 100644 --- a/solr/solrj/src/test/org/apache/solr/client/solrj/InputStreamResponseTest.java +++ b/solr/solrj/src/test/org/apache/solr/client/solrj/InputStreamResponseTest.java @@ -20,6 +20,7 @@ import java.io.ByteArrayInputStream; import java.io.IOException; +import java.nio.charset.StandardCharsets; import org.apache.solr.SolrTestCase; import org.apache.solr.client.solrj.impl.InputStreamResponseParser; import org.apache.solr.common.SolrException; @@ -54,7 +55,7 @@ public void testAllowsAccessToStatusAndStream() throws IOException { assertEquals(200, inputStreamResponse.getHttpStatus()); try (final var is = inputStreamResponse.getResponseStream()) { - final var streamVal = new String(is.readAllBytes()); + final var streamVal = new String(is.readAllBytes(), StandardCharsets.UTF_8); assertEquals("hello", streamVal); } } From 583e07aed516565bb6686bc583439c6741525a7e Mon Sep 17 00:00:00 2001 From: Jason Gerlowski Date: Tue, 14 Jan 2025 11:43:09 -0500 Subject: [PATCH 5/7] Use generated req/rsp classes in ZK API tests --- .../handler/admin/ZookeeperReadAPITest.java | 93 ++++++++++--------- 1 file changed, 47 insertions(+), 46 deletions(-) diff --git a/solr/core/src/test/org/apache/solr/handler/admin/ZookeeperReadAPITest.java b/solr/core/src/test/org/apache/solr/handler/admin/ZookeeperReadAPITest.java index fd87fe340bc..0c2867362f3 100644 --- a/solr/core/src/test/org/apache/solr/handler/admin/ZookeeperReadAPITest.java +++ b/solr/core/src/test/org/apache/solr/handler/admin/ZookeeperReadAPITest.java @@ -17,20 +17,18 @@ package org.apache.solr.handler.admin; -import static org.apache.solr.common.util.StrUtils.split; -import static org.apache.solr.common.util.Utils.getObjectByPath; import static org.hamcrest.Matchers.containsInAnyOrder; -import com.fasterxml.jackson.databind.ObjectMapper; import java.net.URL; +import java.nio.charset.StandardCharsets; import java.util.Map; import java.util.stream.Collectors; -import org.apache.solr.client.api.model.ZooKeeperListChildrenResponse; +import org.apache.commons.io.IOUtils; import org.apache.solr.client.api.model.ZooKeeperStat; import org.apache.solr.client.solrj.impl.HttpSolrClient; +import org.apache.solr.client.solrj.request.ZookeeperReadApi; import org.apache.solr.cloud.SolrCloudTestCase; import org.apache.solr.common.SolrException; -import org.apache.solr.common.util.Utils; import org.apache.zookeeper.CreateMode; import org.junit.After; import org.junit.Before; @@ -69,20 +67,29 @@ public void tearDown() throws Exception { @Test public void testZkread() throws Exception { try (HttpSolrClient client = new HttpSolrClient.Builder(baseUrl.toString()).build()) { - Object o = - Utils.executeGET(client.getHttpClient(), basezk + "/security.json", Utils.JSONCONSUMER); - assertNotNull(o); - o = Utils.executeGET(client.getHttpClient(), basezkls + "/configs", Utils.JSONCONSUMER); + final var securityJsonRequest = new ZookeeperReadApi.ReadNode("/security.json"); + final var securityJsonResponse = securityJsonRequest.process(client); + assertEquals(200, securityJsonResponse.getHttpStatus()); + try (final var stream = securityJsonResponse.getResponseStream()) { + final var securityJsonContent = IOUtils.toString(stream, StandardCharsets.UTF_8); + assertNotNull(securityJsonContent); + } + + final var configListRequest = new ZookeeperReadApi.ListNodes("/configs"); + final var configListResponse = configListRequest.process(client).getParsed(); assertEquals( - "16", - String.valueOf(getObjectByPath(o, true, split(":/configs:_default:dataLength", ':')))); + 16, configListResponse.unknownProperties().get("/configs").get("_default").dataLength); assertEquals( - "16", String.valueOf(getObjectByPath(o, true, split(":/configs:conf:dataLength", ':')))); - assertEquals("0", String.valueOf(getObjectByPath(o, true, split("/stat/version", '/')))); - - o = Utils.executeGET(client.getHttpClient(), basezk + "/configs", Utils.JSONCONSUMER); - assertTrue(((Map) o).containsKey("zkData")); - assertEquals("empty", ((Map) o).get("zkData")); + 16, configListResponse.unknownProperties().get("/configs").get("conf").dataLength); + assertEquals(0, configListResponse.stat.version); + + final var configDataRequest = new ZookeeperReadApi.ReadNode("/configs"); + final var configDataResponse = configDataRequest.process(client); + // /configs exists but has no data, so API returns '200 OK' with empty response body + assertEquals(200, configDataResponse.getHttpStatus()); + try (final var stream = configDataResponse.getResponseStream()) { + assertEquals("", IOUtils.toString(stream, StandardCharsets.UTF_8)); + } byte[] bytes = new byte[1024 * 5]; for (int i = 0; i < bytes.length; i++) { @@ -92,17 +99,16 @@ public void testZkread() throws Exception { cluster .getZkClient() .create("/configs/_default/testdata", bytes, CreateMode.PERSISTENT, true); - Utils.executeGET( - client.getHttpClient(), - basezk + "/configs/_default/testdata", - is -> { - byte[] newBytes = new byte[bytes.length]; - is.read(newBytes); - for (int i = 0; i < newBytes.length; i++) { - assertEquals(bytes[i], newBytes[i]); - } - return null; - }); + + final var testDataRequest = new ZookeeperReadApi.ReadNode("/configs/_default/testdata"); + final var testDataResponse = testDataRequest.process(client); + assertEquals(200, testDataResponse.getHttpStatus()); + try (final var stream = testDataResponse.getResponseStream()) { + final var foundContents = IOUtils.toByteArray(stream); + for (int i = 0; i < foundContents.length; i++) { + assertEquals(foundContents[i], bytes[i]); + } + } } finally { cluster.getZkClient().delete("/configs/_default/testdata", -1, true); } @@ -112,15 +118,13 @@ public void testZkread() throws Exception { @Test public void testRequestingDataFromNonexistentNodeReturnsAnError() throws Exception { try (HttpSolrClient client = new HttpSolrClient.Builder(baseUrl.toString()).build()) { - final SolrException expected = + final var missingNodeReq = new ZookeeperReadApi.ReadNode("/configs/_default/nonexistentnode"); + final var missingNodeResponse = missingNodeReq.process(client); + assertEquals(404, missingNodeResponse.getHttpStatus()); + + final var expected = expectThrows( - SolrException.class, - () -> { - Utils.executeGET( - client.getHttpClient(), - basezk + "/configs/_default/nonexistentnode", - Utils.JSONCONSUMER); - }); + SolrException.class, () -> missingNodeResponse.getResponseStreamIfSuccessful()); assertEquals(404, expected.code()); } } @@ -128,26 +132,23 @@ public void testRequestingDataFromNonexistentNodeReturnsAnError() throws Excepti @Test public void testCanListChildNodes() throws Exception { try (HttpSolrClient client = new HttpSolrClient.Builder(baseUrl.toString()).build()) { - final ZooKeeperListChildrenResponse response = - Utils.executeGET( - client.getHttpClient(), - basezkls + "/configs/_default", - is -> { - return new ObjectMapper().readValue(is, ZooKeeperListChildrenResponse.class); - }); + final var listDefaultFilesReq = new ZookeeperReadApi.ListNodes("/configs/_default"); + final var listDefaultFilesResponse = listDefaultFilesReq.process(client).getParsed(); // At the top level, the response contains a key with the value of the specified zkPath - assertEquals(1, response.unknownProperties().size()); + assertEquals(1, listDefaultFilesResponse.unknownProperties().size()); assertEquals( "/configs/_default", - response.unknownProperties().keySet().stream().collect(Collectors.toList()).get(0)); + listDefaultFilesResponse.unknownProperties().keySet().stream() + .collect(Collectors.toList()) + .get(0)); // Under the specified zkPath is a key for each child, with values being that stat for that // node. // The actual stat values vary a good bit so aren't very useful to assert on, so let's just // make sure all of the expected child nodes were found. final Map childStatsByPath = - response.unknownProperties().get("/configs/_default"); + listDefaultFilesResponse.unknownProperties().get("/configs/_default"); assertEquals(6, childStatsByPath.size()); assertThat( childStatsByPath.keySet(), From e2d422589e7c95a1db50509ccaf4557510c7ce2c Mon Sep 17 00:00:00 2001 From: Jason Gerlowski Date: Fri, 17 Jan 2025 21:11:52 -0500 Subject: [PATCH 6/7] Address review comments, rd 1 --- .../solr/client/solrj/impl/InputStreamResponseParser.java | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) diff --git a/solr/solrj/src/java/org/apache/solr/client/solrj/impl/InputStreamResponseParser.java b/solr/solrj/src/java/org/apache/solr/client/solrj/impl/InputStreamResponseParser.java index 06b52626ef4..50ea192dea7 100644 --- a/solr/solrj/src/java/org/apache/solr/client/solrj/impl/InputStreamResponseParser.java +++ b/solr/solrj/src/java/org/apache/solr/client/solrj/impl/InputStreamResponseParser.java @@ -20,6 +20,7 @@ import java.io.Reader; import org.apache.solr.client.solrj.ResponseParser; import org.apache.solr.common.util.NamedList; +import org.apache.solr.common.util.SimpleOrderedMap; /** * Simply puts the InputStream into an entry in a NamedList named "stream". @@ -54,7 +55,7 @@ public NamedList processResponse(InputStream body, String encoding) { public static NamedList createInputStreamNamedList( int httpStatus, InputStream inputStream) { - final var nl = new NamedList(); + final var nl = new SimpleOrderedMap<>(); nl.add(STREAM_KEY, inputStream); nl.add(HTTP_STATUS_KEY, httpStatus); return nl; From f78e6bed4b069470f3f26eba4e01afc4a43ce24a Mon Sep 17 00:00:00 2001 From: Jason Gerlowski Date: Fri, 17 Jan 2025 21:42:32 -0500 Subject: [PATCH 7/7] Fix precommit, 1 --- .../org/apache/solr/handler/admin/ZookeeperReadAPITest.java | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/solr/core/src/test/org/apache/solr/handler/admin/ZookeeperReadAPITest.java b/solr/core/src/test/org/apache/solr/handler/admin/ZookeeperReadAPITest.java index 0c2867362f3..928109df2e0 100644 --- a/solr/core/src/test/org/apache/solr/handler/admin/ZookeeperReadAPITest.java +++ b/solr/core/src/test/org/apache/solr/handler/admin/ZookeeperReadAPITest.java @@ -104,7 +104,7 @@ public void testZkread() throws Exception { final var testDataResponse = testDataRequest.process(client); assertEquals(200, testDataResponse.getHttpStatus()); try (final var stream = testDataResponse.getResponseStream()) { - final var foundContents = IOUtils.toByteArray(stream); + final var foundContents = stream.readAllBytes(); for (int i = 0; i < foundContents.length; i++) { assertEquals(foundContents[i], bytes[i]); }