Skip to content
Merged
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 @@ -44,6 +44,14 @@ public annotation class Uri
* A leading `/` is decorative for an authority-less URI: with no authority a segment path roots only
* under one, so `@PathTemplate("/a/{id}")` renders as `a/7` (not `/a/7`); once an authority is present
* the path re-roots regardless.
*
* Matching `net/http.ServeMux`, every hole must occupy a whole `/`-delimited path segment: literal text
* sharing a segment with a hole is rejected at parse time rather than silently re-segmented when the path
* is composed, so `@PathTemplate("/reports/{id}.json")` and `@PathTemplate("v{version}")` both throw
* [KuriBindException] instead of building `/reports/5/.json` or `v/2`. Two holes must also be separated
* by a `/`, so `@PathTemplate("{a}{b}")` throws [KuriBindException] too: the composer emits one full
* segment per hole, so adjacent holes can never merge into the single shared segment their spelling
* implies.
*/
@Retention(AnnotationRetention.RUNTIME)
@Target(AnnotationTarget.CLASS)
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -55,6 +55,7 @@ internal class PathTemplate private constructor(
}
if (literal.isNotEmpty()) tokens.add(PathToken.Literal(literal.toString()))
requireCatchAllIsFinal(holes, tokens, template)
requireHolesAtSegmentBoundaries(tokens, template)
check(holes.all { it.name.isNotEmpty() }) { "hole names validated above" }
return PathTemplate(tokens, holes)
}
Expand Down Expand Up @@ -112,5 +113,47 @@ internal class PathTemplate private constructor(
throw KuriBindException("catch-all '{${last.name}...}' must be the final token: $template")
}
}

/**
* A hole must occupy a whole path segment, matching `net/http.ServeMux`: literal text sharing a
* segment with a hole (`{id}.json`, `v{version}`) is rejected rather than silently re-segmented
* when the path is composed (issue #82). A hole is exempt from this check only at a true template
* boundary (start/end of string, i.e. no neighboring token at all) — any other neighbor, including
* another hole with nothing between them (`{a}{b}`), is a violation unless it's a [PathToken.Literal]
* carrying the `/` at the shared edge. The composer always emits one full segment per [PathToken.Hole]
* regardless of adjacency, so two adjacent holes can never merge into the single shared segment their
* spelling implies.
*/
private fun requireHolesAtSegmentBoundaries(
tokens: List<PathToken>,
template: String,
) {
for (i in tokens.indices) {
val hole = tokens[i] as? PathToken.Hole ?: continue
if (!isBoundedBefore(tokens, i) || !isBoundedAfter(tokens, i)) {
throw KuriBindException(
"template hole '{${hole.name}}' must occupy a whole '/'-delimited path segment: $template",
)
}
}
}

/** True when the hole at [index] has no preceding token, or a literal predecessor ending in `/`. */
private fun isBoundedBefore(
tokens: List<PathToken>,
index: Int,
): Boolean {
val before = tokens.getOrNull(index - 1) ?: return true
return before is PathToken.Literal && before.raw.endsWith('/')
}

/** True when the hole at [index] has no following token, or a literal successor starting with `/`. */
private fun isBoundedAfter(
tokens: List<PathToken>,
index: Int,
): Boolean {
val after = tokens.getOrNull(index + 1) ?: return true
return after is PathToken.Literal && after.raw.startsWith('/')
}
}
}
Original file line number Diff line number Diff line change
Expand Up @@ -249,6 +249,18 @@ private class NestedNullHole(
@BindUrl val middle: HoleIdCarrier?,
)

// Issue #82 repro: a hole followed by a literal suffix sharing the same segment (`{id}.json`).
@PathTemplate("/reports/{id}.json")
private class ReportRequest(
@Path("id") val id: Int,
)

// Issue #82 repro: a literal prefix followed by a hole sharing the same segment (`v{version}`).
@PathTemplate("v{version}")
private class VersionRequest(
@Path("version") val version: Int,
)

class BindingExecutorTest {
private val executor = BindingExecutor(PlanCompiler(KotlinReflectMemberScanner()))

Expand Down Expand Up @@ -467,4 +479,16 @@ class BindingExecutorTest {
// short-circuits before the inner read and the hole resolves to null — a fail-fast bind error.
assertFailsWith<KuriBindException> { bind(NestedNullHole(null)) }
}

@Test
fun `fails to bind a template whose hole shares a segment with a literal suffix`() {
// Issue #82: "/reports/{id}.json" must not silently re-segment to ["reports", "5", ".json"].
assertFailsWith<KuriBindException> { bind(ReportRequest(5)) }
}

@Test
fun `fails to bind a template whose hole shares a segment with a literal prefix`() {
// Issue #82: "v{version}" must not silently re-segment to ["v", "2"].
assertFailsWith<KuriBindException> { bind(VersionRequest(2)) }
}
}
Original file line number Diff line number Diff line change
Expand Up @@ -52,9 +52,10 @@ class PathTemplateTest {
}

@Test
fun `parses adjacent holes with no literal between them`() {
val t = PathTemplate.parse("/{a}{b}")
assertEquals(listOf("a", "b"), t.holes.map { it.name })
fun `rejects adjacent holes with no literal between them`() {
// The composer emits one full segment per hole regardless of adjacency, so "/{a}{b}" would
// compose identically to "/{a}/{b}" despite its spelling implying a single merged segment.
assertFailsWith<KuriBindException> { PathTemplate.parse("/{a}{b}") }
}

@Test
Expand All @@ -75,4 +76,53 @@ class PathTemplateTest {
// parsed hole name "a{b" carries a stray brace and must be rejected.
assertFailsWith<KuriBindException> { PathTemplate.parse("/{a{b}") }
}

@Test
fun `rejects a hole followed by a literal suffix in the same segment`() {
// Issue #82: "/reports/{id}.json" would otherwise re-segment to ["reports", "5", ".json"].
assertFailsWith<KuriBindException> { PathTemplate.parse("/reports/{id}.json") }
}

@Test
fun `rejects a literal prefix followed by a hole in the same segment`() {
// Issue #82: "v{version}" would otherwise re-segment to ["v", "2"] instead of one "v2" segment.
assertFailsWith<KuriBindException> { PathTemplate.parse("v{version}") }
}

@Test
fun `rejects a catch-all hole sharing a segment with a literal prefix`() {
assertFailsWith<KuriBindException> { PathTemplate.parse("/files{path...}") }
}

@Test
fun `rejects a hole with an unbounded literal on both sides at once`() {
// "/a{id}b/c" fails the check twice over: "a" doesn't end in '/' and "b" doesn't start with '/'.
assertFailsWith<KuriBindException> { PathTemplate.parse("/a{id}b/c") }
}

@Test
fun `parses a hole immediately followed by a slash-rooted literal`() {
val t = PathTemplate.parse("/reports/{id}/detail")
assertEquals(
listOf(
PathToken.Literal("/reports/"),
PathToken.Hole("id", catchAll = false),
PathToken.Literal("/detail"),
),
t.tokens,
)
}

@Test
fun `parses a hole at the very start of the template with no leading literal`() {
val t = PathTemplate.parse("{id}/detail")
// Mirrors the template-end boundary (already covered by the catch-all test) but for the start.
assertEquals(
listOf(
PathToken.Hole("id", catchAll = false),
PathToken.Literal("/detail"),
),
t.tokens,
)
}
}