micronaut-projects / micronaut-openapi

Generates OpenAPI / Swagger Documentation for Micronaut projects
https://micronaut-projects.github.io/micronaut-openapi/latest/guide/index.html
Apache License 2.0
79 stars 92 forks source link

Nullable "$refs" in oneOfs with no other options create unnecessary interfaces that break both Java/Kotlin Models at Runtime #1679

Closed scprek closed 1 week ago

scprek commented 1 month ago

Expected Behavior

Building off #1667

No interface? just the exact DTO as nullable

openapi-generator-cli generate -i test.yaml -g kotlin -o output/directory -p serializationLibrary=jackson,generateOneOfAnyOfWrappers=true,library=jvm-ktor

Upstream creates an interface still

```kotlin ata class FavorDTOMerchant(var actualInstance: Any? = null) { class CustomTypeAdapterFactory : TypeAdapterFactory { override fun create(gson: Gson, type: TypeToken): TypeAdapter? { if (!FavorDTOMerchant::class.java.isAssignableFrom(type.rawType)) { return null // this class only serializes 'FavorDTOMerchant' and its subtypes } val elementAdapter = gson.getAdapter(JsonElement::class.java) val adapterMerchantDTO = gson.getDelegateAdapter(this, TypeToken.get(MerchantDTO::class.java)) @Suppress("UNCHECKED_CAST") return object : TypeAdapter() { @Throws(IOException::class) override fun write(out: JsonWriter,value: FavorDTOMerchant?) { if (value?.actualInstance == null) { elementAdapter.write(out, null) return } // check if the actual instance is of the type `MerchantDTO` if (value.actualInstance is MerchantDTO) { val element = adapterMerchantDTO.toJsonTree(value.actualInstance as MerchantDTO?) elementAdapter.write(out, element) return } throw IOException("Failed to serialize as the type doesn't match oneOf schemas: MerchantDTO") } @Throws(IOException::class) override fun read(jsonReader: JsonReader): FavorDTOMerchant { val jsonElement = elementAdapter.read(jsonReader) var match = 0 val errorMessages = ArrayList() var actualAdapter: TypeAdapter<*> = elementAdapter // deserialize MerchantDTO try { // validate the JSON object to see if any exception is thrown MerchantDTO.validateJsonElement(jsonElement) actualAdapter = adapterMerchantDTO match++ //log.log(Level.FINER, "Input data matches schema 'MerchantDTO'") } catch (e: Exception) { // deserialization failed, continue errorMessages.add(String.format("Deserialization for MerchantDTO failed with `%s`.", e.message)) //log.log(Level.FINER, "Input data does not match schema 'MerchantDTO'", e) } if (match == 1) { val ret = FavorDTOMerchant() ret.actualInstance = actualAdapter.fromJsonTree(jsonElement) return ret } throw IOException(String.format("Failed deserialization for FavorDTOMerchant: %d classes match result, expected 1. Detailed failure message for oneOf schemas: %s. JSON: %s", match, errorMessages, jsonElement.toString())) } }.nullSafe() as TypeAdapter } } } ```

And MerchantDTO does not implement the interface

data class MerchantDTO (

    @field:JsonProperty("id")
    val id: kotlin.Any? = null,

    @field:JsonProperty("name")
    val name: kotlin.Any? = null
) {

}

Actual Behaviour

Creates interface with Top level Schema prefix (FavorDTO)

package com.favordelivery.coreapi.model

@Generated("io.micronaut.openapi.generator.KotlinMicronautClientCodegen")
interface FavorDTOMerchant {

}

And in the FavorDTO it has this as the field instead of MerchantDTO directly

    @field:Nullable
    @field:Valid
    @field:JsonProperty(JSON_PROPERTY_MERCHANT)
    @field:JsonInclude(JsonInclude.Include.USE_DEFAULTS)
    var merchant: FavorDTOMerchant? = null,

Runtime error

Client 'core-api': Error decoding HTTP response body: Error decoding JSON stream for type [favorDtoResponse]: Cannot construct instance of `com.favordelivery.coreapi.model.FavorDTOMerchant` (no Creators, like default constructor, exist): abstract types either need to be mapped to concrete types, have custom deserializer, or contain additional type information
io.micronaut.http.client.exceptions.HttpClientResponseException: Client 'core-api': Error decoding HTTP response body: Error decoding JSON stream for type [favorDtoResponse]: Cannot construct instance of `com.favordelivery.coreapi.model.FavorDTOMerchant` (no Creators, like default constructor, exist): abstract types either need to be mapped to concrete types, have custom deserializer, or contain additional type information
 at [Source: REDACTED (`StreamReadFeature.INCLUDE_SOURCE_IN_LOCATION` disabled); line: 1, column: 2515] (through reference chain: com.favordelivery.coreapi.model.FavorDtoResponse["favor"]->com.favordelivery.coreapi.model.FavorDTO["merchant"])

Steps To Reproduce

components:
  schemas:
    FavorDTO:
      properties:
        merchant:
          nullable: true
          oneOf:
            - $ref: '#/components/schemas/MerchantDTO'
    MerchantDTO:
      description: 'Class MerchantDTO'
      properties:
        id: {}
        name: {}

Environment Information

No response

Example Application

No response

Version

4.5.1

altro3 commented 1 month ago

Hm... I checked it with 6.12.0-SNAPSHOT and can't reprodcue it. My swagger file:

openapi: 3.0.0
info:
  title: 'Order API'
  version: v6
servers:
  -
    url: 'https://{environment}.com/api/{apiVersion}'
    variables:
      environment:
        enum:
          - api-dev.test.com
        default: api.dev.test.com
      apiVersion:
        enum:
          - v1
          - v2
          - v3
          - v4
          - v5
          - v6
          - v7
        default: v6
paths:
  '/orders/{id}':
    get:
      tags:
        - Order
      operationId: getOrderById
      parameters:
        -
          $ref: '#/components/parameters/OrderId'
      responses:
        '200':
          description: OK
          content:
            application/json:
              schema:
                $ref: '#/components/schemas/FavorDTO'
components:
  parameters:
    OrderId:
      name: id
      in: path
      description: 'The ID'
      required: true
      schema:
        type: string
  schemas:
    FavorDTO:
      properties:
        merchant:
          nullable: true
          oneOf:
            - $ref: '#/components/schemas/MerchantDTO'
    MerchantDTO:
      description: 'Class MerchantDTO'
      properties:
        id: {}
        name: {}

Generated 3 classes:

FavorDTO:

/**
 * FavorDTO
 *
 * @param merchant
 */
@Serdeable
@JsonPropertyOrder(
        FavorDTO.JSON_PROPERTY_MERCHANT
)
@Generated("io.micronaut.openapi.generator.KotlinMicronautClientCodegen")
data class FavorDTO(
    @field:Nullable
    @field:Valid
    @field:JsonProperty(JSON_PROPERTY_MERCHANT)
    @field:JsonInclude(JsonInclude.Include.USE_DEFAULTS)
    var merchant: FavorDTOMerchant? = null,
) {

    companion object {

        const val JSON_PROPERTY_MERCHANT = "merchant"
    }
}

Interface FavorDTOMerchant for oneof:

@Generated("io.micronaut.openapi.generator.KotlinMicronautClientCodegen")
interface FavorDTOMerchant {

}

MerchantDTO:

/**
 * Class MerchantDTO
 *
 * @param id
 * @param name
 */
@Serdeable
@JsonPropertyOrder(
        MerchantDTO.JSON_PROPERTY_ID,
        MerchantDTO.JSON_PROPERTY_NAME
)
@Generated("io.micronaut.openapi.generator.KotlinMicronautClientCodegen")
data class MerchantDTO(
    @field:Nullable
    @field:JsonProperty(JSON_PROPERTY_ID)
    @field:JsonInclude(JsonInclude.Include.USE_DEFAULTS)
    var id: Any? = null,
    @field:Nullable
    @field:JsonProperty(JSON_PROPERTY_NAME)
    @field:JsonInclude(JsonInclude.Include.USE_DEFAULTS)
    var name: Any? = null,
): FavorDTOMerchant {

    companion object {

        const val JSON_PROPERTY_ID = "id"
        const val JSON_PROPERTY_NAME = "name"
    }
}
scprek commented 1 month ago

Ok. What was the test you ran? I simply ran the http request and it failed to deserialize the response

altro3 commented 1 month ago

You wrote that the error is that the FavorDTOMerchant interface is being created, the merchant field is of type FavorDTOMerchant , but here’s the problem, the MerchantDTO class does not implement the MerchantDTO interface.

If this is not the problem, then please make a small reproducer project so that I can understand what the error is

altro3 commented 1 month ago

Yes, now I understand what the problem is. It turns out that the interface cannot be used without a discriminator if you have not declared a custom binder for the object... Then it’s clear why the standard generator creates a CustomTypeAdapterFactory... hmm, we need to finish this solution.... Ok, I’ll see how the standard generator works and based on it I will complete our solution

altro3 commented 1 month ago

In general, I see that fixing your situation is not so easy, because the model on which you are trying to generate code is crooked.

I have a request to you, try to generate code with standard java and kotlin generators and, if you get a working code, then show me what should be generated as a result for everything to work. Because I tried this and got non-working code in all three cases. But I need to understand what needs to be generated for it to work correctly.

scprek commented 1 month ago

Yeah I agree it's weird. I'll first open an issue on the swagger php library and ask why they do nullable refs that way. I don't need them to be nullable personally so having an option to just allow them to be "not required" would suffice.

https://stackoverflow.com/a/48114924

https://github.com/OAI/OpenAPI-Specification/issues/1368

altro3 commented 2 weeks ago

@scprek I think, I fixed all problems with oneOf here: https://github.com/micronaut-projects/micronaut-openapi/pull/1732

Now useOneOfInterfaces property is true by default - that's why you see generated interface. But with nex gradle plugin version you will have ability to turn off this feature and with useOneOfInterfaces=false you will see what you want

scprek commented 2 weeks ago

Ok, so I tried it with the plugin snapshot version so I could set useOneOfInterfaces.set(false) and it look

Slightly different example from above, but same concept just different field. So I have a nullable ref generated by the Swagger PHP library.

          "customer_contact_preference": {
            "oneOf": [
              {
                "$ref": "#/components/schemas/CustomerContactPreferenceDTO"
              }
            ],
            "nullable": true
          },

components

      "CustomerContactPreferenceDTO": {
        "properties": {
          "contact_type": {
            "type": "string"
          },
          "message": {
            "type": "string"
          },
          "icon_url": {
            "type": "string"
          }
        },
        "type": "object"
      },

Then Generated Kotlin

    @field:Nullable
    @field:Valid
    @field:JsonProperty(JSON_PROPERTY_CUSTOMER_CONTACT_PREFERENCE)
    @field:JsonInclude(JsonInclude.Include.USE_DEFAULTS)
    var customerContactPreference: FavorDTOCustomerContactPreference? = null,

Follow up question on difference between upstream kotlin generator and micronaut. It appears micronaut's is making a super data class with all params and then it has the JsonTypeInfo, JsonSubTypes right?

But in the upstream they don't have any fields in the data class and just a TypeAdapterFactory. Just want to confirm my understanding.

Micronaut OpenAPI

@JsonIgnoreProperties(
        value = ["version"], // ignore manually set version, it will be automatically generated by Jackson during serialization
        allowSetters = true // allows the version to be set during deserialization
)
@JsonTypeInfo(use = JsonTypeInfo.Id.NAME, property = "version", visible = true)
@JsonSubTypes(
        JsonSubTypes.Type(value = CancellationReasonTypesV1::class, name = "-1"),
        JsonSubTypes.Type(value = CancellationReasonTypesV2::class, name = "0"),
        JsonSubTypes.Type(value = CancellationReasonTypesV2::class, name = "1"),
        JsonSubTypes.Type(value = CancellationReasonTypesV2::class, name = "2"),
        JsonSubTypes.Type(value = CancellationReasonTypesV2::class, name = "3"),
        JsonSubTypes.Type(value = CancellationReasonTypesV3::class, name = "4")
)
data class CancellationReasonTypesDTO(
    @field:Nullable
    @field:JsonProperty(JSON_PROPERTY_REASONS)
    @field:JsonInclude(JsonInclude.Include.USE_DEFAULTS)
    var reasons: List<@Valid CancellationTypeDto>? = null,
    @field:Nullable
    @field:JsonProperty(JSON_PROPERTY_TITLE)
    @field:JsonInclude(JsonInclude.Include.USE_DEFAULTS)
    var title: String? = null,
    @field:Nullable
    @field:JsonProperty(JSON_PROPERTY_COMMENTS_FIELD_PROMPT)
    @field:JsonInclude(JsonInclude.Include.USE_DEFAULTS)
    var commentsFieldPrompt: String? = null,

Kotlin Generator

-g kotlin -o output/directory -p serializationLibrary=jackson,generateOneOfAnyOfWrappers=false,library=jvm-ktor

 */
@JsonTypeInfo(use = JsonTypeInfo.Id.NAME, include = JsonTypeInfo.As.PROPERTY, property = "version", visible = true)
@JsonSubTypes(
    JsonSubTypes.Type(value = CancellationReasonTypesV1::class, name = "-1"),
    JsonSubTypes.Type(value = CancellationReasonTypesV2::class, name = "0"),
    JsonSubTypes.Type(value = CancellationReasonTypesV2::class, name = "1"),
    JsonSubTypes.Type(value = CancellationReasonTypesV2::class, name = "2"),
    JsonSubTypes.Type(value = CancellationReasonTypesV2::class, name = "3"),
    JsonSubTypes.Type(value = CancellationReasonTypesV3::class, name = "4")
)

data class CancellationReasonTypesDTO(var actualInstance: Any? = null) {

    class CustomTypeAdapterFactory : TypeAdapterFactory {
        override fun <T> create(gson: Gson, type: TypeToken<T>): TypeAdapter<T>? {
            if (!CancellationReasonTypesDTO::class.java.isAssignableFrom(type.rawType)) {
                return null // this class only serializes 'CancellationReasonTypesDTO' and its subtypes
            }
            val elementAdapter = gson.getAdapter(JsonElement::class.java)
            val adapterCancellationReasonTypesV1 = gson.getDelegateAdapter(this, TypeToken.get(CancellationReasonTypesV1::class.java))
            val adapterCancellationReasonTypesV2 = gson.getDelegateAdapter(this, TypeToken.get(CancellationReasonTypesV2::class.java))
            val adapterCancellationReasonTypesV3 = gson.getDelegateAdapter(this, TypeToken.get(CancellationReasonTypesV3::class.java))
altro3 commented 2 weeks ago

Sorry, but I don't understand from your comment what's wrong. You give the example of the Kotlin generator, but the code it generates looks like a crutch. Here, the capabilities of another library are used - gson, but we use jackson.

So can you clearly write to me - what code was generated, and what code should be generated. You shouldn't compare our generator with the Kotlin generator, because we have completely different templates for code generation.

I checked what was generated by our generator - I did not see any problems with the code. So I need a specific description of the problem

scprek commented 2 weeks ago

Ah never mind . I specified Jackson in the options to generate but looks like it still used gson there.

No issues.