Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Handle Invalid WSL URIs #1057

Merged
merged 5 commits into from
Sep 7, 2022
Merged

Handle Invalid WSL URIs #1057

merged 5 commits into from
Sep 7, 2022

Conversation

jpogran
Copy link
Contributor

@jpogran jpogran commented Aug 30, 2022

This adds another piece of logic to attempt to detect UNC WSL Uri as the workspace to open, and return a more informative error so the client can handle prompting the user.

The UNC WSL Uri being passed is in the form of \\wsl$\Ubuntu\home\james\some\path. If the user wants to edit files in the WSL instance, they should be opening the workspace in the Remote WSL extension.

When the VS Code sends the path to terraform-ls, it is urlencoded as \\wsl%24\Ubuntu\home\james\some\path. This fails uri.IsURIValid, which is incorrect because it is a valid path that needs to be decoded. Even if it is decoded, further on in the uri package we make assumptions about file paths that ignore the server portion of a UNC path, resulting in terraform-ls attempting to use Ubuntu\home\james\some\path. A future PR can address that.

Works with, but not required, hashicorp/vscode-terraform#1219

Closes #656

@jpogran jpogran self-assigned this Aug 30, 2022
@jpogran jpogran added the enhancement New feature or request label Aug 30, 2022
@radeksimko radeksimko marked this pull request as ready for review September 1, 2022 05:49
@radeksimko radeksimko requested a review from a team as a code owner September 1, 2022 05:49
Copy link
Member

@radeksimko radeksimko left a comment

Choose a reason for hiding this comment

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

I left suggestions in-line.

I reckon we'll also need to update this test

func TestInitialize_withInvalidRootURI(t *testing.T) {
tmpDir := TempDir(t)
ls := langserver.NewLangServerMock(t, NewMockSession(&MockSessionInput{
TerraformCalls: &exec.TerraformMockCalls{
PerWorkDir: map[string][]*mock.Call{
tmpDir.Path(): validTfMockCalls(),
},
},
}))
stop := ls.Start(t)
defer stop()
ls.CallAndExpectError(t, &langserver.CallRequest{
Method: "initialize",
ReqParams: `{
"capabilities": {},
"processId": 12345,
"rootUri": "meh"
}`}, code.SystemError.Err())
}

We could also add a similar one with a WSL URI but it's probably not worth it since we're comparing error type only anyway without the extra Data field.

internal/uri/uri.go Outdated Show resolved Hide resolved
internal/langserver/handlers/initialize.go Outdated Show resolved Hide resolved
Copy link
Member

@radeksimko radeksimko left a comment

Choose a reason for hiding this comment

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

Just one minor question/suggestion regarding the test, merge at will.

One thing I was aiming to achieve with the extra Data field was that we'd be able to compare it more easily with just == or === on the client side. Therefore I don't fully understand the motivation behind startsWith method here, but we can discuss it in that PR rather than here.

Even if it is decoded, further on in the uri package we make assumptions about file paths that ignore the server portion of a UNC path, resulting in terraform-ls attempting to use Ubuntu\home\james\some\path. A future PR can address that.

Yep - we can address that as part of #74

internal/uri/uri_test.go Outdated Show resolved Hide resolved
@radeksimko radeksimko changed the title Handle Invalid WSL Uri Handle Invalid WSL URIs Sep 2, 2022
This adds another piece of logic to attempt to detect UNC WSL Uri as the workspace to open, and return a more informative error so the client can handle prompting the user.

The UNC WSL Uri being passed is in the form of `\\wsl$\Ubuntu\home\james\some\path`. If the user wants to edit files in the WSL instance, they should be opening the workspace in the Remote WSL extension.

When the VS Code sends the path to terraform-ls, it is urlencoded as `\\wsl%24\Ubuntu\home\james\some\path`. This fails `uri.IsURIValid`, which is incorrect because it is a valid path that needs to be decoded. Even if it is decoded, further on in the `uri` package we make assumptions about file paths that ignore the server portion of a UNC path, resulting in terraform-ls attempting to use `Ubuntu\home\james\some\path`. A future PR can address that.
@jpogran jpogran merged commit c29b678 into main Sep 7, 2022
@jpogran jpogran deleted the wsluri branch September 7, 2022 12:50
@radeksimko radeksimko added this to the v0.29.2 milestone Sep 12, 2022
@github-actions
Copy link

This functionality has been released in v0.29.2 of the language server.

For further feature requests or bug reports with this functionality, please create a new GitHub issue following the template. Thank you!

@github-actions
Copy link

I'm going to lock this pull request because it has been closed for 30 days ⏳. This helps our maintainers find and focus on the active issues.
If you have found a problem that seems related to this change, please open a new issue and complete the issue template so we can capture all the details necessary to investigate further.

@github-actions github-actions bot locked as resolved and limited conversation to collaborators Oct 13, 2022
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
enhancement New feature or request
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Better guide WSL users opening UNC \\wsl$\ paths
2 participants