SOLR-17548: Switch all public Java APIs from File to Path #2907

mlbiscoc · 2024-12-13T15:07:11Z

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

Description

Move from File to the more modern java.nio.file.Path

Solution

Tried to find all of the public constructors/methods where File is being used or returned as a type and do it's conversion to Path instead. If the conversion was simple, also changed the File methods being used to its equivalent Path methods to stop using the toFile() call.

There were some places where it required a much larger rework which felt at of scope of this already large number of changes. For those I placed a // TODO SOLR-8282 move to PATH

Tests

Modified most uses of File to Path and many of the changes revolve around me removing the toPath().

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

Really excited about this refactoring! I made some comments, mostly around "hey, this is a nice chnage" or "hey, is this an opporutnity" versus "please change this" type comments.

One thing I am curious is how do we know all the conversions are done? I like the addition of the TODO to point out where more refactorings can be done. I guess I am wondering, once we have a lot of .toString() or .toPath(), how do we go back and remove the excess conversion loops? Also, i wonder if we need to add when this is done some forbiddenapi entries for file? I triggered the CI process... hopefully CI will run on all your other commits.

epugh · 2024-12-13T20:24:58Z

solr/benchmark/src/java/org/apache/solr/bench/MiniClusterState.java

@@ -267,7 +267,7 @@ public void startMiniCluster(int nodeCount) {
        cluster =
            new MiniSolrCloudCluster.Builder(nodeCount, miniClusterBaseDir)
                .formatZkServer(false)
-                .addConfig("conf", getFile("src/resources/configs/cloud-minimal/conf").toPath())
+                .addConfig("conf", getFile("src/resources/configs/cloud-minimal/conf"))


love seeing the changed version is shorter and simpler than the original one!

epugh · 2024-12-13T20:26:05Z

solr/core/src/java/org/apache/solr/cloud/SolrZkServer.java

-  public void setDataDir(File dataDir) {
-    this.dataDir = dataDir;
+  public void setDataDir(Path dataDir) {
+    this.dataDir = dataDir.toFile();


is this an example of where you felt changing things got out of hand? Otherwise, seems like if dataDir was a Path, then no .toFile()??

Yeah this one is setting dataDir from an import existing in org.apache.zookeeper.server.quorum.QuorumPeerConfig which is outside of Solr. If you see toFile() it is most likely because it either got out of hand and I put TODO to come back later or it wasn't possible to use Path strictly

epugh · 2024-12-13T20:26:40Z

solr/core/src/java/org/apache/solr/core/DirectoryFactory.java

@@ -339,6 +339,8 @@ public String getDataHome(CoreDescriptor cd) throws IOException {

  public void cleanupOldIndexDirectories(
      final String dataDirPath, final String currentIndexDirPath, boolean afterCoreReload) {
+
+    // TODO SOLR-8282 move to PATH


Your stepwise logic is probably much better than a "lets change it all in one fell swoop!"

epugh · 2024-12-13T20:30:20Z

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

@@ -294,7 +294,7 @@ public void testDoFilesMode() throws IOException {
    postTool.recursive = 0;
    postTool.dryRun = true;
    postTool.solrUpdateUrl = URI.create("http://localhost:8983/solr/fake/update");
-    File dir = getFile("exampledocs");
+    File dir = getFile("exampledocs").toFile();


Since we just do a toString, maybe we can simplify this line???

matbe change postFiles to take in a list of paths?

Feels out of scope for now but agreed maybe we can simplify here. Unless you feel strongly about it, will add a TODO and I can do on a separate PR.

epugh · 2024-12-13T20:31:10Z

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

@@ -303,7 +303,7 @@ public void testDoFilesMode() throws IOException {
  public void testDetectingIfRecursionPossibleInFilesMode() throws IOException {
    PostTool postTool = new PostTool();
    postTool.recursive = 1; // This is the default
-    File dir = getFile("exampledocs");
+    File dir = getFile("exampledocs").toFile();


likewise, maybe we just make strings and avoid going thoruh the file type?

Or stay as a Path?

Agreed. I actually had done a lot of this work already across the repo but there were so many tests using File and I tried to avoid the conversions or going through File, the changes ended up over 800+ lines... So holding those code changes on my Fork for now.

For now, I was planning on keeping the conversions of toFile() and toPath() and we can move out of all of these conversions as best as possible on another PR. Should be easy with a search. I think here I was trying to get as many of the APIs return types and parameters off of File to keep in scope so it was reviewable.

epugh · 2024-12-13T21:11:38Z

solr/core/src/test/org/apache/solr/search/TestRecovery.java

This feels like maybe an opportunity to change from using either Strings and Files to just path and avoid some conversions??

Read comment above

epugh · 2024-12-13T21:11:57Z

solr/cross-dc-manager/src/test/org/apache/solr/crossdc/manager/RetryQueueIntegrationTest.java

@@ -157,7 +157,7 @@ private static MiniSolrCloudCluster startCluster(

    // new SolrCloudTestCase.Builder(1, baseDir).addConfig("conf",
    // getFile("configs/cloud-minimal/conf").toPath()).configure();
-    cluster.uploadConfigSet(getFile("configs/cloud-minimal/conf").toPath(), "conf");
+    cluster.uploadConfigSet(getFile("configs/cloud-minimal/conf"), "conf");


epugh · 2024-12-13T21:12:24Z

...es/scripting/src/test/org/apache/solr/scripting/update/ScriptUpdateProcessorFactoryTest.java

@@ -44,7 +44,7 @@ public static void beforeClass() throws Exception {
    initCore(
        "solrconfig-script-updateprocessor.xml",
        "schema.xml",
-        getFile("scripting/solr").getAbsolutePath());
+        getFile("scripting/solr").toAbsolutePath().toString());


Maybe better to change initCore?

Same reply as comment above.

epugh · 2024-12-13T21:12:45Z

solr/solrj/src/test/org/apache/solr/client/solrj/impl/CloudHttp2SolrClientRetryTest.java

@@ -41,7 +41,6 @@ public static void setupCluster() throws Exception {
        .addConfig(
            "conf",
            getFile("solrj")
-                .toPath()


mlbiscoc · 2024-12-14T04:54:01Z

Really excited about this refactoring! I made some comments, mostly around "hey, this is a nice chnage" or "hey, is this an opporutnity" versus "please change this" type comments.

One thing I am curious is how do we know all the conversions are done? I like the addition of the TODO to point out where more refactorings can be done. I guess I am wondering, once we have a lot of .toString() or .toPath(), how do we go back and remove the excess conversion loops? Also, i wonder if we need to add when this is done some forbiddenapi entries for file? I triggered the CI process... hopefully CI will run on all your other commits.

Thanks for helping review @epugh! I found all imports of java.io.File and tried to remove it as much as possible. Mostly tried to fix using File if it was trival, otherwise you'll see the toFile() or toPath() This is a first pass through and looks like tests pass. Second pass would probably be to try and remove toFile()ortoPath()as well as creation of anynew File()` as much as possible. If we can make it a forbidden API, the build will tell us where all of them are being called to continue with clean up.

I found the File api was used so much in Tests, when I was doing conversions the number of lines changes became too much for this PR. I am holding those on my fork for now and hopefully I can maybe push that PR after this one.

epugh · 2024-12-14T11:11:01Z

Sounds like you have a strong plan for moving forward @mlbiscoc and that's awesome. Do holler when you are ready for review and commit.

mlbiscoc · 2024-12-14T13:35:11Z

Sounds like you have a strong plan for moving forward @mlbiscoc and that's awesome. Do holler when you are ready for review and commit.

This should be ready for review unless you want me to bring in those other changes but was going for that in a separate PR

epugh · 2024-12-14T14:06:28Z

Cool... I'll leave it open for a few days to see what else crops up.. Ping me if we don't merge it by mid week!

mlbiscoc added 6 commits December 12, 2024 16:29

Move public Java APIs from File to Path

1ee430d

Move File objects to toPath()

ac5f63e

SolrTestCaseJ4 getFile() returns Path instead

58d52b7

More cleanup on toPath()

45b2712

Fixed tests from wrong Path conversion logic

f851b1f

Fix S3InstallShardTest

e7f9be1

MiniClusterState change to Path completely

5035793

epugh reviewed Dec 13, 2024

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

SOLR-17548: Switch all public Java APIs from File to Path #2907

SOLR-17548: Switch all public Java APIs from File to Path #2907

mlbiscoc commented Dec 13, 2024

epugh left a comment

epugh Dec 13, 2024

epugh Dec 13, 2024

mlbiscoc Dec 14, 2024

epugh Dec 13, 2024

epugh Dec 13, 2024

epugh Dec 13, 2024

mlbiscoc Dec 14, 2024

epugh Dec 13, 2024

epugh Dec 13, 2024

mlbiscoc Dec 14, 2024

epugh Dec 13, 2024

mlbiscoc Dec 14, 2024

epugh Dec 13, 2024

epugh Dec 13, 2024

mlbiscoc Dec 14, 2024

epugh Dec 13, 2024

mlbiscoc commented Dec 14, 2024

epugh commented Dec 14, 2024

mlbiscoc commented Dec 14, 2024

epugh commented Dec 14, 2024

SOLR-17548: Switch all public Java APIs from File to Path #2907

Are you sure you want to change the base?

SOLR-17548: Switch all public Java APIs from File to Path #2907

Conversation

mlbiscoc commented Dec 13, 2024

Description

Solution

Tests

Checklist

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

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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mlbiscoc commented Dec 14, 2024

epugh commented Dec 14, 2024

mlbiscoc commented Dec 14, 2024

epugh commented Dec 14, 2024