From 0d6a49ebea0d421d2f97f357580cdbc7d2d2372a Mon Sep 17 00:00:00 2001 From: Benoit Quenaudon Date: Mon, 19 Jun 2023 14:20:02 +0200 Subject: [PATCH 1/2] Support symlink for NioFSWrappingFS --- .../kotlin/okio/AbstractFileSystemTest.kt | 37 +++++++++++++----- .../okio/NioFileSystemWrappingFileSystem.kt | 38 ++++++++++--------- .../kotlin/okio/FileHandleFileSystemTest.kt | 2 +- .../kotlin/okio/JvmSystemFileSystemTest.kt | 13 ++++++- 4 files changed, 61 insertions(+), 29 deletions(-) diff --git a/okio-testing-support/src/commonMain/kotlin/okio/AbstractFileSystemTest.kt b/okio-testing-support/src/commonMain/kotlin/okio/AbstractFileSystemTest.kt index 3fbe32156f..666c6ab305 100644 --- a/okio-testing-support/src/commonMain/kotlin/okio/AbstractFileSystemTest.kt +++ b/okio-testing-support/src/commonMain/kotlin/okio/AbstractFileSystemTest.kt @@ -47,6 +47,7 @@ abstract class AbstractFileSystemTest( ) { val base: Path = temporaryDirectory / "${this::class.simpleName}-${randomToken(16)}" private val isNodeJsFileSystem = fileSystem::class.simpleName?.startsWith("NodeJs") ?: false + private val isWrappingJimFileSystem = this::class.simpleName?.contains("JimFileSystem") ?: false @BeforeTest fun setUp() { @@ -72,13 +73,17 @@ abstract class AbstractFileSystemTest( val cwdString = cwd.toString() val slash = Path.DIRECTORY_SEPARATOR assertTrue(cwdString) { - cwdString.endsWith("okio${slash}okio") || - cwdString.endsWith("${slash}okio-parent-okio-js-legacy-test") || - cwdString.endsWith("${slash}okio-parent-okio-js-ir-test") || - cwdString.endsWith("${slash}okio-parent-okio-nodefilesystem-js-ir-test") || - cwdString.endsWith("${slash}okio-parent-okio-nodefilesystem-js-legacy-test") || - cwdString.contains("/CoreSimulator/Devices/") || // iOS simulator. - cwdString == "/" // Android emulator. + if (isWrappingJimFileSystem) { + cwdString.endsWith("work") + } else { + cwdString.endsWith("okio${slash}okio") || + cwdString.endsWith("${slash}okio-parent-okio-js-legacy-test") || + cwdString.endsWith("${slash}okio-parent-okio-js-ir-test") || + cwdString.endsWith("${slash}okio-parent-okio-nodefilesystem-js-ir-test") || + cwdString.endsWith("${slash}okio-parent-okio-nodefilesystem-js-legacy-test") || + cwdString.contains("/CoreSimulator/Devices/") || // iOS simulator. + cwdString == "/" // Android emulator. + } } } @@ -90,12 +95,19 @@ abstract class AbstractFileSystemTest( } @Test - fun canonicalizeNoSuchFile() { + fun canonicalizeAbsolutePathNoSuchFile() { assertFailsWith { fileSystem.canonicalize(base / "no-such-file") } } + @Test + fun canonicalizeRelativePathNoSuchFile() { + assertFailsWith { + fileSystem.canonicalize("no-such-file".toPath()) + } + } + @Test fun canonicalizeFollowsSymlinkDirectories() { if (!supportsSymlink()) return @@ -260,6 +272,10 @@ abstract class AbstractFileSystemTest( fileSystem.write("a.txt".toPath()) { writeUtf8("hello, world!") } + } else if (isWrappingJimFileSystem) { + fileSystem.write("a.txt".toPath()) { + writeUtf8("hello, world!") + } } val entries = fileSystem.list(".".toPath()) @@ -319,6 +335,10 @@ abstract class AbstractFileSystemTest( fileSystem.write("a.txt".toPath()) { writeUtf8("hello, world!") } + } else if (isWrappingJimFileSystem) { + fileSystem.write("a.txt".toPath()) { + writeUtf8("hello, world!") + } } val entries = fileSystem.listOrNull(".".toPath()) @@ -2404,7 +2424,6 @@ abstract class AbstractFileSystemTest( if (windowsLimitations) return false return when (fileSystem::class.simpleName) { "JvmSystemFileSystem", - "NioFileSystemWrappingFileSystem", -> false else -> true } diff --git a/okio/src/jvmMain/kotlin/okio/NioFileSystemWrappingFileSystem.kt b/okio/src/jvmMain/kotlin/okio/NioFileSystemWrappingFileSystem.kt index 4131e09eb9..cab8ab51b2 100644 --- a/okio/src/jvmMain/kotlin/okio/NioFileSystemWrappingFileSystem.kt +++ b/okio/src/jvmMain/kotlin/okio/NioFileSystemWrappingFileSystem.kt @@ -17,7 +17,7 @@ package okio import java.io.InterruptedIOException import java.nio.channels.FileChannel -import java.nio.file.FileSystem as JavaNioFileSystem +import java.nio.file.FileSystem as NioFileSystem import java.nio.file.Files import java.nio.file.NoSuchFileException import java.nio.file.Path as NioPath @@ -36,17 +36,14 @@ import okio.Path.Companion.toOkioPath * A file system that wraps a `java.nio.file.FileSystem` and executes all operations in the context of the wrapped file * system. */ -internal class NioFileSystemWrappingFileSystem(javaNioFileSystem: JavaNioFileSystem) : NioSystemFileSystem() { - // TODO(Benoit) How do deal with multiple directories? Test with Windows someday. - private val delegateRoot = javaNioFileSystem.rootDirectories.first() - +internal class NioFileSystemWrappingFileSystem(private val nioFileSystem: NioFileSystem) : NioSystemFileSystem() { /** * On a `java.nio.file.FileSystem`, `java.nio.file.Path` are stateful and hold a reference to the file system they * got provided from. We need to [resolve][java.nio.file.Path.resolve] all okio paths before doing operations on the * nio file system in order for things to work properly. */ private fun Path.resolve(readSymlink: Boolean = false): NioPath { - val resolved = delegateRoot.resolve(toString()) + val resolved = nioFileSystem.getPath(toString()) return if (readSymlink && resolved.isSymbolicLink()) { resolved.readSymbolicLink() } else { @@ -54,6 +51,14 @@ internal class NioFileSystemWrappingFileSystem(javaNioFileSystem: JavaNioFileSys } } + override fun canonicalize(path: Path): Path { + try { + return path.resolve(readSymlink = true).toRealPath().toOkioPath() + } catch (e: NoSuchFileException) { + throw FileNotFoundException("no such file: $this") + } + } + override fun metadataOrNull(path: Path): FileMetadata? { return metadataOrNull(path.resolve()) } @@ -74,17 +79,7 @@ internal class NioFileSystemWrappingFileSystem(javaNioFileSystem: JavaNioFileSys return null } } - val result = entries.mapTo(mutableListOf()) { entry -> - // TODO(Benoit) This whole block can surely be improved. - val path = dir / entry.toOkioPath() - if (dir.isRelative) { - // All `entries` are absolute and resolving them against `dir` won't have any effect. We need to manually - // relativize them back. - nioDir.relativize(path.resolve()).toOkioPath() - } else { - path - } - } + val result = entries.mapTo(mutableListOf()) { entry -> dir / entry.toString() } result.sort() return result } @@ -195,7 +190,14 @@ internal class NioFileSystemWrappingFileSystem(javaNioFileSystem: JavaNioFileSys } override fun createSymlink(source: Path, target: Path) { - Files.createSymbolicLink(source.resolve(), target.resolve()) + val sourceNioPath = source.resolve() + val targetNioPath = + if (source.isAbsolute && target.isRelative) { + sourceNioPath.parent.resolve(target.toString()) + } else { + target.resolve() + } + Files.createSymbolicLink(sourceNioPath, targetNioPath) } override fun toString(): String = "NioFileSystemWrappingFileSystem" diff --git a/okio/src/jvmTest/kotlin/okio/FileHandleFileSystemTest.kt b/okio/src/jvmTest/kotlin/okio/FileHandleFileSystemTest.kt index 06baf8a060..a36ee0075f 100644 --- a/okio/src/jvmTest/kotlin/okio/FileHandleFileSystemTest.kt +++ b/okio/src/jvmTest/kotlin/okio/FileHandleFileSystemTest.kt @@ -59,7 +59,7 @@ class FileHandleFileSystemTest : AbstractFileSystemTest( } } -class FileHandleNioFileSystemWrapperFileSystemTest : AbstractFileSystemTest( +class FileHandleNioJimFileSystemWrapperFileSystemTest : AbstractFileSystemTest( clock = Clock.System, fileSystem = Jimfs .newFileSystem( diff --git a/okio/src/jvmTest/kotlin/okio/JvmSystemFileSystemTest.kt b/okio/src/jvmTest/kotlin/okio/JvmSystemFileSystemTest.kt index 1463727c0c..fc4b7c0910 100644 --- a/okio/src/jvmTest/kotlin/okio/JvmSystemFileSystemTest.kt +++ b/okio/src/jvmTest/kotlin/okio/JvmSystemFileSystemTest.kt @@ -18,6 +18,7 @@ package okio import com.google.common.jimfs.Configuration import com.google.common.jimfs.Jimfs import java.io.InterruptedIOException +import java.nio.file.FileSystems import kotlin.test.assertEquals import kotlin.test.assertFalse import kotlin.test.fail @@ -59,7 +60,7 @@ class JvmSystemFileSystemTest : AbstractFileSystemTest( } } -class NioFileSystemWrappingFileSystemTest : AbstractFileSystemTest( +class NioJimFileSystemWrappingFileSystemTest : AbstractFileSystemTest( clock = Clock.System, fileSystem = Jimfs .newFileSystem( @@ -73,3 +74,13 @@ class NioFileSystemWrappingFileSystemTest : AbstractFileSystemTest( allowAtomicMoveFromFileToDirectory = true, temporaryDirectory = FileSystem.SYSTEM_TEMPORARY_DIRECTORY, ) + +class NioDefaultFileSystemWrappingFileSystemTest : AbstractFileSystemTest( + clock = Clock.System, + fileSystem = FileSystems.getDefault().asOkioFileSystem(), + windowsLimitations = false, + allowClobberingEmptyDirectories = Path.DIRECTORY_SEPARATOR == "\\", + allowAtomicMoveFromFileToDirectory = false, + allowRenameWhenTargetIsOpen = Path.DIRECTORY_SEPARATOR != "\\", + temporaryDirectory = FileSystem.SYSTEM_TEMPORARY_DIRECTORY, +) From a681001390e9f4a31aaabed753ee5e014bea14ef Mon Sep 17 00:00:00 2001 From: Benoit Quenaudon Date: Tue, 20 Jun 2023 20:34:28 +0200 Subject: [PATCH 2/2] Resolve symlink at the right place --- .../kotlin/okio/AbstractFileSystemTest.kt | 23 +++++++++- .../okio/NioFileSystemWrappingFileSystem.kt | 45 +++++++------------ .../kotlin/okio/FileHandleFileSystemTest.kt | 21 +++++---- 3 files changed, 51 insertions(+), 38 deletions(-) diff --git a/okio-testing-support/src/commonMain/kotlin/okio/AbstractFileSystemTest.kt b/okio-testing-support/src/commonMain/kotlin/okio/AbstractFileSystemTest.kt index 666c6ab305..8601d66265 100644 --- a/okio-testing-support/src/commonMain/kotlin/okio/AbstractFileSystemTest.kt +++ b/okio-testing-support/src/commonMain/kotlin/okio/AbstractFileSystemTest.kt @@ -2176,7 +2176,7 @@ abstract class AbstractFileSystemTest( } @Test - fun symlinkMetadata() { + fun absoluteSymlinkMetadata() { if (!supportsSymlink()) return val target = base / "symlink-target" @@ -2191,6 +2191,22 @@ abstract class AbstractFileSystemTest( assertInRange(sourceMetadata.createdAt, minTime, maxTime) } + @Test + fun relativeSymlinkMetadata() { + if (!supportsSymlink()) return + + val target = "symlink-target".toPath() + val source = base / "symlink-source" + + val minTime = clock.now() + fileSystem.createSymlink(source, target) + val maxTime = clock.now() + + val sourceMetadata = fileSystem.metadata(source) + assertEquals(target, sourceMetadata.symlinkTarget) + assertInRange(sourceMetadata.createdAt, minTime, maxTime) + } + @Test fun createSymlinkSourceAlreadyExists() { if (!supportsSymlink()) return @@ -2231,6 +2247,7 @@ abstract class AbstractFileSystemTest( @Test fun openSymlinkSink() { if (!supportsSymlink()) return + if (isJimFileSystem()) return val target = base / "symlink-target" val source = base / "symlink-source" @@ -2507,6 +2524,10 @@ abstract class AbstractFileSystemTest( return windowsLimitations && fileSystem::class.simpleName == "JvmSystemFileSystem" } + private fun isJimFileSystem(): Boolean { + return "JimfsFileSystem" in fileSystem.toString() + } + private fun isNodeJsFileSystemOnWindows(): Boolean { return windowsLimitations && fileSystem::class.simpleName == "NodeJsFileSystem" } diff --git a/okio/src/jvmMain/kotlin/okio/NioFileSystemWrappingFileSystem.kt b/okio/src/jvmMain/kotlin/okio/NioFileSystemWrappingFileSystem.kt index cab8ab51b2..8737f8c8ca 100644 --- a/okio/src/jvmMain/kotlin/okio/NioFileSystemWrappingFileSystem.kt +++ b/okio/src/jvmMain/kotlin/okio/NioFileSystemWrappingFileSystem.kt @@ -18,18 +18,18 @@ package okio import java.io.InterruptedIOException import java.nio.channels.FileChannel import java.nio.file.FileSystem as NioFileSystem -import java.nio.file.Files import java.nio.file.NoSuchFileException import java.nio.file.Path as NioPath import java.nio.file.StandardCopyOption import java.nio.file.StandardOpenOption import kotlin.io.path.createDirectory +import kotlin.io.path.createSymbolicLinkPointingTo +import kotlin.io.path.deleteExisting import kotlin.io.path.exists import kotlin.io.path.inputStream -import kotlin.io.path.isSymbolicLink import kotlin.io.path.listDirectoryEntries +import kotlin.io.path.moveTo import kotlin.io.path.outputStream -import kotlin.io.path.readSymbolicLink import okio.Path.Companion.toOkioPath /** @@ -38,24 +38,19 @@ import okio.Path.Companion.toOkioPath */ internal class NioFileSystemWrappingFileSystem(private val nioFileSystem: NioFileSystem) : NioSystemFileSystem() { /** - * On a `java.nio.file.FileSystem`, `java.nio.file.Path` are stateful and hold a reference to the file system they - * got provided from. We need to [resolve][java.nio.file.Path.resolve] all okio paths before doing operations on the - * nio file system in order for things to work properly. + * On a [java.nio.file.FileSystem], paths are stateful and hold a reference to the file system they got provided from. + * Using [getPath][NioFileSystem.getPath], we ask [nioFileSystem] to wrap the [Path]'s value in order to set itself as + * its provider which is needed for operations on the nio file system to work properly. */ - private fun Path.resolve(readSymlink: Boolean = false): NioPath { - val resolved = nioFileSystem.getPath(toString()) - return if (readSymlink && resolved.isSymbolicLink()) { - resolved.readSymbolicLink() - } else { - resolved - } + private fun Path.resolve(): NioPath { + return nioFileSystem.getPath(toString()) } override fun canonicalize(path: Path): Path { try { - return path.resolve(readSymlink = true).toRealPath().toOkioPath() + return path.resolve().toRealPath().toOkioPath() } catch (e: NoSuchFileException) { - throw FileNotFoundException("no such file: $this") + throw FileNotFoundException("no such file: $path") } } @@ -115,7 +110,7 @@ internal class NioFileSystemWrappingFileSystem(private val nioFileSystem: NioFil override fun source(file: Path): Source { try { - return file.resolve(readSymlink = true).inputStream().source() + return file.resolve().inputStream().source() } catch (e: NoSuchFileException) { throw FileNotFoundException("no such file: $file") } @@ -126,7 +121,7 @@ internal class NioFileSystemWrappingFileSystem(private val nioFileSystem: NioFil if (mustCreate) add(StandardOpenOption.CREATE_NEW) } try { - return file.resolve(readSymlink = true) + return file.resolve() .outputStream(*openOptions.toTypedArray()) .sink() } catch (e: NoSuchFileException) { @@ -161,8 +156,7 @@ internal class NioFileSystemWrappingFileSystem(private val nioFileSystem: NioFil // Note that `java.nio.file.FileSystem` allows atomic moves of a file even if the target is an existing directory. override fun atomicMove(source: Path, target: Path) { try { - Files.move( - source.resolve(), + source.resolve().moveTo( target.resolve(), StandardCopyOption.ATOMIC_MOVE, StandardCopyOption.REPLACE_EXISTING, @@ -181,7 +175,7 @@ internal class NioFileSystemWrappingFileSystem(private val nioFileSystem: NioFil } val nioPath = path.resolve() try { - Files.delete(nioPath) + nioPath.deleteExisting() } catch (e: NoSuchFileException) { if (mustExist) throw FileNotFoundException("no such file: $path") } catch (e: IOException) { @@ -190,15 +184,8 @@ internal class NioFileSystemWrappingFileSystem(private val nioFileSystem: NioFil } override fun createSymlink(source: Path, target: Path) { - val sourceNioPath = source.resolve() - val targetNioPath = - if (source.isAbsolute && target.isRelative) { - sourceNioPath.parent.resolve(target.toString()) - } else { - target.resolve() - } - Files.createSymbolicLink(sourceNioPath, targetNioPath) + source.resolve().createSymbolicLinkPointingTo(target.resolve()) } - override fun toString(): String = "NioFileSystemWrappingFileSystem" + override fun toString() = nioFileSystem::class.simpleName!! } diff --git a/okio/src/jvmTest/kotlin/okio/FileHandleFileSystemTest.kt b/okio/src/jvmTest/kotlin/okio/FileHandleFileSystemTest.kt index a36ee0075f..021ffb4cbb 100644 --- a/okio/src/jvmTest/kotlin/okio/FileHandleFileSystemTest.kt +++ b/okio/src/jvmTest/kotlin/okio/FileHandleFileSystemTest.kt @@ -19,6 +19,7 @@ import com.google.common.jimfs.Configuration import com.google.common.jimfs.Jimfs import java.nio.file.FileSystems import kotlinx.datetime.Clock +import okio.FileHandleFileSystemTest.FileHandleTestingFileSystem import okio.FileSystem.Companion.asOkioFileSystem /** @@ -61,13 +62,15 @@ class FileHandleFileSystemTest : AbstractFileSystemTest( class FileHandleNioJimFileSystemWrapperFileSystemTest : AbstractFileSystemTest( clock = Clock.System, - fileSystem = Jimfs - .newFileSystem( - when (Path.DIRECTORY_SEPARATOR == "\\") { - true -> Configuration.windows() - false -> Configuration.unix() - }, - ).asOkioFileSystem(), + fileSystem = FileHandleTestingFileSystem( + Jimfs + .newFileSystem( + when (Path.DIRECTORY_SEPARATOR == "\\") { + true -> Configuration.windows() + false -> Configuration.unix() + }, + ).asOkioFileSystem(), + ), windowsLimitations = false, allowClobberingEmptyDirectories = true, allowAtomicMoveFromFileToDirectory = true, @@ -76,7 +79,9 @@ class FileHandleNioJimFileSystemWrapperFileSystemTest : AbstractFileSystemTest( class FileHandleNioDefaultFileSystemWrapperFileSystemTest : AbstractFileSystemTest( clock = Clock.System, - fileSystem = FileSystems.getDefault().asOkioFileSystem(), + fileSystem = FileHandleTestingFileSystem( + FileSystems.getDefault().asOkioFileSystem(), + ), windowsLimitations = false, allowClobberingEmptyDirectories = Path.DIRECTORY_SEPARATOR == "\\", allowAtomicMoveFromFileToDirectory = false,