gerlowskija commented on code in PR #4264:
URL: https://github.com/apache/solr/pull/4264#discussion_r3044953893
##########
solr/api/src/java/org/apache/solr/client/api/endpoint/ConfigsetsApi.java:
##########
@@ -71,6 +79,46 @@ SolrJerseyResponse
deleteConfigSet(@PathParam("configSetName") String configSetN
throws Exception;
}
+ /**
+ * V2 API definition for downloading an existing configset as a ZIP archive.
+ *
+ * <p>Equivalent to GET /api/configsets/{configSetName}/download
+ */
+ @Path("/configsets/{configSetName}")
+ interface Download {
+ @GET
+ @Path("/download")
Review Comment:
[-1] `/download` isn't very REST-ful. The fact that we're
fetching/downloading the resource is already kindof implied by the HTTP 'GET'
verb/method.
Could we drop it and make this "just" `GET /api/configsets/{configsetName}`?
##########
solr/core/src/java/org/apache/solr/handler/configsets/DownloadConfigSet.java:
##########
@@ -0,0 +1,141 @@
+/*
+ * 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.configsets;
+
+import static
org.apache.solr.security.PermissionNameProvider.Name.CONFIG_READ_PERM;
+
+import jakarta.inject.Inject;
+import jakarta.ws.rs.core.Response;
+import jakarta.ws.rs.core.StreamingOutput;
+import java.io.ByteArrayOutputStream;
+import java.io.IOException;
+import java.io.InputStream;
+import java.nio.file.FileVisitResult;
+import java.nio.file.Files;
+import java.nio.file.Path;
+import java.nio.file.SimpleFileVisitor;
+import java.nio.file.attribute.BasicFileAttributes;
+import java.util.zip.ZipEntry;
+import java.util.zip.ZipOutputStream;
+import org.apache.commons.io.file.PathUtils;
+import org.apache.solr.client.api.endpoint.ConfigsetsApi;
+import org.apache.solr.common.SolrException;
+import org.apache.solr.common.util.StrUtils;
+import org.apache.solr.core.ConfigSetService;
+import org.apache.solr.core.CoreContainer;
+import org.apache.solr.jersey.PermissionName;
+import org.apache.solr.request.SolrQueryRequest;
+import org.apache.solr.response.SolrQueryResponse;
+
+/** V2 API implementation for downloading a configset as a zip file. */
+public class DownloadConfigSet extends ConfigSetAPIBase implements
ConfigsetsApi.Download {
+
+ @Inject
+ public DownloadConfigSet(
+ CoreContainer coreContainer,
+ SolrQueryRequest solrQueryRequest,
+ SolrQueryResponse solrQueryResponse) {
+ super(coreContainer, solrQueryRequest, solrQueryResponse);
+ }
+
+ @Override
+ @PermissionName(CONFIG_READ_PERM)
+ public Response downloadConfigSet(String configSetName, String displayName)
throws Exception {
+ if (StrUtils.isNullOrEmpty(configSetName)) {
+ throw new SolrException(
+ SolrException.ErrorCode.BAD_REQUEST, "No configset name provided to
download");
+ }
+ if (!configSetService.checkConfigExists(configSetName)) {
+ throw new SolrException(
+ SolrException.ErrorCode.NOT_FOUND, "ConfigSet " + configSetName + "
not found!");
+ }
+ final String resolvedDisplayName =
+ StrUtils.isNullOrEmpty(displayName) ? configSetName : displayName;
+ return buildZipResponse(configSetService, configSetName,
resolvedDisplayName);
+ }
+
+ /**
+ * Build a ZIP download {@link Response} for the given configset.
+ *
+ * @param configSetService the service to use for downloading the configset
files
+ * @param configSetId the internal configset name to download (may differ
from displayName, e.g.
+ * for schema-designer's mutable copies)
+ * @param displayName the user-visible name used to derive the download
filename
+ */
+ public static Response buildZipResponse(
+ ConfigSetService configSetService, String configSetId, String
displayName)
+ throws IOException {
+ final byte[] zipBytes = zipConfigSet(configSetService, configSetId);
+ final String safeName = displayName.replaceAll("[^a-zA-Z0-9_\\-.]", "_");
+ final String fileName = safeName + "_configset.zip";
+ return Response.ok((StreamingOutput) outputStream ->
outputStream.write(zipBytes))
+ .type("application/zip")
+ .header("Content-Disposition", "attachment; filename=\"" + fileName +
"\"")
+ .build();
+ }
+
+ /**
+ * Download the named configset from {@link ConfigSetService} and return its
contents as a ZIP
+ * archive byte array.
+ */
+ public static byte[] zipConfigSet(ConfigSetService configSetService, String
configSetId)
+ throws IOException {
+ ByteArrayOutputStream baos = new ByteArrayOutputStream();
+ Path tmpDirectory = Files.createTempDirectory("configset-download-");
+ try {
+ configSetService.downloadConfig(configSetId, tmpDirectory);
+ try (ZipOutputStream zipOut = new ZipOutputStream(baos)) {
Review Comment:
[Q] Am I right in thinking that all of this code was copied over from
SchemaDesignerConfigSetHelper? If so - should we delete the copy over in
SDCSH? Or at least have SDCSH call the public methods here?
##########
solr/core/src/java/org/apache/solr/handler/configsets/DownloadConfigSet.java:
##########
@@ -0,0 +1,141 @@
+/*
+ * 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.configsets;
+
+import static
org.apache.solr.security.PermissionNameProvider.Name.CONFIG_READ_PERM;
+
+import jakarta.inject.Inject;
+import jakarta.ws.rs.core.Response;
+import jakarta.ws.rs.core.StreamingOutput;
+import java.io.ByteArrayOutputStream;
+import java.io.IOException;
+import java.io.InputStream;
+import java.nio.file.FileVisitResult;
+import java.nio.file.Files;
+import java.nio.file.Path;
+import java.nio.file.SimpleFileVisitor;
+import java.nio.file.attribute.BasicFileAttributes;
+import java.util.zip.ZipEntry;
+import java.util.zip.ZipOutputStream;
+import org.apache.commons.io.file.PathUtils;
+import org.apache.solr.client.api.endpoint.ConfigsetsApi;
+import org.apache.solr.common.SolrException;
+import org.apache.solr.common.util.StrUtils;
+import org.apache.solr.core.ConfigSetService;
+import org.apache.solr.core.CoreContainer;
+import org.apache.solr.jersey.PermissionName;
+import org.apache.solr.request.SolrQueryRequest;
+import org.apache.solr.response.SolrQueryResponse;
+
+/** V2 API implementation for downloading a configset as a zip file. */
+public class DownloadConfigSet extends ConfigSetAPIBase implements
ConfigsetsApi.Download {
+
+ @Inject
+ public DownloadConfigSet(
+ CoreContainer coreContainer,
+ SolrQueryRequest solrQueryRequest,
+ SolrQueryResponse solrQueryResponse) {
+ super(coreContainer, solrQueryRequest, solrQueryResponse);
+ }
+
+ @Override
+ @PermissionName(CONFIG_READ_PERM)
+ public Response downloadConfigSet(String configSetName, String displayName)
throws Exception {
Review Comment:
[-1] We should be using `StreamingOutput` here instead of `Response`.
StreamingOutput is what we've been trying to standardize on for these "raw
output" APIs. Our code-gen templates are set up to support it, etc.
See https://issues.apache.org/jira/browse/SOLR-17562 for more details.
##########
solr/api/src/java/org/apache/solr/client/api/endpoint/ConfigsetsApi.java:
##########
@@ -71,6 +79,46 @@ SolrJerseyResponse
deleteConfigSet(@PathParam("configSetName") String configSetN
throws Exception;
}
+ /**
+ * V2 API definition for downloading an existing configset as a ZIP archive.
+ *
+ * <p>Equivalent to GET /api/configsets/{configSetName}/download
+ */
+ @Path("/configsets/{configSetName}")
+ interface Download {
+ @GET
+ @Path("/download")
+ @Operation(
+ summary = "Download a configset as a ZIP archive.",
+ tags = {"configsets"},
+ extensions = {
+ @Extension(properties = {@ExtensionProperty(name =
RAW_OUTPUT_PROPERTY, value = "true")})
+ })
+ @Produces("application/zip")
+ Response downloadConfigSet(
+ @PathParam("configSetName") String configSetName,
+ @QueryParam("displayName") String displayName)
+ throws Exception;
+ }
+
+ /**
+ * V2 API definition for reading a single file from an existing configset.
+ *
+ * <p>Equivalent to GET /api/configsets/{configSetName}/file?path=...
+ */
+ @Path("/configsets/{configSetName}")
+ interface GetFile {
+ @GET
+ @Path("/file")
+ @Produces(MediaType.TEXT_PLAIN)
+ @Operation(
+ summary = "Get the contents of a file in a configset.",
+ tags = {"configsets"})
+ ConfigSetFileContentsResponse getConfigSetFile(
+ @PathParam("configSetName") String configSetName, @QueryParam("path")
String filePath)
Review Comment:
[-0] IMO "filePath" should be a path parameter rather than a
query-parameter. That would allow this API to mirror the
"update-configset-file" API really well, and also bring it into line with what
we've done for other similar APIs in filestore and elsewhere.
To be explicit, I'm suggesting this API be: `GET
/api/configsets/<configSetName>/files/some/file/path.txt`
##########
solr/solr-ref-guide/modules/configuration-guide/pages/configsets-api.adoc:
##########
@@ -226,6 +288,78 @@ This behavior can be disabled with the parameter
`overwrite=false`, in which cas
====
======
+[[configsets-get-file]]
+== Get a Single File from a Configset
+
+This command retrieves the contents of a single file from an existing
configset.
Review Comment:
ditto, re: replacing "command" with "API" or less v1 specific language
##########
solr/solr-ref-guide/modules/configuration-guide/pages/configsets-api.adoc:
##########
@@ -226,6 +288,78 @@ This behavior can be disabled with the parameter
`overwrite=false`, in which cas
====
======
+[[configsets-get-file]]
+== Get a Single File from a Configset
+
+This command retrieves the contents of a single file from an existing
configset.
+This is useful for inspecting individual configuration files without
downloading the entire configset.
+
+The command takes the following parameters:
+
+`name`::
++
+[%autowidth,frame=none]
+|===
+s|Required |Default: none
+|===
++
+The name of the configset containing the file.
+
+`filePath`::
++
+[%autowidth,frame=none]
+|===
+s|Required |Default: none
+|===
++
+The path to the file within the configset (e.g., `solrconfig.xml` or
`lang/stopwords_en.txt`).
+
+The response will be a JSON object containing:
Review Comment:
[-0] Why "JSON" here? v2 APIs in general support javabin, xml, etc. in
addition to JSON. Is this API specifically JSON-only in some way? If not,
then we might not want to be overly specific here. Maybe drop the word "JSON"
and leave the rest as-is?
##########
solr/core/src/java/org/apache/solr/handler/configsets/CloneConfigSet.java:
##########
@@ -35,7 +35,11 @@
import org.apache.solr.request.SolrQueryRequest;
import org.apache.solr.response.SolrQueryResponse;
-/** V2 API implementation for ConfigsetsApi.Clone */
+/**
+ * V2 API implementation for creating a new configset form an existing one.
+ *
+ * <p>This API (GET /v2/configsets) is analogous to the v1
/admin/configs?action=CREATE command.
Review Comment:
[0] I can't remember why, but I think public opinion among the committers
turned against the `/v2/` prefix at some point. Technically it still works,
but we've been using `/api` in docs, etc. and not really mentioning `/v2/`
##########
solr/api/src/java/org/apache/solr/client/api/endpoint/ConfigsetsApi.java:
##########
@@ -71,6 +79,46 @@ SolrJerseyResponse
deleteConfigSet(@PathParam("configSetName") String configSetN
throws Exception;
}
+ /**
+ * V2 API definition for downloading an existing configset as a ZIP archive.
+ *
+ * <p>Equivalent to GET /api/configsets/{configSetName}/download
+ */
+ @Path("/configsets/{configSetName}")
+ interface Download {
+ @GET
+ @Path("/download")
+ @Operation(
+ summary = "Download a configset as a ZIP archive.",
+ tags = {"configsets"},
+ extensions = {
+ @Extension(properties = {@ExtensionProperty(name =
RAW_OUTPUT_PROPERTY, value = "true")})
+ })
+ @Produces("application/zip")
+ Response downloadConfigSet(
+ @PathParam("configSetName") String configSetName,
+ @QueryParam("displayName") String displayName)
Review Comment:
[Q] From looking ahead at code in this PR, it looks like 'displayName' is
used primarily (solely?) to inform the "attachment name" part of a
"Content-Disposition" response header.
Do I have that right? Or am I missing another usage?
##########
solr/api/src/java/org/apache/solr/client/api/endpoint/ConfigsetsApi.java:
##########
@@ -71,6 +79,46 @@ SolrJerseyResponse
deleteConfigSet(@PathParam("configSetName") String configSetN
throws Exception;
}
+ /**
+ * V2 API definition for downloading an existing configset as a ZIP archive.
+ *
+ * <p>Equivalent to GET /api/configsets/{configSetName}/download
+ */
+ @Path("/configsets/{configSetName}")
+ interface Download {
+ @GET
+ @Path("/download")
+ @Operation(
+ summary = "Download a configset as a ZIP archive.",
+ tags = {"configsets"},
+ extensions = {
+ @Extension(properties = {@ExtensionProperty(name =
RAW_OUTPUT_PROPERTY, value = "true")})
+ })
+ @Produces("application/zip")
+ Response downloadConfigSet(
+ @PathParam("configSetName") String configSetName,
+ @QueryParam("displayName") String displayName)
+ throws Exception;
+ }
+
+ /**
+ * V2 API definition for reading a single file from an existing configset.
+ *
+ * <p>Equivalent to GET /api/configsets/{configSetName}/file?path=...
+ */
+ @Path("/configsets/{configSetName}")
+ interface GetFile {
+ @GET
+ @Path("/file")
+ @Produces(MediaType.TEXT_PLAIN)
+ @Operation(
+ summary = "Get the contents of a file in a configset.",
+ tags = {"configsets"})
+ ConfigSetFileContentsResponse getConfigSetFile(
Review Comment:
[Q] Can you add a bit of context around the decision to return a structured
POJO here vs. "just" the verbatim file bytes approach we take in filestore,
replication handler, ZooKeeperReadAPI, etc.
The structured POJO route seems to conflict a bit with the
`@Produces(TEXT_PLAIN)` annotation above, unless I'm missing something?
How does this behave if a user puts a small binary file in their configset?
##########
solr/core/src/java/org/apache/solr/handler/configsets/UploadConfigSet.java:
##########
@@ -40,6 +40,11 @@
import org.slf4j.Logger;
import org.slf4j.LoggerFactory;
+/**
+ * V2 API implementation for uploading a configset as a zip file.
+ *
+ * <p>This API (GET /v2/configsets) is analogous to the v1
/admin/configs?action=UPLOAD command.
Review Comment:
Ditto, re: `/v2/`
##########
solr/api/src/java/org/apache/solr/client/api/endpoint/ConfigsetsApi.java:
##########
@@ -71,6 +79,46 @@ SolrJerseyResponse
deleteConfigSet(@PathParam("configSetName") String configSetN
throws Exception;
}
+ /**
+ * V2 API definition for downloading an existing configset as a ZIP archive.
+ *
+ * <p>Equivalent to GET /api/configsets/{configSetName}/download
Review Comment:
[-0] Not sure what you're trying to say here, but this could use some
word-smithing. There's no v1 counterpart to this API which is where I
typically see the "equivalent to" language.
Alternately If you're trying to highlight what the API actually looks like,
then maybe "Available at" works better? (Or omit altogether since the `@Path`
annotation is a just a line or two below)
##########
solr/core/src/java/org/apache/solr/handler/configsets/DownloadConfigSet.java:
##########
@@ -0,0 +1,141 @@
+/*
+ * 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.configsets;
+
+import static
org.apache.solr.security.PermissionNameProvider.Name.CONFIG_READ_PERM;
+
+import jakarta.inject.Inject;
+import jakarta.ws.rs.core.Response;
+import jakarta.ws.rs.core.StreamingOutput;
+import java.io.ByteArrayOutputStream;
+import java.io.IOException;
+import java.io.InputStream;
+import java.nio.file.FileVisitResult;
+import java.nio.file.Files;
+import java.nio.file.Path;
+import java.nio.file.SimpleFileVisitor;
+import java.nio.file.attribute.BasicFileAttributes;
+import java.util.zip.ZipEntry;
+import java.util.zip.ZipOutputStream;
+import org.apache.commons.io.file.PathUtils;
+import org.apache.solr.client.api.endpoint.ConfigsetsApi;
+import org.apache.solr.common.SolrException;
+import org.apache.solr.common.util.StrUtils;
+import org.apache.solr.core.ConfigSetService;
+import org.apache.solr.core.CoreContainer;
+import org.apache.solr.jersey.PermissionName;
+import org.apache.solr.request.SolrQueryRequest;
+import org.apache.solr.response.SolrQueryResponse;
+
+/** V2 API implementation for downloading a configset as a zip file. */
+public class DownloadConfigSet extends ConfigSetAPIBase implements
ConfigsetsApi.Download {
+
+ @Inject
+ public DownloadConfigSet(
+ CoreContainer coreContainer,
+ SolrQueryRequest solrQueryRequest,
+ SolrQueryResponse solrQueryResponse) {
+ super(coreContainer, solrQueryRequest, solrQueryResponse);
+ }
+
+ @Override
+ @PermissionName(CONFIG_READ_PERM)
+ public Response downloadConfigSet(String configSetName, String displayName)
throws Exception {
+ if (StrUtils.isNullOrEmpty(configSetName)) {
+ throw new SolrException(
+ SolrException.ErrorCode.BAD_REQUEST, "No configset name provided to
download");
+ }
+ if (!configSetService.checkConfigExists(configSetName)) {
+ throw new SolrException(
+ SolrException.ErrorCode.NOT_FOUND, "ConfigSet " + configSetName + "
not found!");
+ }
+ final String resolvedDisplayName =
+ StrUtils.isNullOrEmpty(displayName) ? configSetName : displayName;
+ return buildZipResponse(configSetService, configSetName,
resolvedDisplayName);
+ }
+
+ /**
+ * Build a ZIP download {@link Response} for the given configset.
+ *
+ * @param configSetService the service to use for downloading the configset
files
+ * @param configSetId the internal configset name to download (may differ
from displayName, e.g.
+ * for schema-designer's mutable copies)
+ * @param displayName the user-visible name used to derive the download
filename
+ */
+ public static Response buildZipResponse(
+ ConfigSetService configSetService, String configSetId, String
displayName)
+ throws IOException {
+ final byte[] zipBytes = zipConfigSet(configSetService, configSetId);
+ final String safeName = displayName.replaceAll("[^a-zA-Z0-9_\\-.]", "_");
+ final String fileName = safeName + "_configset.zip";
+ return Response.ok((StreamingOutput) outputStream ->
outputStream.write(zipBytes))
+ .type("application/zip")
+ .header("Content-Disposition", "attachment; filename=\"" + fileName +
"\"")
Review Comment:
[Q] "Content-Disposition" is a browser-based header AFAICT - it's primarily
a hint for browsers as to whether something's an attachment or not. Is it
worth having here? Why would we include such a header here, but not on (e.g.)
the filestore API or in ReplicationHandler, or somewhere similar that's
downloading file content?
##########
solr/core/src/test/org/apache/solr/handler/configsets/UploadConfigSetAPITest.java:
##########
@@ -0,0 +1,427 @@
+/*
+ * 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.configsets;
+
+import static org.apache.solr.SolrTestCaseJ4.assumeWorkingMockito;
+import static org.mockito.Mockito.mock;
+import static org.mockito.Mockito.when;
+
+import java.io.ByteArrayInputStream;
+import java.io.ByteArrayOutputStream;
+import java.io.InputStream;
+import java.nio.charset.StandardCharsets;
+import java.nio.file.Files;
+import java.nio.file.Path;
+import java.util.zip.ZipEntry;
+import java.util.zip.ZipOutputStream;
+import org.apache.solr.SolrTestCase;
+import org.apache.solr.common.SolrException;
+import org.apache.solr.core.CoreContainer;
+import org.apache.solr.core.FileSystemConfigSetService;
+import org.junit.Before;
+import org.junit.BeforeClass;
+import org.junit.Test;
+
+/** Unit tests for {@link UploadConfigSet}. */
+public class UploadConfigSetAPITest extends SolrTestCase {
Review Comment:
[Q] Can you share a bit of context around this test file please? I'm not
going to complain about new tests, but I'm surprised to find that a third of
this PR is tests for an API that wasn't touched by the PR 😛
##########
solr/solr-ref-guide/modules/configuration-guide/pages/configsets-api.adoc:
##########
@@ -82,6 +82,68 @@ The output will look like:
}
----
+[[configsets-download]]
+== Download a Configset
+
+The `download` command downloads an entire configset as a zipped file.
Review Comment:
[0] The "command" language here is unfortunate. It fits the v1 APIs and
their `action=LIST|UPLOAD|CREATE|etc` syntax well, but doesn't make sense for
the v2 API IMO.
Maybe something like the following would be a bit clearer:
> The v2 API allows configsets to be downloaded as a single zipped file.
This is useful for backing up configsets, sharing ...
> The download API takes the following parameters:
--
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]
