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.

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

OpenApi AnyOf with odata functions/actions generate invalid return types #990

Closed
andrueastman opened this issue Jan 3, 2022 · 5 comments · Fixed by #1396
Closed

OpenApi AnyOf with odata functions/actions generate invalid return types #990

andrueastman opened this issue Jan 3, 2022 · 5 comments · Fixed by #1396
Assignees
Labels
fixed generator Issues or improvements relater to generation capabilities. type:bug A broken experience

Comments

@andrueastman
Copy link
Member

andrueastman commented Jan 3, 2022

Taking a look at an OData action such as createUploadSession we have the schema as below.

  '/groups/{group-id}/calendar/events/{event-id}/attachments/microsoft.graph.createUploadSession':
    post:
      tags:
        - groups.Actions
      summary: Invoke action createUploadSession
      operationId: groups.group.calendar.events.event.attachments.createUploadSession
      parameters:
        - name: group-id
          in: path
          description: 'key: id of group'
          required: true
          schema:
            type: string
          x-ms-docs-key-type: group
        - name: event-id
          in: path
          description: 'key: id of event'
          required: true
          schema:
            type: string
          x-ms-docs-key-type: event
      requestBody:
        description: Action parameters
        content:
          application/json:
            schema:
              type: object
              properties:
                AttachmentItem:
                  $ref: '#/components/schemas/microsoft.graph.attachmentItem'
        required: true
      responses:
        '200':
          description: Success
          content:
            application/json:
              schema:
                anyOf:
                  - $ref: '#/components/schemas/microsoft.graph.uploadSession'
                nullable: true
        default:
          $ref: '#/components/responses/error'
      x-ms-docs-operation-type: action

From this schema we see the response schema has the anyOf attribute similar to other odata actions such as getSchedule etc.

Due to the code here, since the anyOf is at the top level (this doesn't seem to happen when the anyOf property is present in a class/type property such as in the requestBody parameters in the getSchedule ), the generator generates a union type which nests the desired type when the API will return the anyOf type on the top level of the json response.

This results in generating a request builder that returns a CreateUploadSessionResponse instance where the UploadSession type is a nested property rather than the requestBuilder returning the UploadSession. Ref here

Questions

  • Is it right/valid to have the anyOf when there is only one type present in the list?
  • If the above is valid, maybe we should look into flattening the properties in the anyOf list in these scenarios?

cc @baywet

@baywet baywet self-assigned this Jan 4, 2022
@baywet baywet added blocked This work can't be done until an external dependent work is done. type:bug A broken experience needs more information labels Jan 4, 2022
@baywet baywet added this to the TypeWriter Replacement milestone Jan 4, 2022
@baywet baywet added the generator Issues or improvements relater to generation capabilities. label Jan 4, 2022
@baywet
Copy link
Member

baywet commented Jan 4, 2022

Thanks for reporting that @andrueastman .
This is something I had already noticed and reported a couple of months in the conversion library, see microsoft/OpenAPI.NET.OData#119
It hasn't been addressed yet but it's part of the issue list I want to be fixed for Kiota.

Now that I'm re-reading the issue, it looks like it could be caused by the nullable property, which doesn't seem to be part of the original schema definition (i.e. upload session doesn't have nullable set).

With all that being said, I'm wondering what nullable "means" in our context, if we followed the strict definition, our models should be structs unless marked as nullable. But that'd be an issue in a lot of languages for inheritance (and since all entities inherit from Entity...)

Here is what we could do:

  • fix the conversion process to stop adding that nullable information/additional schema node (my original thought)
  • fix something else in Kiota on how we handle nullable
  • work the issue around to flatten the empty schema node (and lose the nullable information)
  • something else?

if solution 3 feels like a hack, I'm hesitant between 1 & 2 and I'd like @darrelmiller input with regards to what's the right thing to do here?

@baywet
Copy link
Member

baywet commented Jan 5, 2022

Update: from my investigation (see linked issue in previous comment) the behavior of the conversion library seems to be conforming to the OpenAPI and OData specifications. I think the only question for @darrelmiller is: how should we handle nullable and non-nullable properties/schemas in Kiota?

@darrelmiller
Copy link
Member

Sooo much to unpack.

JSON Schema allows schemas like say a property value can be either an object or null:

{ 
  "type": [ "object", null ]
}

OpenAPI prior to 3.1 does not allow JSON Schemas that look like this.
OpenAPI 3.0 introduced the nullable keyword to allow objects to be described as object or null. It was not a good idea. The nullable keyword will be going away in the future.

This case is a special case where the schema is being used to define the entire response body. I believe in this case the nullable keyword is indicating that the payload maybe empty. Not really null. Certainly not a payload that has the characters null in the body.
If this API might return an empty body, then the description should be,

      responses:
        '200':
          description: Success
          content:
            application/json:
              schema:
                  $ref: '#/components/schemas/microsoft.graph.uploadSession'
         '204': 
            description: no content

You can't have a 200 response with a content-type of application/json and then just have no body. That's not valid JSON. This feels like it is a OpenApi.Net.OData issue.

@RabebOthmani RabebOthmani moved this to Todo in Kiota Jan 24, 2022
@baywet
Copy link
Member

baywet commented Jan 24, 2022

but what about a property on upload session? (that'd be a nullable complex type)

@baywet baywet moved this from Todo to In Progress in Kiota Feb 18, 2022
@darrelmiller
Copy link
Member

@baywet Go squish this to return an UploadSession and not CreateUploadSessionResponse

                anyOf:
                  - $ref: '#/components/schemas/microsoft.graph.uploadSession'
                nullable: true

@baywet baywet removed the blocked This work can't be done until an external dependent work is done. label Feb 18, 2022
@baywet baywet moved this from In Progress to Todo in Kiota Feb 18, 2022
@baywet baywet moved this from Todo to In Progress in Kiota Mar 15, 2022
baywet added a commit that referenced this issue Mar 15, 2022
@baywet baywet added the fixed label Mar 15, 2022
baywet added a commit that referenced this issue Mar 15, 2022
- fixes #990 a bug where the generator would introduce unnecessary union types for nullables
Repository owner moved this from In Progress to Done in Kiota Mar 15, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
fixed generator Issues or improvements relater to generation capabilities. type:bug A broken experience
Projects
Archived in project
3 participants