Skip to content

Allow failing on duplicate keys #1990

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

Open
tjerkw opened this issue Jul 13, 2022 · 10 comments
Open

Allow failing on duplicate keys #1990

tjerkw opened this issue Jul 13, 2022 · 10 comments
Assignees

Comments

@tjerkw
Copy link

tjerkw commented Jul 13, 2022

WHY

The JSON format allows for duplicate keys in objects:

{'a': 2, 'a': 4}

but the spec says it "SHOULD" not be done.

Different parsers implement it differently.
It would by nice to configure the Json parses to fail on duplicate keys

HOW

Allow a setting on Json to fail on duplicate keys:

val json = kotlinx.serialization.json.Json {
        ignoreDuplicateKeys = false (default should be true
    }
``
@tjerkw tjerkw added the feature label Jul 13, 2022
@qwwdfsad
Copy link
Member

qwwdfsad commented Sep 5, 2022

Unless there are compelling use cases, we would prefer not to introduce such a feature: it's way too easy to misuse it and start ignoring backend errors or API misconfiguration.

A class-specific workaround would be to write your own serializer (or use JsonTransforming one) that deliberately skips duplicates

@Lysoun
Copy link

Lysoun commented Oct 8, 2022

A use case of such a feature is to check strict JSON validity. Since the spec does not recommend using duplicate keys, it seems rather natural to allow the user to forbid such a thing. Especially when trying to check the equality between two JSON, the following JSON appear to be the same using kotlinx serializers but they are not

{key: 'value1', key: 'value2'} 
{key: 'value2'}

I don't understand how it is so easy to misuse such a feature though. The error message should be clear and give the key that was duplicated. Since it would be an option that the user has to set if they want such a strict check, well, if they don't want that check they just have to not set the option, which would anyway be the default behaviour.

Are you still firmly against it or can I make a PR for this feature? I'm pretty sure it would be very easy to implement, making a little change in JsonTreeReader.

@qwwdfsad
Copy link
Member

qwwdfsad commented Oct 8, 2022

I honestly was sure that we do fail on duplicate keys and that the request was about allowing duplicate keys, thus misreading the sentence.

The request indeed is reasonable and seems like it should be on by default, though it requires a proper damage assessment.

@qwwdfsad qwwdfsad added the json label Oct 8, 2022
Lysoun added a commit to Lysoun/kotlinx.serialization that referenced this issue Oct 9, 2022
@Lysoun
Copy link

Lysoun commented Oct 9, 2022

@qwwdfsad Hi, I've been working in this issue but I have trouble running the tests with the JVM. I get this error:

> Task :kotlinx-serialization-core:compileKotlinJvm FAILED
e: Module java.base cannot be found in the module graph

Guess it must be linked to my version of the JDK. I tried using 1.8, 11 and 17 (configured in my project structure in IntelliJ and in my preferences, for kotlin compilation target) but it always fails. It seems to be a problem with the project gradle configuration but I'm very surprised that the contributors did not see that.
Since I have a test that is not passing yet (I tested it using js), it makes it very hard to debug it. Do you know something about this problem?

@qwwdfsad
Copy link
Member

Are you encountering this error when running tests from IDE or from the console?

Checked clean build on my machine:

qwwdfsad@qwwdfsad kotlinx.serialization % java -version
java version "11.0.1" 2018-10-16 LTS
Java(TM) SE Runtime Environment 18.9 (build 11.0.1+13-LTS)
Java HotSpot(TM) 64-Bit Server VM 18.9 (build 11.0.1+13-LTS, mixed mode)
qwwdfsad@qwwdfsad kotlinx.serialization % ./gradlew :kotlinx-serialization-json:jvmTest
Starting a Gradle Daemon, 1 incompatible Daemon could not be reused, use --status for details
... executes normally in dev branch... 

I am not aware of this particular problem, but I've seen multiple times that Gradle sporadically fails on JPMS-related functionality.
I suggest killing old daemons, cleaning .gradle build cache and try again

@Lysoun
Copy link

Lysoun commented Oct 10, 2022

I did it running the tests from my IDE. I'll try your suggestions, thank you.

@Lysoun
Copy link

Lysoun commented Oct 27, 2022

I managed to run the tests! What I had to do:

  • make sure I had a jdk 11 installed
  • make sure it was my default jdk
    I'm pretty sure there must be something fishy about the project's gradle configuration because when I ran the compilation task from a terminal, gradle logged this:
'compileJava' task (current target is 11) and 'compileKotlin' task (current target is 1.8) jvm target compatibility should be set to the same Java version.
Using Kotlin compiler version: 1.7.20-RC

The compileKotlin tasks must not be set correctly... Is there an issue for this or should I create one?

@sandwwraith
Copy link
Member

The difference is caused by the fact that we should have Kotlin class files produced with majorVersion = 52 (for Android to read), thus, Java 8 target. But we also have to build a multi-release JAR to support the Java module system — and module-info.java is compiled with Java 11. I don't see any possibilities to unify this

@Lysoun
Copy link

Lysoun commented Oct 27, 2022

That means that the only way to run the JVM tests is to set java 11 as default version on one's computer, which can be pretty troublesome when it's not.

Lysoun added a commit to Lysoun/kotlinx.serialization that referenced this issue Oct 27, 2022
Lysoun added a commit to Lysoun/kotlinx.serialization that referenced this issue Oct 27, 2022
@CLOVIS-AI
Copy link

Hi! Do you have an estimate of when this can be fixed? 🙏

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

No branches or pull requests

6 participants