Deserialize JSON using KotlinX Serialization instead of Gson

[PattaFeuFeu]

Description

In this PR, I replace GSON as the JSON (de-)serialiser of choice with KotlinX’ Serialization library.

#224 mentioned Jackson as one of the options but KotlinX Serialization seems like a better fit to me with it being a KotlinX library and thus hopefully even better support and more “kotlin-y” APIs.
The Java usage sometimes gets a bit clunky but that shouldn’t be an issue as we don’t expose the serializer. I have made it an internal dependency, as opposed to GSON which was an api instead of implementation gradle dependency.

The usage of GSON in this project was pretty basic but if we need more advanced customer deserializers, this Medium article about this exact migration could come in handy.

Type of Change

• Bug fix (non-breaking change which fixes an issue)
• New feature (non-breaking change which adds functionality)
• Breaking change (fix or feature that would cause existing functionality to not work as expected)
• This change requires a documentation update
• Dependency replacement

How Has This Been Tested?

I ran gradle check and tested the Kotlin and Java sample app with a test instance

Checklist

• My code follows the style guidelines of this project
• I have performed a self-review of my code
• I have made corresponding changes to the documentation if necessary
• I have added tests that prove my fix is effective or that my feature works
• New and existing unit tests pass locally with my changes

---

History of this description as some comments already referenced the old state:

This PR should in the end close #224.

State of this PR

This PR is not yet entirely ready for review. It’s just a first work in progress that I’d like your feedback on. @andregasser

1. Is this a good way forward?
2. Is my change in the test JSON data from Int to String for id-related properties okay? (see e.g. Status API doc which mentions “Type: String (cast from an integer but not guaranteed to be a number)”
3. Are the code-style related changes okay?

Code style

When I applied code style settings from this project in IntelliJ IDEA, auto-formatting changed loads of lines so I assume there’s no style added to the project? I went for the Kotlin defaults then which seemed to mostly not change too much. I did not merge auto-formatting changes unrelated to this PR’s main changes though.

Usually, I use argument names heavily but this project doesn’t seem to do that. Was that a conscious choice? If so, I’m glad to remove the few additions again. If not, I’ll keep them. 😊

Same for var/val types and fun return types. I usually explicitly add them as well but also couldn’t see that everywhere here.

Still open

I needed to litter the JSON config in a few different files and for finishing up I’d like to find a better approach here that would still ensure that we’re using the same config everywhere without adding too much coupling. I already don’t like the JSON serializer being part of MastododonClient but I wanted to keep this PR as close to the original GSON usage as possible for now.

[andregasser]

Hello @PattaFeuFeu
First, thanks for your work so far. Of course, your approach regarding KotlinX is totally fine. Honestly, I am not a Kotlin expert and thus was not aware of the existence of this functionality. But after checking out the docs a bit. I also think this is the right way to go. As you say, it seems better supported / integrated and also more lightweight I guess. :-)

Regarding code style: Detekt for Kotlin code and PMD plus checkstyle for Java code should run once you run gradle check.

Regarding your other questions, I will need to have a look into the code a bit more. Hopefully I can do this tomorrow.

[G10xy]

I am in complete agreement with @PattaFeuFeu. Kotlinx is much better and fits very good for the purposes of this library. By the way, terrific PR

[andregasser]

Is my change in the test JSON data from Int to String for id-related properties okay? (see e.g. Status API doc which mentions “Type: String (cast from an integer but not guaranteed to be a number)”

This is totally fine, even better! Quickly checked Mastodon API docs and that's actually how the data is returned from the server. So all good! 👍

[andregasser]

Is this a good way forward?

Definitively. Go ahead! Looking forward to your changes!

[andregasser]

I have gone through your code changes and must say, that this is looking very good so far. Looking forward to some test runs, once it is ready 🙂Thanks for the effort you put into this! Highly appreciated!

[codecov]

Codecov Report

Merging #270 (98c1d56) into master (06c53e5) will decrease coverage by 11.21%.
The diff coverage is 17.94%.

Additional details and impacted files

@@              Coverage Diff              @@
##             master     #270       +/-   ##
=============================================
- Coverage     47.07%   35.87%   -11.21%     
+ Complexity      357      348        -9     
=============================================
  Files           100      101        +1     
  Lines          3006     3033       +27     
  Branches        164      183       +19     
=============================================
- Hits           1415     1088      -327     
- Misses         1522     1825      +303     
- Partials         69      120       +51     

Files	Coverage Δ
...e/src/main/kotlin/social/bigbone/JsonSerializer.kt	100.00% <100.00%> (ø)
...n/kotlin/social/bigbone/api/method/MediaMethods.kt	100.00% <100.00%> (ø)
...src/main/kotlin/social/bigbone/api/entity/Error.kt	85.71% <66.66%> (-14.29%)	⬇️
.../kotlin/social/bigbone/api/entity/FilterKeyword.kt	88.88% <75.00%> (-11.12%)	⬇️
...src/main/kotlin/social/bigbone/api/entity/Token.kt	90.90% <80.00%> (-9.10%)	⬇️
...ain/kotlin/social/bigbone/api/entity/data/Focus.kt	62.50% <66.66%> (-4.17%)	⬇️
.../kotlin/social/bigbone/api/entity/data/PollData.kt	90.90% <80.00%> (-9.10%)	⬇️
.../src/main/kotlin/social/bigbone/MastodonRequest.kt	83.33% <83.33%> (-3.77%)	⬇️
.../social/bigbone/api/entity/data/InstanceVersion.kt	40.00% <0.00%> (-60.00%)	⬇️
...c/main/kotlin/social/bigbone/api/entity/Context.kt	42.85% <0.00%> (-57.15%)	⬇️
... and 32 more

[PattaFeuFeu]

@andregasser I think I’m mostly done. I will update the description soon and will add the pull request template to the bottom of the PR description with additional info.

How do we want to handle the decrease in code coverage? As all tests are still passing and I didn’t really add new functionality and rather replaced a dependency, I would personally ignore it (which also is the reason why I’m a bit sceptical of code coverage tools as “false positives” like this can happen easily).

If you’d like, I could potentially add some additional tests for edge cases. I introduced nullability in a few places where previously potential nullability was just ignored or covered up in a generic exception.

[andregasser]

How do we want to handle the decrease in code coverage? As all tests are still passing and I didn’t really add new functionality and rather replaced a dependency, I would personally ignore it (which also is the reason why I’m a bit sceptical of code coverage tools as “false positives” like this can happen easily).

If you’d like, I could potentially add some additional tests for edge cases. I introduced nullability in a few places where previously potential nullability was just ignored or covered up in a generic exception.

Do not worry about the test coverage tool output too much, as I am also not 100% happy with it yet. Because of this, I also did not set codecov as a mandatory check to be passed before we can merge changes. It is more of informational nature for the moment. Once I find more time, I'll have a look at it and try to improve / replace it.

I will do some test runs based on the changes in this PR very soon.

Thanks for the update 🙂

[andregasser]

Overall, it looks very good, I have just added two minor comments. Thx!

[bocops]

This looks good to me! Regarding some of the mostly code style questions brought up earlier:

• changing id-related properties in test data to string: this is correct. We've had to change all id types to strings earlier this year, the test data is probably mostly left over from before that change.
• using argument names: I don't think it has ever been a conscious choice not to use them. I prefer them for clarity as well, so as far as I am concerned, we should keep it this way and use argument names throughout.

[andregasser]

• using argument names: I don't think it has ever been a conscious choice not to use them. I prefer them for clarity as well, so as far as I am concerned, we should keep it this way and use argument names throughout.

Fine for me as well. Let's use argument names in a consistent way. Thanks for your review, @bocops !

[andregasser]

Ok!