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 @@ -19,7 +19,9 @@ import org.dexpace.kuri.query.QueryParameters
/**
* Decodes one flat `@Serializable` class from a [QueryParameters]. Absent optional elements are skipped
* (so their declared default applies); an absent required element fails. A list element delegates to
* [QueryListDecoder], reading every repeated value for the name via `getAll`.
* [QueryListDecoder], reading every repeated value for the name via `getAll`. A list property present
* but empty carries no repeated values of its own, so it is recognized instead via
* [emptyListMarkerName] — see [isPresentEmptyList].
*/
@Suppress("TooManyFunctions") // One decode method per primitive kind, mandated by the AbstractDecoder contract.
internal class QueryDecoder(
Expand All @@ -29,27 +31,56 @@ internal class QueryDecoder(

private var index = -1
private var currentName: String = ""
private var currentIsEmptyListMarker: Boolean = false
private var entered = false

override fun decodeElementIndex(descriptor: SerialDescriptor): Int {
while (++index < descriptor.elementsCount) {
val name = descriptor.getElementName(index)
if (params.has(name) || !descriptor.isElementOptional(index)) {
val emptyListMarker = isPresentEmptyList(descriptor, index, name)
val present = params.has(name) || emptyListMarker
if (present || !descriptor.isElementOptional(index)) {
currentName = name
currentIsEmptyListMarker = emptyListMarker && !params.has(name)
return index
}
}
return CompositeDecoder.DECODE_DONE
}

/**
* True when the element at [index] is a list property and its [emptyListMarkerName] marker is
* present, i.e. it was explicitly encoded as present-but-empty rather than omitted. Scoped to list
* elements only, so a foreign query string that happens to contain a `<scalarField>[]` pair cannot
* spuriously mark a scalar property present. [decodeElementIndex] also uses this (via
* [currentIsEmptyListMarker]) to make [decodeNotNullMark] report a marker-only match as not-null, so
* a nullable list property reaches [beginStructure] instead of `NullableSerializer` short-circuiting
* it to `null`.
*/
private fun isPresentEmptyList(
descriptor: SerialDescriptor,
index: Int,
name: String,
): Boolean {
val isList = descriptor.getElementDescriptor(index).kind == StructureKind.LIST
return isList && params.has(emptyListMarkerName(name))
}

override fun beginStructure(descriptor: SerialDescriptor): CompositeDecoder {
if (descriptor.kind == StructureKind.LIST) return QueryListDecoder(params.getAll(currentName))
if (entered) throw SerializationException("nested objects are not supported by the query format")
entered = true
return this
}

override fun decodeNotNullMark(): Boolean = params.has(currentName)
/**
* `false` only when the current element is genuinely absent. A list property present solely via its
* [emptyListMarkerName] marker (no `name=value` pairs of its own) still counts as not-null: without
* this, `kotlinx.serialization`'s `NullableSerializer` would short-circuit a nullable list straight
* to `null` on seeing `params.has(currentName)` fail, never reaching [beginStructure] to decode the
* empty [QueryListDecoder] the marker represents.
*/
override fun decodeNotNullMark(): Boolean = params.has(currentName) || currentIsEmptyListMarker

override fun decodeString(): String =
params[currentName] ?: throw SerializationException("missing query parameter '$currentName'")
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -9,6 +9,7 @@ package org.dexpace.kuri.serde
import kotlinx.serialization.ExperimentalSerializationApi
import kotlinx.serialization.SerializationException
import kotlinx.serialization.descriptors.SerialDescriptor
import kotlinx.serialization.descriptors.StructureKind
import kotlinx.serialization.encoding.AbstractEncoder
import kotlinx.serialization.encoding.CompositeEncoder
import kotlinx.serialization.modules.EmptySerializersModule
Expand Down Expand Up @@ -54,10 +55,23 @@ internal class QueryEncoder : AbstractEncoder() {
return true
}

/**
* Starts a list property. A non-empty list is carried entirely by its repeated `name=value`
* pairs, so no marker is needed. An empty list has no elements to repeat, which would otherwise
* make it indistinguishable on the wire from the property being absent altogether (see
* [emptyListMarkerName]) — so this adds that marker up front, before [QueryListEncoder] contributes
* zero further pairs. Scoped to [StructureKind.LIST] to mirror the decode side's
* `isPresentEmptyList`, which only recognizes the marker for list elements; the format doesn't
* currently support `Map`-typed properties, so this is hardening rather than a reachable fix.
*/
override fun beginCollection(
descriptor: SerialDescriptor,
collectionSize: Int,
): CompositeEncoder = QueryListEncoder(requireName(), builder)
): CompositeEncoder {
val name = requireName()
if (collectionSize == 0 && descriptor.kind == StructureKind.LIST) builder.add(emptyListMarkerName(name), null)
return QueryListEncoder(name, builder)
}

override fun encodeValue(value: Any) {
builder.add(requireName(), value.toString())
Expand Down Expand Up @@ -95,3 +109,27 @@ internal class QueryListEncoder(
builder.add(name, enumDescriptor.getElementName(index))
}
}

/**
* Suffix marking the wire-level "present but empty" sentinel for a list property, appended to its
* declared name (e.g. `tags` -> `tags[]`). `[`/`]` are never percent-encoded by the query name encode
* set, so the marker stays literal in the encoded string. A Kotlin property name cannot itself contain
* `[`/`]`, so the marker cannot collide with a declared property's default serial name; a property whose
* serial name is deliberately overridden via `@SerialName` to end in `[]` could still collide — not a
* supported/tested shape.
*/
private const val EMPTY_LIST_MARKER_SUFFIX: String = "[]"

/**
* The wire name of the empty-collection marker for a list property declared as [name].
*
* A list property's non-empty state is fully carried by its repeated `name=value` pairs; an empty list
* has none, which is indistinguishable from the property being entirely absent (and would therefore
* fall back to its declared default on decode instead of decoding to an empty list). [QueryEncoder]
* emits this marker as a bare (no `=`) pair when a list encodes to zero elements, and [QueryDecoder]
* treats its presence as "present, zero elements" without contributing any element itself.
*
* @param name the list property's declared (unsuffixed) name.
* @return [name] with [EMPTY_LIST_MARKER_SUFFIX] appended.
*/
internal fun emptyListMarkerName(name: String): String = name + EMPTY_LIST_MARKER_SUFFIX
Original file line number Diff line number Diff line change
Expand Up @@ -29,6 +29,11 @@ import org.dexpace.kuri.query.QueryParameters
* default is omitted from the encoded output, keeping the query string minimal; an absent required
* property raises a [kotlinx.serialization.SerializationException]. Nested `@Serializable` objects are
* rejected — model them at the call site or bind them separately.
*
* A list property that differs from its default by being explicitly empty has no element pairs to
* repeat, which would otherwise read back as simply absent and fall to the default instead of decoding
* to an empty list. That case is carried by a `name[]` marker pair (e.g. `tags[]`) instead — see
* `emptyListMarkerName` — so "present but empty" and "absent" stay distinguishable on the wire.
*/
public object QueryParametersFormat {
/**
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -74,6 +74,11 @@ private data class Nested(
val inner: Search,
)

@Serializable
private data class Foo(
val tags: List<String> = listOf("x"),
)

@Serializable
private data class Flag(
val flag: Boolean,
Expand All @@ -84,6 +89,11 @@ private data class Flags(
val flags: List<Boolean>,
)

@Serializable
private data class NullableTags(
val tags: List<String>? = null,
)

class SerdeTest {
@Test
fun `url serializes as its canonical string in json`() {
Expand Down Expand Up @@ -206,6 +216,48 @@ class SerdeTest {
assertEquals(original, QueryParametersFormat.decodeFromQueryString<AllLists>(query))
}

@Test
fun `an explicitly-empty list round-trips to an empty list rather than the declared default`() {
// Regression test for #86: encoding Foo(tags = emptyList()) against a non-empty default
// (listOf("x")) must not be indistinguishable from tags being absent altogether.
val encoded = QueryParametersFormat.encodeToQueryString(Foo(tags = emptyList()))
assertEquals("tags[]", encoded)
assertEquals(Foo(tags = emptyList()), QueryParametersFormat.decodeFromQueryString<Foo>(encoded))
}

@Test
fun `a field genuinely absent from the query still falls back to its declared default`() {
assertEquals(Foo(tags = listOf("x")), QueryParametersFormat.decodeFromQueryString<Foo>(""))
}

@Test
fun `a list left at its declared empty default is still omitted from the encoded output`() {
// Search.tags defaults to emptyList(), so Search(q = "only") leaves it unchanged: no marker,
// no repeated pairs, matching the existing default-omission contract.
assertEquals("q=only", QueryParametersFormat.encodeToQueryString(Search(q = "only")))
assertEquals(emptyList(), QueryParametersFormat.decodeFromQueryString<Search>("q=only").tags)
}

@Test
fun `a non-empty list still round-trips without an empty-list marker`() {
val original = Foo(tags = listOf("a", "b", "c"))
val encoded = QueryParametersFormat.encodeToQueryString(original)
assertEquals("tags=a&tags=b&tags=c", encoded)
assertEquals(original, QueryParametersFormat.decodeFromQueryString<Foo>(encoded))
}

@Test
fun `an explicitly-empty nullable list round-trips to an empty list rather than null`() {
// Regression for #86: NullableSerializer calls decodeNotNullMark() before ever reaching
// QueryListDecoder. With only the empty-list marker on the wire (no tags=... pairs),
// params.has("tags") is false, so a marker-blind decodeNotNullMark() would short-circuit
// straight to null instead of decoding an empty list.
val encoded = QueryParametersFormat.encodeToQueryString(NullableTags(tags = emptyList()))
assertEquals("tags[]", encoded)
val decoded = QueryParametersFormat.decodeFromQueryString<NullableTags>(encoded)
assertEquals(NullableTags(tags = emptyList()), decoded)
}

@Test
fun `a null optional value is omitted when encoding and decodes back to null`() {
val withoutNickname = Contact(name = "ada", nickname = null)
Expand Down