Skip to content

Commit

Permalink
Encode filepaths to be safe on Windows
Browse files Browse the repository at this point in the history
This changes the file paths to use characters that are safe for Windows.

Channges the output of the following:
* Package cache directory
* Generated pkl-doc files
* Kotlin generated code

Unsafe characters are encoded as (<hex>).
For example, the colon character `:` is encoded as `(3a)`.

Additionally, this changes the cache directory prefix (package-1 to
package-2).

Follows the design of apple/pkl-evolution#3
  • Loading branch information
bioball committed May 13, 2024
1 parent a4cf786 commit de4d78b
Show file tree
Hide file tree
Showing 36 changed files with 185 additions and 106 deletions.
2 changes: 1 addition & 1 deletion pkl-cli/src/test/kotlin/org/pkl/cli/CliEvaluatorTest.kt
Original file line number Diff line number Diff line change
Expand Up @@ -1164,7 +1164,7 @@ result = someLib.x
"""
.trimIndent()
)
assertThat(tempDir.resolve("package-1")).doesNotExist()
assertThat(tempDir.resolve("package-2")).doesNotExist()
}

@Test
Expand Down
28 changes: 14 additions & 14 deletions pkl-cli/src/test/kotlin/org/pkl/cli/CliPackageDownloaderTest.kt
Original file line number Diff line number Diff line change
Expand Up @@ -56,12 +56,12 @@ class CliPackageDownloaderTest {
noTransitive = true
)
cmd.run()
assertThat(tempDir.resolve("package-1/localhost:0/birds@0.5.0/birds@0.5.0.zip")).exists()
assertThat(tempDir.resolve("package-1/localhost:0/birds@0.5.0/birds@0.5.0.json")).exists()
assertThat(tempDir.resolve("package-1/localhost:0/fruit@1.0.5/fruit@1.0.5.zip")).exists()
assertThat(tempDir.resolve("package-1/localhost:0/fruit@1.0.5/fruit@1.0.5.json")).exists()
assertThat(tempDir.resolve("package-1/localhost:0/fruit@1.1.0/fruit@1.1.0.zip")).exists()
assertThat(tempDir.resolve("package-1/localhost:0/fruit@1.1.0/fruit@1.1.0.json")).exists()
assertThat(tempDir.resolve("package-2/localhost(3a)0/birds@0.5.0/birds@0.5.0.zip")).exists()
assertThat(tempDir.resolve("package-2/localhost(3a)0/birds@0.5.0/birds@0.5.0.json")).exists()
assertThat(tempDir.resolve("package-2/localhost(3a)0/fruit@1.0.5/fruit@1.0.5.zip")).exists()
assertThat(tempDir.resolve("package-2/localhost(3a)0/fruit@1.0.5/fruit@1.0.5.json")).exists()
assertThat(tempDir.resolve("package-2/localhost(3a)0/fruit@1.1.0/fruit@1.1.0.zip")).exists()
assertThat(tempDir.resolve("package-2/localhost(3a)0/fruit@1.1.0/fruit@1.1.0.json")).exists()
}

@Test
Expand Down Expand Up @@ -90,9 +90,9 @@ class CliPackageDownloaderTest {
noTransitive = true
)
cmd.run()
assertThat(tempDir.resolve(".my-cache/package-1/localhost:0/birds@0.5.0/birds@0.5.0.zip"))
assertThat(tempDir.resolve(".my-cache/package-2/localhost(3a)0/birds@0.5.0/birds@0.5.0.zip"))
.exists()
assertThat(tempDir.resolve(".my-cache/package-1/localhost:0/birds@0.5.0/birds@0.5.0.json"))
assertThat(tempDir.resolve(".my-cache/package-2/localhost(3a)0/birds@0.5.0/birds@0.5.0.json"))
.exists()
}

Expand All @@ -113,8 +113,8 @@ class CliPackageDownloaderTest {
noTransitive = true
)
cmd.run()
assertThat(tempDir.resolve("package-1/localhost:0/birds@0.5.0/birds@0.5.0.zip")).exists()
assertThat(tempDir.resolve("package-1/localhost:0/birds@0.5.0/birds@0.5.0.json")).exists()
assertThat(tempDir.resolve("package-2/localhost(3a)0/birds@0.5.0/birds@0.5.0.zip")).exists()
assertThat(tempDir.resolve("package-2/localhost(3a)0/birds@0.5.0/birds@0.5.0.json")).exists()
}

@Test
Expand Down Expand Up @@ -228,9 +228,9 @@ class CliPackageDownloaderTest {
noTransitive = false
)
.run()
assertThat(tempDir.resolve("package-1/localhost:0/birds@0.5.0/birds@0.5.0.zip")).exists()
assertThat(tempDir.resolve("package-1/localhost:0/birds@0.5.0/birds@0.5.0.json")).exists()
assertThat(tempDir.resolve("package-1/localhost:0/fruit@1.0.5/fruit@1.0.5.zip")).exists()
assertThat(tempDir.resolve("package-1/localhost:0/fruit@1.0.5/fruit@1.0.5.json")).exists()
assertThat(tempDir.resolve("package-2/localhost(3a)0/birds@0.5.0/birds@0.5.0.zip")).exists()
assertThat(tempDir.resolve("package-2/localhost(3a)0/birds@0.5.0/birds@0.5.0.json")).exists()
assertThat(tempDir.resolve("package-2/localhost(3a)0/fruit@1.0.5/fruit@1.0.5.zip")).exists()
assertThat(tempDir.resolve("package-2/localhost(3a)0/fruit@1.0.5/fruit@1.0.5.json")).exists()
}
}
Original file line number Diff line number Diff line change
Expand Up @@ -22,6 +22,7 @@ import java.net.URI
import java.util.*
import org.pkl.core.*
import org.pkl.core.util.CodeGeneratorUtils
import org.pkl.core.util.IoUtils

data class KotlinCodegenOptions(
/** The characters to use for indenting generated Kotlin code. */
Expand Down Expand Up @@ -89,7 +90,7 @@ class KotlinCodeGenerator(

private val propertyFileName: String
get() =
"resources/META-INF/org/pkl/config/java/mapper/classes/${moduleSchema.moduleName}.properties"
"resources/META-INF/org/pkl/config/java/mapper/classes/${IoUtils.encodePath(moduleSchema.moduleName)}.properties"

private val propertiesFile: String
get() {
Expand Down Expand Up @@ -195,7 +196,7 @@ class KotlinCodeGenerator(
}

private fun relativeOutputPathFor(moduleName: String): String {
val nameParts = moduleName.split(".")
val nameParts = moduleName.split(".").map(IoUtils::encodePath)
val dirPath = nameParts.dropLast(1).joinToString("/")
val fileName = nameParts.last().replaceFirstChar { it.titlecaseChar() }
return if (dirPath.isEmpty()) {
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -1501,6 +1501,24 @@ class KotlinCodeGeneratorTest {
confirmSerDe(bigStruct)
}

@Test
fun `encoded file paths`(@TempDir path: Path) {
val kotlinCode =
generateKotlinFiles(
path,
PklModule(
"FooBar.pkl",
"""
module `Foo*Bar`
someProp: String
"""
.trimIndent()
)
)
assertThat(kotlinCode).containsKey("kotlin/Foo(2a)Bar.kt")
}

private fun generateFiles(tempDir: Path, vararg pklModules: PklModule): Map<String, String> {
val pklFiles = pklModules.map { it.writeToDisk(tempDir.resolve("pkl/${it.name}.pkl")) }
val evaluator = Evaluator.preconfigured()
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -51,7 +51,14 @@ class PackageServer : AutoCloseable {
const val FRUIT_1_1_SHA = "8d982761d182f2185e4180c82190791d9a60c721cb3393bb2e946fab90131e8c"

fun populateCacheDir(cacheDir: Path) {
val basePath = cacheDir.resolve("package-1/localhost:$PORT")
doPopulateCacheDir(cacheDir.resolve("package-2/localhost(3a)$PORT"))
}

fun populateLegacyCacheDir(cacheDir: Path) {
doPopulateCacheDir(cacheDir.resolve("package-1/localhost:$PORT"))
}

private fun doPopulateCacheDir(basePath: Path) {
basePath.deleteRecursively()
Files.walk(packagesDir).use { stream ->
stream.forEach { source ->
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -22,6 +22,7 @@
import java.util.Set;
import org.pkl.config.java.InvalidMappingException;
import org.pkl.core.PClassInfo;
import org.pkl.core.util.IoUtils;
import org.pkl.core.util.Nullable;

/**
Expand Down Expand Up @@ -77,7 +78,7 @@ private static void initClassMappings(String pklModuleName) {
loadedModules.add(pklModuleName);
var url =
ClassRegistry.class.getResourceAsStream(
CLASSES_DIRECTORY + "/" + pklModuleName + ".properties");
CLASSES_DIRECTORY + "/" + IoUtils.encodePath(pklModuleName) + ".properties");
if (url == null) {
return;
}
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -413,7 +413,7 @@ static final class DiskCachedPackageResolver extends AbstractPackageResolver {

private final Path tmpDir;

private static final String CACHE_DIR_PREFIX = "package-1";
private static final String CACHE_DIR_PREFIX = "package-2";

@GuardedBy("lock")
private final EconomicMap<PackageUri, FileSystem> fileSystems = EconomicMaps.create();
Expand All @@ -438,12 +438,14 @@ private String getEffectivePackageUriPath(PackageUri packageUri) {
return path;
}
var checksumIdx = path.lastIndexOf("::");
return path.substring(0, checksumIdx);
return IoUtils.encodePath(path.substring(0, checksumIdx));
}

private Path getRelativePath(PackageUri uri) {
return Path.of(
CACHE_DIR_PREFIX, uri.getUri().getAuthority(), getEffectivePackageUriPath(uri));
CACHE_DIR_PREFIX,
IoUtils.encodePath(uri.getUri().getAuthority()),
getEffectivePackageUriPath(uri));
}

private String getLastSegmentName(PackageUri packageUri) {
Expand Down
24 changes: 24 additions & 0 deletions pkl-core/src/main/java/org/pkl/core/util/IoUtils.java
Original file line number Diff line number Diff line change
Expand Up @@ -597,6 +597,30 @@ public static URI fixTripleSlashUri(URI baseUri, URI newUri) {
return newUri;
}

/**
* Windows reserves characters {@code <>:"\|?*} in filenames.
*
* <p>For any such characters, enclose their decimal character code with parentheses. Verbatim
* {@code (} is encoded as {@code ((}.
*/
public static String encodePath(String path) {
if (path.isEmpty()) return path;
var sb = new StringBuilder();
for (var i = 0; i < path.length(); i++) {
var character = path.charAt(i);
switch (character) {
case '<', '>', ':', '"', '\\', '|', '?', '*' -> {
sb.append('(');
sb.append(ByteArrayUtils.toHex(new byte[] {(byte) character}));
sb.append(")");
}
case '(' -> sb.append("((");
default -> sb.append(path.charAt(i));
}
}
return sb.toString();
}

private static int getExclamationMarkIndex(String jarUri) {
var index = jarUri.indexOf('!');
if (index == -1) {
Expand Down
10 changes: 10 additions & 0 deletions pkl-core/src/test/kotlin/org/pkl/core/util/IoUtilsTest.kt
Original file line number Diff line number Diff line change
Expand Up @@ -431,4 +431,14 @@ class IoUtilsTest {
IoUtils.readString(URI("http://example.com").toURL())
}
}

@Test
fun `encodePath encodes characters reserved on windows`() {
assertThat(IoUtils.encodePath("foo:bar")).isEqualTo("foo(3a)bar")
assertThat(IoUtils.encodePath("<>:\"\\|?*")).isEqualTo("(3c)(3e)(3a)(22)(5c)(7c)(3f)(2a)")
assertThat(IoUtils.encodePath("foo(3a)bar")).isEqualTo("foo((3a)bar")
assertThat(IoUtils.encodePath("(")).isEqualTo("((")
assertThat(IoUtils.encodePath("3a)")).isEqualTo("3a)")
assertThat(IoUtils.encodePath("foo/bar/baz")).isEqualTo("foo/bar/baz")
}
}
2 changes: 1 addition & 1 deletion pkl-doc/src/main/kotlin/org/pkl/doc/DocGenerator.kt
Original file line number Diff line number Diff line change
Expand Up @@ -121,7 +121,7 @@ class DocGenerator(

private fun createSymlinks(currentPackagesData: List<PackageData>) {
for (packageData in currentPackagesData) {
val basePath = outputDir.resolve(packageData.ref.pkg)
val basePath = outputDir.resolve(packageData.ref.pkg.pathEncoded)
val src = basePath.resolve(packageData.ref.version)
val dest = basePath.resolve("current")
if (dest.exists() && dest.isSameFileAs(src)) continue
Expand Down
4 changes: 2 additions & 2 deletions pkl-doc/src/main/kotlin/org/pkl/doc/DocPackageInfo.kt
Original file line number Diff line number Diff line change
Expand Up @@ -203,9 +203,9 @@ data class DocPackageInfo(
when {
!moduleName.startsWith(prefix) -> null
else -> {
val modulePath = moduleName.substring(prefix.length).replace('.', '/')
val modulePath = moduleName.substring(prefix.length).replace('.', '/').pathEncoded
if (documentation == null) {
"$name/$version/$modulePath/index.html".toUri()
"${name.pathEncoded}/$version/$modulePath/index.html".toUri()
} else {
documentation.resolve("$modulePath/index.html")
}
Expand Down
38 changes: 17 additions & 21 deletions pkl-doc/src/main/kotlin/org/pkl/doc/DocScope.kt
Original file line number Diff line number Diff line change
Expand Up @@ -71,10 +71,7 @@ internal sealed class DocScope {
fun resolveModuleNameToRelativeDocUrl(name: String): URI? =
resolveModuleNameToDocUrl(name)?.let { IoUtils.relativize(it, url) }

abstract fun resolveModuleNameToSourceUrl(
name: String,
sourceLocation: Member.SourceLocation
): URI?
abstract fun resolveModuleNameToSourceUrl(name: String, sourceLocation: SourceLocation): URI?

/** Resolves the given method name relative to this scope. */
abstract fun resolveMethod(name: String): MethodScope?
Expand Down Expand Up @@ -207,10 +204,7 @@ internal class SiteScope(
else -> null
}

override fun resolveModuleNameToSourceUrl(
name: String,
sourceLocation: Member.SourceLocation
): URI? =
override fun resolveModuleNameToSourceUrl(name: String, sourceLocation: SourceLocation): URI? =
when {
name.startsWith("pkl.") -> {
val path = "/stdlib/${name.substring(4)}.pkl"
Expand Down Expand Up @@ -253,7 +247,9 @@ internal class PackageScope(
private val moduleScopes: Map<String, ModuleScope> by lazy {
modules.associate { module ->
val docUrl =
url.resolve(getModulePath(module.moduleName, modulePrefix).uriEncoded + "/index.html")
url.resolve(
getModulePath(module.moduleName, modulePrefix).pathEncoded.uriEncoded + "/index.html"
)
module.moduleName to ModuleScope(module, docUrl, this)
}
}
Expand All @@ -262,9 +258,11 @@ internal class PackageScope(
ModuleScope(pklBaseModule, resolveModuleNameToDocUrl("pkl.base")!!, null)
}

override val url: URI by lazy { parent.url.resolve("./$name/$version/index.html") }
override val url: URI by lazy { parent.url.resolve("./${name.pathEncoded}/$version/index.html") }

override val dataUrl: URI by lazy { parent.url.resolve("./data/$name/$version/index.js") }
override val dataUrl: URI by lazy {
parent.url.resolve("./data/${name.pathEncoded}/$version/index.js")
}

fun getModule(name: String): ModuleScope = moduleScopes.getValue(name)

Expand Down Expand Up @@ -387,11 +385,11 @@ internal class ClassScope(
override val url: URI by lazy {
// `isModuleClass` distinction is relevant when this scope is a link target
if (clazz.isModuleClass) parentUrl
else parentUrl.resolve("${clazz.simpleName.uriEncodedComponent}.html")
else parentUrl.resolve("${clazz.simpleName.pathEncoded.uriEncodedComponent}.html")
}

override val dataUrl: URI by lazy {
parent!!.dataUrl.resolve("${clazz.simpleName.uriEncodedComponent}.js")
parent!!.dataUrl.resolve("${clazz.simpleName.pathEncoded.uriEncodedComponent}.js")
}

override fun getMethod(name: String): MethodScope? =
Expand All @@ -403,10 +401,8 @@ internal class ClassScope(
override fun resolveModuleNameToDocUrl(name: String): URI? =
parent!!.resolveModuleNameToDocUrl(name)

override fun resolveModuleNameToSourceUrl(
name: String,
sourceLocation: Member.SourceLocation
): URI? = parent!!.resolveModuleNameToSourceUrl(name, sourceLocation)
override fun resolveModuleNameToSourceUrl(name: String, sourceLocation: SourceLocation): URI? =
parent!!.resolveModuleNameToSourceUrl(name, sourceLocation)

override fun resolveMethod(name: String): MethodScope? =
clazz.methods[name]?.let { MethodScope(it, this) }
Expand Down Expand Up @@ -438,7 +434,7 @@ internal class TypeAliasScope(
// only used for page scopes
throw UnsupportedOperationException("resolveModuleNameToDocUrl")

override fun resolveModuleNameToSourceUrl(name: String, sourceLocation: Member.SourceLocation) =
override fun resolveModuleNameToSourceUrl(name: String, sourceLocation: SourceLocation) =
// only used for page scopes
throw UnsupportedOperationException("resolveModuleNameToSourceUrl")

Expand All @@ -464,7 +460,7 @@ internal class MethodScope(val method: PClass.Method, override val parent: DocSc
// only used for page scopes
throw UnsupportedOperationException("resolveModuleNameToDocUrl")

override fun resolveModuleNameToSourceUrl(name: String, sourceLocation: Member.SourceLocation) =
override fun resolveModuleNameToSourceUrl(name: String, sourceLocation: SourceLocation) =
// only used for page scopes
throw UnsupportedOperationException("resolveModuleNameToSourceUrl")

Expand Down Expand Up @@ -494,7 +490,7 @@ internal class PropertyScope(
// only used for page scopes
throw UnsupportedOperationException("resolveModuleNameToDocUrl")

override fun resolveModuleNameToSourceUrl(name: String, sourceLocation: Member.SourceLocation) =
override fun resolveModuleNameToSourceUrl(name: String, sourceLocation: SourceLocation) =
// only used for page scopes
throw UnsupportedOperationException("resolveModuleNameToSourceUrl")

Expand Down Expand Up @@ -525,7 +521,7 @@ internal class ParameterScope(val name: String, override val parent: DocScope) :
// only used for page scopes
throw UnsupportedOperationException("resolveModuleNameToDocUrl")

override fun resolveModuleNameToSourceUrl(name: String, sourceLocation: Member.SourceLocation) =
override fun resolveModuleNameToSourceUrl(name: String, sourceLocation: SourceLocation) =
// only used for page scopes
throw UnsupportedOperationException("resolveModuleNameToSourceUrl")

Expand Down
8 changes: 5 additions & 3 deletions pkl-doc/src/main/kotlin/org/pkl/doc/PackageDataGenerator.kt
Original file line number Diff line number Diff line change
Expand Up @@ -37,9 +37,11 @@ import org.pkl.core.util.IoUtils
internal class PackageDataGenerator(private val outputDir: Path) {
fun generate(pkg: DocPackage) {
val path =
outputDir.resolve(pkg.name).resolve(pkg.version).resolve("package-data.json").apply {
createParentDirectories()
}
outputDir
.resolve(pkg.name.pathEncoded)
.resolve(pkg.version)
.resolve("package-data.json")
.apply { createParentDirectories() }
PackageData(pkg).write(path)
}

Expand Down
Loading

0 comments on commit de4d78b

Please sign in to comment.