Skip to content

SOLR-18057: Prefer Path.resolve over Path.of for derived paths#4174

Merged
epugh merged 1 commit intoapache:mainfrom
openworld-maker:codex/SOLR-18057-path-resolve-cleanup
Mar 3, 2026
Merged

SOLR-18057: Prefer Path.resolve over Path.of for derived paths#4174
epugh merged 1 commit intoapache:mainfrom
openworld-maker:codex/SOLR-18057-path-resolve-cleanup

Conversation

@openworld-maker
Copy link
Contributor

This PR addresses SOLR-18057 by eliminating unnecessary Path→String→Path conversions and using Path.resolve(...) when the base is already a Path (starting with BinaryFieldTest noted in the issue).

No intended behavior change; refactor only.

What changed

  • Refactored TestBinaryField to use TEST_HOME().resolve("collection1").resolve("conf") and resolve(...) for file paths.
  • Applied the same cleanup pattern in related tests where path bases were already Path:
    • TestRawTransformer
    • TestCustomCoreProperties
    • TestCoreDiscovery
    • TestLazyCores
    • CoreAdminCreateDiscoverTest
    • CoreAdminHandlerTest
    • ReplicationTestHelper
    • TestSystemIdResolver

Validation

  • Focused tests passed:
    • :solr:core:test --tests org.apache.solr.schema.TestBinaryField
    • :solr:core:test --tests org.apache.solr.util.TestSystemIdResolver
    • :solr:core:test --tests org.apache.solr.response.TestRawTransformer
    • :solr:core:test --tests org.apache.solr.TestCustomCoreProperties
  • ./gradlew check was attempted and hit unrelated environment/flaky issues (CloudSolrClientCacheTest flake and RAT task wiring check in this local environment).

Copy link
Contributor

@epugh epugh left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This looks reasonable. Would it be possible to use our various validation tools to prevent this from happening in the future?

@dsmiley
Copy link
Contributor

dsmiley commented Mar 2, 2026

Tooling: There's nothing to ban. I wish there was a PR/review mechanism that would basically point out usages of Path.of that are new/added for scrutiny. Notice that the JIRA issue includes a comment by me with a GitHub "CodeQL" query. I'm not yet sure if there's a mechanism GitHub offers to support this use-case.

@epugh
Copy link
Contributor

epugh commented Mar 2, 2026

I was thinking that one of ecj or error prone or forbidden APIs checkers would keep this from coming back.

@epugh
Copy link
Contributor

epugh commented Mar 3, 2026

I dug around a bit and the only way to enforce this would be to write a custom BugChecker in Error Prone which probably isn't worth it.

@epugh
Copy link
Contributor

epugh commented Mar 3, 2026

I ran the tests and they all passed local...

Copy link
Contributor

@epugh epugh left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

tests pass..

@epugh epugh merged commit b891851 into apache:main Mar 3, 2026
4 of 5 checks passed
epugh pushed a commit that referenced this pull request Mar 3, 2026
Co-authored-by: Kamlendra <kamlendrachauhan21@gmail.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants