Skip to content

Rewrite TreeJsonDecoder main decoding loop to be identical to StreamingJsonDecoder #3220

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Open
qwwdfsad wants to merge 1 commit into
base: dev
Choose a base branch
from
+101 −86
Open

Rewrite TreeJsonDecoder main decoding loop to be identical to StreamingJsonDecoder #3220

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
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
35 changes: 35 additions & 0 deletions formats/json-tests/commonTest/src/kotlinx/serialization/features/JsonNamesTest.kt
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -86,6 +86,22 @@ class JsonNamesTest : JsonTestBase() {
}
}

@Test
fun testLastPropertyWinsOnAlternativeNames() {
val cases = mapOf(
"""{"data":"first","foo":"second"}""" to "second", // primary then alternative

@sandwwraith sandwwraith Jul 21, 2026

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm a bit concerned about this change. Currently, @SerialName/property name has priority over the alternate one, and it is even documented. I hope no one actually relies on it, though. I'd rather prefer to try to restore the original behavior, but it looks like though it is not possible in this implementation unless we fix #1990 along the way

@Snipesy Snipesy Jul 28, 2026

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This appears to be colliding with a whole different issue. Currently on all releases with @SerialName the behavior of TreeJsonDecoder versus StreamingJsonDecoder diverges.

@Serializable
data class Config(
    @SerialName("foo") val a: String? = null, // also works without SerialName
    @JsonNames("foo") val b: String? = null,
)

fun test() {
    val input = """{"foo":"X"}"""

    println(Json.decodeFromString<Config>(input))
    println(Json.decodeFromJsonElement<Config>(Json.parseToJsonElement(input)))
}
Config(a=X, b=null) // Streaming
Config(a=X, b=X) // Tree

On qwwdfsad/tree-json-opto it inadvertently fixes the divergence and the 2 align.

🙃

"""{"foo":"first","data":"second"}""" to "second", // alternative then primary
"""{"foo":"first","_foo":"second"}""" to "second", // two alternatives
"""{"foo":"first","foo":"second"}""" to "second", // Two primary
)
for ((input, expected) in cases) {
parameterizedCoercingTest { json, streaming, _ ->
val data = json.decodeFromString(WithNames.serializer(), input, jsonTestingMode = streaming).data
assertEquals(expected, data, "Failed to parse input '$input' with streaming=$streaming")
}
}
}

@Test
fun testThrowsAnErrorOnDuplicateNames() {
val serializer = CollisionWithAlternate.serializer()
Expand All @@ -101,4 +117,23 @@ class JsonNamesTest : JsonTestBase() {
}
}
}

@Test
fun testMissingFieldExceptionOnDuplicateAlternativeNames() {
val serializer = CollisionWithAlternate.serializer()
for (input in listOf("{}", """{"data":"a"}""")) {
parameterizedCoercingTest { json, streaming, msg ->
assertFailsWithMessage<MissingFieldException>("required", "$msg, input=$input") {
json.decodeFromString(serializer, input, jsonTestingMode = streaming)
}
}
}
parameterizedCoercingTest { json, streaming, msg ->
assertEquals(
CollisionWithAlternate("a", "b"),
json.decodeFromString(serializer, """{"data":"a","foo":"b"}""", jsonTestingMode = streaming),
msg
)
}
}
}
152 changes: 66 additions & 86 deletions formats/json/commonMain/src/kotlinx/serialization/json/internal/TreeJsonDecoder.kt
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -24,7 +24,7 @@ public fun <T> readJson(
): T {
val discriminator = (previousDecoder as? PolymorphicJsonDecoder)?.discriminator
val input = when (element) {
is JsonObject -> JsonTreeDecoder(json, element, discriminator)
is JsonObject -> JsonTreeDecoder(json, element, deserializer.descriptor, discriminator)
is JsonArray -> JsonTreeListDecoder(json, element)
is JsonLiteral, JsonNull -> JsonPrimitiveDecoder(json, element as JsonPrimitive)
}
Expand All @@ -34,9 +34,13 @@ public fun <T> readJson(
internal fun <T> Json.readPolymorphicJson(
discriminator: String,
element: JsonObject,
// Note: this is an actual deserializer, not a polymorphic one
deserializer: DeserializationStrategy<T>
): T {
return JsonTreeDecoder(this, element, discriminator, deserializer.descriptor).decodeSerializableValue(deserializer)
val descriptor = deserializer.descriptor
return JsonTreeDecoder(
this, element, descriptor, discriminator, descriptor
).decodeSerializableValue(deserializer)
}

private sealed class AbstractJsonTreeDecoder(
Expand Down Expand Up @@ -64,18 +68,18 @@ private sealed class AbstractJsonTreeDecoder(
return decodeSerializableValuePolymorphic(deserializer, ::renderTagStack)
}

override fun composeName(parentName: String, childName: String): String = childName
final override fun composeName(parentName: String, childName: String): String = childName

override fun beginStructure(descriptor: SerialDescriptor): CompositeDecoder {
val currentObject = currentObject()
return when (descriptor.kind) {
StructureKind.LIST, is PolymorphicKind -> JsonTreeListDecoder(json, cast(currentObject, descriptor))
StructureKind.MAP -> json.selectMapMode(
descriptor,
{ JsonTreeMapDecoder(json, cast(currentObject, descriptor)) },
{ JsonTreeMapDecoder(json, cast(currentObject, descriptor), descriptor) },
{ JsonTreeListDecoder(json, cast(currentObject, descriptor)) }
)
else -> JsonTreeDecoder(json, cast(currentObject, descriptor), polymorphicDiscriminator)
else -> JsonTreeDecoder(json, cast(currentObject, descriptor), descriptor, polymorphicDiscriminator)
}
}

Expand Down Expand Up @@ -197,122 +201,98 @@ private class JsonPrimitiveDecoder(
override fun decodeElementIndex(descriptor: SerialDescriptor): Int = 0

override fun currentElement(tag: String): JsonElement {
require(tag === PRIMITIVE_TAG) { "This input can only handle primitives with '$PRIMITIVE_TAG' tag" }
require(tag == PRIMITIVE_TAG) { "This input can only handle primitives with '$PRIMITIVE_TAG' tag" }
return value
}
}

private open class JsonTreeDecoder(
json: Json,
override val value: JsonObject,
descriptor: SerialDescriptor,
polymorphicDiscriminator: String? = null,
private val polyDescriptor: SerialDescriptor? = null
) : AbstractJsonTreeDecoder(json, value, polymorphicDiscriminator) {
private var position = 0
private var forceNull: Boolean = false

override fun decodeElementIndex(descriptor: SerialDescriptor): Int {
while (position < descriptor.elementsCount) {
val name = descriptor.getTag(position++)
val index = position - 1
forceNull = false

if (name in value || setForceNull(descriptor, index)) {
// if forceNull is true, then decodeNotNullMark returns false and `null` is automatically inserted
// by Decoder.decodeIfNullable
if (!configuration.coerceInputValues) return index

if (json.tryCoerceValue(
descriptor, index,
{ currentElementOrNull(name) is JsonNull },
{ (currentElementOrNull(name) as? JsonPrimitive)?.contentOrNull },
{ // an unknown enum value should be coerced to null via decodeNotNullMark if explicitNulls=false :
if (setForceNull(descriptor, index)) return index
}
)
) continue // do not read coerced value
// Pointer to the current entry of JsonObject that is being decoded
private val entries: Iterator<Map.Entry<String, JsonElement>> by lazy(LazyThreadSafetyMode.NONE) {

@sandwwraith sandwwraith Jul 21, 2026

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is lazy really needed here? decodeElementIndex is always called when class is not empty, so it looks like we can strip one level of indirection. And if value.isEmpty() you can simply have null iterator or something like that

value.entries.iterator()
}

private val elementMarker: JsonElementMarker? = if (configuration.explicitNulls) null else JsonElementMarker(descriptor)

// Cached results of entries.next() to be used in tagged protocol
private var currentName: String? = null
private var currentValue: JsonElement? = null

override fun decodeElementIndex(descriptor: SerialDescriptor): Int {
val entries = entries
while (entries.hasNext()) {
val entry = entries.next()
val key = entry.key
val index = descriptor.getJsonNameIndex(json, key)
if (index != CompositeDecoder.UNKNOWN_NAME) {
if (configuration.coerceInputValues && coerceInputValue(descriptor, index, entry.value)) {
continue
}
elementMarker?.mark(index)
currentName = key
currentValue = entry.value
return index
}
if (key != polymorphicDiscriminator && !descriptor.ignoreUnknownKeys(json)) {
throwUnknownKey(key)
}
}
return CompositeDecoder.DECODE_DONE
val markerIndex = elementMarker?.nextUnmarkedIndex() ?: CompositeDecoder.DECODE_DONE
if (markerIndex != CompositeDecoder.DECODE_DONE) {
currentName = descriptor.getElementName(markerIndex)
currentValue = null
}
return markerIndex
}

private fun setForceNull(descriptor: SerialDescriptor, index: Int): Boolean {
forceNull = !json.configuration.explicitNulls
&& !descriptor.isElementOptional(index) && descriptor.getElementDescriptor(index).isNullable
return forceNull
}
private fun coerceInputValue(descriptor: SerialDescriptor, index: Int, element: JsonElement): Boolean =
json.tryCoerceValue(
descriptor, index,
{ element is JsonNull },
{ (element as? JsonPrimitive)?.contentOrNull }
)

override fun decodeNotNullMark(): Boolean {
return !forceNull && super.decodeNotNullMark()
}

override fun elementName(descriptor: SerialDescriptor, index: Int): String {
val strategy = descriptor.namingStrategy(json)
val baseName = descriptor.getElementName(index)
if (strategy == null) {
if (!configuration.useAlternativeNames) return baseName
// Fast path, do not go through ConcurrentHashMap.get
// Note, it blocks ability to detect collisions between the primary name and alternate,
// but it eliminates a significant performance penalty (about -15% without this optimization)
if (baseName in value.keys) return baseName
}
// Slow path
val deserializationNamesMap = json.deserializationNamesMap(descriptor)
value.keys.find { deserializationNamesMap[it] == index }?.let {
return it
}

val fallbackName = strategy?.serialNameForJson(
descriptor,
index,
baseName
) // Key not found exception should be thrown with transformed name, not original
return fallbackName ?: baseName
return !(elementMarker?.isUnmarkedNull ?: false) && super.decodeNotNullMark()
}

override fun currentElement(tag: String): JsonElement = value.getValue(tag)
override fun elementName(descriptor: SerialDescriptor, index: Int): String =
currentName ?: descriptor.getElementName(index)

fun currentElementOrNull(tag: String): JsonElement? = value[tag]
override fun currentElement(tag: String): JsonElement = currentValue ?: value.getValue(tag)

override fun beginStructure(descriptor: SerialDescriptor): CompositeDecoder {
// polyDiscriminator needs to be preserved so the check for unknown keys
// in endStructure can filter polyDiscriminator out.
// polyDiscriminator needs to be preserved so the discriminator key can be filtered out.
if (descriptor === polyDescriptor) {
return JsonTreeDecoder(
json, cast(currentObject(), polyDescriptor), polymorphicDiscriminator, polyDescriptor
json, cast(currentObject(), polyDescriptor), descriptor, polymorphicDiscriminator, polyDescriptor
)
}

return super.beginStructure(descriptor)
}

override fun endStructure(descriptor: SerialDescriptor) {
if (descriptor.ignoreUnknownKeys(json) || descriptor.kind is PolymorphicKind) return
// Validate keys
val strategy = descriptor.namingStrategy(json)

@Suppress("DEPRECATION_ERROR")
val names: Set<String> = when {
strategy == null && !configuration.useAlternativeNames -> descriptor.jsonCachedSerialNames()
strategy != null -> json.deserializationNamesMap(descriptor).keys
else -> descriptor.jsonCachedSerialNames() + json.schemaCache[descriptor, JsonDeserializationNamesKey]?.keys.orEmpty()
}

for (key in value.keys) {
if (key !in names && key != polymorphicDiscriminator) {
throw decodingExceptionOf(
"Encountered an unknown key '$key'",
renderTagStack(),
ignoreUnknownKeysHint
) { value.toString() }
}
}
private fun throwUnknownKey(key: String): Nothing {
throw decodingExceptionOf(
"Encountered an unknown key '$key'",
renderTagStack(),
ignoreUnknownKeysHint
) { value.toString() }
}
}

private class JsonTreeMapDecoder(json: Json, override val value: JsonObject) : JsonTreeDecoder(json, value) {
private class JsonTreeMapDecoder(
json: Json,
override val value: JsonObject,
descriptor: SerialDescriptor
) : JsonTreeDecoder(json, value, descriptor) {
private val keys = value.keys.toList()
private val size: Int = keys.size * 2
private var position = -1
Expand Down