From 39d866a0680fa8a3be7e4edd862e34c6555129ea Mon Sep 17 00:00:00 2001 From: Eric Pugh Date: Wed, 28 Oct 2026 07:25:16 -0400 Subject: [PATCH 1/2] Simplify zip-directory code found by /simplify review - Utils.zipDirectory(Path) was the only shape used by two of its three callers (CreateTool, DownloadConfigSet), which both feed the bytes into an HTTP request body - fine for them, but it forced RemoteSolrBackend to buffer the whole configset in memory just to write it straight back out to a temp file it already had open. Added a streaming zipDirectory(Path, OutputStream) overload with the walk/skip-hidden/ normalize-separator logic in one place; zipDirectory(Path) now delegates to it via a ByteArrayOutputStream, and RemoteSolrBackend calls the streaming form directly against Files.newOutputStream(tempZip), restoring the original single-pass I/O. - Removed CreateTool.zipConfigSet(Path), a pure one-line passthrough to Utils.zipDirectory with no behavior of its own; both call sites (the production upload call and CreateToolTest) now call Utils.zipDirectory directly. - Added a test confirming the two zipDirectory overloads produce equivalent archives (compared by decoded entry name/content, not raw bytes - ZipEntry defaults to the current time when unset, so two back-to-back calls aren't guaranteed byte-identical). Verified: CreateToolTest, CreateToolPicocliTest, and TestSchemaDesignerConfigSetHelper (14 tests) all pass; full compileJava compileTestJava across the repo is clean; ./gradlew tidy clean. --- .../java/org/apache/solr/cli/CreateTool.java | 7 +- .../org/apache/solr/cli/CreateToolTest.java | 37 ++++++--- .../org/apache/solr/common/util/Utils.java | 76 +++++++++++-------- .../apache/solr/cloud/RemoteSolrBackend.java | 5 +- 4 files changed, 76 insertions(+), 49 deletions(-) diff --git a/solr/core/src/java/org/apache/solr/cli/CreateTool.java b/solr/core/src/java/org/apache/solr/cli/CreateTool.java index 9e58a157ab2..8ec27435be5 100644 --- a/solr/core/src/java/org/apache/solr/cli/CreateTool.java +++ b/solr/core/src/java/org/apache/solr/cli/CreateTool.java @@ -346,7 +346,7 @@ private void createCollection(CloudSolrClient cloudSolrClient, CreateParams para + " using the Configsets V2 API"); var uploadReq = new ConfigsetsApi.UploadConfigSet( - confName, new ByteArrayInputStream(zipConfigSet(confPath))); + confName, new ByteArrayInputStream(Utils.zipDirectory(confPath))); uploadReq.process(cloudSolrClient); } @@ -388,11 +388,6 @@ private void createCollection(CloudSolrClient cloudSolrClient, CreateParams para echo(endMessage); } - /** Zips the contents of a configset directory for upload. */ - static byte[] zipConfigSet(Path confPath) throws IOException { - return Utils.zipDirectory(confPath); - } - private Path getFullConfDir(Path solrInstallDir, Path confDirName) { return CLIUtils.getConfigSetsDir(solrInstallDir).resolve(confDirName); } diff --git a/solr/core/src/test/org/apache/solr/cli/CreateToolTest.java b/solr/core/src/test/org/apache/solr/cli/CreateToolTest.java index a814ba529de..d761d04832b 100644 --- a/solr/core/src/test/org/apache/solr/cli/CreateToolTest.java +++ b/solr/core/src/test/org/apache/solr/cli/CreateToolTest.java @@ -18,13 +18,17 @@ package org.apache.solr.cli; import java.io.ByteArrayInputStream; +import java.io.ByteArrayOutputStream; +import java.io.IOException; +import java.nio.charset.StandardCharsets; import java.nio.file.Files; import java.nio.file.Path; -import java.util.HashSet; -import java.util.Set; +import java.util.HashMap; +import java.util.Map; import java.util.zip.ZipEntry; import java.util.zip.ZipInputStream; import org.apache.solr.cloud.SolrCloudTestCase; +import org.apache.solr.common.util.Utils; import org.apache.solr.util.SecurityJson; import org.junit.BeforeClass; import org.junit.Test; @@ -96,20 +100,31 @@ public void testZipConfigSetSkipsHiddenFilesAndIncludesDirectoryEntries() throws Path hiddenDir = Files.createDirectory(confDir.resolve(".hiddenDir")); Files.writeString(hiddenDir.resolve("nope.txt"), "should not be zipped either"); - byte[] zipBytes = CreateTool.zipConfigSet(confDir); + byte[] zipBytes = Utils.zipDirectory(confDir); + Map entries = readZipEntries(zipBytes); - Set entryNames = new HashSet<>(); + assertTrue(entries.containsKey("solrconfig.xml")); + assertTrue(entries.containsKey("lang/")); + assertTrue(entries.containsKey("lang/stopwords.txt")); + assertFalse(entries.containsKey(".hidden-file")); + assertTrue(entries.keySet().stream().noneMatch(name -> name.startsWith(".hiddenDir"))); + + // the OutputStream-writing overload should produce an equivalent archive (not necessarily + // byte-identical: ZipEntry defaults to the current time when none is set explicitly) + ByteArrayOutputStream streamed = new ByteArrayOutputStream(); + Utils.zipDirectory(confDir, streamed); + assertEquals(entries, readZipEntries(streamed.toByteArray())); + } + + /** Maps each zip entry name to its contents (directory entries map to the empty string). */ + private static Map readZipEntries(byte[] zipBytes) throws IOException { + Map entries = new HashMap<>(); try (ZipInputStream zipIn = new ZipInputStream(new ByteArrayInputStream(zipBytes))) { ZipEntry entry; while ((entry = zipIn.getNextEntry()) != null) { - entryNames.add(entry.getName()); + entries.put(entry.getName(), new String(zipIn.readAllBytes(), StandardCharsets.UTF_8)); } } - - assertTrue(entryNames.contains("solrconfig.xml")); - assertTrue(entryNames.contains("lang/")); - assertTrue(entryNames.contains("lang/stopwords.txt")); - assertFalse(entryNames.contains(".hidden-file")); - assertTrue(entryNames.stream().noneMatch(name -> name.startsWith(".hiddenDir"))); + return entries; } } diff --git a/solr/solrj/src/java/org/apache/solr/common/util/Utils.java b/solr/solrj/src/java/org/apache/solr/common/util/Utils.java index 810cf086132..46dcadc990f 100644 --- a/solr/solrj/src/java/org/apache/solr/common/util/Utils.java +++ b/solr/solrj/src/java/org/apache/solr/common/util/Utils.java @@ -1064,41 +1064,55 @@ public static ByteBuffer toByteArray(InputStream is) throws IOException { */ public static byte[] zipDirectory(Path rootPath) throws IOException { ByteArrayOutputStream baos = new ByteArrayOutputStream(); - try (ZipOutputStream zipOut = new ZipOutputStream(baos)) { - Files.walkFileTree( - rootPath, - new SimpleFileVisitor<>() { - @Override - public FileVisitResult preVisitDirectory(Path dir, BasicFileAttributes attrs) - throws IOException { - if (Files.isHidden(dir)) { - return FileVisitResult.SKIP_SUBTREE; - } - String dirName = rootPath.relativize(dir).toString().replace('\\', '/'); - if (!dirName.isEmpty()) { - if (!dirName.endsWith("/")) { - dirName += "/"; - } - zipOut.putNextEntry(new ZipEntry(dirName)); - zipOut.closeEntry(); - } - return FileVisitResult.CONTINUE; - } + zipDirectory(rootPath, baos); + return baos.toByteArray(); + } - @Override - public FileVisitResult visitFile(Path file, BasicFileAttributes attrs) - throws IOException { - if (Files.isHidden(file)) { - return FileVisitResult.CONTINUE; + /** + * Zips the contents of {@code rootPath}, writing the archive to {@code out} instead of buffering + * it in memory. See {@link #zipDirectory(Path)} for the skipped/normalized entry rules. Does not + * close {@code out}. + * + * @param rootPath the directory to zip + * @param out the stream to write the zip archive to + */ + public static void zipDirectory(Path rootPath, OutputStream out) throws IOException { + ZipOutputStream zipOut = new ZipOutputStream(out); + Files.walkFileTree( + rootPath, + new SimpleFileVisitor<>() { + @Override + public FileVisitResult preVisitDirectory(Path dir, BasicFileAttributes attrs) + throws IOException { + if (Files.isHidden(dir)) { + return FileVisitResult.SKIP_SUBTREE; + } + String dirName = rootPath.relativize(dir).toString().replace('\\', '/'); + if (!dirName.isEmpty()) { + if (!dirName.endsWith("/")) { + dirName += "/"; } - String entryName = rootPath.relativize(file).toString().replace('\\', '/'); - zipOut.putNextEntry(new ZipEntry(entryName)); - Files.copy(file, zipOut); + zipOut.putNextEntry(new ZipEntry(dirName)); zipOut.closeEntry(); + } + return FileVisitResult.CONTINUE; + } + + @Override + public FileVisitResult visitFile(Path file, BasicFileAttributes attrs) + throws IOException { + if (Files.isHidden(file)) { return FileVisitResult.CONTINUE; } - }); - } - return baos.toByteArray(); + String entryName = rootPath.relativize(file).toString().replace('\\', '/'); + zipOut.putNextEntry(new ZipEntry(entryName)); + Files.copy(file, zipOut); + zipOut.closeEntry(); + return FileVisitResult.CONTINUE; + } + }); + // finish() rather than close(): writes the zip's central directory without closing the + // caller's stream, since this method doesn't own it. + zipOut.finish(); } } diff --git a/solr/test-framework/src/java/org/apache/solr/cloud/RemoteSolrBackend.java b/solr/test-framework/src/java/org/apache/solr/cloud/RemoteSolrBackend.java index 3ef526a0176..bb20ff9e7c3 100644 --- a/solr/test-framework/src/java/org/apache/solr/cloud/RemoteSolrBackend.java +++ b/solr/test-framework/src/java/org/apache/solr/cloud/RemoteSolrBackend.java @@ -17,6 +17,7 @@ package org.apache.solr.cloud; import java.io.IOException; +import java.io.OutputStream; import java.nio.file.Files; import java.nio.file.Path; import java.util.List; @@ -67,7 +68,9 @@ public SolrClient getSolrClient() { public void uploadConfigSet(Path configDir, String name) throws SolrServerException, IOException { Path tempZip = Files.createTempFile("configset-", ".zip"); try { - Files.write(tempZip, Utils.zipDirectory(configDir)); + try (OutputStream out = Files.newOutputStream(tempZip)) { + Utils.zipDirectory(configDir, out); + } new ConfigSetAdminRequest.Upload() .setConfigSetName(name) .setUploadFile(tempZip, "application/zip") From eef282c578a3bd1a71d9dd1ed875022c7e116045 Mon Sep 17 00:00:00 2001 From: Eric Pugh Date: Wed, 28 Oct 2026 07:33:46 -0400 Subject: [PATCH 2/2] Trim two comments to plain technical English --- solr/core/src/test/org/apache/solr/cli/CreateToolTest.java | 4 ++-- solr/solrj/src/java/org/apache/solr/common/util/Utils.java | 4 ++-- 2 files changed, 4 insertions(+), 4 deletions(-) diff --git a/solr/core/src/test/org/apache/solr/cli/CreateToolTest.java b/solr/core/src/test/org/apache/solr/cli/CreateToolTest.java index d761d04832b..169767c65e2 100644 --- a/solr/core/src/test/org/apache/solr/cli/CreateToolTest.java +++ b/solr/core/src/test/org/apache/solr/cli/CreateToolTest.java @@ -109,8 +109,8 @@ public void testZipConfigSetSkipsHiddenFilesAndIncludesDirectoryEntries() throws assertFalse(entries.containsKey(".hidden-file")); assertTrue(entries.keySet().stream().noneMatch(name -> name.startsWith(".hiddenDir"))); - // the OutputStream-writing overload should produce an equivalent archive (not necessarily - // byte-identical: ZipEntry defaults to the current time when none is set explicitly) + // Equivalent, not necessarily byte-identical: ZipEntry defaults to the current time + // when unset, so two back-to-back calls can differ at the byte level. ByteArrayOutputStream streamed = new ByteArrayOutputStream(); Utils.zipDirectory(confDir, streamed); assertEquals(entries, readZipEntries(streamed.toByteArray())); diff --git a/solr/solrj/src/java/org/apache/solr/common/util/Utils.java b/solr/solrj/src/java/org/apache/solr/common/util/Utils.java index 46dcadc990f..97fa69fc8ab 100644 --- a/solr/solrj/src/java/org/apache/solr/common/util/Utils.java +++ b/solr/solrj/src/java/org/apache/solr/common/util/Utils.java @@ -1111,8 +1111,8 @@ public FileVisitResult visitFile(Path file, BasicFileAttributes attrs) return FileVisitResult.CONTINUE; } }); - // finish() rather than close(): writes the zip's central directory without closing the - // caller's stream, since this method doesn't own it. + // finish(), not close(): flushes the central directory without closing out, which this + // method doesn't own. zipOut.finish(); } }