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

Add ability to disallow repeated keys in CBOR #2681

Open
wants to merge 4 commits into
base: dev
Choose a base branch
from

Conversation

timmc
Copy link
Contributor

@timmc timmc commented May 20, 2024

Fixes #2662 by adding a visitKey method to CompositeDecoder; map and set serializers should call this so that decoders have an opportunity to throw an error when a duplicate key is detected.

A new config option Cbor.forbidDuplicateKeys can be set to true to enable this new behavior. This can form the basis of a Strict Mode in the future.

Also fixes a typo in an unrelated method docstring.

Fixes Kotlin#2662 by adding
a `visitKey` method to `CompositeDecoder`; map and set serializers should
call this so that decoders have an opportunity to throw an error when a
duplicate key is detected.

A new config option `Cbor.allowDuplicateKeys` can be set to false to
enable this new behavior. This can form the basis of a Strict Mode in the
future.

Also fixes a typo in an unrelated method docstring.
* duplicate keys are encountered.
*/
@ExperimentalSerializationApi
public fun visitKey(key: Any?) { }
Copy link
Contributor Author

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. :-)

Copy link
Contributor

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.

Copy link
Contributor

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.

Copy link
Contributor Author

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.)

Copy link
Contributor

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.

Copy link
Contributor

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).

import kotlinx.serialization.DuplicateMapKeyException
import kotlin.test.Test

class CborStrictModeTest {
Copy link
Contributor Author

Choose a reason for hiding this comment

The 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 visitKey call to some inlined code somewhere, but I haven't found the appropriate callsite yet.

Copy link
Contributor Author

Choose a reason for hiding this comment

The 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?

Copy link
Contributor

Choose a reason for hiding this comment

The 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 { "a": "something", "a": "something else" }). To support that your format will need to plug in to decodeElementIndex and mark/record seen indices. Note that for some formats collections can be validly expressed as repeated key/value pairs (protobuf does this), so depending on how CBOR works you may need to do a slightly more extensive check.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Hmm. I had previously tried modifying decodeElementIndex and wasn't getting the results I expected. Maybe I'll take another crack at 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! I see, the decodeElementIndex in CborReader. Got it.

@timmc timmc mentioned this pull request May 20, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants