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.

Sign up for GitHub

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

Jump to bottom

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

Open
andreaTP opened this issue Apr 2, 2024 · 11 comments · May be fixed by #4435
Open

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

andreaTP opened this issue Apr 2, 2024 · 11 comments · May be fixed by #4435
Assignees
@andreaTP
Labels
generator Issues or improvements relater to generation capabilities. type:bug A broken experience
Milestone
Backlog

Comments

@andreaTP
Copy link
Contributor

andreaTP commented Apr 2, 2024

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

I'll open a PR shortly with the initial findings.
This was referenced Apr 2, 2024
Closed
Draft
@geometrikal
Copy link

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)

@andrueastman andrueastman added this to the Kiota v1.14 milestone Apr 5, 2024
@andreaTP
Copy link
Contributor Author

andreaTP commented Apr 9, 2024

@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
Copy link

@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
Copy link
Contributor Author

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

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

@geometrikal
Copy link

@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
Copy link
Contributor Author

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
Copy link
Member

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
Copy link
Member

baywet commented Apr 22, 2024

@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 baywet modified the milestones: Kiota v1.14, Kiota v1.15 May 2, 2024
@pjmagee

This comment was marked as off-topic.

Sign in to view
@baywet
Copy link
Member

baywet commented May 17, 2024

@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

@baywet baywet added type:bug A broken experience generator Issues or improvements relater to generation capabilities. status:waiting-for-author-feedback Issue that we've responded but needs author feedback to close labels May 17, 2024
@andreaTP
Copy link
Contributor Author

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 🙏

@microsoft-github-policy-service microsoft-github-policy-service bot added Needs: Attention 👋 and removed status:waiting-for-author-feedback Issue that we've responded but needs author feedback to close labels May 20, 2024
@baywet baywet modified the milestones: Kiota v1.15, Backlog May 22, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees

@andreaTP andreaTP

Labels
generator Issues or improvements relater to generation capabilities. type:bug A broken experience
Projects
Kiota
Status: In Progress 🚧
Milestone
Backlog
Development

Successfully merging a pull request may close this issue.

[Python] Initial fixes using simple oneOfs with discriminator andreaTP/kiota
6 participants
@pjmagee @darrelmiller @geometrikal @andreaTP @andrueastman @baywet