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

Reply via email to