Skip to content

Commit

Permalink
Fix parsing File paths with special characters. (#2601)
Browse files Browse the repository at this point in the history
* Fix parsing File paths with special characters.

* Fix PathMapper as well.

* Fix test.
  • Loading branch information
colinrtwhite authored Oct 27, 2024
1 parent 7a6ad60 commit cbef364
Show file tree
Hide file tree
Showing 10 changed files with 179 additions and 85 deletions.
2 changes: 2 additions & 0 deletions coil-core/api/android/coil-core.api
Original file line number Diff line number Diff line change
Expand Up @@ -233,6 +233,8 @@ public final class coil3/Uri {
}

public final class coil3/UriKt {
public static final fun Uri (Ljava/lang/String;Ljava/lang/String;Ljava/lang/String;Ljava/lang/String;Ljava/lang/String;Ljava/lang/String;)Lcoil3/Uri;
public static synthetic fun Uri$default (Ljava/lang/String;Ljava/lang/String;Ljava/lang/String;Ljava/lang/String;Ljava/lang/String;Ljava/lang/String;ILjava/lang/Object;)Lcoil3/Uri;
public static final fun getFilePath (Lcoil3/Uri;)Ljava/lang/String;
public static final fun getPathSegments (Lcoil3/Uri;)Ljava/util/List;
public static final fun toUri (Ljava/lang/String;)Lcoil3/Uri;
Expand Down
1 change: 1 addition & 0 deletions coil-core/api/coil-core.klib.api
Original file line number Diff line number Diff line change
Expand Up @@ -993,6 +993,7 @@ final fun coil3.size/Size(kotlin/Int, coil3.size/Dimension): coil3.size/Size //
final fun coil3.size/Size(kotlin/Int, kotlin/Int): coil3.size/Size // coil3.size/Size|Size(kotlin.Int;kotlin.Int){}[0]
final fun coil3.size/SizeResolver(coil3.size/Size): coil3.size/SizeResolver // coil3.size/SizeResolver|SizeResolver(coil3.size.Size){}[0]
final fun coil3/ImageLoader(coil3/PlatformContext): coil3/ImageLoader // coil3/ImageLoader|ImageLoader(coil3.PlatformContext){}[0]
final fun coil3/Uri(kotlin/String? = ..., kotlin/String? = ..., kotlin/String? = ..., kotlin/String? = ..., kotlin/String? = ..., kotlin/String = ...): coil3/Uri // coil3/Uri|Uri(kotlin.String?;kotlin.String?;kotlin.String?;kotlin.String?;kotlin.String?;kotlin.String){}[0]
final inline fun (coil3.size/Dimension).coil3.size/pxOrElse(kotlin/Function0<kotlin/Int>): kotlin/Int // coil3.size/pxOrElse|[email protected](kotlin.Function0<kotlin.Int>){}[0]
final inline fun (coil3.util/IntPair).coil3.util/component1(): kotlin/Int // coil3.util/component1|[email protected](){}[0]
final inline fun (coil3.util/IntPair).coil3.util/component2(): kotlin/Int // coil3.util/component2|[email protected](){}[0]
Expand Down
2 changes: 2 additions & 0 deletions coil-core/api/jvm/coil-core.api
Original file line number Diff line number Diff line change
Expand Up @@ -205,6 +205,8 @@ public final class coil3/Uri {
}

public final class coil3/UriKt {
public static final fun Uri (Ljava/lang/String;Ljava/lang/String;Ljava/lang/String;Ljava/lang/String;Ljava/lang/String;Ljava/lang/String;)Lcoil3/Uri;
public static synthetic fun Uri$default (Ljava/lang/String;Ljava/lang/String;Ljava/lang/String;Ljava/lang/String;Ljava/lang/String;Ljava/lang/String;ILjava/lang/Object;)Lcoil3/Uri;
public static final fun getFilePath (Lcoil3/Uri;)Ljava/lang/String;
public static final fun getPathSegments (Lcoil3/Uri;)Ljava/util/List;
public static final fun toUri (Ljava/lang/String;)Lcoil3/Uri;
Expand Down

This file was deleted.

Original file line number Diff line number Diff line change
@@ -0,0 +1,57 @@
package coil3.fetch

import coil3.ImageLoader
import coil3.request.Options
import coil3.size.Size
import coil3.test.utils.context
import coil3.test.utils.copyAssetToFile
import coil3.toUri
import coil3.util.ASSET_FILE_PATH_ROOT
import coil3.util.SCHEME_FILE
import kotlin.test.assertEquals
import kotlin.test.assertIs
import kotlin.test.assertNotNull
import kotlin.test.assertNull
import kotlinx.coroutines.test.runTest
import okio.Path.Companion.toPath
import org.junit.Test

class FileUriFetcherTest {
private val fetcherFactory = FileUriFetcher.Factory()

@Test
fun basic() = runTest {
val file = "$SCHEME_FILE://${context.copyAssetToFile("normal.jpg")}".toUri()
val options = Options(context, size = Size(100, 100))
val fetcher = fetcherFactory.create(file, options, ImageLoader(context))

assertNotNull(fetcher)

val result = fetcher.fetch()

assertIs<SourceFetchResult>(result)
assertEquals("image/jpeg", result.mimeType)
assertNotNull(result.source.file())
}

@Test
fun doesNotHandleAssetUris() {
val uri = "$SCHEME_FILE:///$ASSET_FILE_PATH_ROOT/asset.jpg".toUri()
assertNull(fetcherFactory.create(uri, Options(context), ImageLoader(context)))
}

@Test
fun doesNotHandleGenericString() {
val uri = "generic_string".toUri()
assertNull(fetcherFactory.create(uri, Options(context), ImageLoader(context)))
}

/** Regression test: https://github.com/coil-kt/coil/issues/1513 */
@Test
fun ignoresAfterPathCorrectly() = runTest {
val uri = "$SCHEME_FILE:///sdcard/file.jpg?query=value&query2=value#fragment".toUri()
val fetcher = assertNotNull(fetcherFactory.create(uri, Options(context), ImageLoader(context)))
val result = assertIs<SourceFetchResult>(fetcher.fetch())
assertEquals("/sdcard/file.jpg".toPath(), result.source.file())
}
}
60 changes: 59 additions & 1 deletion coil-core/src/commonMain/kotlin/coil3/Uri.kt
Original file line number Diff line number Diff line change
Expand Up @@ -4,7 +4,9 @@ import kotlin.jvm.JvmOverloads
import okio.Path

/**
* A uniform resource locator (https://www.w3.org/Addressing/URL/url-spec.html).
* A uniform resource locator. See [RFC 3986](https://tools.ietf.org/html/rfc3986).
*
* @see toUri
*/
class Uri internal constructor(
private val data: String,
Expand All @@ -30,6 +32,62 @@ class Uri internal constructor(
}
}

/**
* Create a [Uri] from parts without parsing.
*
* @see toUri
*/
fun Uri(
scheme: String? = null,
authority: String? = null,
path: String? = null,
query: String? = null,
fragment: String? = null,
separator: String = Path.DIRECTORY_SEPARATOR,
): Uri {
require(scheme != null || authority != null || path != null || query != null || fragment != null) {
"At least one of scheme, authority, path, query, or fragment must be non-null."
}

return Uri(
data = buildData(scheme, authority, path, query, fragment),
separator = separator,
scheme = scheme,
authority = authority,
path = path,
query = query,
fragment = fragment,
)
}

private fun buildData(
scheme: String?,
authority: String?,
path: String?,
query: String?,
fragment: String?,
) = buildString {
if (scheme != null) {
append(scheme)
append(':')
}
if (authority != null) {
append("//")
append(authority)
}
if (path != null) {
append(path)
}
if (query != null) {
append('?')
append(query)
}
if (fragment != null) {
append('#')
append(fragment)
}
}

/**
* Return the separate segments of the [Uri.path].
*/
Expand Down
3 changes: 1 addition & 2 deletions coil-core/src/commonMain/kotlin/coil3/map/PathMapper.kt
Original file line number Diff line number Diff line change
Expand Up @@ -2,12 +2,11 @@ package coil3.map

import coil3.Uri
import coil3.request.Options
import coil3.toUri
import coil3.util.SCHEME_FILE
import okio.Path

internal class PathMapper : Mapper<Path, Uri> {
override fun map(data: Path, options: Options): Uri {
return "$SCHEME_FILE://$data".toUri()
return Uri(scheme = SCHEME_FILE, path = data.toString())
}
}
63 changes: 17 additions & 46 deletions coil-core/src/commonTest/kotlin/coil3/map/PathMapperTest.kt
Original file line number Diff line number Diff line change
Expand Up @@ -10,58 +10,29 @@ import kotlin.test.assertEquals
import okio.Path.Companion.toPath

class PathMapperTest : RobolectricTest() {

private val mapper = PathMapper()

@Test
fun basic() {
val expected = "$SCHEME_FILE:///path/to/file".toUri()
val actual = mapper.map("/path/to/file".toPath(), Options(context))
assertEquals(expected, actual)
val file = "/path/to/file".toPath()
val uri = "$SCHEME_FILE:/path/to/file".toUri()
assertEquals(uri, mapper.map(file, Options(context)))
}

// @Test
// fun noScheme() {
// val uri = "/path/to/file".toUri()
// assertEquals(File("/path/to/file"), mapper.map(uri, Options(context)))
// }
//
// @Test
// fun doesNotHandleAssetUris() {
// val uri = "$SCHEME_FILE:///$ASSET_FILE_PATH_ROOT/asset.jpg".toUri()
// assertNull(mapper.map(uri, Options(context)))
// }
//
// @Test
// fun doesNotHandleGenericString() {
// val uri = "generic_string".toUri()
// assertNull(mapper.map(uri, Options(context)))
// }
//
// /** Regression test: https://github.com/coil-kt/coil/issues/1344 */
// @Test
// fun parsesPoundCharacterCorrectly() {
// val path = "/sdcard/fi#le.jpg"
// assertEquals(File(path), mapper.map(path.toUri(), Options(context)))
// }
//
// /** Regression test: https://github.com/coil-kt/coil/issues/1513 */
// @Test
// fun ignoresAfterPathCorrectly() {
// val expected = File("/sdcard/file.jpg")
// val uri = "$SCHEME_FILE:///sdcard/file.jpg?query=value&query2=value#fragment".toUri()
// assertEquals(expected, mapper.map(uri, Options(context)))
// }
//
// /** Regression test: https://github.com/coil-kt/coil/issues/1513 */
// @Test
// fun decodesEncodedPath() {
// val expected = File("/sdcard/Some File.jpg")
// val uri = "$SCHEME_FILE:///sdcard/Some%20File.jpg".toUri()
// assertEquals(expected, mapper.map(uri, Options(context)))
// }
/** Regression test: https://github.com/coil-kt/coil/issues/1344 */
@Test
fun parsesPoundCharacterCorrectly() {
val path = "/sdcard/fi#le.jpg"
val file = "/sdcard/fi#le.jpg".toPath()
assertEquals(path, mapper.map(file, Options(context)).path)
}

companion object {
const val ASSET_FILE_PATH_ROOT = "android_asset"
/** Regression test: https://github.com/coil-kt/coil/issues/1513 */
@Test
fun decodesEncodedPath() {
val path = "/sdcard/Some+File.jpg"
val uri = "$SCHEME_FILE:$path".toUri()
val file = path.toPath()
assertEquals(uri, mapper.map(file, Options(context)))
}
}
4 changes: 2 additions & 2 deletions coil-core/src/jvmCommonMain/kotlin/coil3/map/FileMapper.kt
Original file line number Diff line number Diff line change
Expand Up @@ -2,11 +2,11 @@ package coil3.map

import coil3.Uri
import coil3.request.Options
import coil3.toUri
import coil3.util.SCHEME_FILE
import java.io.File

class FileMapper : Mapper<File, Uri> {
override fun map(data: File, options: Options): Uri {
return data.path.toUri()
return Uri(scheme = SCHEME_FILE, path = data.path)
}
}
38 changes: 38 additions & 0 deletions coil-core/src/jvmCommonTest/kotlin/coil3/map/FileMapperTest.kt
Original file line number Diff line number Diff line change
@@ -0,0 +1,38 @@
package coil3.map

import coil3.request.Options
import coil3.test.utils.RobolectricTest
import coil3.test.utils.context
import coil3.toUri
import coil3.util.SCHEME_FILE
import java.io.File
import kotlin.test.assertEquals
import org.junit.Test

class FileMapperTest : RobolectricTest() {
private val mapper = FileMapper()

@Test
fun basic() {
val file = File("/path/to/file")
val uri = "$SCHEME_FILE:/path/to/file".toUri()
assertEquals(uri, mapper.map(file, Options(context)))
}

/** Regression test: https://github.com/coil-kt/coil/issues/1344 */
@Test
fun parsesPoundCharacterCorrectly() {
val path = "/sdcard/fi#le.jpg"
val file = File("/sdcard/fi#le.jpg")
assertEquals(path, mapper.map(file, Options(context)).path)
}

/** Regression test: https://github.com/coil-kt/coil/issues/1513 */
@Test
fun decodesEncodedPath() {
val path = "/sdcard/Some+File.jpg"
val uri = "$SCHEME_FILE:$path".toUri()
val file = File(path)
assertEquals(uri, mapper.map(file, Options(context)))
}
}

0 comments on commit cbef364

Please sign in to comment.