mlbiscoc commented on code in PR #2924: URL: https://github.com/apache/solr/pull/2924#discussion_r1929246536
########## solr/core/src/java/org/apache/solr/core/SolrResourceLoader.java: ########## @@ -373,12 +372,12 @@ public InputStream openResource(String resource) throws IOException { // Delegate to the class loader (looking into $INSTANCE_DIR/lib jars). // We need a ClassLoader-compatible (forward-slashes) path here! - InputStream is = classLoader.getResourceAsStream(resource.replace(File.separatorChar, '/')); + InputStream is = classLoader.getResourceAsStream(resource); // This is a hack just for tests (it is not done in ZKResourceLoader)! // TODO can we nuke this? if (is == null && System.getProperty("jetty.testMode") != null) { - is = classLoader.getResourceAsStream(("conf/" + resource).replace(File.separatorChar, '/')); + is = classLoader.getResourceAsStream(Path.of("conf").resolve(resource).toString()); Review Comment: I relooked at `File.separatorChar` throughout this PR and moved them to `FileSystems.getDefault().getSeparator()` where it might make sense instead of moving it to PATH to avoid as little breaking as possible. ########## solr/core/src/test/org/apache/solr/core/ResourceLoaderTest.java: ########## @@ -81,10 +81,9 @@ public void testEscapeInstanceDir() throws Exception { // UNC paths assertTrue( assertThrows( - SolrResourceNotFoundException.class, - () -> loader.openResource("\\\\192.168.10.10\\foo").close()) + SolrException.class, () -> loader.openResource("\\\\192.168.10.10\\foo").close()) Review Comment: I did so because of a change from this [comment I made](https://github.com/apache/solr/pull/2924#discussion_r1918615357) from switching to assertNotUnc. ########## solr/core/src/java/org/apache/solr/rest/ManagedResourceStorage.java: ########## @@ -203,9 +202,8 @@ public OutputStream openOutputStream(String storedResourceId) throws IOException @Override public boolean delete(String storedResourceId) throws IOException { - // TODO SOLR-8282 move to PATH - File storedFile = new File(storageDir, storedResourceId); - return deleteIfFile(storedFile.toPath()); + Path storedFile = Path.of(storageDir, storedResourceId); Review Comment: So we need to move `storageDir` to Path entirely in this class but in another JIRA? I can skip this, but we will need to keep `new File(storageDir, storedResourceId);` ########## solr/core/src/java/org/apache/solr/util/VersionedFile.java: ########## @@ -41,44 +41,48 @@ public class VersionedFile { */ public static InputStream getLatestFile(String dirName, String fileName) throws FileNotFoundException { - // TODO SOLR-8282 move to PATH - Collection<File> oldFiles = null; + Collection<Path> oldFiles = null; final String prefix = fileName + '.'; - File f = new File(dirName, fileName); + Path f = Path.of(dirName, fileName); InputStream is = null; // there can be a race between checking for a file and opening it... // the user may have just put a new version in and deleted an old version. // try multiple times in a row. for (int retry = 0; retry < 10 && is == null; retry++) { try { - if (!f.exists()) { - File dir = new File(dirName); - String[] names = - dir.list( - new FilenameFilter() { - @Override - public boolean accept(File dir, String name) { - return name.startsWith(prefix); - } - }); - Arrays.sort(names); - f = new File(dir, names[names.length - 1]); + if (!Files.exists(f)) { + Path dir = Path.of(dirName); + List<Path> fileList; + + try (Stream<Path> files = Files.list(dir)) { + fileList = + files + .filter((file) -> file.getFileName().toString().startsWith(prefix)) Review Comment: Reading java docs I see `The elements of the stream are Path objects that are obtained as if by resolving the name of the directory entry against dir.` So I believe that means that the path could be relative but the prefix will contain the directory is resolved against. So if we are checking the file starts with a specific prefix, we need `getFileName()` ########## solr/core/src/java/org/apache/solr/core/SolrPaths.java: ########## @@ -94,7 +94,7 @@ public static void assertNoPathTraversal(Path pathToAssert) { /** Asserts that a path is not a Windows UNC path */ public static void assertNotUnc(Path pathToAssert) { - if (OS.isFamilyWindows() && pathToAssert.toString().startsWith("\\\\")) { + if (OS.isFamilyWindows() || pathToAssert.toString().startsWith("\\\\")) { Review Comment: I mentioned it in this comment [here](https://github.com/apache/solr/pull/2924#discussion_r1918615357) This was done because I changed `openResource` inside of `SolrResourceLoader` to use this assert method instead of it checking `.startsWith("\\\\")`. I am assuming from the name its asserting that it is not a UNC path, not necessarily just because of the OS being used but probably isn't right. I would need to see why it was an AND in the history. Maybe this should be called `assertNotUNCorWindowsOS` or just make a different assert just checking that `.startsWith("\\\\")` ... ########## solr/core/src/java/org/apache/solr/filestore/DistribFileStore.java: ########## @@ -80,19 +80,20 @@ public DistribFileStore(CoreContainer coreContainer) { } @Override - public Path getRealpath(String path) { + public Path getRealPath(String path) { return _getRealPath(path, solrHome); } private static Path _getRealPath(String path, Path solrHome) { - if (File.separatorChar == '\\') { - path = path.replace('/', File.separatorChar); - } - SolrPaths.assertNotUnc(Path.of(path)); - while (path.startsWith(File.separator)) { // Trim all leading slashes + Path dir = Path.of(path); + SolrPaths.assertNotUnc(dir); Review Comment: You are saying to just remove the UNC check? Not sure I follow but I figured this function should just not be accessible for UNC in general instead of it trying its best to `getRealPath` ########## solr/core/src/java/org/apache/solr/util/SystemIdResolver.java: ########## @@ -167,9 +167,9 @@ public InputSource resolveEntity(String publicId, String systemId) throws IOExce } public static String createSystemIdFromResourceName(String name) { - name = name.replace(File.separatorChar, '/'); + Path pathName = Path.of(name); Review Comment: Sorry bad change. Brought it back with `FileSystems.getDefault().getSeparator()` ########## solr/core/src/java/org/apache/solr/util/VersionedFile.java: ########## @@ -41,44 +41,47 @@ public class VersionedFile { */ public static InputStream getLatestFile(String dirName, String fileName) throws FileNotFoundException { - // TODO SOLR-8282 move to PATH - Collection<File> oldFiles = null; + Collection<Path> oldFiles = null; final String prefix = fileName + '.'; - File f = new File(dirName, fileName); + Path f = Path.of(dirName, fileName); InputStream is = null; // there can be a race between checking for a file and opening it... // the user may have just put a new version in and deleted an old version. // try multiple times in a row. for (int retry = 0; retry < 10 && is == null; retry++) { try { - if (!f.exists()) { - File dir = new File(dirName); - String[] names = - dir.list( - new FilenameFilter() { - @Override - public boolean accept(File dir, String name) { - return name.startsWith(prefix); - } - }); - Arrays.sort(names); - f = new File(dir, names[names.length - 1]); + if (!Files.exists(f)) { + Path dir = Path.of(dirName); + List<Path> fileList; + + try (Stream<Path> files = Files.list(dir)) { + fileList = + files + .filter((file) -> file.getFileName().toString().startsWith(prefix)) + .sorted() + .toList(); + } catch (IOException e) { + throw new SolrException( + SolrException.ErrorCode.SERVER_ERROR, "Unable to list files in " + dir, e); + } + + f = Path.of(dir.toString(), fileList.getLast().getFileName().toString()); oldFiles = new ArrayList<>(); - for (int i = 0; i < names.length - 1; i++) { - oldFiles.add(new File(dir, names[i])); + for (int i = 0; i < fileList.size() - 1; i++) { + oldFiles.add(Path.of(dir.toString(), fileList.get(i).toString())); } } - is = new FileInputStream(f); + is = new FileInputStream(f.toString()); Review Comment: Thats fair. I had a previous comment where I said the change wasn't 1:1 but here it is. Wasn't sure what I ran into then but for the PR where I changed to Path and there is a `new FileInputStream` I changed the `new FileInputStream` to `Files.newInputStream()`. We can tackle the rest in another. ########## solr/core/src/java/org/apache/solr/handler/IndexFetcher.java: ########## @@ -1484,28 +1484,25 @@ private void copyTmpConfFiles2Conf(Path tmpconfDir) { int numTempPathElements = tmpconfDir.getNameCount(); for (Path path : makeTmpConfDirFileList(tmpconfDir)) { Path oldPath = confPath.resolve(path.subpath(numTempPathElements, path.getNameCount())); + try { Files.createDirectories(oldPath.getParent()); } catch (IOException e) { throw new SolrException( ErrorCode.SERVER_ERROR, "Unable to mkdirs: " + oldPath.getParent(), e); } if (Files.exists(oldPath)) { - File oldFile = oldPath.toFile(); // TODO SOLR-8282 move to PATH - File backupFile = - new File(oldFile.getPath() + "." + getDateAsStr(new Date(oldFile.lastModified()))); - if (!backupFile.getParentFile().exists()) { - status = backupFile.getParentFile().mkdirs(); - if (!status) { - throw new SolrException( - ErrorCode.SERVER_ERROR, "Unable to mkdirs: " + backupFile.getParentFile()); - } - } - status = oldFile.renameTo(backupFile); - if (!status) { + try { + Path backupFile = + Path.of( Review Comment: Moved to `resolveSibling` to append the extension to the filename instead. ########## solr/core/src/java/org/apache/solr/util/VersionedFile.java: ########## @@ -41,44 +41,48 @@ public class VersionedFile { */ public static InputStream getLatestFile(String dirName, String fileName) Review Comment: Added a TODO for now ########## solr/core/src/java/org/apache/solr/schema/ManagedIndexSchemaFactory.java: ########## @@ -412,17 +410,17 @@ private void upgradeToManagedSchema() { "nor under SolrConfig.getConfigDir() or the current directory. ", "PLEASE REMOVE THIS FILE."); } else { - File upgradedSchemaFile = new File(nonManagedSchemaFile + UPGRADED_SCHEMA_EXTENSION); - if (nonManagedSchemaFile.renameTo(upgradedSchemaFile)) { + Path upgradedSchemaFile = Path.of(nonManagedSchemaFile + UPGRADED_SCHEMA_EXTENSION); Review Comment: Moved to `resolveSibling` ########## solr/core/src/java/org/apache/solr/filestore/DistribFileStore.java: ########## @@ -80,19 +80,20 @@ public DistribFileStore(CoreContainer coreContainer) { } @Override - public Path getRealpath(String path) { + public Path getRealPath(String path) { return _getRealPath(path, solrHome); } private static Path _getRealPath(String path, Path solrHome) { - if (File.separatorChar == '\\') { - path = path.replace('/', File.separatorChar); - } - SolrPaths.assertNotUnc(Path.of(path)); - while (path.startsWith(File.separator)) { // Trim all leading slashes + Path dir = Path.of(path); + SolrPaths.assertNotUnc(dir); + + // Strip the path of from being absolute to become relative to resolve with SolrHome + while (path.startsWith("/")) { Review Comment: Not sure how that way would be equal to this. From my understanding, all this wants to do it strip the beginning so it is not absolute and into a relative path regardless of what the `dir` string was. I changed it to a different way that seems to be equal -- 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-unsubscr...@solr.apache.org For queries about this service, please contact Infrastructure at: us...@infra.apache.org --------------------------------------------------------------------- To unsubscribe, e-mail: issues-unsubscr...@solr.apache.org For additional commands, e-mail: issues-h...@solr.apache.org