Skip to content

Conversation

@newtork
Copy link
Contributor

@newtork newtork commented Jan 3, 2025

Context

https://github.tools.sap/AI/ai-sdk-java-backlog/issues/129

Enable this kind of specification:

components:
  schema:
    Fanta:
      type: object
      properties:
        sodaType:
          type: string
+       flavor:
+         oneOf:
+           - type: string
+           - type: integer
+           - type: array
+             items:
+               type: string
+           - type: object
+             properties:
+               foo: string
+               bar: integer

to generate the code that compiles and allows for deserialization:

public interface FantaFlavor
{
    record InnerString( String value) implements FantaFlavor {}
    @JsonCreator
    static InnerString create( String val )
    {
        return new InnerString(val);
    }

    record InnerInteger( Integer value) implements FantaFlavor {}
    @JsonCreator
    static InnerInteger create( Integer val )
    {
        return new InnerInteger(val);
    }

    record InnerFantaFlavorOneOf( FantaFlavorOneOf value) implements FantaFlavor {}
    @JsonCreator
    static InnerFantaFlavorOneOf create( FantaFlavorOneOf val )
    {
        return new InnerFantaFlavorOneOf(val);
    }
}

Known limitations:

  • Deserialization may fail for more than 1 non-primitive option. Jackson is unable to determine the JsonCreator.

Feature scope:

Add feature toggle

  • useOneOfCreators

Definition of Done

  • Functionality scope stated & covered
  • Tests cover the scope above
  • Error handling created / updated & covered by the tests above
  • Documentation updated
  • Release notes updated (internal feature toggle)

@newtork newtork marked this pull request as draft January 3, 2025 10:17
@newtork newtork marked this pull request as ready for review January 7, 2025 10:31
@newtork newtork changed the title [OpenAPI] Improve oneof feat(openapi-generator): Allow for primitives in anyOf/oneOf component schema definitions Jan 10, 2025
@newtork newtork added please merge Request to merge a pull request please review Request to review a pull request labels Jan 13, 2025
Copy link
Contributor

@CharlesDuboisSAP CharlesDuboisSAP left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

<useOneOfCreators>true</useOneOfCreators>
<enumUnknownDefaultCase>true</enumUnknownDefaultCase>
</additionalProperties>
<enableOneOfAnyOfGeneration>true</enableOneOfAnyOfGeneration>
Copy link
Contributor

@CharlesDuboisSAP CharlesDuboisSAP Jan 14, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why were the other classes (OneOf, AnyOf) not affected by this change?
Why wasn't it there in the first place?

Copy link
Contributor Author

@newtork newtork Jan 29, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Omg, I went deep into the rabbit hole. I found that our check for (unsupported) anyof / oneof was not sufficient!

  • oneOf and anyOf are not checked when they are part of schema root; only when they are deeper in object hierarchy, e.g. when being part of properties fields

See #checkForValidatorsInSchema(JsonNode)

image

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think this was intentional, because on the root level the generation was already working for some cases. IIRC

# Conflicts:
#	datamodel/openapi/openapi-api-sample/src/test/java/com/sap/cloud/sdk/datamodel/openapi/sample/api/OneOfDeserializationTest.java
#	datamodel/openapi/openapi-generator/src/main/resources/openapi-generator/mustache-templates/oneof_interface.mustache
@newtork newtork merged commit 4f04d09 into main Jan 29, 2025
14 checks passed
@newtork newtork deleted the oneof branch January 29, 2025 13:40
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

please merge Request to merge a pull request please review Request to review a pull request

Projects

None yet

Development

Successfully merging this pull request may close these issues.

5 participants