gerlowskija commented on code in PR #4264:
URL: https://github.com/apache/solr/pull/4264#discussion_r3081947285
##########
solr/api/src/java/org/apache/solr/client/api/endpoint/ConfigsetsApi.java:
##########
@@ -95,54 +91,63 @@ interface Download {
@Extension(properties = {@ExtensionProperty(name =
RAW_OUTPUT_PROPERTY, value = "true")})
})
@Produces("application/zip")
- Response downloadConfigSet(
- @PathParam("configSetName") String configSetName,
- @QueryParam("displayName") String displayName)
- throws Exception;
+ Response downloadConfigSet(@PathParam("configSetName") String
configSetName) throws Exception;
}
/**
* V2 API definition for reading a single file from an existing configset.
*
- * <p>Equivalent to GET /api/configsets/{configSetName}/file?path=...
+ * <p>Returns the raw bytes of the file, suitable for both text and binary
files.
+ *
+ * <p>Equivalent to GET /api/configsets/{configSetName}/files/{filePath}
*/
@Path("/configsets/{configSetName}")
interface GetFile {
@GET
- @Path("/file")
- @Produces(MediaType.TEXT_PLAIN)
+ @Path("/files/{filePath:.+}")
+ @Produces(MediaType.APPLICATION_OCTET_STREAM)
@Operation(
- summary = "Get the contents of a file in a configset.",
- tags = {"configsets"})
- ConfigSetFileContentsResponse getConfigSetFile(
- @PathParam("configSetName") String configSetName, @QueryParam("path")
String filePath)
+ summary = "Get the raw contents of a file in a configset.",
+ tags = {"configsets"},
+ extensions = {
+ @Extension(properties = {@ExtensionProperty(name =
RAW_OUTPUT_PROPERTY, value = "true")})
+ })
+ StreamingOutput getConfigSetFile(
+ @PathParam("configSetName") String configSetName,
@PathParam("filePath") String filePath)
throws Exception;
}
/**
- * V2 API definitions for uploading a configset, in whole or part.
+ * V2 API definition for uploading an entire configset as a ZIP archive.
*
* <p>Equivalent to the existing v1 API /admin/configs?action=UPLOAD
*/
@Path("/configsets/{configSetName}")
interface Upload {
@PUT
- @Operation(summary = "Create a new configset.", tags = "configsets")
+ @Operation(summary = "Upload a configset as a ZIP archive.", tags =
"configsets")
SolrJerseyResponse uploadConfigSet(
@PathParam("configSetName") String configSetName,
@QueryParam("overwrite") Boolean overwrite,
@QueryParam("cleanup") Boolean cleanup,
@RequestBody(required = true) InputStream requestBody)
throws IOException;
+ }
+ /**
+ * V2 API definition for putting a single file to an existing configset.
+ *
+ * <p>This endpoint allows updating individual configuration files without
re-uploading the entire
+ * configset. The file path is specified as part of the URL path.
+ */
+ @Path("/configsets/{configSetName}")
+ interface PutFile {
@PUT
@Path("{filePath:.+}")
Review Comment:
[-0] We should probably add `/files` to the path here to align with what we
now have for `getConfigSetFile` above.
##########
solr/core/src/java/org/apache/solr/core/FileSystemConfigSetService.java:
##########
@@ -182,6 +182,11 @@ public void uploadFileToConfig(
}
if (overwriteOnExists || !Files.exists(configsetFilePath)) {
+ // Create parent directories if they don't exist (similar to ZK's
makePath)
Review Comment:
[Q] Why is this code here?
This feels kindof suspect at a glance, from a security perspective. The
risk is a bit lower in ZK, but since we're on the filesystem here we probably
shouldn't blindly be creating directories based on a user-specified parameter.
If we're 100% certain that there's no path-escape or any other funny-business
possible here, maybe this is fine. But I'd think this through really carefully
if you haven't already.
##########
solr/core/src/java/org/apache/solr/handler/configsets/DownloadConfigSet.java:
##########
@@ -63,28 +63,20 @@ public Response downloadConfigSet(String configSetName,
String displayName) thro
throw new SolrException(
SolrException.ErrorCode.NOT_FOUND, "ConfigSet " + configSetName + "
not found!");
}
- final String resolvedDisplayName =
- StrUtils.isNullOrEmpty(displayName) ? configSetName : displayName;
- return buildZipResponse(configSetService, configSetName,
resolvedDisplayName);
+ return buildZipResponse(configSetService, configSetName);
}
/**
* 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
+ * @param configSetId the internal configset name to download
Review Comment:
[0] "internal" probably isn't needed here anymore.
##########
solr/core/src/java/org/apache/solr/handler/configsets/UploadConfigSet.java:
##########
@@ -108,27 +109,20 @@ public SolrJerseyResponse uploadConfigSet(
@Override
@PermissionName(CONFIG_EDIT_PERM)
- public SolrJerseyResponse uploadConfigSetFile(
- String configSetName,
- String filePath,
- Boolean overwrite,
- Boolean cleanup,
- InputStream requestBody)
- throws IOException {
+ public SolrJerseyResponse putConfigSetFile(
Review Comment:
[0] I liked the old method name better, fwiw. `uploadConfigSetFile` is
clearer than `putConfigSetFile`
##########
solr/api/src/java/org/apache/solr/client/api/endpoint/ConfigsetsApi.java:
##########
@@ -95,54 +91,63 @@ interface Download {
@Extension(properties = {@ExtensionProperty(name =
RAW_OUTPUT_PROPERTY, value = "true")})
})
@Produces("application/zip")
- Response downloadConfigSet(
- @PathParam("configSetName") String configSetName,
- @QueryParam("displayName") String displayName)
- throws Exception;
+ Response downloadConfigSet(@PathParam("configSetName") String
configSetName) throws Exception;
}
/**
* V2 API definition for reading a single file from an existing configset.
*
- * <p>Equivalent to GET /api/configsets/{configSetName}/file?path=...
+ * <p>Returns the raw bytes of the file, suitable for both text and binary
files.
+ *
+ * <p>Equivalent to GET /api/configsets/{configSetName}/files/{filePath}
*/
@Path("/configsets/{configSetName}")
interface GetFile {
@GET
- @Path("/file")
- @Produces(MediaType.TEXT_PLAIN)
+ @Path("/files/{filePath:.+}")
+ @Produces(MediaType.APPLICATION_OCTET_STREAM)
@Operation(
- summary = "Get the contents of a file in a configset.",
- tags = {"configsets"})
- ConfigSetFileContentsResponse getConfigSetFile(
- @PathParam("configSetName") String configSetName, @QueryParam("path")
String filePath)
+ summary = "Get the raw contents of a file in a configset.",
+ tags = {"configsets"},
+ extensions = {
+ @Extension(properties = {@ExtensionProperty(name =
RAW_OUTPUT_PROPERTY, value = "true")})
+ })
+ StreamingOutput getConfigSetFile(
+ @PathParam("configSetName") String configSetName,
@PathParam("filePath") String filePath)
throws Exception;
}
/**
- * V2 API definitions for uploading a configset, in whole or part.
+ * V2 API definition for uploading an entire configset as a ZIP archive.
*
* <p>Equivalent to the existing v1 API /admin/configs?action=UPLOAD
*/
@Path("/configsets/{configSetName}")
interface Upload {
@PUT
- @Operation(summary = "Create a new configset.", tags = "configsets")
+ @Operation(summary = "Upload a configset as a ZIP archive.", tags =
"configsets")
SolrJerseyResponse uploadConfigSet(
@PathParam("configSetName") String configSetName,
@QueryParam("overwrite") Boolean overwrite,
@QueryParam("cleanup") Boolean cleanup,
@RequestBody(required = true) InputStream requestBody)
throws IOException;
+ }
+ /**
+ * V2 API definition for putting a single file to an existing configset.
+ *
+ * <p>This endpoint allows updating individual configuration files without
re-uploading the entire
+ * configset. The file path is specified as part of the URL path.
+ */
+ @Path("/configsets/{configSetName}")
+ interface PutFile {
@PUT
@Path("{filePath:.+}")
- @Operation(summary = "Create a new configset.", tags = "configsets")
- SolrJerseyResponse uploadConfigSetFile(
+ @Operation(summary = "Upload a single file to a configset.", tags =
"configsets")
+ SolrJerseyResponse putConfigSetFile(
@PathParam("configSetName") String configSetName,
@PathParam("filePath") String filePath,
- @QueryParam("overwrite") Boolean overwrite,
Review Comment:
[Q] Can you please add some context on why these boolean flags are going
away?
'Overwrite' in particular seems like a nice safety check that I imagine
users would like to use.
##########
solr/core/src/test/org/apache/solr/cloud/TestConfigSetsAPI.java:
##########
@@ -765,15 +735,9 @@ private void testNewSingleFileAfterSchemaAPI(boolean v2)
throws Exception {
zkClient, configsetName, configsetSuffix,
"test/upload/path/solrconfig.xml"));
}
- @Test
- public void testSingleWithCleanupV1() throws Exception {
- testSingleWithCleanup(false);
- }
+ // Single file uploads do not support cleanup parameter
- @Test
- public void testSingleWithCleanupV2() throws Exception {
- testSingleWithCleanup(true);
- }
+ // V2 API not tested: single file uploads do not support cleanup parameter
Review Comment:
[Q] Can you add a little context on what this comment refers to?
The method immediately following this comment has a 'v2' method param...so,
something seems a little mixed up here. If we're never going to invoke the
method below with a 'true' param value, why are we keeping that codepath in the
first place?
##########
solr/core/src/test/org/apache/solr/handler/configsets/GetConfigSetFileAPITest.java:
##########
@@ -126,10 +135,70 @@ public void testEmptyFileReturnsEmptyContent() throws
Exception {
createConfigSetWithFile(configSetName, filePath, "");
final var api = new GetConfigSetFile(mockCoreContainer, null, null);
- final ConfigSetFileContentsResponse response =
api.getConfigSetFile(configSetName, filePath);
+ final StreamingOutput streamingOutput =
api.getConfigSetFile(configSetName, filePath);
+
+ assertNotNull(streamingOutput);
+
+ // Read the streamed bytes
+ ByteArrayOutputStream baos = new ByteArrayOutputStream();
+ streamingOutput.write(baos);
+
+ assertEquals(0, baos.size());
+ }
+
+ @Test
+ public void testBinaryFilePreservedWithStreamingOutput() throws Exception {
+ // This test demonstrates that binary files are now correctly handled
+ // by returning raw bytes via StreamingOutput instead of UTF-8 String.
+ final String configSetName = "binarytest";
+ final String filePath = "logo.png";
+
+ // Create a file with binary data simulating a small PNG image header
+ // PNG signature: 89 50 4E 47 0D 0A 1A 0A (first 8 bytes of any PNG)
+ // Plus some additional binary data that would corrupt as UTF-8
+ byte[] binaryData =
+ new byte[] {
+ (byte) 0x89,
+ 0x50,
+ 0x4E,
+ 0x47,
+ 0x0D,
+ 0x0A,
+ 0x1A,
+ 0x0A, // PNG signature
+ (byte) 0xFF,
+ (byte) 0xFE,
+ 0x00,
+ 0x01,
+ (byte) 0x80,
+ (byte) 0xDE,
+ (byte) 0xAD,
+ (byte) 0xBE,
+ (byte) 0xEF // Binary data
+ };
+
+ Path configDir = configSetBase.resolve(configSetName);
+ Files.createDirectories(configDir);
+ Files.write(configDir.resolve(filePath), binaryData);
+
+ final var api = new GetConfigSetFile(mockCoreContainer, null, null);
+ final StreamingOutput streamingOutput =
api.getConfigSetFile(configSetName, filePath);
+
+ assertNotNull(streamingOutput);
+
+ // Read the streamed bytes
+ ByteArrayOutputStream baos = new ByteArrayOutputStream();
+ streamingOutput.write(baos);
+ byte[] responseBytes = baos.toByteArray();
+
+ // SUCCESS: Binary data is preserved exactly!
+ assertArrayEquals(
+ "Binary file content should be preserved byte-for-byte", binaryData,
responseBytes);
- assertNotNull(response);
- assertEquals(filePath, response.path);
- assertEquals("", response.content);
+ // Verify PNG signature is intact
+ assertEquals((byte) 0x89, responseBytes[0]);
Review Comment:
[-0] We already did an 'assertArrayEquals' above to validate that we
received exactly what we expected...what do these assertions provide in
addition to that?
##########
solr/solr-ref-guide/modules/upgrade-notes/pages/major-changes-in-solr-10.adoc:
##########
@@ -300,3 +331,7 @@ Older segments will continue to be readable.
WARNING: After upgrading to Solr 10.1, downgrading to an earlier Solr 10.0.x
version may fail because the older version does not include the `Lucene104`
codec needed to read the newly written segments.
If you require the ability to roll back, back up your indexes before upgrading.
+
+=== API Changes
+
+Solr 10.1 changes the V1 API signature for uploading a single file to remove
the overwrite parameter. Previously the default for `overwrite` is false when
using the v1 API, but true when using the v2 API.
Review Comment:
[-0] I'm not sure I understand what this means. Does "API signature" here
mean the method-signature in the Java code? Or are saying that the v1 API no
longer supports this param at all?
I must've missed it earlier on in my review, but if we're removing support
for a parameter from v1 can you share a bit more context on why that was done?
##########
solr/core/src/test/org/apache/solr/handler/configsets/PutConfigSetFileAPITest.java:
##########
@@ -0,0 +1,120 @@
+/*
+ * 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.InputStream;
+import java.nio.charset.StandardCharsets;
+import java.nio.file.Files;
+import java.nio.file.Path;
+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#putConfigSetFile} (PutFile
interface). */
+public class PutConfigSetFileAPITest extends SolrTestCase {
+
+ private CoreContainer mockCoreContainer;
+ private FileSystemConfigSetService configSetService;
+ private Path configSetBase;
+
+ @BeforeClass
+ public static void ensureWorkingMockito() {
+ assumeWorkingMockito();
+ }
+
+ @Before
+ public void initConfigSetService() {
+ configSetBase = createTempDir("configsets");
+ // Use an anonymous subclass to access the protected testing constructor
+ configSetService = new FileSystemConfigSetService(configSetBase) {};
+ mockCoreContainer = mock(CoreContainer.class);
+ when(mockCoreContainer.getConfigSetService()).thenReturn(configSetService);
+ }
+
+ /** Creates a configset with files on disk for testing overwrites. */
+ private void createExistingConfigSet(String configSetName, String...
filePathAndContent)
+ throws Exception {
+ Path configDir = configSetBase.resolve(configSetName);
+ Files.createDirectories(configDir);
+ for (int i = 0; i < filePathAndContent.length; i += 2) {
+ String filePath = filePathAndContent[i];
+ String content = filePathAndContent[i + 1];
+ Path fullPath = configDir.resolve(filePath);
+ Files.createDirectories(fullPath.getParent());
+ Files.writeString(fullPath, content, StandardCharsets.UTF_8);
+ }
+ }
+
+ @Test
+ public void testSingleFileUploadSuccess() throws Exception {
+ final String configSetName = "singlefile";
+ final String filePath = "solrconfig.xml";
+ final String content = "<config/>";
+ InputStream fileStream = new
ByteArrayInputStream(content.getBytes(StandardCharsets.UTF_8));
+
+ final var api = new UploadConfigSet(mockCoreContainer, null, null);
+ final var response = api.putConfigSetFile(configSetName, filePath,
fileStream);
+
+ assertNotNull(response);
Review Comment:
[0] IMO these not-null checks don't provide a ton of value. If you want to
validate the response, look at the responseHeader 'status' field, or make sure
that the 'error' property is null/empty, or...
##########
solr/core/src/test/org/apache/solr/handler/configsets/GetConfigSetFileAPITest.java:
##########
@@ -114,8 +120,11 @@ public void testFileNotFoundInConfigSetThrowsNotFound()
throws Exception {
Files.createDirectories(configSetBase.resolve(configSetName));
final var api = new GetConfigSetFile(mockCoreContainer, null, null);
- final var ex =
- assertThrows(SolrException.class, () ->
api.getConfigSetFile(configSetName, "missing.xml"));
+ final StreamingOutput streamingOutput =
api.getConfigSetFile(configSetName, "missing.xml");
Review Comment:
[-0/Q] Shouldn't there be a SolrException that gets thrown from
`getConfigSetFile`? We want to be returning 404's on "not found" cases.
##########
solr/core/src/java/org/apache/solr/handler/configsets/UploadConfigSet.java:
##########
@@ -108,27 +109,20 @@ public SolrJerseyResponse uploadConfigSet(
@Override
@PermissionName(CONFIG_EDIT_PERM)
- public SolrJerseyResponse uploadConfigSetFile(
- String configSetName,
- String filePath,
- Boolean overwrite,
- Boolean cleanup,
- InputStream requestBody)
- throws IOException {
+ public SolrJerseyResponse putConfigSetFile(
+ String configSetName, String filePath, InputStream requestBody) throws
IOException {
final var response = instantiateJerseyResponse(SolrJerseyResponse.class);
ensureConfigSetUploadEnabled();
SolrIdentifierValidator.validateConfigSetName(configSetName);
- boolean overwritesExisting =
configSetService.checkConfigExists(configSetName);
+ boolean overwrite = true;
Review Comment:
[Q] I've commented elsewhere asking why 'overwrite' is removed in this PR.
IMO they're probably worth keeping.
But if we do remove them...what purpose does this variable serve? At a
glance I don't see it used anywhere?
##########
solr/core/src/test/org/apache/solr/handler/configsets/GetConfigSetFileAPITest.java:
##########
@@ -126,10 +135,70 @@ public void testEmptyFileReturnsEmptyContent() throws
Exception {
createConfigSetWithFile(configSetName, filePath, "");
final var api = new GetConfigSetFile(mockCoreContainer, null, null);
- final ConfigSetFileContentsResponse response =
api.getConfigSetFile(configSetName, filePath);
+ final StreamingOutput streamingOutput =
api.getConfigSetFile(configSetName, filePath);
+
+ assertNotNull(streamingOutput);
+
+ // Read the streamed bytes
+ ByteArrayOutputStream baos = new ByteArrayOutputStream();
+ streamingOutput.write(baos);
+
+ assertEquals(0, baos.size());
+ }
+
+ @Test
+ public void testBinaryFilePreservedWithStreamingOutput() throws Exception {
+ // This test demonstrates that binary files are now correctly handled
+ // by returning raw bytes via StreamingOutput instead of UTF-8 String.
+ final String configSetName = "binarytest";
+ final String filePath = "logo.png";
+
+ // Create a file with binary data simulating a small PNG image header
+ // PNG signature: 89 50 4E 47 0D 0A 1A 0A (first 8 bytes of any PNG)
+ // Plus some additional binary data that would corrupt as UTF-8
+ byte[] binaryData =
+ new byte[] {
+ (byte) 0x89,
+ 0x50,
+ 0x4E,
+ 0x47,
+ 0x0D,
+ 0x0A,
+ 0x1A,
+ 0x0A, // PNG signature
+ (byte) 0xFF,
+ (byte) 0xFE,
+ 0x00,
+ 0x01,
+ (byte) 0x80,
+ (byte) 0xDE,
+ (byte) 0xAD,
+ (byte) 0xBE,
+ (byte) 0xEF // Binary data
+ };
+
+ Path configDir = configSetBase.resolve(configSetName);
+ Files.createDirectories(configDir);
+ Files.write(configDir.resolve(filePath), binaryData);
+
+ final var api = new GetConfigSetFile(mockCoreContainer, null, null);
+ final StreamingOutput streamingOutput =
api.getConfigSetFile(configSetName, filePath);
+
+ assertNotNull(streamingOutput);
+
+ // Read the streamed bytes
+ ByteArrayOutputStream baos = new ByteArrayOutputStream();
Review Comment:
[0] This snippet appears a bunch in this test class and should probably be a
helper method to enable reuse.
##########
solr/core/src/test/org/apache/solr/handler/configsets/PutConfigSetFileAPITest.java:
##########
@@ -0,0 +1,120 @@
+/*
+ * 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.InputStream;
+import java.nio.charset.StandardCharsets;
+import java.nio.file.Files;
+import java.nio.file.Path;
+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#putConfigSetFile} (PutFile
interface). */
+public class PutConfigSetFileAPITest extends SolrTestCase {
+
+ private CoreContainer mockCoreContainer;
+ private FileSystemConfigSetService configSetService;
+ private Path configSetBase;
+
+ @BeforeClass
+ public static void ensureWorkingMockito() {
+ assumeWorkingMockito();
+ }
+
+ @Before
+ public void initConfigSetService() {
+ configSetBase = createTempDir("configsets");
+ // Use an anonymous subclass to access the protected testing constructor
+ configSetService = new FileSystemConfigSetService(configSetBase) {};
+ mockCoreContainer = mock(CoreContainer.class);
+ when(mockCoreContainer.getConfigSetService()).thenReturn(configSetService);
+ }
+
+ /** Creates a configset with files on disk for testing overwrites. */
+ private void createExistingConfigSet(String configSetName, String...
filePathAndContent)
Review Comment:
[-1] Nothing ever calls this.
##########
solr/core/src/test/org/apache/solr/cloud/TestConfigSetsAPI.java:
##########
@@ -1117,6 +1001,90 @@ public void testUploadWithForbiddenContent() throws
Exception {
assertEquals(400, res);
}
+ @Test
+ public void testGetFile() throws Exception {
+ String configSetName = "regular";
+ String configSetSuffix = "testGetFile";
+
+ // First upload a configset
+ uploadConfigSetWithAssertions(configSetName, configSetSuffix, null);
+
+ try (SolrZkClient zkClient =
+ new SolrZkClient.Builder()
+ .withUrl(cluster.getZkServer().getZkAddress())
+ .withTimeout(AbstractZkTestCase.TIMEOUT, TimeUnit.MILLISECONDS)
+ .withConnTimeOut(45000, TimeUnit.MILLISECONDS)
+ .build()) {
+
+ // Verify files exist in ZK
+ assertTrue(
+ zkClient.exists("/configs/" + configSetName + configSetSuffix +
"/solrconfig.xml"));
+ assertTrue(
+ zkClient.exists("/configs/" + configSetName + configSetSuffix +
"/managed-schema.xml"));
+
+ // Test getting a root-level file via V2 API - now returns raw bytes via
StreamingOutput
+ String baseUrl =
cluster.getJettySolrRunners().get(0).getBaseURLV2().toString();
+ String getFileUrl =
+ baseUrl + "/configsets/" + configSetName + configSetSuffix +
"/files/solrconfig.xml";
+
+ HttpClient httpClient =
cluster.getJettySolrRunners().get(0).getSolrClient().getHttpClient();
+ ContentResponse response =
httpClient.newRequest(getFileUrl).method(HttpMethod.GET).send();
+
+ // Response should be raw bytes (application/octet-stream), not JSON
Review Comment:
[-0] Some of your code comments here and elsewhere have a habit of referring
back to earlier versions of this PR. They're helpful at review time for me,
but IMO they're not going to age well once the PR is merged.
To take this one as an example: I understand why you're talking about JSON
here - because we almost went that route in this PR. But to anyone that
stumbles on this comment post-merge without that context, it's going to sound
out of left field: "Well of course, why *would* the response be JSON? We're
fetching a file."
"now returns raw bytes via StreamingOutput" on L1025 is in a similar
category IMO.
##########
solr/core/src/test/org/apache/solr/handler/configsets/PutConfigSetFileAPITest.java:
##########
@@ -0,0 +1,120 @@
+/*
+ * 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.InputStream;
+import java.nio.charset.StandardCharsets;
+import java.nio.file.Files;
+import java.nio.file.Path;
+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#putConfigSetFile} (PutFile
interface). */
+public class PutConfigSetFileAPITest extends SolrTestCase {
+
+ private CoreContainer mockCoreContainer;
+ private FileSystemConfigSetService configSetService;
+ private Path configSetBase;
+
+ @BeforeClass
+ public static void ensureWorkingMockito() {
+ assumeWorkingMockito();
+ }
+
+ @Before
+ public void initConfigSetService() {
+ configSetBase = createTempDir("configsets");
+ // Use an anonymous subclass to access the protected testing constructor
+ configSetService = new FileSystemConfigSetService(configSetBase) {};
+ mockCoreContainer = mock(CoreContainer.class);
+ when(mockCoreContainer.getConfigSetService()).thenReturn(configSetService);
+ }
+
+ /** Creates a configset with files on disk for testing overwrites. */
+ private void createExistingConfigSet(String configSetName, String...
filePathAndContent)
+ throws Exception {
+ Path configDir = configSetBase.resolve(configSetName);
+ Files.createDirectories(configDir);
+ for (int i = 0; i < filePathAndContent.length; i += 2) {
+ String filePath = filePathAndContent[i];
+ String content = filePathAndContent[i + 1];
+ Path fullPath = configDir.resolve(filePath);
+ Files.createDirectories(fullPath.getParent());
+ Files.writeString(fullPath, content, StandardCharsets.UTF_8);
+ }
+ }
+
+ @Test
+ public void testSingleFileUploadSuccess() throws Exception {
+ final String configSetName = "singlefile";
+ final String filePath = "solrconfig.xml";
+ final String content = "<config/>";
+ InputStream fileStream = new
ByteArrayInputStream(content.getBytes(StandardCharsets.UTF_8));
+
+ final var api = new UploadConfigSet(mockCoreContainer, null, null);
+ final var response = api.putConfigSetFile(configSetName, filePath,
fileStream);
+
+ assertNotNull(response);
+
+ // Verify the file was uploaded
+ byte[] uploadedData =
configSetService.downloadFileFromConfig(configSetName, filePath);
+ assertEquals(content, new String(uploadedData, StandardCharsets.UTF_8));
+ }
+
+ @Test
+ public void testSingleFileWithEmptyPathThrowsBadRequest() {
+ final String configSetName = "emptypath";
+ InputStream fileStream = new
ByteArrayInputStream("<config/>".getBytes(StandardCharsets.UTF_8));
+
+ final var api = new UploadConfigSet(mockCoreContainer, null, null);
+
+ // Test with empty string
+ final var ex =
+ assertThrows(
+ SolrException.class, () -> api.putConfigSetFile(configSetName, "",
fileStream));
Review Comment:
[0] Are we pre-creating 'configSetName' as a configset in some way? (Or is
ConfigSetService mocked in some way such that it always exists?)
If not, shouldn't the API be returning a 404 or something bc the configset
doesn't exist?
--
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]
