kiota icon indicating copy to clipboard operation
kiota copied to clipboard

`allOf` - Properties not generated - C#

Open gadcam opened this issue 11 months ago • 10 comments

Hello, In the case of an allOf if there are properties directly written in an object along side a $ref the properties are not taken into account in the generated code. Moving up the properties outside the allOf fix the issue

for schema_name, schema in data['components']['schemas'].items():
    if 'allOf' in schema and schema['allOf']:
        all_of_properties = schema['allOf'][0].get('properties', {})
        schema.setdefault('properties', {}).update(all_of_properties)
        schema['allOf'] = [ref for ref in schema['allOf']
                           if isinstance(ref, dict) and '$ref' in ref]

Seems related to #4325 but not the same exact bug.

Here is an example to illustrate the bug

File 1 - Broken

---
openapi: 3.0.3
servers:
  - url: https://example.com
info:
  title: example
  version: 0.0.1
paths:
  /path:
    post:
      requestBody:
        content:
          application/json:
            schema:
              $ref: "#/components/schemas/ComponentOne"
      responses:
        "201":
          description: Created
          content:
            application/json:
              schema:
                type: string
components:
  schemas:
    ComponentOne:
      title: ComponentOne
      allOf:
        - type: object
          properties:
            propOne:
              type: string
        - $ref: "#/components/schemas/ComponentTwo"
    ComponentTwo:
      properties:
        anotherField:
          type: string

File 2 - Working

---
openapi: 3.0.3
servers:
  - url: https://example.com
info:
  title: example
  version: 0.0.1
paths:
  /path:
    post:
      requestBody:
        content:
          application/json:
            schema:
              $ref: "#/components/schemas/ComponentOne"
      responses:
        "201":
          description: Created
          content:
            application/json:
              schema:
                type: string
components:
  schemas:
    ComponentOne:
      title: ComponentOne
      type: object
      properties:
        propOne:
          type: string
      allOf:
        - $ref: "#/components/schemas/ComponentTwo"
    ComponentTwo:
      properties:
        anotherField:
          type: string

gadcam avatar Mar 15 '24 14:03 gadcam

Hi @gadcam, Thanks for using kiota and for reporting this. Can you confirm whether you might have inverted the broken and working example please? The reason I'm asking is because the descriptions for Microsoft Graph contain thousands of instances of your "broken" example" and we'd have caught it by now I assume. But we don't have instances of your "working" example.

baywet avatar Mar 18 '24 18:03 baywet

@baywet Bonjour Vincent, I am afraid that the samples were correctly labelled. (the Python fix I used can convince you they are)

the descriptions for Microsoft Graph contain thousands of instances of your "broken" example

Then I suppose the problem might involve a more complex structure. What can I do to provide a better minimal reproducible example ? Should I write a test ? If so where would be the correct place ? Thank you a lot for your help!

Here is another example which I just copy pasted from the files and redacted

  1. Before the Python script ran
AAA:
      title: AAA
      description: AAA
      allOf:
        - type: object
          properties:
            BBB:
              $ref: '#/components/schemas/DDD'
            CCC:
              $ref: '#/components/schemas/DDD'
        - $ref: '#/components/schemas/DDD'
  1. After the Python script ran
    AAA:
      allOf:
      - $ref: '#/components/schemas/DDD'
      description: AAA
      properties:
        CCC:
          $ref: '#/components/schemas/DDD'
        BBB:
          $ref: '#/components/schemas/DDD'
      title: AAA

So not exactly the previous Working example: the order is not exactly the same but the structure is the same.

gadcam avatar Mar 18 '24 22:03 gadcam

Thanks for the additional details here.

Just to confirm, I double checked the Microsoft Graph OAI descriptions and here is the general pattern employed for over 2000 models:

    microsoft.graph.user:
      allOf:
        - $ref: '#/components/schemas/microsoft.graph.directoryObject'
        - title: user
          required:
            - '@odata.type'
          type: object
          properties:
            aboutMe:
              type: string
              description: A freeform text entry field for the user to describe themselves. Returned only on $select.
              nullable: true

Which maps to your broken example (except for the order, but hopefully it's not just an ordering thing, worth checking for sanity though)

Tests would go here https://github.com/microsoft/kiota/blob/972c8f86932f5c079c502b97f3c91aa967d46d99/tests/Kiota.Builder.Tests/KiotaBuilderTests.cs#L7174 https://github.com/microsoft/kiota/blob/972c8f86932f5c079c502b97f3c91aa967d46d99/tests/Kiota.Builder.Tests/KiotaBuilderTests.cs#L7121

Duplicating them to account for the additional scenarios that are currently not tested for would probably be a good first step and might outline something I'm not seeing by just looking at the provided descriptions.

baywet avatar Mar 19 '24 16:03 baywet

it's not just an ordering thing

tested and looks like it's not an ordering thing, at least, in the absence of other edge cases kicking in ( e.g. #4325 )

andreaTP avatar Mar 20 '24 16:03 andreaTP

Started looking into this with #4381 which I believe will fix this one as well, but I need to design a unit test for it.

baywet avatar Mar 21 '24 18:03 baywet

update: I did manage to fix this unrecognized inheritance case, and remove a bunch of dependencies on schema titles which we didn't want to begin with. I did run into a regression with types trimming I'll need to look into further

baywet avatar Mar 21 '24 21:03 baywet

I have been attempting to reduce this again, this is what I have:

Common description:

openapi: 3.0.3
servers:
  - url: https://example.com
info:
  title: example
  version: 0.0.1
paths:
  /path:
    post:
      requestBody:
        content:
          application/json:
            schema:
              $ref: "#/components/schemas/ComponentList"
      responses:
        "201":
          description: Created
          content:
            application/json:
              schema:
                type: string
components:
  schemas:
    Component:
      type: object
      properties:
        prop:
          type: string
    ListMeta:
      type: object
      properties:
        size:
          type: number

Adding the definition of ComponentList:

Working:

    ComponentList:
      type: object
      properties:
        items:
          type: array
          items:
            $ref: "#/components/schemas/Component"
      allOf:
        - $ref: "#/components/schemas/ListMeta"

and working as well:

    ComponentList:
      type: object
      allOf:
        - $ref: "#/components/schemas/ListMeta"
        - type: object
          properties:
            items:
              type: array
              items:
                $ref: "#/components/schemas/Component"

results in the expected:

@dataclass
class ComponentList(ListMeta):
    # The items property
    items: Optional[List[Component]] = None

unfortunately switching the order of the allOfs components produces an unexpected result, the entity used by the endpoint becomes ListMeta and the Python hierarchy is mapped "the other way around":

    ComponentList:
      type: object
      allOf:
        - type: object
          properties:
            items:
              type: array
              items:
                $ref: "#/components/schemas/Component"
        - $ref: "#/components/schemas/ListMeta"
@dataclass
class ListMeta(ComponentList):
    # The size property
    size: Optional[float] = None

it's 100% reproducible with kiota 1.12.0 and using a command like:

kiota generate -l python -c PostsClient -n client -d list-issue.yaml -o tmp-list --clean-output --log-level Trace

andreaTP avatar Mar 22 '24 12:03 andreaTP

This issue has been automatically marked as stale because it has been marked as requiring author feedback but has not had any activity for 4 days. It will be closed if no further activity occurs within 3 days of this comment.

(just removed the tag so the bot leaves us alone on this one)

Update: I have paused work on this because of other priorities at the moment, and I'll be out the next two weeks.

baywet avatar Mar 26 '24 12:03 baywet

Thanks for sharing openly your plans @baywet , I have found a few more edge cases and will try to report with minimal reproducers along.

andreaTP avatar Mar 26 '24 13:03 andreaTP

part of what might be making a difference, and looking at #4074 is the presence/absence of type: object at different levels. We'll need to look into this further

baywet avatar May 02 '24 18:05 baywet

Short update: working on #4074 has fixed this as well in #4381 Still running validation on this PR, but things are looking promising so far!

baywet avatar May 07 '24 19:05 baywet