SOLR-16903: Migrate all tests using File to NIO Path #3263

mlbiscoc · 2025-03-13T21:05:15Z

https://issues.apache.org/jira/browse/SOLR-16903

Description

WIP - Leaving in draft state for now as there is still more work from refactor needed but most tests pass so can be semi-reviewed.

Migrate all imports using File to Path. There should be no more imports using File in the project. We are close to making File a forbidden API after this but still more work needed.

Checklist

Please review the following and check all that apply:

I have reviewed the guidelines for How to Contribute and my code conforms to the standards described there to the best of my ability.
I have created a Jira issue and added the issue ID to my pull request title.
I have given Solr maintainers access to contribute to my PR branch. (optional but recommended, not available for branches on forks living under an organisation)
I have developed this patch against the main branch.
I have run ./gradlew check.
I have added tests for my changes.
I have added documentation for the Reference Guide

epugh · 2025-03-13T21:55:27Z

solr/core/src/java/org/apache/solr/blockcache/BlockDirectory.java

-      File file = new File(directory, name);
-      if (!file.exists()) {
+      Path directory = ((FSDirectory) in).getDirectory();
+      Path file = directory.resolve(name);


Maybe just purely cosmentic, but should this really be path instead of file? In all the places?

No. I like 'file' to differentiate from a 'dir'. 'path' is generic if could be either.

epugh

Wow, so much work! In general I love the removing of the indirection that using Path is showing....

epugh · 2025-03-13T21:56:46Z

solr/core/src/test/org/apache/solr/SolrInfoBeanTest.java

    ClassLoader cld = h.getCore().getResourceLoader().getClassLoader();
    String path = pckgname.replace('.', '/');
    Enumeration<URL> resources = cld.getResources(path);
    while (resources.hasMoreElements()) {
      final URI uri = resources.nextElement().toURI();
      if (!"file".equalsIgnoreCase(uri.getScheme())) continue;
-      final File f = new File(uri);
+      final Path f = Path.of(uri);


path instead of f?

epugh · 2025-03-13T21:57:47Z

solr/core/src/test/org/apache/solr/SolrInfoBeanTest.java

-          }
+    for (Path directory : directories) {
+      if (Files.exists(directory)) {
+        try (Stream<Path> files = Files.list(directory)) {


Is the try helping? Is this a try-with-resources pattern?

epugh · 2025-03-13T22:00:34Z

solr/core/src/test/org/apache/solr/cli/SolrProcessManagerTest.java

@@ -84,9 +83,11 @@ private static int findAvailablePort() throws IOException {
  private static Pair<Integer, Process> createProcess(int port, boolean https) throws IOException {
    // Get the path to the java executable from the current JVM
    String classPath =
-        Arrays.stream(System.getProperty("java.class.path").split(File.pathSeparator))
+        Arrays.stream(
+                System.getProperty("java.class.path").split(System.getProperty("path.separator")))


I'm suprised we don't have sort of nicer way of doing this than System.getProperty.. Is this common enough that we should have a helper?

Is "FileSystems.getDefault().getSeparator();" a possiblity?

epugh · 2025-03-13T22:00:58Z

solr/core/src/test/org/apache/solr/cli/StreamToolTest.java


    StreamTool.LocalCatStream catStream =
-        new StreamTool.LocalCatStream(localFile.getAbsolutePath(), -1);
+        new StreamTool.LocalCatStream(localFile.toAbsolutePath().toString(), -1);


Should we just pass in a path here?

epugh · 2025-03-13T22:13:23Z

solr/core/src/test/org/apache/solr/cli/TestSolrCLIRunExample.java


    String[] toolArgs =
        new String[] {
          "-e", "techproducts",
-          "--server-dir", solrServerDir.getAbsolutePath(),
-          "--example-dir", solrExampleDir.getAbsolutePath(),
+          "--server-dir", solrServerDir.toAbsolutePath().toString(),


Shame that this requires all these .toString.. Is there a smarter way of handling this toolArgs? var toolArgs? And then internally we call .toString on anything passed in?????

dsmiley

I reviewed a subset of this but I'll pause until you react to this feedback.(1) Don't use Path.of if you can instead resolve() against the existing path.
(2) Don't rely on referring to the separator char for string concatenation if the scenario could simply be path.resolve()

dsmiley · 2025-03-14T02:32:23Z

solr/core/src/test/org/apache/solr/cli/SolrCLIZkToolsTest.java

@@ -279,12 +292,16 @@ public void testCp() throws Exception {
    // src and cp3 are valid

    String localNamed =
-        tmp.normalize() + File.separator + "localnamed" + File.separator + "renamed.txt";
+        tmp.normalize()


shouldn't we use resolve() instead?

dsmiley · 2025-03-14T02:32:42Z

solr/core/src/test/org/apache/solr/cli/SolrCLIZkToolsTest.java

-          "file:" + srcPathCheck.normalize().toAbsolutePath() + File.separator + "solrconfig.xml",
+          "file:"
+              + srcPathCheck.normalize().toAbsolutePath()
+              + FileSystems.getDefault().getSeparator()


shouldn't we use resolve instead? And everywhere else in this source file probably.

dsmiley · 2025-03-14T02:33:43Z

solr/core/src/test/org/apache/solr/cli/SolrCLIZkToolsTest.java

@@ -417,7 +434,7 @@ public void testCp() throws Exception {
    res = cpTool.runTool(SolrCLI.processCommandLineArgs(cpTool, args));
    assertEquals("Copy should have succeeded.", 0, res);

-    Path locEmpty = Paths.get(tmp2.toAbsolutePath().toString(), "stopwords", "emptyfile");
+    Path locEmpty = Path.of(tmp2.toAbsolutePath().toString(), "stopwords", "emptyfile");


Use resolve here instead? and in many other places similarly

mlbiscoc added 7 commits March 6, 2025 09:36

Part 1 of refactor

3860bc3

Fix TODOs so it's easier to find

be9d7a0

Part 2 of refactor

3ef37ee

Part 3 of refactor

36489ea

Build issues from import

9cd71c9

Fix tests to be runnable

37a4911

Fix additional failing tests

2c36bee

epugh reviewed Mar 13, 2025

View reviewed changes

dsmiley requested changes Mar 14, 2025

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

SOLR-16903: Migrate all tests using File to NIO Path #3263

SOLR-16903: Migrate all tests using File to NIO Path #3263

mlbiscoc commented Mar 13, 2025

epugh Mar 13, 2025

dsmiley Mar 14, 2025

epugh left a comment

epugh Mar 13, 2025

epugh Mar 13, 2025

epugh Mar 13, 2025

epugh Mar 13, 2025

epugh Mar 13, 2025

epugh Mar 13, 2025

dsmiley left a comment

dsmiley Mar 14, 2025

dsmiley Mar 14, 2025

dsmiley Mar 14, 2025

SOLR-16903: Migrate all tests using File to NIO Path #3263

Are you sure you want to change the base?

SOLR-16903: Migrate all tests using File to NIO Path #3263

Conversation

mlbiscoc commented Mar 13, 2025

Description

Checklist

Choose a reason for hiding this comment

Choose a reason for hiding this comment

epugh left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

dsmiley left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment