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

Add the ability to change the request or response body content type based on an annotation #405

Closed
baywet opened this issue Jul 20, 2023 · 9 comments · Fixed by #500
Closed
Assignees
Labels
priority:p2 Medium. Generally has a work-around and a smaller sub-set of customers is affected. SLA <=30 days status:needs-discussion An issue that requires more discussion internally before resolving type:enhancement Enhancement request targeting an existing experience.
Milestone

Comments

@baywet
Copy link
Member

baywet commented Jul 20, 2023

OneNote leverages multipart/form to create/update one note pages so the page, attachments and more can be pushed in a single call.

Currently the resulting OpenAPI description contains a basic JSON input/output for that endpoint which is incorrect.

We should:

  1. identify a capability annotation that could carry the information in the metadata
  2. if none exist, define a new one
  3. implement parsing it
  4. implement the change in projection based on the metadata
  5. add transforms in the metadata repo
@baywet baywet added type:enhancement Enhancement request targeting an existing experience. priority:p3 Nice to have. Customer impact is very minimal priority:p2 Medium. Generally has a work-around and a smaller sub-set of customers is affected. SLA <=30 days and removed priority:p3 Nice to have. Customer impact is very minimal labels Jul 20, 2023
@baywet
Copy link
Member Author

baywet commented Jul 24, 2023

Here is what the result should look like (for the request body) in the case of OneNote

openapi: 3.0.1
info:
  title: Example
  description: Example
  version: 1.0.1
servers:
  - url: https://example.org
paths:
  /directoryObject:
    post:
      requestBody:
        content:
          multipart/form-data:
            schema:
              type: object
              properties:
                id:
                  type: string
                  format: uuid
                address:
                  $ref: '#/components/schemas/address'
                profileImage:
                  type: string
                  format: binary
            encoding:
              id:
                contentType: text/plain
              address:
                contentType: application/json
              profileImage:
                contentType: image/png
        responses:
          '204':
            content:
              application/json:
                schema:
                  type: string
components:
  schemas:
    address:
      type: object
      properties:
        street:
          type: string
        city:
          type: string

@irvinesunday irvinesunday self-assigned this Aug 2, 2023
@irvinesunday
Copy link
Contributor

We can leverage the Org.OData.Core.V1.MediaType annotation, for example:

<EntityType Name="onenote" BaseType="graph.entity">
  <NavigationProperty Name="pages" Type="Collection(graph.onenotePage)" ContainsTarget="true">
    <Annotation Term="Org.OData.Core.V1.MediaType" String="application/xhtml+xml" />
  </NavigationProperty>
</EntityType>

We can then check for the above annotation here:

Constants.ApplicationJsonMediaType,
new OpenApiMediaType
{
Schema = schema
}

@irvinesunday irvinesunday added this to the OData: 1.5 milestone Aug 2, 2023
@baywet
Copy link
Member Author

baywet commented Aug 2, 2023

Thanks for the additional information.
How do you differentiate between the request and the response media type using that annotation? What if the media type is specific to an operation?

@irvinesunday irvinesunday modified the milestones: OData: 1.5, OData: 1.6 Dec 7, 2023
@irvinesunday
Copy link
Contributor

irvinesunday commented Jan 25, 2024

To address the potentially different MIME types required for different operations, I propose adding new properties to the below capability annotations for their respective methods:

1. PUT/PATCH scenarios:
I propose updating the UpdateRestrictionsType complex type by adding two new properties. These will be available through the UpdateRestrictions capability annotation.

      <ComplexType Name="UpdateRestrictionsType">
        <Property Name="AcceptableContentTypes" Type="Collection(Edm.String)" Nullable="false">
          <Annotation Term="Core.Description" String="Lists the MIME types acceptable for the request body content"/>
        </Property>
        <Property Name="AcceptableResponseTypes" Type="Collection(Edm.String)" Nullable="false">
          <Annotation Term="Core.Description" String="Lists the MIME types acceptable for the response returned by the request" />
        </Property>
  
        <!-- other properties -->

      </ComplexType>

2. POST method scenarios:
I propose updating the InsertRestrictionsType complex type by adding two new properties. These will be available through the InsertRestrictions capability annotation. Similar to the above proposal for UpdateRestrictionsType

      <ComplexType Name="InsertRestrictionsType">
        <Property Name="AcceptableContentTypes" Type="Collection(Edm.String)" Nullable="false">
          <Annotation Term="Core.Description" String="Lists the MIME types acceptable for the request body content"/>
        </Property>
        <Property Name="AcceptableResponseTypes" Type="Collection(Edm.String)" Nullable="false">
          <Annotation Term="Core.Description" String="Lists the MIME types acceptable for the response returned by the request" />
        </Property>
  
        <!-- other properties -->

      </ComplexType>

Thoughts? @darrelmiller @baywet @mikepizzo

@irvinesunday irvinesunday added the status:needs-discussion An issue that requires more discussion internally before resolving label Jan 26, 2024
@baywet
Copy link
Member Author

baywet commented Jan 26, 2024

Thanks for following up on this. I think we should make the annotations' names a bit more explicit, what do you think of:

  • AcceptableContentTypes -> RequestContentTypes
  • AcceptableResponseTypes -> ResponseContentTypes

@darrelmiller
Copy link
Member

I was going to suggest:

  • SupportedRequestTypes
  • AcceptableResponseTypes

but I think I like @baywet's suggestions better.

Have @garethj-msft and @MaximRouiller seen this proposal?

@MaximRouiller
Copy link
Member

We haven't. Reading this now. Give me some time to ingest the information.

@MaximRouiller
Copy link
Member

Alright, I have a few questions.

  • Are we trying to specify the exact MIME of the Edm.Stream?
  • I don't think setting the acceptable MIME type on the collection of pages make sense since the content of individual pages/resources of a oneNote payload could technically be different MIME types. I would see that definition belonging closer to the <Property Name="content" Type="Edm.Stream" /> property.

I might be completely off but I'm happy to jump on a call to clarify my viewpoint.

@irvinesunday
Copy link
Contributor

Alright, I have a few questions.

  • Are we trying to specify the exact MIME of the Edm.Stream?
  • I don't think setting the acceptable MIME type on the collection of pages make sense since the content of individual pages/resources of a oneNote payload could technically be different MIME types. I would see that definition belonging closer to the <Property Name="content" Type="Edm.Stream" /> property.

I might be completely off but I'm happy to jump on a call to clarify my viewpoint.

@MaximRouiller
We already have the annotation Org.OData.Core.V1.AcceptableMediaTypes that applies to Edm.Stream types:

image
doc source

And we already use it in our metadata, example:

image

However, this annotation doesn't specify the operations in which the MIME types are applicable, and hence they will be applied to all the operations in the generated path.

The problem we have at hand is specifying specific MIME types for specific operations, in cases where they could be different for the different operations.

To answer your questions:

  1. Yes, we are trying to specify the range of acceptable MIME types for a property that is not necessarily of type Edm.Stream.
  2. Changing the type will break the navigation property, pages
    image

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
priority:p2 Medium. Generally has a work-around and a smaller sub-set of customers is affected. SLA <=30 days status:needs-discussion An issue that requires more discussion internally before resolving type:enhancement Enhancement request targeting an existing experience.
Projects
None yet
4 participants