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.

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

Jump to bottom

cbor: check inline value classes if marked as @ByteString (fixes #2187) #2466

Merged
merged 2 commits into from
Oct 20, 2023
Merged

cbor: check inline value classes if marked as @ByteString (fixes #2187) #2466

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
dfee01c
cbor: check inline value classes if marked as @ByteString (fixes #2187)
the-eater Oct 6, 2023
6292f2c
cbor: add test for unlabeled inner bytestrings
the-eater Oct 18, 2023
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
8 changes: 8 additions & 0 deletions formats/cbor/commonMain/src/kotlinx/serialization/cbor/internal/Encoding.kt
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -70,6 +70,8 @@ internal open class CborWriter(private val cbor: Cbor, protected val encoder: Cb
if (encodeByteArrayAsByteString && serializer.descriptor == ByteArraySerializer().descriptor) {
encoder.encodeByteString(value as ByteArray)
} else {
encodeByteArrayAsByteString = encodeByteArrayAsByteString || serializer.descriptor.isInlineByteString()

super.encodeSerializableValue(serializer, value)
}
}
Expand Down Expand Up @@ -278,6 +280,7 @@ internal open class CborReader(private val cbor: Cbor, protected val decoder: Cb
@Suppress("UNCHECKED_CAST")
decoder.nextByteString() as T
} else {
decodeByteArrayAsByteString = decodeByteArrayAsByteString || deserializer.descriptor.isInlineByteString()
Copy link
Member

Choose a reason for hiding this comment

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

This looks a bit suspicious. If the current descriptor is not a ByteArraySerializer().descriptor and not a value class over @ByteString ByteArray, why we should still try to decode it as ByteArray?

If this is to support cases when value class property is not annotated itself, like this:

@Serializable data class Outer(@ByteString val i: InnerValue)

@Serializable 
@JvmInline value class InnerValue(val b: ByteArray)

Then I don't see tests for it.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

oh good point, yes that was the intended outcome! I'm not sure if that is wanted, but I can see how it might be unwanted? I guess I could probably expose it as an option in the serializer?

I will add the tests anyhow :)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Tests added

Copy link
Member

@sandwwraith sandwwraith Oct 18, 2023 •
edited
Loading

Choose a reason for hiding this comment

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

I'm not sure if that is wanted

I'm also unsure because I didn't see any requests for that. At the first look, it sounds reasonable, but I do not use byte strings that often

super.decodeSerializableValue(deserializer)
}
}
Expand Down Expand Up @@ -636,6 +639,11 @@ private fun SerialDescriptor.isByteString(index: Int): Boolean {
return getElementAnnotations(index).find { it is ByteString } != null
}

private fun SerialDescriptor.isInlineByteString(): Boolean {
// inline item classes should only have 1 item
return isInline && isByteString(0)
Copy link
Member

Choose a reason for hiding this comment

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

Yes, this is a correct way to do it

}


private val normalizeBaseBits = SINGLE_PRECISION_NORMALIZE_BASE.toBits()

Expand Down
28 changes: 28 additions & 0 deletions formats/cbor/commonTest/src/kotlinx/serialization/cbor/CborReaderTest.kt
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -633,6 +633,34 @@ class CborReaderTest {
)
}

@Test
fun testReadValueClassWithByteString() {
assertContentEquals(
expected = byteArrayOf(0x11, 0x22, 0x33),
actual = Cbor.decodeFromHexString<ValueClassWithByteString>("43112233").x
)
}

@Test
fun testReadValueClassCustomByteString() {
assertEquals(
expected = ValueClassWithCustomByteString(CustomByteString(0x11, 0x22, 0x33)),
actual = Cbor.decodeFromHexString("43112233")
)
}

@Test
fun testReadValueClassWithUnlabeledByteString() {
assertContentEquals(
expected = byteArrayOf(
0x11,
0x22,
0x33
),
actual = Cbor.decodeFromHexString<ValueClassWithUnlabeledByteString>("43112233").x.x
)
}

@Test
fun testIgnoresTagsOnStrings() {
/*
Expand Down
24 changes: 24 additions & 0 deletions formats/cbor/commonTest/src/kotlinx/serialization/cbor/CborWriterTest.kt
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -122,4 +122,28 @@ class CbrWriterTest {
actual = Cbor.encodeToHexString(TypeWithNullableCustomByteString(null))
)
}

@Test
fun testWriteValueClassWithByteString() {
assertEquals(
expected = "43112233",
actual = Cbor.encodeToHexString(ValueClassWithByteString(byteArrayOf(0x11, 0x22, 0x33)))
)
}

@Test
fun testWriteValueClassCustomByteString() {
assertEquals(
expected = "43112233",
actual = Cbor.encodeToHexString(ValueClassWithCustomByteString(CustomByteString(0x11, 0x22, 0x33)))
)
}

@Test
fun testWriteValueClassWithUnlabeledByteString() {
assertEquals(
expected = "43112233",
actual = Cbor.encodeToHexString(ValueClassWithUnlabeledByteString(ValueClassWithUnlabeledByteString.Inner(byteArrayOf(0x11, 0x22, 0x33))))
)
}
}
19 changes: 18 additions & 1 deletion formats/cbor/commonTest/src/kotlinx/serialization/cbor/SampleClasses.kt
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -8,6 +8,7 @@ import kotlinx.serialization.*
import kotlinx.serialization.builtins.*
import kotlinx.serialization.descriptors.*
import kotlinx.serialization.encoding.*
import kotlin.jvm.*

@Serializable
data class Simple(val a: String)
Expand Down Expand Up @@ -110,4 +111,20 @@ class CustomByteStringSerializer : KSerializer<CustomByteString> {
data class TypeWithCustomByteString(@ByteString val x: CustomByteString)

@Serializable
data class TypeWithNullableCustomByteString(@ByteString val x: CustomByteString?)
data class TypeWithNullableCustomByteString(@ByteString val x: CustomByteString?)

@JvmInline
@Serializable
value class ValueClassWithByteString(@ByteString val x: ByteArray)

@JvmInline
@Serializable
value class ValueClassWithCustomByteString(@ByteString val x: CustomByteString)

@JvmInline
@Serializable
value class ValueClassWithUnlabeledByteString(@ByteString val x: Inner) {
@JvmInline
@Serializable
value class Inner(val x: ByteArray)
}