OpenAPI generation of `OptionalValue` may be incorrect

[mmurto]

There may be something in OpenAPI generation in Kotlin or the API handler, specifically for OptionalValue. @mnonnenmacher can you take a look?

The relevant parts of current OpenaAPI for UpdateOrganization:

      "UpdateOrganization" : {
        "title" : "UpdateOrganization",
        "required" : [ "description", "name" ],
        "type" : "object",
        "properties" : {
          "description" : {
            "$ref" : "#/components/schemas/OptionalValue<String>"
          },
          "name" : {
            "$ref" : "#/components/schemas/OptionalValue<String>"
          }
        }
      },
      "OptionalValue<String>" : {
        "title" : "OptionalValue<String>",
        "anyOf" : [ {
          "$ref" : "#/components/schemas/Absent"
        }, {
          "$ref" : "#/components/schemas/Present<*>"
        } ]
      },
      "Absent" : {
        "title" : "Absent",
        "type" : "object",
        "properties" : { }
      },
      "Present<*>" : {
        "title" : "Present<*>",
        "required" : [ "value" ],
        "type" : "object",
        "properties" : {
          "value" : {
            "title" : "*",
            "type" : "object"
          }
        }
      },

The correct payload for updating organization name should then be:

{
  "name": {
    "value": "new name"
  },
}

For some reason, the old version of @7nohe/openapi-react-query-codegen doesn't correctly generate the TypeScript interfaces for OptionalValue, so it has ended accepting the following that, according to the OpenAPI spec, should not be accepted by the API :

{
  "name": "new name"
}

For some reason, the API does accept it and modifies the organization name accordingly. If the code is modified to abide to the OpenAPI spec (that the new version of openapi-react-query-codegen does), we get a payload of

{
  "name": {
    "value": "Org 4"
  },
  "description": {
    "value": "Test"
  }
}

that returns the following error:

{
    "message": "Invalid request body.",
    "cause": "Illegal input: Unexpected JSON token at offset 8: Expected beginning of the string, but got { at path: $.name\nJSON input: {\"name\":{\"value\":\"Org 4\"},\"description\":{\"value\":\"Test\"}}"
}

This leads me to believe that the OpenAPI spec representation of OptionalValue does not match with what the API actually accepts.

Originally posted by @mmurto in https://github.com/eclipse-apoapsis/ort-server/issues/797#issuecomment-2288054766

[mmurto]

One thing to consider is whether JSON Patch would make sense for the API's patch operations.

[mnonnenmacher]

It could be that this is fixed by switching from schema-kenerator-reflection to schema-kenerator-serialization, I will give it a try.

[mmurto]

Though now that I read this through again, I think it may be the other way around, the OpenAPI spec is generated almost correctly: OptionalValue<String> contains Present<*> which has a property value with type of object, while it should probably be Present<String> with a nullable value: string.

But at least in my testing the API actually expected and accepted a payload of { "name": "string" }, so maybe there's a bug in the route handler and not the schema?

[mnonnenmacher]

The inconsistency should be cause by the fact that there is custom serializer for the OptionalValue class which reflection is not aware of. In JSON, the correct way to not update a value would be to simply not mention it, but in Kotlin we need the OptionalValue class to define the difference between null and not present.

[mmurto]

If we need to be able to set already present value to null, I believe we do need that for JSON as well.

[mnonnenmacher]

Yes, that's done by explicitly setting the value to null, for example, { name: null }.

[mnonnenmacher]

It could be that this is fixed by switching from schema-kenerator-reflection to schema-kenerator-serialization, I will give it a try.

schema-kenerator-serialization creates other problems in the generated schema because the SerialDescriptor from kotlinx.serialization does not provide information about generic type arguments, so I did #824 instead.

[mmurto]

The issue with new client generation is now fixed, but I think the spec is still incorrect. New spec for update:

      "UpdateOrganization" : {
        "title" : "UpdateOrganization",
        "type" : "object",
        "properties" : {
          "description" : {
            "title" : "String",
            "type" : "string"
          },
          "name" : {
            "title" : "String",
            "type" : "string"
          }
        }
      },

I believe the correct type for name and description should be type: ["null", "string"], see this SO answer.

[mmurto]

I believe the correct type for name and description should be type: ["null", "string"], see this SO answer.

Actually, name is probably correct as is, but description should be null | string

[mnonnenmacher]

This is a bug in schema-kenerator, I have created an issue: https://github.com/SMILEY4/schema-kenerator/issues/14