Support for Kotlin Serialization #327

ulrikandersen · 2024-10-17T13:23:30Z

This PR builds on the ideas in #249 (tracked in #89) to introduce support for kotlinx.serialization (KxS) annotations on the generated models.

A new --serialization-library option allows the user to choose between JACKSON and KOTLINX_SERIALIZATION.
The Jackson implementation is the default and has not changed, and users will get the same output as previously.

For date and datetime kotlinx-datetime is used to make the generated code work in multi platform / non-JVM environments. Date is translated to kotlinx.datetime.LocalDate and datetime to kotlinx.datetime.Instant.

Unsupported features

When KxS is chosen additionalProperties: true is not supported. This is due to fact that supporting a map<String,Any?> is quite tricky (if at all possible?) with Kotlinx.Serialization, whereas Jackson has JsonAnyGetter/JsonAnySetter to support this.
We might be able to configure the property with @Contextual to instruct the plugin to resolve serializer at runtime.
Related: Support for additional properties Kotlin/kotlinx.serialization#1978.
URI and UUID are translated to strings as no native Kotlin types exist (yet?).
number without format is currently translated to Java's BigDecimal which will not work in non-JVM Kotlin. Works fine for number/float and number/double as these are translated into Kotlin types.

Tests
I am unsure how many more tests for the examples we want. Many of the examples are concerned with translating OpenAPI into models, and are not specific to the serialization annotations. Any opinions/input/suggestion are welcome.

TODO

Add support for basic models.
Add support for enums.
Add support for discriminated oneOf.
Look into how to support fields that are currently output as Java's UUID, OffsetDateTime, etc.
Add tests for some examples.
Add end-2-end tests.
Rename JacksonModelGenerator to simply ModelGenerator (in future PR).

and add basic support for Kotlinx Serialization

Expose UUID and URI as string as native Kotlin type do not exist

ulrikandersen · 2024-11-10T20:08:28Z

@cjbooms Since this is a new feature, I am thinking that once you (and possibly others) have reviewed the implementation, it will be beneficial to get feedback from people that make use of Kotlin Serialization in their projects - both backend and mobile.

Since it does not change existing code generation output I think we can safely be released once we are happy with the implementation. Possibly as a "beta" release, if you are more comfortable with that.

cjbooms · 2024-11-10T20:51:17Z

@cjbooms Since this is a new feature, I am thinking that once you (and possibly others) have reviewed the implementation, it will be beneficial to get feedback from people that make use of Kotlin Serialization in their projects - both backend and mobile.

Since it does not change existing code generation output I think we can safely be released once we are happy with the implementation. Possibly as a "beta" release, if you are more comfortable with that.

Nice work, this will be a great addition. I look forward to reviewing this tomorrow or the next day.

Yeah, I'm not worried about doing anything special for release. Let's just throw it out there and see what the feedback is. We're not going to break anyone, and can easily improve things in future releases as it gets used.

ulrikandersen · 2024-11-12T16:01:51Z

It is worth noting that, due to the requirement of serializers being present at compile time, the generated Event does actually not compile. This is due to the untyped object data being generated as Map<String, Any?>.

In order for this to work, we need to annotate Any with @Contextual, which will enable (require) the user to provide a custom serializer for Any, which will be resolved at runtime.

I have made a PoC over here, but it requires polishing before being mergeable. It does not have to block this PR, but it is useful info to have in the review process.

cjbooms

Great work. Impressed with how little impact on the existing generation code structures you managed to achieve.
A few nit picks and clarifications for my own benefit, but I'm happy to release and get community feedback if that's what you want.

cjbooms · 2024-11-13T22:51:59Z

src/main/kotlin/com/cjbooms/fabrikt/generators/model/JacksonModelGenerator.kt

@@ -67,13 +68,15 @@ import java.io.Serializable
 import java.net.MalformedURLException
 import java.net.URL

-class JacksonModelGenerator(
+class JacksonModelGenerator( // TODO: Rename to ModelGenerator


👍 Will do in a PR once this has been merged.

cjbooms · 2024-11-13T22:56:28Z

src/main/kotlin/com/cjbooms/fabrikt/model/SerializationAnnotations.kt

+import com.squareup.kotlinpoet.TypeName
+import com.squareup.kotlinpoet.TypeSpec
+
+sealed interface SerializationAnnotations {


This is a nice abstraction. I can imagine you had lots of nasty branching logic before you introduced it. kudos

@zettelmj gave me the idea in #249 and from there it was pretty smooth 🙌

cjbooms · 2024-11-13T22:59:03Z

src/main/kotlin/com/cjbooms/fabrikt/model/KotlinTypeInfo.kt

+                }
+                OasType.DateTime -> {
+                    if (MutableSettings.serializationLibrary() == KOTLINX_SERIALIZATION) KotlinxInstant
+                    else getOverridableDateTimeType()


Does kotlinx only have one date-time, or may we also need an overridable function for it?

There's only one and you would normally use Instant for datetimes and thus no need for overrides ✌️

cjbooms · 2024-11-13T23:11:32Z

src/test/resources/examples/githubApi/models/kotlinx/Event.kt

+  public val entityId: String,
+  @SerialName("data")
+  @get:NotNull
+  public val `data`: Map<String, Any?>,


wrt your pre-review comment around @Contextual, are we being inconsistent by generating this, but omitting additional properties? I'm wondering would it be better to error out, rather than produce code that:
a.) Doesn't compile
b.) Results in data loss (is it possible that kotlinx will quietly drop additional properties?)

Or maybe we should add the annotation to give the user a better hint to implement the required serializer?

Also, do you have any idea if there is a KotlinX issue we can track for this feature to be natively supported?

Lets be consistent and error out in case untyped objects or additional properties are found in the schema. I have added this to the PR.

Supporting untyped objects with @Contextual is totally viable, but we need to figure out how to build it in a user friendly way.

Support for additionalProperties is being tracked in this issue by @pschichtel.

additional properties not supported

We might be able to support them in the future by annotating Any with @contextual

ulrikandersen · 2024-11-18T07:26:48Z

@cjbooms Thanks for performing an initial review 🙏 Once you’ve had a chance to go through the changes, I’d love to get this released so we can hear what the community thinks.

ulrikandersen added 4 commits October 17, 2024 15:28

Add --serialization-library option

2a3ee2b

Add function to determine discriminator mapping key

59bf0a9

Move serialization annotations to separate objects

f7b269c

and add basic support for Kotlinx Serialization

Add end2end KxS tests for a basic OpenAPI

e410879

ulrikandersen force-pushed the kotlinx-serialization branch from ad489c7 to e410879 Compare October 17, 2024 13:28

ulrikandersen self-assigned this Oct 17, 2024

ulrikandersen changed the title ~~Support Kotlin Serialization~~ Support for Kotlin Serialization Oct 17, 2024

ulrikandersen added 8 commits October 18, 2024 10:49

Remove redundant return types

7d2b2df

Add kotlinx.serialization support for enums

4aa3449

Setup enum constant building correctly

bbf95f7

Add test for discriminatedOneOf

7a88c69

Use kotlinx.datetime for date and datetime

872cd26

Expose UUID and URI as string as native Kotlin type do not exist

Add kotlin serialization test for github api

b7e0c5c

Remove experimental note

c6f4190

Cleanup

9bdfd47

ulrikandersen marked this pull request as ready for review November 10, 2024 19:50

ulrikandersen requested a review from cjbooms November 10, 2024 19:56

Remove unused Jackson deps

dce401e

cjbooms reviewed Nov 13, 2024

View reviewed changes

ulrikandersen added 4 commits November 16, 2024 21:25

Remove github models for kotlinx

c8466b7

additional properties not supported

Error out in case additionalProperties is found in schema

db462f0

Untyped objects not supported for kotlinx

7ce48b2

We might be able to support them in the future by annotating Any with @contextual

Mention kotlinx serialization in Features

56cd5c7

ulrikandersen requested a review from cjbooms November 17, 2024 07:55

OkHttp client currently only supports Jackson

c0d9012

ulrikandersen mentioned this pull request Nov 18, 2024

support kotlinx serialization #249

Closed

cjbooms approved these changes Nov 18, 2024

View reviewed changes

cjbooms merged commit 22944d3 into cjbooms:master Nov 18, 2024
1 check passed

ulrikandersen mentioned this pull request Nov 18, 2024

JacksonModelGenerator is now simply ModelGenerator #330

Merged

ulrikandersen deleted the kotlinx-serialization branch December 20, 2024 11:55

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Support for Kotlin Serialization #327

Support for Kotlin Serialization #327

ulrikandersen commented Oct 17, 2024 •

edited

Loading

ulrikandersen commented Nov 10, 2024 •

edited

Loading

cjbooms commented Nov 10, 2024

ulrikandersen commented Nov 12, 2024 •

edited

Loading

cjbooms left a comment

cjbooms Nov 13, 2024

ulrikandersen Nov 18, 2024

cjbooms Nov 13, 2024

ulrikandersen Nov 16, 2024

cjbooms Nov 13, 2024

ulrikandersen Nov 16, 2024

cjbooms Nov 13, 2024 •

edited

Loading

ulrikandersen Nov 17, 2024 •

edited

Loading

ulrikandersen commented Nov 18, 2024

Support for Kotlin Serialization #327

Support for Kotlin Serialization #327

Conversation

ulrikandersen commented Oct 17, 2024 • edited Loading

ulrikandersen commented Nov 10, 2024 • edited Loading

cjbooms commented Nov 10, 2024

ulrikandersen commented Nov 12, 2024 • edited Loading

cjbooms left a comment

Choose a reason for hiding this comment

cjbooms Nov 13, 2024

Choose a reason for hiding this comment

ulrikandersen Nov 18, 2024

Choose a reason for hiding this comment

cjbooms Nov 13, 2024

Choose a reason for hiding this comment

ulrikandersen Nov 16, 2024

Choose a reason for hiding this comment

cjbooms Nov 13, 2024

Choose a reason for hiding this comment

ulrikandersen Nov 16, 2024

Choose a reason for hiding this comment

cjbooms Nov 13, 2024 • edited Loading

Choose a reason for hiding this comment

ulrikandersen Nov 17, 2024 • edited Loading

Choose a reason for hiding this comment

ulrikandersen commented Nov 18, 2024

ulrikandersen commented Oct 17, 2024 •

edited

Loading

ulrikandersen commented Nov 10, 2024 •

edited

Loading

ulrikandersen commented Nov 12, 2024 •

edited

Loading

cjbooms Nov 13, 2024 •

edited

Loading

ulrikandersen Nov 17, 2024 •

edited

Loading