Skip to content
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.

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

encodeToJsonElement on an inline class throws "No tag in stack for requested element" #1774

Closed
bartvanheukelom opened this issue Nov 23, 2021 · 13 comments
Assignees
Labels

Comments

@bartvanheukelom
Copy link

bartvanheukelom commented Nov 23, 2021

Describe the bug

@Serializable @JvmInline
value class Name(val v: String)

Encoding a value of that class to a JSON string works as expected, but encodeToJsonElement throws the exception in the title.

This only occurs when serializing the value directly. It works as expected when embedded in a data class.

It can be worked around by creating a custom serializer that effectively does the same thing as the generated one would.

To Reproduce
For a unit test that reproduces it and shows various workarounds that do work, see https://github.com/bartvanheukelom/jbali/blob/java17/src/jvmTest/java/org/jbali/kotser/TaggedEncoderBugTest.kt

Excepted Behaviour

  1. For it to, well, work :D
  2. Generally speaking, I've seen various situations over the years where encoding something to a string worked but to an element didn't. This surprises me, since I would expect these 2 encoders to have like 80% of their implementation in common, where the differences in the 20% wouldn't cause bugs such as these. But that is just an outsider's perspective, so please correct me if I'm wrong! I'm not complaining, this is after all a great and free library, but I am curious what's up.

Environment

  • Kotlin version: 1.6.0
  • Library version: 1.3.0 & 1.3.1
  • Kotlin platforms: JVM
  • Gradle version: 7.2
@sandwwraith sandwwraith self-assigned this Nov 24, 2021
@sandwwraith
Copy link
Member

Actually, their implementation is very different, because the string is decoded/encoded one-by-one token, while JsonElement is encoded via an internal map structure. That's also the source of the bug — top-level value classes simply can't provide the key for this map. It also wasn't working for primitives but was fixed (#465) before value classes landed in serialization

@bartvanheukelom
Copy link
Author

bartvanheukelom commented Nov 24, 2021

Thanks for your reaction and for quickly fixing this specific bug. Just to be annoying, I've looked at that commit and came up with some more cases where I think this error would still occur. Please see TaggedEncoderUndescriptiveTest in https://github.com/bartvanheukelom/jbali/blob/java17/src/jvmTest/java/org/jbali/kotser/TaggedEncoderBugTest.kt
(this also relates to #1775 / #1768)

As to the JsonEncoder implementation, I understand that it's different and that's what causes these bugs. In my original comment I actually meant to say that, purely seen from the outside (as if it didn't exist yet), I would think they should have 80% in common, since that seems the most practical.

Consider that something like this does work:

inline fun <reified T> Json.encodeToJsonElementAlt(obj): JsonElement =
    decodeFromString(encodeToString(obj))

could JsonTreeEncoder (if that's the right one) not just do something like that, without the extra steps?

@sandwwraith
Copy link
Member

So you're telling about the case where we provide a custom serializer with CONTEXTUAL descriptor that sometimes calls plain encodeString()? This is not quite right according to serial kinds contract and can be workarounded by encodeSerializableElement(String.serializer(), ...) (however it may be indeed a performance hit in case of e.g. Int)

I think this is a bit out of the scope of this issue and should be considered in #1775

@bartvanheukelom
Copy link
Author

bartvanheukelom commented Nov 24, 2021

It's two cases:

  • testUndescriptiveSer: the custom CONTEXTUAL serializer that calls encodeString, and yeah I figured it violated some contract, but also that with Revisit a buildSerialDescriptor #1775 in mind, it may still be relevant
  • testContextual: here ContextualSerializer is used completely by the book (right?), so it should work

Update: actually the problem in case 1 is not the direct use of encodeString. That's my mistake, I actually intended to use encodeSerializableValue(String.serializer().. , and now that I have changed it to do that, it still presents the same problem (as expected, since it basically does the same thing as ContextualSerializer).

The underlying real issue is that any CONTEXTUAL serializer will not be able to properly answer needTopLevelTag without also being given the actual value that should be encoded. But since it's only called by encodeSerializableValue, that looks like an easy fix.

@bartvanheukelom
Copy link
Author

I also added a test for decodeFromJsonElement and, somewhat to my surprise, it Just Worked™. So I checked out:

internal fun <T> Json.readJson(element: JsonElement, deserializer: DeserializationStrategy<T>): T {
    val input = when (element) {
        is JsonObject -> JsonTreeDecoder(this, element)
        is JsonArray -> JsonTreeListDecoder(this, element)
        is JsonLiteral, JsonNull -> JsonPrimitiveDecoder(this, element as JsonPrimitive)
    }
    return input.decodeSerializableValue(deserializer)
}

and it made sense. Then I was kind of surprised that the write side isn't "mirrorred" but instead JsonTreeEncoder chooses when to delegate to JsonPrimitiveEncoder.

internal fun <T> Json.writeJson(value: T, serializer: SerializationStrategy<T>): JsonElement {
    lateinit var result: JsonElement
    val encoder = JsonTreeEncoder(this) { result = it }
    encoder.encodeSerializableValue(serializer, value)
    return result
}

But I don't know enough about the internals to give any value judgement of that :)

@sandwwraith
Copy link
Member

I see, top-level ContextualSerializer should also be considered

@sandwwraith
Copy link
Member

Hm, it seems that it should work without additional handling, because first contextual serializer resolves actual serializer, and only after the encodeSerializableValue with needTopLevelTag is called. But it's still possible to add special handling to be more future-proof

@bubenheimer
Copy link

I seem to be having the same type of problem, with a value class wrapping List. Wrapping ImmutableList instead does not help, either. Default generated serializer. Serialization V1.3.3 and V1.4.0-RC, Kotlin 1.7.10. Thoughts?

@bubenheimer
Copy link

bubenheimer commented Aug 16, 2022

Can this issue be reopened, please? The List-wrapping inline class scenario is trivial to reproduce via Json serialization (encodeToJsonElement), e.g. using:

@Serializable
@JvmInline
value class Exploring(val value: List<Int>)

@qwwdfsad qwwdfsad reopened this Aug 16, 2022
@jasonxh
Copy link

jasonxh commented Jan 11, 2023

I'm hitting the same issue with a non-list repro. On top of the encoding bug, I'm seeing an error in the decoding path as well:

@Serializable
@JvmInline
value class Outer(val inner: Inner)

@Serializable
data class Inner(val n: Int)

val o = Outer(Inner(10))
Json.encodeToJsonElement(Outer.serializer(), o) // throws SerializationException("No tag in stack for requested element")

val elem = json.parseToJsonElement("""{"n":10}""")
Json.decodeFromJsonElement(Outer.serializer(), elem) // throws IndexOutOfBoundsException("Index -1 out of bounds for length 0")

@christiandeange
Copy link

I'm able to reproduce @jasonxh's issue on Kotlinx serialization 1.8.10. Oddly enough it seems to happen only when deserializing from a JsonElement. If you're deserializing from a String then everything works normally:

Json.decodeFromJsonElement(Outer.serializer(), elem)     // java.lang.IndexOutOfBoundsException: Index -1 out of bounds for length 0
Json.decodeFromJsonElement<Outer>(elem)                  // java.lang.IndexOutOfBoundsException: Index -1 out of bounds for length 0
Json.decodeFromString<Outer>(json.encodeToString(elem))  // Outer(inner=Inner(n=10))

Is there any acknowledgement from the Kotlin team that this error will be looked at soon? It was reopened months ago with little to no activity since then and is fairly simple to reproduce.

@sandwwraith
Copy link
Member

TBH I've missed the fact that it was reopened. I'll revisit this soon.

sandwwraith added a commit that referenced this issue Mar 20, 2023
- Value class is located at top-level, but wraps non-primitive and thus does not fall in 'primitive on top-level' branch
- Value class is a subclass in a polymorphic hierarchy, but either is primitive or explicitly recorded without type info

Note that type info is omitted in the latter case and 'can't add type info to primitive' error is not thrown deliberately, as
there seems to be use-cases for that.

Fixes #1774
Fixes #2159
xBaank pushed a commit to xBaank/kotlinx.serialization that referenced this issue Apr 20, 2023
- Value class is located at top-level, but wraps non-primitive and thus does not fall in 'primitive on top-level' branch
- Value class is a subclass in a polymorphic hierarchy, but either is primitive or explicitly recorded without type info

Note that type info is omitted in the latter case and 'can't add type info to primitive' error is not thrown deliberately, as
there seems to be use-cases for that.

Fixes Kotlin#1774
Fixes Kotlin#2159
@jschneider
Copy link

I got this problem again with this code. Probably some kind of misuse of the serializers?
But since it seems to work when serializing to String it might be a bug:

import kotlinx.serialization.KSerializer
import kotlinx.serialization.SerializationException
import kotlinx.serialization.descriptors.SerialDescriptor
import kotlinx.serialization.descriptors.buildClassSerialDescriptor
import kotlinx.serialization.encoding.Decoder
import kotlinx.serialization.encoding.Encoder
import kotlinx.serialization.json.Json
import kotlinx.serialization.serializer

fun main() {
  println("Hello")

  val json = Json.encodeToString(MySimpleSerializer, "Hello, world") //works
  println("Encoding to string worked: $json")


  Json.encodeToJsonElement(MySimpleSerializer, "Hello, world") //does *not* work
}

object MySimpleSerializer : KSerializer<Any> {
  override val descriptor: SerialDescriptor = buildClassSerialDescriptor("MySimpleSerializer") {
  }

  override fun serialize(encoder: Encoder, value: Any) {
    try {
      val serializer = serializer(value::class.java)
      println("Serializer: $serializer of class ${serializer::class.qualifiedName} }")
      serializer.serialize(encoder, value)
    } catch (e: Exception) {
      throw SerializationException("Failed to serialize [${value::class.java.name}]", e)
    }
  }

  override fun deserialize(decoder: Decoder): Any {
    throw UnsupportedOperationException("Not supported")
  }
}

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

No branches or pull requests

7 participants