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

Fixing file path for saving new files from chat in windows #6489

Open
wants to merge 2 commits into
base: main
Choose a base branch
from
Open
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
Original file line number Diff line number Diff line change
Expand Up @@ -232,9 +232,15 @@ class CodyAgentClient(private val project: Project, private val webview: NativeW
var fileName = "Untitled.$ext".removeSuffix(".")
var outputDir: VirtualFile? =
if (params.defaultUri != null) {
val defaultUriPath = Paths.get(params.defaultUri)
fileName = defaultUriPath.fileName.toString()
VfsUtil.findFile(defaultUriPath.parent, true)
try {
val defaultUriPath = Paths.get(params.defaultUri)
Copy link
Member

Choose a reason for hiding this comment

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

defaultUri is a URI so it should be parsed into a path like this

Paths.get(URI.create(params.defaultUri))

This has accidentally worked on other platforms because it assumes file:// is part of the path name. Wrapping in try/catch won't fix the root problem.

Copy link
Contributor

Choose a reason for hiding this comment

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

@olafurpg maybe it's a good time to rewrite agent API to use Path objects instead of strings? There is quite a few corner cases in the way paths can be encoded (especially on windows) and discrepancies between agent and client path parsing hit us many times in past.

Copy link
Member

Choose a reason for hiding this comment

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

You mean the auto-generated bindings? We encode these as TypeScript string on the cody side and don't have a static type to distinguish between URIs and normal strings.

Copy link
Member

Choose a reason for hiding this comment

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

For good and bad, we rely on JSON.stringify, which doesn't expose hooks to customize serialization of URI instances, so we need to rely on some form of discipline

fileName = defaultUriPath.fileName.toString()
val parentDir = VfsUtil.findFile(defaultUriPath.parent, true)
parentDir
} catch (e: Exception) {
logger.error("code222: Error processing defaultUri", e)
null
}
} else {
project.guessProjectDir()
}
Expand All @@ -248,9 +254,14 @@ class CodyAgentClient(private val project: Project, private val webview: NativeW

val saveFileFuture = CompletableFuture<String>()
runInEdt {
val dialog = FileChooserFactory.getInstance().createSaveFileDialog(descriptor, project)
val result = dialog.save(outputDir, fileName)
saveFileFuture.complete(result?.file?.path)
try {
val dialog = FileChooserFactory.getInstance().createSaveFileDialog(descriptor, project)
val result = dialog.save(outputDir, fileName)
saveFileFuture.complete(result?.file?.path)
} catch (e: Exception) {
logger.error("code222: Error in save dialog", e)
saveFileFuture.completeExceptionally(e)
}
}

return saveFileFuture
Expand Down
24 changes: 18 additions & 6 deletions jetbrains/src/main/kotlin/com/sourcegraph/utils/CodyEditorUtil.kt
Original file line number Diff line number Diff line change
Expand Up @@ -197,19 +197,26 @@ object CodyEditorUtil {

fun findFileOrScratch(project: Project, uriString: String): VirtualFile? {
try {
val uri = URI.create(uriString)

val normalizedUri = if (uriString.matches(Regex("^[A-Za-z]:.+"))) {
"file:///" + uriString.replace('\\', '/')
} else {
uriString
}
val uri = URI.create(normalizedUri)
if (ConfigUtil.isIntegrationTestModeEnabled()) {
return TempFileSystem.getInstance().refreshAndFindFileByPath(uri.path)
} else {
val fixedUri = if (uriString.startsWith("untitled")) uri.withScheme("file") else uri
return LocalFileSystem.getInstance().refreshAndFindFileByNioFile(fixedUri.toPath())
val fixedUri = if (uriString.startsWith("untitled")) {
uri.withScheme("file")
} else uri
val path = fixedUri.toPath()
return LocalFileSystem.getInstance().refreshAndFindFileByNioFile(path)
}
} catch (e: URISyntaxException) {
// Let's try scratch files
val fileName = uriString.substringAfterLast(':').trimStart('/', '\\')
return ScratchRootType.getInstance()
.findFile(project, fileName, ScratchFileService.Option.existing_only)
.findFile(project, fileName, ScratchFileService.Option.existing_only)
}
}

Expand All @@ -219,7 +226,12 @@ object CodyEditorUtil {
content: String? = null
): VirtualFile? {
try {
val uri = URI.create(uriString)
val normalizedUri = if (uriString.matches(Regex("^[A-Za-z]:.+"))) {
"file:///" + uriString.replace('\\', '/')
} else {
uriString
}
val uri = URI.create(normalizedUri)

val fileUri = uri.withScheme("file")
if (!fileUri.toPath().exists()) {
Expand Down
37 changes: 37 additions & 0 deletions jetbrains/src/main/kotlin/com/sourcegraph/utils/CodyUriUtil.kt
Original file line number Diff line number Diff line change
@@ -0,0 +1,37 @@
package com.sourcegraph.utils

import java.io.File
import java.net.URI
import java.nio.file.Path
import java.nio.file.Paths

object CodyUriUtil {
/**
* Normalizes a URI string to a consistent format across platforms.
*/
@JvmStatic
fun normalizeUri(uriString: String): URI {
Copy link
Contributor

Choose a reason for hiding this comment

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

That is not really returning normalised URI, just a normal one.
You would have to call .normalize() on it at the end to make that true.

Copy link
Contributor

Choose a reason for hiding this comment

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

Also, you created a normalizeUri method which converts stinf to URI but you are not using it anywhere in your code?

if (uriString.isBlank()) {
throw IllegalArgumentException("URI string cannot be empty")
}

return when {
uriString.contains("://") -> URI.create(uriString)
uriString.startsWith("untitled:") -> URI.create(uriString).withScheme("file")
File(uriString).isAbsolute -> File(uriString).toURI()
else -> Paths.get(uriString).toUri()
}
}

@JvmStatic
fun toPath(uriString: String): Path =
Paths.get(normalizeUri(uriString))

@JvmStatic
Copy link
Contributor

Choose a reason for hiding this comment

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

I would not encourage using API which operates on strings in jetbrains.
One should always convert to URI and then compare the objects directly.
And for that we should have tests.

fun areEquivalent(uri1: String, uri2: String): Boolean =
normalizeUri(uri1).normalize() == normalizeUri(uri2).normalize()

@JvmStatic
fun toNormalizedString(uriString: String): String =
normalizeUri(uriString).toString()
}
130 changes: 130 additions & 0 deletions jetbrains/src/test/kotlin/utils/CodyUriUtilTest.kt
Original file line number Diff line number Diff line change
@@ -0,0 +1,130 @@
package com.sourcegraph.utils

import org.junit.Assert.*
import org.junit.Test
import java.io.File
import java.net.URI

class CodyUriUtilTest {

@Test
fun `test standard file URIs`() {
val expected = "file:///path/to/file.txt"
val inputs = listOf(
"/path/to/file.txt",
"file:///path/to/file.txt",
File("/path/to/file.txt").absolutePath
)

inputs.forEach { input ->
val result = CodyUriUtil.normalizeUri(input)
assertEquals(expected, result.toString())
}
}

@Test
fun `test Windows paths`() {
val inputs = mapOf(
"C:\\Users\\test\\file.txt" to "file:///C:/Users/test/file.txt",
"C:/Users/test/file.txt" to "file:///C:/Users/test/file.txt",
"file:///C:/Users/test/file.txt" to "file:///C:/Users/test/file.txt"
)

inputs.forEach { (input, expected) ->
val result = CodyUriUtil.normalizeUri(input)
assertEquals(expected, result.toString())
}
}

@Test
fun `test WSL paths`() {
val inputs = mapOf(
"\\\\wsl\$\\Ubuntu\\home\\user\\file.txt" to "file:////wsl$/Ubuntu/home/user/file.txt",
"//wsl$/Ubuntu/home/user/file.txt" to "file:////wsl$/Ubuntu/home/user/file.txt"
)

inputs.forEach { (input, expected) ->
val result = CodyUriUtil.normalizeUri(input)
assertEquals(expected, result.toString())
}
}

@Test
fun `test untitled URIs`() {
val input = "untitled:Untitled-1"
val result = CodyUriUtil.normalizeUri(input)
assertEquals("file:Untitled-1", result.toString())
}

@Test
fun `test network paths`() {
val inputs = mapOf(
"\\\\server\\share\\file.txt" to "file:////server/share/file.txt",
"//server/share/file.txt" to "file:////server/share/file.txt"
)

inputs.forEach { (input, expected) ->
val result = CodyUriUtil.normalizeUri(input)
assertEquals(expected, result.toString())
}
}

@Test
fun `test URI equivalence`() {
val equivalentPairs = listOf(
Pair("C:\\Users\\test\\file.txt", "file:///C:/Users/test/file.txt"),
Pair("/path/to/file.txt", "file:///path/to/file.txt"),
Pair("\\\\server\\share\\file.txt", "file:////server/share/file.txt")
)

equivalentPairs.forEach { (uri1, uri2) ->
assertTrue(CodyUriUtil.areEquivalent(uri1, uri2))
}
}

@Test
fun `test path conversion`() {
val inputs = mapOf(
"file:///path/to/file.txt" to "/path/to/file.txt",
"file:///C:/Users/test/file.txt" to "C:\\Users\\test\\file.txt"
)

inputs.forEach { (input, expected) ->
val result = CodyUriUtil.toPath(input)
assertEquals(File(expected).absolutePath, result.toFile().absolutePath)
}
}

@Test(expected = IllegalArgumentException::class)
fun `test empty URI string`() {
CodyUriUtil.normalizeUri("")
}

@Test
fun `test special characters in paths`() {
val inputs = mapOf(
"/path/with spaces/file.txt" to "file:///path/with%20spaces/file.txt",
"/path/with#hash/file.txt" to "file:///path/with%23hash/file.txt",
"/path/with?question/file.txt" to "file:///path/with%3Fquestion/file.txt"
)

inputs.forEach { (input, expected) ->
val result = CodyUriUtil.normalizeUri(input)
assertEquals(expected, result.toString())
}
}

@Test
fun `test remote URIs`() {
val inputs = listOf(
"http://example.com/file.txt",
"https://example.com/file.txt",
"ftp://example.com/file.txt"
)

inputs.forEach { input ->
val result = CodyUriUtil.normalizeUri(input)
assertEquals(input, result.toString())
}
}
}
Loading