fix: use Path.toUri() to handle WSL paths correctly #1378

tim-sh · 2025-11-27T10:46:29Z

The previous implementation used File.toURI(), which incorrectly encodes the server name of a UNC path (including WSL paths like \\wsl$\...) into the URI's path component. This resulted in invalid URIs with four leading slashes (e.g., file:////wsl$/...).

This change updates the logic to use the modern java.nio.file.Path.toUri() method. This correctly parses the server name (e.g., wsl$) into the URI's authority component, producing a valid and standards-compliant URI (e.g., file://wsl$/...).

Also:

add a new test case to verify the correct handling of WSL paths.
add the JUnit Jupiter engine dependency to the build to enable the execution of the test.

tim-sh · 2025-11-27T10:54:22Z

Converted to draft due to open to-dos:

replace a couple of remaining direct calls to File#toURI with toUri
clarify what to do with the rootPath (as opposed to Uri) → let's leave that one out of the equation for now, since it's not the core problem here – and LSP4IJ just calls VirtualFile#getPath on an element of what the IJ BaseProjectDirectories.Companion#getBaseDirectories method returns, which is pretty generic
test on Windows

@angelozerr Would you prefer the first ones in separate PRs?

angelozerr · 2025-11-27T10:58:57Z

@tim-sh thanks for your contribution.

I plan to do a release soon,I don't know how it will take time to fix this issue?

angelozerr · 2025-11-27T11:01:23Z

Windows CI build is failling https://github.com/redhat-developer/lsp4ij/actions/runs/19733661567/job/56541191426?pr=1378#step:5:74 but it is perhaps normal (you have not finished your work).

angelozerr · 2025-11-27T13:40:01Z

Do you need this PR for the next release?

tim-sh · 2025-11-27T13:54:09Z

@angelozerr Thanks for asking! Yes, that would be phantastic. I'm working to get it done now.

angelozerr · 2025-11-27T14:00:29Z

@angelozerr Thanks for asking! Yes, that would be phantastic. I'm working to get it done now.

Ok I wanted to create release next week, do you think it will possible?

The previous implementation used `File.toURI()`, which incorrectly encodes the server name of a UNC path (including WSL paths like `\\wsl$\...`) into the URI's path component. This resulted in invalid URIs with four leading slashes (e.g., `file:////wsl$/...`). This change updates the logic to use the modern `java.nio.file.Path.toUri()` method. This correctly parses the server name (e.g., `wsl$`) into the URI's authority component, producing a valid and standards-compliant URI (e.g., `file://wsl$/...`). Also: - add a new test case to verify the correct handling of WSL paths. - add the JUnit Jupiter engine dependency to the build to enable the execution of the test.

src/main/java/com/redhat/devtools/lsp4ij/LSPIJUtils.java

src/main/java/com/redhat/devtools/lsp4ij/features/formatting/AbstractLSPFormattingService.java

src/test/java/com/redhat/devtools/lsp4ij/LSPIJUtils_toUriTest.java

src/main/java/com/redhat/devtools/lsp4ij/LSPIJUtils.java

src/test/java/com/redhat/devtools/lsp4ij/LSPIJUtils_toUriTest.java

angelozerr · 2025-11-27T20:52:55Z

src/main/java/com/redhat/devtools/lsp4ij/LSPIJUtils.java

-        } catch (URISyntaxException e) {
-            LOGGER.warn(e.getLocalizedMessage(), e);
-            return file.getAbsoluteFile().toURI();
+            return file.toPath().toUri();


Please add a comment Hich explains why path is used by adding in comments some url sample

angelozerr · 2025-11-27T20:55:03Z

The PR seems very nice. My only fear is that it doesnt cover some cases and introduce some bugs

tim-sh · 2025-11-28T11:21:35Z

The PR seems very nice. My only fear is that it doesnt cover some cases and introduce some bugs

Thanks for the feedback! I can understand that fear.
Is there any way I could test the version of this PR from a nightly-like build? This way I could do a first smoke test for both Linux and Windows.

tim-sh · 2025-11-28T11:26:35Z

I will also add a test comparing the old and the new result and asserting that it only changes for WSL or other paths starting with a host name. What do you think, would that provide sufficient safety?

angelozerr · 2025-11-28T12:12:57Z

I will also add a test comparing the old and the new result and asserting that it only changes for WSL or other paths starting with a host name. What do you think, would that provide sufficient safety?

Your tests is great. If you want to test I suggest that you read section build zip from readme.

I will also test on my side

tim-sh · 2025-11-28T13:48:27Z

@angelozerr Thanks! I will do that. Also, I saw that the core functionality doesn't even seem to work, which will have to be debugged. Might not manage all that today but hopefully on Monday.

angelozerr · 2025-11-28T13:54:48Z

@tim-sh please note that test fails https://github.com/redhat-developer/lsp4ij/actions/runs/19746312974/job/56630539800?pr=1378#step:5:79

tim-sh · 2025-11-28T13:56:24Z

@tim-sh please note that test fails https://github.com/redhat-developer/lsp4ij/actions/runs/19746312974/job/56630539800?pr=1378#step:5:79

Yes, as mentioned, I'll have to debug this…

angelozerr · 2025-11-28T14:00:53Z

@tim-sh please note that test fails https://github.com/redhat-developer/lsp4ij/actions/runs/19746312974/job/56630539800?pr=1378#step:5:79

Yes, as mentioned, I'll have to debug this…

Thanks!

Once test will pass, I will try your PR on my side.

angelozerr · 2025-12-02T15:26:31Z

@tim-sh I would like to create a release this week. Even if your PR makes sense, it is too dangerous to merge it for the release. Hope you will understand.

tim-sh · 2025-12-02T16:16:40Z

@angelozerr I see. So far, I haven't been able to get the Windows build running in a VM, but will continue to work on it on Thu. If it's not included in lsp4ij for now, I'd see where we can override to test this.

However, since I understand you have a positive outlook on this PR in general, is there any way we could test a build output from this PR?

angelozerr · 2025-12-02T18:28:56Z

@angelozerr I see. So far, I haven't been able to get the Windows build running in a VM, but will continue to work on it on Thu.

I am on windows os, i will investigate the problem after the release.

If it's not included in lsp4ij for now, I'd see where we can override to test this.

I fear for formatting you will not able to override it. For other features you should implement your own file uri support.

However, since I understand you have a positive outlook on this PR in general, is there any way we could test a build output from this PR?

Except installing the build zip, no

tim-sh marked this pull request as draft November 27, 2025 10:53

openshift-ci bot added the do-not-merge/work-in-progress label Nov 27, 2025

tim-sh mentioned this pull request Nov 27, 2025

The server cannot be started if rootUri is a WSL path #1236

Open

tim-sh added 3 commits November 27, 2025 15:42

test: soften assertion to refer to the four slashes only

a00d374

chore: use util method throughout

409c48a

tim-sh force-pushed the feat/wslSupport branch from b2e15f9 to 409c48a Compare November 27, 2025 14:47

angelozerr reviewed Nov 27, 2025

View reviewed changes

src/main/java/com/redhat/devtools/lsp4ij/LSPIJUtils.java Outdated Show resolved Hide resolved

angelozerr reviewed Nov 27, 2025

View reviewed changes

src/main/java/com/redhat/devtools/lsp4ij/features/formatting/AbstractLSPFormattingService.java Outdated Show resolved Hide resolved

angelozerr reviewed Nov 27, 2025

View reviewed changes

src/test/java/com/redhat/devtools/lsp4ij/LSPIJUtils_toUriTest.java Show resolved Hide resolved

tim-sh added 3 commits November 27, 2025 15:56

test: add test for paths with spaces

3dd90d9

refactor: method overload

af18652

docs: header and javadoc

6859b8e

tim-sh marked this pull request as ready for review November 27, 2025 15:10

openshift-ci bot removed the do-not-merge/work-in-progress label Nov 27, 2025

tim-sh changed the title ~~fix(lsp): use Path.toUri() to handle WSL paths correctly~~ fix: use Path.toUri() to handle WSL paths correctly Nov 27, 2025

angelozerr reviewed Nov 27, 2025

View reviewed changes

src/main/java/com/redhat/devtools/lsp4ij/LSPIJUtils.java Outdated Show resolved Hide resolved

fix: handle edge cases in URI conversion

574323f

angelozerr reviewed Nov 27, 2025

View reviewed changes

src/test/java/com/redhat/devtools/lsp4ij/LSPIJUtils_toUriTest.java Outdated Show resolved Hide resolved

test: split URI tests for Windows resp. Unix

ae90356

angelozerr reviewed Nov 27, 2025

View reviewed changes

fix: use Path.toUri() to handle WSL paths correctly #1378

Are you sure you want to change the base?

fix: use Path.toUri() to handle WSL paths correctly #1378

Uh oh!

Conversation

tim-sh commented Nov 27, 2025

Uh oh!

tim-sh commented Nov 27, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

angelozerr commented Nov 27, 2025

Uh oh!

angelozerr commented Nov 27, 2025

Uh oh!

angelozerr commented Nov 27, 2025

Uh oh!

tim-sh commented Nov 27, 2025

Uh oh!

angelozerr commented Nov 27, 2025

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

angelozerr Nov 27, 2025

Choose a reason for hiding this comment

Uh oh!

angelozerr commented Nov 27, 2025

Uh oh!

tim-sh commented Nov 28, 2025

Uh oh!

tim-sh commented Nov 28, 2025

Uh oh!

angelozerr commented Nov 28, 2025

Uh oh!

tim-sh commented Nov 28, 2025

Uh oh!

angelozerr commented Nov 28, 2025

Uh oh!

tim-sh commented Nov 28, 2025

Uh oh!

angelozerr commented Nov 28, 2025

Uh oh!

angelozerr commented Dec 2, 2025

Uh oh!

tim-sh commented Dec 2, 2025

Uh oh!

angelozerr commented Dec 2, 2025

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

tim-sh commented Nov 27, 2025 •

edited

Loading