You are viewing a plain text version of this content. The canonical link for it is here.
Posted to issues@solr.apache.org by "gerlowskija (via GitHub)" <gi...@apache.org> on 2025/01/14 15:06:02 UTC
[PR] SOLR-17562: Unify JAX-RS "raw response" endpoints [solr]
gerlowskija opened a new pull request, #3032:
URL: https://github.com/apache/solr/pull/3032
https://issues.apache.org/jira/browse/SOLR-17562
# Description
Several v2 APIs return raw files or streams of data, including: ZooKeeperReadAPI, NodeFileStore, and CoreReplication.fetchFile.
But the APIs vary slightly in how they support this: ZooKeeperReadAPI uses the deprecated "ContentStream" with "RawResponseWriter", NodeFileStore directly attaches a "SolrCore.RawWriter" to the underlying SolrQueryResponse, and CoreReplication follows the JAX-RS best practice of using the "StreamingOutput" interface.
The biggest hurdle to aligning on a single approach is modifying the templates used by our SolrJ code generation to handle the new approach (and "big stream of data" style responses in general).
# Solution
This PR tackles this by introducing a new SolrResponse implementation, `InputStreamResponse`, to wrap the NamedList produced by the existing `InputStreamResponseParser`. (InputStreamResponseParser creates a NamedList with two entries, "responseStatus" (an int) and "stream" (an InputStream)). The new SolrResponse type is used as the response type by our code-gen template for any "raw" responses annotated with a "rawOutput" tag introduced by this PR.
Docs are added in `dev-docs/v2-api-conventions.adoc` to highlight this setup.
NOTE: this PR provides a unified approach for "raw" API responses (e.g. index files returned by replication APIs, blobs stored in the filestore, etc.). Callers of these APIs essentially get a glorified InputStream to work with on the client side. The PR doesn't aim to handle other APIs that stream more structured data, e.g. the doc-stream returned by `/export`. We can likely do better than a "glorified InputStream" in these cases, but this PR makes no attempt to provide that.
# Tests
Additional unit tests in `InputStreamResponseTest`. Existing tests for the ZK and replication APIs continue to pass.
# Checklist
Please review the following and check all that apply:
- [x] I have reviewed the guidelines for [How to Contribute](https://github.com/apache/solr/blob/main/CONTRIBUTING.md) and my code conforms to the standards described there to the best of my ability.
- [x] I have created a Jira issue and added the issue ID to my pull request title.
- [x] I have given Solr maintainers [access](https://help.github.com/en/articles/allowing-changes-to-a-pull-request-branch-created-from-a-fork) to contribute to my PR branch. (optional but recommended, not available for branches on forks living under an organisation)
- [x] I have developed this patch against the `main` branch.
- [ ] I have run `./gradlew check`.
- [x] I have added tests for my changes.
- [x] I have added documentation
--
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
Re: [PR] SOLR-17562: Unify JAX-RS "raw response" endpoints [solr]
Posted by "dsmiley (via GitHub)" <gi...@apache.org>.
dsmiley commented on code in PR #3032:
URL: https://github.com/apache/solr/pull/3032#discussion_r1920943164
##########
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";
Review Comment:
Okay; not worth changing the name over.
--
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
Re: [PR] SOLR-17562: Unify JAX-RS "raw response" endpoints [solr]
Posted by "dsmiley (via GitHub)" <gi...@apache.org>.
dsmiley commented on code in PR #3032:
URL: https://github.com/apache/solr/pull/3032#discussion_r1915979535
##########
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}.
+ *
+ * <p>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<Integer, Boolean> HTTP_OK_VALIDATOR = (status) -> 200 == status;
+ // True if the HTTP status is neither a 4xx or a 5xx error.
+ public static final Function<Integer, Boolean> NON_ERROR_CODE_VALIDATOR =
+ (status) -> status < 399;
+
+ @Override
+ public void setResponse(NamedList<Object> 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
+ *
+ * <p>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<Object> 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')
+ *
+ * <p>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<Integer, Boolean> statusValidator) {
+ validateExpectedStatus(statusValidator);
+ return getResponseStream();
+ }
+
+ private void validateExpectedStatus(Function<Integer, Boolean> 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);
Review Comment:
I question that it's this method's job to use ObjectReleaseTracker
--
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
Re: [PR] SOLR-17562: Unify JAX-RS "raw response" endpoints [solr]
Posted by "dsmiley (via GitHub)" <gi...@apache.org>.
dsmiley commented on code in PR #3032:
URL: https://github.com/apache/solr/pull/3032#discussion_r1919153386
##########
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";
Review Comment:
would httpStatus be a better name? (assuming it's 200 not 0 when things are good)
##########
solr/solrj/src/java/org/apache/solr/client/solrj/impl/InputStreamResponseParser.java:
##########
@@ -44,4 +51,12 @@ public NamedList<Object> processResponse(Reader reader) {
public NamedList<Object> processResponse(InputStream body, String encoding) {
throw new UnsupportedOperationException();
}
+
+ public static NamedList<Object> createInputStreamNamedList(
+ int httpStatus, InputStream inputStream) {
+ final var nl = new NamedList<Object>();
Review Comment:
Please always use a SimpleOrderedMap when the keys don't repeat
--
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
Re: [PR] SOLR-17562: Unify JAX-RS "raw response" endpoints [solr]
Posted by "gerlowskija (via GitHub)" <gi...@apache.org>.
gerlowskija commented on code in PR #3032:
URL: https://github.com/apache/solr/pull/3032#discussion_r1920929167
##########
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";
Review Comment:
I agree that "httpStatus" is much better name-wise, but changing the name of the key would break any SolrJ users that make use of InputStreamResponseParser.
We could tweak the 9.x backport of this PR to make the status-code available under both 'responseStatus' and 'httpStatus' (and "deprecate" the former). But that gets pretty far afield from the main goal of this PR, so I might skip it for reasons of scope.
--
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