-
Notifications
You must be signed in to change notification settings - Fork 635
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
Add ability to disallow repeated keys in CBOR #2681
base: dev
Are you sure you want to change the base?
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,45 @@ | ||
package kotlinx.serialization.cbor | ||
|
||
import kotlinx.serialization.assertFailsWithMessage | ||
import kotlinx.serialization.decodeFromByteArray | ||
import kotlinx.serialization.HexConverter | ||
import kotlinx.serialization.DuplicateKeyException | ||
import kotlinx.serialization.Serializable | ||
import kotlin.test.Test | ||
|
||
class CborStrictModeTest { | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. TODO: Add a (passing) test for duplicate key detection when deserializing a data class. I have a test locally that fails -- I think I need to add There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I think I need to modify the serialization plugin's code generator in order to do this! Where does that code even live? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. @timmc The plugin code lives in the main Kotlin repository, but you don't need to go there. I suppose you mean to detect multiple keys in object serializiation (like this Json There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Hmm. I had previously tried modifying There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Oh! I see, the |
||
private val strict = Cbor { forbidDuplicateKeys = true } | ||
|
||
/** Duplicate keys are rejected in generic maps. */ | ||
@Test | ||
fun testDuplicateKeysInMap() { | ||
val duplicateKeys = HexConverter.parseHexBinary("A2617805617806") | ||
assertFailsWithMessage<DuplicateKeyException>("Duplicate keys not allowed. Key appeared twice: x") { | ||
strict.decodeFromByteArray<Map<String, Long>>(duplicateKeys) | ||
} | ||
} | ||
|
||
@Serializable | ||
data class ExampleClass(val x: Long) | ||
|
||
/** Duplicate keys are rejected in classes. */ | ||
@Test | ||
fun testDuplicateKeysInDataClass() { | ||
// {"x": 5, "x", 6} | ||
val duplicateKeys = HexConverter.parseHexBinary("A2617805617806") | ||
assertFailsWithMessage<DuplicateKeyException>("Duplicate keys not allowed. Key appeared twice: x") { | ||
strict.decodeFromByteArray<ExampleClass>(duplicateKeys) | ||
} | ||
} | ||
|
||
/** Duplicate unknown keys are rejected as well. */ | ||
@Test | ||
fun testDuplicateUnknownKeys() { | ||
// {"a": 1, "a", 2, "x", 6} | ||
val duplicateKeys = HexConverter.parseHexBinary("A3616101616102617806") | ||
val cbor = Cbor(strict) { ignoreUnknownKeys = true } | ||
assertFailsWithMessage<DuplicateKeyException>("Duplicate keys not allowed. Key appeared twice: a") { | ||
cbor.decodeFromByteArray<ExampleClass>(duplicateKeys) | ||
} | ||
} | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm not happy with this name but couldn't think of anything better -- suggestions very welcome. :-)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Checking duplication of keys should be more like a kind of common logics, and should be provided via a well-implemented utility class, rather than a function in top-level interface for each format to implement. IMO, most of them will be just some copy-paste.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@xiaozhikang0916 The challenge is that the behaviour needs to be configurable. There is no central configuration mechanism in the library, only formats and
SerialDescriptors
. There is also the need to do this in an ABI/API compatible way. Formats already need to do various things to play nice/support the various library features. This would just be another one. It also makes sense as various formats have their own duplication semantics.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I initially tried plumbing the information through on the deserialization layer and it went very badly. This is only the start of what would be required: dev...timmc:timmc/all-strict (although in practice it would be a config object, not a single boolean -- this was just a failed proof of concept.)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I mean the duplication check in other formats ( if they need ), would share some common logic, like holding a set and putting keys in it. These common logic can be extracted to a utility class.
On the other hand, I am against adding this function in
Decoding
interface. It is implemented by each format with different behaviours, and only for internal using, which means not public, and users writing custom serializers should never call this function.IMO, you may probably leave some instructions for format developers about how to achieve duplication check, rather than a public default funtion in this base interface, and make it open for users.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It would be possible to circumvent the MapSerializer if you have a map specific decoder. This decoder could intercept the decoding of the key, and then check there (before the result is returned to the serializer).
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm happy to implement whatever is needed, but I don't have a good understanding of what would be acceptable to the maintainers. It sounds like visitKey isn't well-liked, and the options might be:
MapLikeSerializer
-like serializer, fully reimplementing the logic. (This would be done via a serializers module, right?)MapLikeSerializer
to a new utility classNoDuplicatesMapLikeSerializer
and add the duplicate detection; formats use this in their serializers module if desired.MapLikeSerializer
to makereadElement
non-final so that it can be overridden by classes likeHashMapSerializer
(or copies of them) rather than copying the implementation.CompositeDecoder
that is either special-purposed for duplicate key detection or is more abstract and allows for a wide range of behaviors (including duplicate key detection).Which of these seems most palatable? Are there other possibilities?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
One concern I have with anything that involves the serializers module is that it risks interference with application-configured serializers. Each relevant format would have to document warnings like "if you specify an alternative serializer for maps, you'll need to reimplement duplicate key detection". Having it be performed in shared code (i.e. MapLikeSerializer) would at least allow the warning to recommend subclassing that shared code.