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

Multiple issues with schema generation: field order, value classes and custom handling #56

Open
4 tasks
Ribesg opened this issue Dec 9, 2022 · 3 comments

Comments

@Ribesg
Copy link

Ribesg commented Dec 9, 2022

Hi, I just tried Tegral OpenAPI Ktor Plugins for the first time. It's kinda nice, but schema generation has multiple problems. I'll provide an example:

Here is my model, hopefully with all its dependencies so that it can be used in a reproducer:

// The body model

@Serializable
data class CommitteesTimeSlotsBody(
    val event: Event,
    val committees: Set<Committee>,
)
All types used
@Serializable
data class Committee(
    val id: CommitteeId,
    val attendance: Set<DateRange>
)

@JvmInline
@Serializable
value class CommitteeId(val value: String) {
    override fun toString() = value
}

@Serializable(with = OffsetDateSerializer::class)
class Date(

    /**
     * Epoch milliseconds
     */
    val time: Long

) : Comparable<Date> {

    companion object {

        fun now() = Date(System.currentTimeMillis())

        fun parse(isoOffsetDate: String): Date =
            OffsetDateTime
                .parse(isoOffsetDate)
                .toInstant()
                .toEpochMilli()
                .let(::Date)

    }

    operator fun plus(duration: Duration): Date =
        Date(time + duration.inWholeMilliseconds)

    operator fun minus(date: Date): Duration {
        require(this >= date) {
            "Date minus provided Date would create negative duration"
        }
        return (time - date.time).milliseconds
    }

    fun format(): String =
        DateTimeFormatter.ISO_OFFSET_DATE_TIME.format(
            ZonedDateTime.ofInstant(Instant.ofEpochMilli(time), ZoneId.of("UTC"))
        )

}

@Serializable
data class DateRange(
    val start: Date,
    val end: Date
) {

    val duration: Duration
        get() = end - start

}

@Serializable
data class Event(
    val id: EventId,
    val schedule: Set<DateRange>,
    val interviewDuration: Duration,
    val pauseDuration: Duration = Duration.ZERO,
)

@JvmInline
@Serializable
value class EventId(val value: String) {
    override fun toString() = value
}

object OffsetDateSerializer : KSerializer<Date> {

    override val descriptor =
        PrimitiveSerialDescriptor("OffsetDateSerializer", PrimitiveKind.STRING)

    override fun deserialize(decoder: Decoder) =
        Date.parse(decoder.decodeString())

    override fun serialize(encoder: Encoder, value: Date) {
        encoder.encodeString(value.format())
    }

}

I have a Resource defined like this:

@Serializable
@Resource("committees-timeslots")
class CommitteesTimeSlots {

    companion object : ResourceDescription by describeResource({
        body {
            json {
                val now = Date.now()
                val schedule = setOf(DateRange(now, now + 3.hours))
                schema(
                    CommitteesTimeSlotsBody(
                        event = Event(
                            id = EventId("event-id"),
                            schedule = schedule,
                            interviewDuration = 10.minutes,
                            pauseDuration = Duration.ZERO,
                        ),
                        committees = setOf(
                            Committee(
                                id = CommitteeId("committee-id"),
                                attendance = schedule,
                            )
                        ),
                    )
                )
            }
        }
    })

}

In an ideal world, this is the generated json schema I'd like to see:

{
  "event": {
    "id": "event-id",
    "schedule": [
      {
        "start": "2022-12-09T09:52:09.460Z",
        "end": "2022-12-09T12:52:09.460Z"
      }
    ],
    "interviewDuration": 600000,
    "pauseDuration": 0
  },
  "committees": [
    {
      "id": "committee-id",
      "attendance": [
        {
          "start": "2022-12-09T09:52:09.460Z",
          "end": "2022-12-09T12:52:09.460Z"
        }
      ]
    }
  ]
}

And this is what I get:

{
  "event": {
    "schedule": [
      {
        "start": {
          "time": 1670579529460
        },
        "end": {
          "time": 1670590329460
        },
        "duration-UwyO8pc": 21600000000000
      }
    ],
    "interviewDuration-UwyO8pc": 1200000000000,
    "id-it7Ee5o": "event-id",
    "pauseDuration-UwyO8pc": 0
  },
  "committees": [
    {
      "attendance": [
        {
          "start": {
            "time": 1670579529460
          },
          "end": {
            "time": 1670590329460
          },
          "duration-UwyO8pc": 21600000000000
        }
      ],
      "id-5s7UXzY": "committee-id"
    }
  ]
}

Here's a list of what I think fails / is lacking:

  • Model field order could be kept, that would be nice
  • Some fields are serialized, but they should not, like the duration property on DateRange
  • Some field names are mangled, looks like it's all value class typed values
  • It would be nice to be able to provide / use custom serializers, for example for my Date type here. Maybe add an option to use kotlinx.serialization somehow, or just a way to pass a custom lambda in an annotation or something
@Ribesg
Copy link
Author

Ribesg commented Dec 9, 2022

Most of these issues can be fixed by cluttering the source code with a lot of Jackson annotations. For anyone passing by with similar issues, here's how I fixed them:

Some fields are serialized, but they should not, like the duration property on DateRange

Fixed using @JsonIgnore, one of two ways:

    @get:JsonIgnore
    val duration: Duration
        get() = end - start
    val duration: Duration
        @JsonIgnore
        get() = end - start

Some field names are mangled, looks like it's all value class typed values

Fixed with @JsonProperty on the getter of value class properties:

@Serializable
data class Committee(
    @get:JsonProperty("id")
    val id: CommitteeId,
    val attendance: Set<DateRange>
)

It would be nice to be able to provide / use custom serializers, for example for my Date type here. Maybe add an option to use kotlinx.serialization somehow, or just a way to pass a custom lambda in an annotation or something

We can actually use custom Jackson serializers, for example for Date:

object DateSerializer : JsonSerializer<Date>() {

    override fun serialize(value: Date, gen: JsonGenerator, serializers: SerializerProvider) {
        gen.writeString(value.format())
    }

}

Then, every time we have a field of type Date in a model:

@Serializable
data class DateRange(
    @get:JsonSerialize(using = DateSerializer::class)
    val start: Date,
    @get:JsonSerialize(using = DateSerializer::class)
    val end: Date,
)

And good luck implementing every single serializer twice for kotlinx.serialization and Jackson.

While it works, this adds a lot of clutter. It also works in my current Kotlin/JVM project, but I have a lot of Kotlin/Multiplatform projects where models are defined in common Kotlin code where JVM annotations aren't allowed, I'd probably have to make a KMP module wrapping Jackson annotations to use them in common code and clutter my whole codebase :)

I also did not find a way to keep the declared field order in the output, yet. Right now the output field order seems to be random. It's not super important as json does not have key order in objects, but here for json that is supposed to be read by humans, it would be cool to have.

@utybo
Copy link
Owner

utybo commented Dec 9, 2022

Hi, thanks for your detailed issue! I unfortunately don't think there's much that can be done in Tegral without reimplementing parts of Swagger Core (which is planned)

For a bit of context (and as you probably guessed), Swagger Core mainly deals with Jackson annotations and internally uses Jackson, but it should be possible to implement the required parts

Model field order could be kept, that would be nice

I'm not sure of whether Tegral OpenAPI, Swagger Core or Jackson is the culprit
here, that will have to be investigated.

  • Some fields are serialized, but they should not, like the duration
    property on DateRange
  • It would be nice to be able to provide / use custom
    serializers, for example for my Date type here. Maybe add an option to use
    kotlinx.serialization somehow, or just a way to pass a custom lambda in an
    annotation or something

Both of these are because Swagger Core only considers Jackson annotations, and not kotlinx.serialization's.

I'll do my best to address all of these issues in #55 and will use your classes as test data.

For the mangling part, I have no idea how inlined value classes are depicted when using reflection, so I'll have to check how that works.

Out of curiosity, is Ktor correctly serializing your data like you'd expect in the JSON schema?

@Ribesg
Copy link
Author

Ribesg commented Dec 15, 2022

Out of curiosity, is Ktor correctly serializing your data like you'd expect in the JSON schema?

I'm not exactly sure what you ask here, but yes, kotlinx.serialization works as expected. The code I provided only lacks some @SerialName("…") annotations on data classes (not fields) so that when logging the serial names of classes don't contain their full package (makes the logs more readable), but other than that it's the full model code and it works.

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

No branches or pull requests

2 participants