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
Fix DefaultJson documentation #3021
Conversation
I'm not sure about this change. It will break the behaviour of existing clients and we better postpone it to majot release. Maybe it's better to fix documentation to reflect an actual behavior? |
43b5149
to
dc8ea25
Compare
I agree, I just update the PR to fix the documentation and also add a test to prove the behavior |
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.
LGTM
@@ -106,6 +106,17 @@ class JsonSerializationTest : AbstractSerializationTest<Json>() { | |||
) | |||
) | |||
} | |||
|
|||
@Test(expected = SerializationException::class) |
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.
Can you please check native tests here? It fails to compile on CI.
Also, we usually use assertFailesWith<SerializationException> { ... }
syntax instead
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.
Done ;)
dc8ea25
to
1a52964
Compare
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.
LGTM
Subsystem
Client
Motivation
The documentation says that the JSON configuration is "mode is not strict so extra json fields are ignored" but is not what is actually doing.
Solution
Add
ignoreUnknownKeys = true
to make the JSON configuration work as expected.