Add Paths#get to forbidden APIs by AndreyBozhko · Pull Request #3289 · apache/solr

AndreyBozhko · 2025-03-26T15:38:26Z

Description

Replace Paths#get with Path#of, and add Paths#get to forbidden apis.
Also cleaned up some of the Path -> String -> Path conversions that seemed unnecessary.

Prompted by the comment here: #3146 (comment)

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

I love that this removes more lines than it adds!

LGTM.

epugh · 2025-03-26T15:48:43Z

solr/core/src/java/org/apache/solr/cli/ConfigSetDownloadTool.java

      // there.
      if (!configSetPath.endsWith("/conf")) {
-        configSetPath = Paths.get(configSetPath.toString(), "conf");
+        configSetPath = configSetPath.resolve("conf");


dsmiley

Thanks!

This PR is certainly going to conflict with @mlbiscoc 's work in #3263
I think that PR should be merged first. He put a ton of time into that one; it'd be a shame to resolve conflicts coming from this one doing some of the same actual changes!

dsmiley · 2025-03-27T14:17:15Z

solr/core/src/java/org/apache/solr/cli/CLIUtils.java


  public static Path getConfigSetsDir(Path solrInstallDir) {
-    Path configSetsPath = Paths.get("server/solr/configsets/");
+    Path configSetsPath = Path.of("server/solr/configsets/");


Don't need "Path.of" here at all; pass this slashy string to resolve

dsmiley · 2025-03-27T14:45:48Z

solr/core/src/java/org/apache/solr/schema/ManagedIndexSchemaFactory.java


-    Path managedSchemaPath =
-        Paths.get(loader.getConfigPath().toString(), managedSchemaResourceName);
+    Path managedSchemaPath = loader.getConfigPath().resolve(managedSchemaResourceName);


dsmiley · 2025-03-27T14:48:55Z

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

    // Ensure that empty files don't become directories (SOLR-11198)

-    Path emptyFile = Paths.get(tmp.toAbsolutePath().toString(), "conf", "stopwords", "emptyfile");
+    Path emptyFile = tmp.resolve("conf").resolve("stopwords").resolve("emptyfile").toAbsolutePath();


please also drop the toAbsolutePath calls on temp dir resolution, as the path is already absolute. Just touch the lines you modify.

dsmiley · 2025-03-27T14:50:24Z

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

    final String standardOutput2 = runtime.getOutput();
    assertTrue(standardOutput2.startsWith("Copying from 'zk:/getNode'"));
-    byte[] fileBytes = Files.readAllBytes(Paths.get(localFile.getAbsolutePath()));
+    byte[] fileBytes = Files.readAllBytes(Path.of(localFile.getAbsolutePath()));


no point in getAbsolutePath

dsmiley · 2025-03-27T14:54:50Z

solr/test-framework/src/test/org/apache/solr/cloud/MiniSolrCloudClusterTest.java

-    // reconsistutes the path from string so we have to go the string route here as well...
-    final Path workDir = Paths.get(createTempDir().toAbsolutePath().toString());
+    // reconstitutes the path from string so we have to go the string route here as well...
+    final Path workDir = Path.of(createTempDir().toAbsolutePath().toString());


no need to go to from Path to String then Path

I tried that initially and that led to broken tests, so I suppose the warning in the comment above still holds...

Oh right (and "duh" on me for not reading the comment :shame:).
In the other PR, I like the different approach we're taking of unwrapping the custom Path, which is more plainly apparent and also searchable.

dsmiley · 2025-03-27T14:57:44Z

solr/core/src/test/org/apache/solr/rest/TestRestManager.java

  @Test
  public void testReloadFromPersistentStorage() throws IOException {
-    SolrResourceLoader loader = new SolrResourceLoader(Paths.get("./"));
+    SolrResourceLoader loader = new SolrResourceLoader(Path.of("./"));


Any place we use "." or "./", it's the same as the empty string, which I think is more canonical. Maybe a separate PR.

mlbiscoc · 2025-03-27T18:30:23Z

Cool! Yeah we are going to be stepping on each other toes here... My PR just had a merge conflict which I need to fix. Going to fix it today or tomorrow and hopefully we can merge it soon. Happy to help with merge conflicts on this PR when it arises. Or anything else in here.

AndreyBozhko · 2025-03-27T21:00:56Z

Thanks - I agree that it makes total sense to wait with this PR.

* add Paths.get to forbidden apis * replace Paths.get with Path.of Co-authored-by: Andrey Bozhko <abozhko@apple.com>

dsmiley · 2025-07-29T20:45:26Z

solr/solrj-zookeeper/src/java/org/apache/solr/common/cloud/ZkMaintenanceUtils.java

The use of Path.of in uploadToZK file is an absolute tragedy. There are lots of nice utility methods in this class that take a Path argument (yay), then convert to a String (boo), then pass to Path.of (nausea), thus assuming it's a file system path. So much for uploading a configSet from a ZIP/JAR.

Not introduced by you of course, but I thought you might care. @AndreyBozhko

Andrey Bozhko added 3 commits March 26, 2025 10:07

add Paths.get to forbidden apis

d438cb5

replace Paths.get with Path.of

5f0cfc1

clean up

b26ff8a

epugh approved these changes Mar 26, 2025

View reviewed changes

dsmiley reviewed Mar 27, 2025

View reviewed changes

Merge branch 'main' into ban-paths-get

200fe98

github-actions bot removed documentation Improvements or additions to documentation cat:index cat:metrics labels Mar 28, 2025

address comment

682f0e7

dsmiley merged commit 470ae76 into apache:main Mar 29, 2025
2 checks passed

colvinco pushed a commit to colvinco/solr that referenced this pull request Apr 4, 2025

Add Paths#get to forbidden APIs (apache#3289)

3fa55d4

* add Paths.get to forbidden apis * replace Paths.get with Path.of Co-authored-by: Andrey Bozhko <abozhko@apple.com>

dsmiley reviewed Jul 29, 2025

View reviewed changes

Conversation

AndreyBozhko commented Mar 26, 2025

Description

Checklist

Uh oh!

epugh left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

dsmiley left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

mlbiscoc commented Mar 27, 2025

Uh oh!

AndreyBozhko commented Mar 27, 2025

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants