diff --git a/kuri-serde-kotlinx/src/commonMain/kotlin/org/dexpace/kuri/serde/QueryDecoder.kt b/kuri-serde-kotlinx/src/commonMain/kotlin/org/dexpace/kuri/serde/QueryDecoder.kt index 5c8f64b..e575470 100644 --- a/kuri-serde-kotlinx/src/commonMain/kotlin/org/dexpace/kuri/serde/QueryDecoder.kt +++ b/kuri-serde-kotlinx/src/commonMain/kotlin/org/dexpace/kuri/serde/QueryDecoder.kt @@ -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( @@ -29,19 +31,41 @@ 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 `[]` 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") @@ -49,7 +73,14 @@ internal class QueryDecoder( 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'") diff --git a/kuri-serde-kotlinx/src/commonMain/kotlin/org/dexpace/kuri/serde/QueryEncoder.kt b/kuri-serde-kotlinx/src/commonMain/kotlin/org/dexpace/kuri/serde/QueryEncoder.kt index d634eb2..9ab3736 100644 --- a/kuri-serde-kotlinx/src/commonMain/kotlin/org/dexpace/kuri/serde/QueryEncoder.kt +++ b/kuri-serde-kotlinx/src/commonMain/kotlin/org/dexpace/kuri/serde/QueryEncoder.kt @@ -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 @@ -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()) @@ -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 diff --git a/kuri-serde-kotlinx/src/commonMain/kotlin/org/dexpace/kuri/serde/QueryParametersFormat.kt b/kuri-serde-kotlinx/src/commonMain/kotlin/org/dexpace/kuri/serde/QueryParametersFormat.kt index eed3e00..7a59f31 100644 --- a/kuri-serde-kotlinx/src/commonMain/kotlin/org/dexpace/kuri/serde/QueryParametersFormat.kt +++ b/kuri-serde-kotlinx/src/commonMain/kotlin/org/dexpace/kuri/serde/QueryParametersFormat.kt @@ -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 { /** diff --git a/kuri-serde-kotlinx/src/commonTest/kotlin/org/dexpace/kuri/serde/SerdeTest.kt b/kuri-serde-kotlinx/src/commonTest/kotlin/org/dexpace/kuri/serde/SerdeTest.kt index b22d325..da9f7f3 100644 --- a/kuri-serde-kotlinx/src/commonTest/kotlin/org/dexpace/kuri/serde/SerdeTest.kt +++ b/kuri-serde-kotlinx/src/commonTest/kotlin/org/dexpace/kuri/serde/SerdeTest.kt @@ -74,6 +74,11 @@ private data class Nested( val inner: Search, ) +@Serializable +private data class Foo( + val tags: List = listOf("x"), +) + @Serializable private data class Flag( val flag: Boolean, @@ -84,6 +89,11 @@ private data class Flags( val flags: List, ) +@Serializable +private data class NullableTags( + val tags: List? = null, +) + class SerdeTest { @Test fun `url serializes as its canonical string in json`() { @@ -206,6 +216,48 @@ class SerdeTest { assertEquals(original, QueryParametersFormat.decodeFromQueryString(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(encoded)) + } + + @Test + fun `a field genuinely absent from the query still falls back to its declared default`() { + assertEquals(Foo(tags = listOf("x")), QueryParametersFormat.decodeFromQueryString("")) + } + + @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("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(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(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)