[Python] Generated code for simple oneOfs doesn't work

[andreaTP]

I'm minimizing some issues found when using oneOfs in OpenAPI descriptions.

I'll open a PR shortly with the initial findings.

[geometrikal]

I will add on to here

The openapi spec I'm trying to generate a client for has somethings like this:

    AnnotationsRead:
      oneOf:
      - $ref: '#/components/schemas/LabeledData'
      - type: string
        format: binary

Generated code for the discriminator is

@staticmethod
    def create_from_discriminator_value(parse_node: Optional[ParseNode] = None) -> AnnotationsRead:
        """
        Creates a new instance of the appropriate class based on discriminator value
        param parse_node: The parse node to use to read the discriminator value and create the object
        Returns: AnnotationsRead
        """
        if not parse_node:
            raise TypeError("parse_node cannot be null.")
        try:
            mapping_value = parse_node.get_child_node("").get_str_value()
        except AttributeError:
            mapping_value = None
        result = AnnotationsRead()
        if mapping_value and mapping_value.casefold() == "LabeledData".casefold():
            from .labeled_data import LabeledData

            result.annotations_read_labeled_data = LabeledData()
        elif mapping_value and mapping_value.casefold() == "LabeledData".casefold():
            from .labeled_data import LabeledData

            result.annotations_read_labeled_data0 = LabeledData()
        elif mapping_value and mapping_value.casefold() == "LabeledData".casefold():
            from .labeled_data import LabeledData

            result.labeled_data = LabeledData()
        elif annotations_read_string_value := parse_node.get_str_value():
            result.annotations_read_string = annotations_read_string_value
        elif annotations_read_string0_value := parse_node.get_str_value():
            result.annotations_read_string0 = annotations_read_string0_value
        elif string_value := parse_node.get_str_value():
            result.string = string_value
        return result

which fails at parse_node.get_child_node("") with ValueError: identifier cannot be None or empty. Looks like there are some repeated values as referenced in another issue.

(C# generated code also stumbles with this)

[andreaTP]

@geometrikal I published a pre-release version from my PR:
https://github.com/andreaTP/kiota-prerelease/releases/tag/v0.0.0-pre%2BandreaTP.initial-fixes-simple-oneOf-py.b46369a

it would be great if you could test my changes from there or rebuilding kiota from my branch.

[geometrikal]

@andreaTP no it still fails, although the repeated fields are not longer there.

I extended one of your test yamls to show the bug:

openapi: 3.0.0
info:
  title: "Derived Types API"
  version: "1.0.0"
servers:
  - url: https://example.org/
paths:
  /fruits: # this method will not downcast to OpenAPI v2 because oneOf is not supported
    get:
      responses:
        200:
          description: ok
          content:
            application/json:
                # The code generator will need to be clever and instead of generating a fruitResponse class
                # with a property for each of the properties, it needs to detect that apple and orange derive from fruit.
                # It can then declare the requestExecutors as returning the base type.
              schema:
                oneOf:
                - $ref: "#/components/schemas/fruit"  # Allowing the base class allows enables evolvabilty
                - $ref: "#/components/schemas/apple"
                - $ref: "#/components/schemas/orange"
  /fruitsWithDiscriminator:
    get:
      responses:
        200:
          description: ok
          content:
            application/json:
              schema:
                discriminator:
                  propertyName: fruitType  # This only works if fruitType has the exact schema name
                allOf:
                  - $ref: "#/components/schemas/fruit"  # Allowing the base class allows enables evolvabilty
  /fruitsWithDiscriminatorWithMapping:
    get:
      responses:
        200:
          description: ok
          content:
            application/json:
              schema:
                discriminator:
                  propertyName: fruitType
                  mapping:          # If mapping doesn't exist, then fallback to base type'
                    apple: '#/components/schemas/apple'
                    orange: '#/components/schemas/orange'
                allOf:
                  - $ref: "#/components/schemas/fruit"  # Allowing the base class allows enables evolvabilty
  /fruitsWithOneOfComponent:
    get:
      responses:
        200:
          description: ok
          content:
            application/json:
              schema:
                $ref: "#/components/schemas/oneOfFruit"

components:
  schemas:
    fruit:
      type: object
      title: fruit  # required temporarily due to a bug in Kiota codemodel
      properties:
        name:
          type: string
        fruitType:
          type: string
    oneOfFruit:
      oneOf:
        - $ref: '#/components/schemas/apple'
        - $ref: '#/components/schemas/orange'
    apple:
      allOf:
        - $ref: '#/components/schemas/fruit'
      type: object
      title: apple
      properties:
         edible:
           type: boolean
         fruitType:
           x-const: apple  # the const keyword is only supported int OpenAPI 3.1
    orange:
      allOf:
        - $ref: '#/components/schemas/fruit'
      type: object
      title: orange
      properties:
         seedless:
           type: boolean
         fruitType:
           x-const: orange

which creates this model with the offending get_child_node("")

from __future__ import annotations
from dataclasses import dataclass, field
from kiota_abstractions.serialization import ComposedTypeWrapper, Parsable, ParseNode, SerializationWriter
from typing import Any, Callable, Dict, List, Optional, TYPE_CHECKING, Union

if TYPE_CHECKING:
    from .apple import Apple
    from .orange import Orange

@dataclass
class OneOfFruit(ComposedTypeWrapper, Parsable):
    """
    Composed type wrapper for classes Apple, Orange
    """
    @staticmethod
    def create_from_discriminator_value(parse_node: Optional[ParseNode] = None) -> OneOfFruit:
        """
        Creates a new instance of the appropriate class based on discriminator value
        param parse_node: The parse node to use to read the discriminator value and create the object
        Returns: OneOfFruit
        """
        if not parse_node:
            raise TypeError("parse_node cannot be null.")
        try:
            mapping_value = parse_node.get_child_node("").get_str_value()
        except AttributeError:
            mapping_value = None
        result = OneOfFruit()
        if mapping_value and mapping_value.casefold() == "apple".casefold():
            from .apple import Apple

            result.apple = Apple()
        elif mapping_value and mapping_value.casefold() == "orange".casefold():
            from .orange import Orange

            result.orange = Orange()
        return result
    
    def get_field_deserializers(self,) -> Dict[str, Callable[[ParseNode], None]]:
        """
        The deserialization information for the current model
        Returns: Dict[str, Callable[[ParseNode], None]]
        """
        from .apple import Apple
        from .orange import Orange

        if hasattr(self, "apple"):
            return self.apple.get_field_deserializers()
        if hasattr(self, "orange"):
            return self.orange.get_field_deserializers()
        return {}
    
    def serialize(self,writer: SerializationWriter) -> None:
        """
        Serializes information the current object
        param writer: Serialization writer to use to serialize this model
        Returns: None
        """
        if not writer:
            raise TypeError("writer cannot be null.")
        if hasattr(self, "apple"):
            writer.write_object_value(None, self.apple)
        elif hasattr(self, "orange"):
            writer.write_object_value(None, self.orange)

[andreaTP]

@geometrikal I see a few additional edge cases being triggered by your description:

• /fruits / response doesn't have a discriminator, this means that the resulting type is undecidable
• fruitType JSONSchema doesn't specify the behavior when you are "merging" fields with clashing names
• x-constdoesn't seem to be supported afaict: https://github.com/search?q=repo%3Amicrosoft%2FOpenAPI.NET+%22x-const%22&type=code

I would reduce/bisect the invariants before moving on in the analysis of your description if possible.

[geometrikal]

@andreaTP I stole the yaml from here: https://github.com/microsoft/kiota/blob/main/tests/Kiota.Builder.IntegrationTests/ModelWithDerivedTypes.yaml and just added the fruitOneOf case.

The actual specification I'm trying to generate for is v261 of cvat https://github.com/cvat-ai/cvat which uses a few components that just consist of a oneOf with two types.

[andreaTP]

Thanks for the context!
Wow, I haven't noticed that some of those patterns are expected to be supported, we should probably create IT tests for all of the languages as we can easily find runtime bugs in the handling.

Waiting for guidance from @baywet or @darrelmiller here

[darrelmiller]

Notes from our community call on 2024-04-12 on this topic.

openapi: 3.1.0

components:
  schemas:
    baseObject:
      type: object
      properties:
        id:
          type: string
        name:
          type: string
        complex:
          type: object
          properties:
            a:
              type: string
            b:
              type: string
            c:
              type: string
    derivedObject:
      allOf:
        - $ref: '#/components/schemas/baseObject'
        - type: object
          properties:
            derivedProperty:
              type: string
            derivedComplex:
              $id: '/complex'
              type: object
              properties:
                d:
                  type: string
                e:
                  type: string
                f:
                  type: string
    derivedObject2:
      allOf:
        - $ref: '#/components/schemas/baseObject'
        - type: object
          properties:
            derivedProperty2:
              type: string
            derivedComplex:
               type: object
               properties:
                 g:
                   type: string
                 h:
                   type: string
                 i:
                   type: string

    diamondObject:
      type: object
      properties:   # Processed first
        derivedComplex:
          type: number
      allOf:
        - type: object  # Processed second
          properties:
            derivedComplex:
              type: number
        - $ref: '#/components/schemas/derivedObject'
        - $ref: '#/components/schemas/derivedObject2'

The $id value in JSON Schema 2020-12 will make this problem much easier to solve because schemas will have an identity. While waiting for $id support in OpenAPI 3.1 and the v2 library we came to this proposal during the meeting.

Duplicate properties targeting the merged type will use the first defined schema for the property based on the array order of the allOf.

• clarification 1 : inline schemas in the allOf should follow same rules as referenced schemas
• clarification 2 : if schema of the parent to the allOf contains a duplicate property then it is prioritized
• Issue a warning during generation for duplicate properties

[baywet]

@darrelmiller did you mean to add this response to another issue? because the whole thread is reference to oneOf and this last reply refers to allOf? or maybe I missing something from the meeting?

[baywet]

@andreaTP @geometrikal could you try your scenarios again with the latest preview from yesterday and confirm whether you observe the problem? We've made significant improvements to the handling of allof edge scenarios with #4668 and #4381

[andreaTP]

Sorry for the delay here @baywet , I can confirm that the resent improvements on allOf are not either positively nor negatively affecting this issue.

This is my reproducer triggered in the it tests:
https://github.com/microsoft/kiota/compare/main...andreaTP:kiota:discriminator-test?expand=1

and here you can find the failure:
https://github.com/andreaTP/kiota/actions/runs/9157492834/job/25174024297#step:14:342

I hope this helps 🙏

[andrueastman]

@andreaTP I believe the issue with the duplicate/superfluous field was resolved with the PR at #5657.

I've added the intergration tests you shared at https://github.com/microsoft/kiota/compare/main...andreaTP:kiota:discriminator-test?expand=1 in the PR #5742 to add the validation to the suite.

Let us know if anything is missed there. Otherwise, I believe we can close this this once that is merged.