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

.Net: fix: distincts parameters before building the OAS function #9840

Closed
wants to merge 2 commits into from
Closed

.Net: fix: distincts parameters before building the OAS function #9840

Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
e0373aa
fix: distincts parameters before building the OAS function
baywet Nov 28, 2024
4c76dfb
fix: netstandard2 compatibility
baywet Nov 28, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
6 changes: 6 additions & 0 deletions dotnet/src/Functions/Functions.OpenApi/OpenApiKernelPluginFactory.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -300,6 +300,12 @@ async Task<RestApiOperationResponse> ExecuteAsync(Kernel kernel, KernelFunction
ParameterType = ConvertParameterDataType(p),
Schema = p.Schema ?? (p.Type is null ? null : KernelJsonSchema.Parse($$"""{"type":"{{p.Type}}"}""")),
})
#if NET6_0_OR_GREATER
.DistinctBy(static p => p.Name, StringComparer.Ordinal)
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

OpenAPI specification allows for non-unique parameters in different parts of a REST request. For example, there may be operations where the parameter Id represents a user id in the path and a product id in the query string. It's also possible for the Id parameter in different parts of the request to represent a single domain concept, such as a user id.

SK does not have enough information to distinguish between these different uses of the parameters with the same name. To address this issue, SK provides a customization mechanism that allows developers to handle such cases: HandleOpenApiDocumentHavingTwoParametersWithSameName....

Using only the first occurrence of a non-unique parameter may not be safe and can lead to data corruption, in cases when each instance represents a property of a different domain entity.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Could we do distinct but composite key instead? Which would be name and location?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The only reason, as far as I understand, to do so would be to get one parameter out of many with the same name in one location. I wonder if it's a valid scenario at all or just an OpenAPI document design/generation bug. Why would anyone have more than one parameter with the same name in a path, query string, header, etc.? And if we decide to do a distinct on the name-location composite key, why should we get the first parameter and not the second one? What if those non-unique parameters in the same location differ by schema, description, required, style, explode, and other properties of the Parameter Object? Which one should SK pick?

Scenarios like that should be handled using the SK OpenAPI customization capabilities and not by SK, because SK does not have enough information to make the right decision.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Here is a scenario that breaks that assumption.
POST operation, with a graph message, which has an odata type property. It also has a body property, which itself has an odata type property.
Due to how the request body fields and subfields are mapped today, we end up with two odata type parameters in this collection.

The true fix would be to map the parameters with different names to reflect the nesting, but I'm not there yet in terms of how much overall of this code I can perform. Both in terms of mental mapping of the code base but also in terms of confidence a large work in this section will get accepted by the repository maintainers. (c.f. my previous PRs)

This is just an incremental step to:

  • get better results.
  • unblock scenarios until we have a better implementation.

I'm happy to put the distinct behind a configuration switch if guidance is provided. Also happy to tweak the distinct to use a composite key so we don't end up removing things with the same name from different places as mentioned earlier. I don't think this fix should be done outside of this method (e.g. proxy/decorator) since it ALL OpenAPI based functions.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can you please provide an example of an OpenAPI document with such a POST operation? Have you disabled dynamic payload construction by setting the OpenApiFunctionExecutionParameters.EnableDynamicPayload parameter to false? Have you tried to provide unique parameter names/arguments for non-unique parameters via customization, as suggested above and demonstrated by this sample - HandleOpenApiDocumentHavingTwoParametersWithSameNameButRelatedToDifferentEntitiesAsync?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Disabling dynamic payload generation: no, we'd like to conserve/improve this mechanism at this point.
Custom payload: for the same reason I did not explore this avenue, requiring customers to build the payload themselves when they are providing an OAS description defeats the point.
Example: there it is: observe the presence of odata type properties at multiple levels of the object tree.

openapi: 3.0.1
info:
  title: OData Service for namespace microsoft.graph - Subset
  description: This OData service is located at https://graph.microsoft.com/v1.0
  version: v1.0
servers:
  - url: https://graph.microsoft.com/v1.0
paths:
  /me/sendMail:
    post:
      tags:
        - me.user
      summary: Invoke action sendMail
      description: 'Send the message specified in the request body using either JSON or MIME format. When using JSON format, you can include a file attachment in the same sendMail action call. When using MIME format: This method saves the message in the Sent Items folder. Alternatively, create a draft message to send later. To learn more about the steps involved in the backend before a mail is delivered to recipients, see here.'
      operationId: me_sendMail
      requestBody:
        $ref: '#/components/requestBodies/sendMailRequestBody'
      responses:
        '204':
          description: Success
components:
  schemas:
    microsoft.graph.message:
      title: message
      required:
        - '@odata.type'
      type: object
      properties:
        id:
          type: string
          description: The unique identifier for an entity. Read-only.
        '@odata.type':
          type: string
        categories:
          type: array
          items:
            type: string
            nullable: true
          description: The categories associated with the item
        changeKey:
          type: string
          description: 'Identifies the version of the item. Every time the item is changed, changeKey changes as well. This allows Exchange to apply changes to the correct version of the object. Read-only.'
          nullable: true
        createdDateTime:
          pattern: '^[0-9]{4,}-(0[1-9]|1[012])-(0[1-9]|[12][0-9]|3[01])T([01][0-9]|2[0-3]):[0-5][0-9]:[0-5][0-9]([.][0-9]{1,12})?(Z|[+-][0-9][0-9]:[0-9][0-9])$'
          type: string
          description: 'The Timestamp type represents date and time information using ISO 8601 format and is always in UTC time. For example, midnight UTC on Jan 1, 2014 is 2014-01-01T00:00:00Z'
          format: date-time
          nullable: true
        lastModifiedDateTime:
          pattern: '^[0-9]{4,}-(0[1-9]|1[012])-(0[1-9]|[12][0-9]|3[01])T([01][0-9]|2[0-3]):[0-5][0-9]:[0-5][0-9]([.][0-9]{1,12})?(Z|[+-][0-9][0-9]:[0-9][0-9])$'
          type: string
          description: 'The Timestamp type represents date and time information using ISO 8601 format and is always in UTC time. For example, midnight UTC on Jan 1, 2014 is 2014-01-01T00:00:00Z'
          format: date-time
          nullable: true
        bccRecipients:
          type: array
          items:
            $ref: '#/components/schemas/microsoft.graph.recipient'
          description: 'The Bcc: recipients for the message.'
        body:
          $ref: '#/components/schemas/microsoft.graph.itemBody'
        bodyPreview:
          type: string
          description: The first 255 characters of the message body. It is in text format.
          nullable: true
        ccRecipients:
          type: array
          items:
            $ref: '#/components/schemas/microsoft.graph.recipient'
          description: 'The Cc: recipients for the message.'
        conversationId:
          type: string
          description: The ID of the conversation the email belongs to.
          nullable: true
        conversationIndex:
          type: string
          description: Indicates the position of the message within the conversation.
          format: base64url
          nullable: true
        flag:
          $ref: '#/components/schemas/microsoft.graph.followupFlag'
        from:
          $ref: '#/components/schemas/microsoft.graph.recipient'
        hasAttachments:
          type: boolean
          description: 'Indicates whether the message has attachments. This property doesn''t include inline attachments, so if a message contains only inline attachments, this property is false. To verify the existence of inline attachments, parse the body property to look for a src attribute, such as <IMG src=''cid:image001.jpg@01D26CD8.6C05F070''>.'
          nullable: true
        importance:
          $ref: '#/components/schemas/microsoft.graph.importance'
        inferenceClassification:
          $ref: '#/components/schemas/microsoft.graph.inferenceClassificationType'
        internetMessageHeaders:
          type: array
          items:
            $ref: '#/components/schemas/microsoft.graph.internetMessageHeader'
          description: A collection of message headers defined by RFC5322. The set includes message headers indicating the network path taken by a message from the sender to the recipient. It can also contain custom message headers that hold app data for the message.  Returned only on applying a $select query option. Read-only.
        internetMessageId:
          type: string
          description: The message ID in the format specified by RFC2822.
          nullable: true
        isDeliveryReceiptRequested:
          type: boolean
          description: Indicates whether a read receipt is requested for the message.
          nullable: true
        isDraft:
          type: boolean
          description: Indicates whether the message is a draft. A message is a draft if it hasn't been sent yet.
          nullable: true
        isRead:
          type: boolean
          description: Indicates whether the message has been read.
          nullable: true
        isReadReceiptRequested:
          type: boolean
          description: Indicates whether a read receipt is requested for the message.
          nullable: true
        parentFolderId:
          type: string
          description: The unique identifier for the message's parent mailFolder.
          nullable: true
        receivedDateTime:
          pattern: '^[0-9]{4,}-(0[1-9]|1[012])-(0[1-9]|[12][0-9]|3[01])T([01][0-9]|2[0-3]):[0-5][0-9]:[0-5][0-9]([.][0-9]{1,12})?(Z|[+-][0-9][0-9]:[0-9][0-9])$'
          type: string
          description: 'The date and time the message was received.  The date and time information uses ISO 8601 format and is always in UTC time. For example, midnight UTC on Jan 1, 2014 is 2014-01-01T00:00:00Z.'
          format: date-time
          nullable: true
        replyTo:
          type: array
          items:
            $ref: '#/components/schemas/microsoft.graph.recipient'
          description: The email addresses to use when replying.
        sender:
          $ref: '#/components/schemas/microsoft.graph.recipient'
        sentDateTime:
          pattern: '^[0-9]{4,}-(0[1-9]|1[012])-(0[1-9]|[12][0-9]|3[01])T([01][0-9]|2[0-3]):[0-5][0-9]:[0-5][0-9]([.][0-9]{1,12})?(Z|[+-][0-9][0-9]:[0-9][0-9])$'
          type: string
          description: 'The date and time the message was sent.  The date and time information uses ISO 8601 format and is always in UTC time. For example, midnight UTC on Jan 1, 2014 is 2014-01-01T00:00:00Z.'
          format: date-time
          nullable: true
        subject:
          type: string
          description: The subject of the message.
          nullable: true
        toRecipients:
          type: array
          items:
            $ref: '#/components/schemas/microsoft.graph.recipient'
          description: 'The To: recipients for the message.'
        uniqueBody:
          $ref: '#/components/schemas/microsoft.graph.itemBody'
        webLink:
          type: string
          description: 'The URL to open the message in Outlook on the web.You can append an ispopout argument to the end of the URL to change how the message is displayed. If ispopout is not present or if it is set to 1, then the message is shown in a popout window. If ispopout is set to 0, the browser shows the message in the Outlook on the web review pane.The message opens in the browser if you are signed in to your mailbox via Outlook on the web. You are prompted to sign in if you are not already signed in with the browser.This URL cannot be accessed from within an iFrame.'
          nullable: true
        attachments:
          type: array
          items:
            $ref: '#/components/schemas/microsoft.graph.attachment'
          description: The fileAttachment and itemAttachment attachments for the message.
        extensions:
          type: array
          items:
            $ref: '#/components/schemas/microsoft.graph.extension'
          description: The collection of open extensions defined for the message. Nullable.
        multiValueExtendedProperties:
          type: array
          items:
            $ref: '#/components/schemas/microsoft.graph.multiValueLegacyExtendedProperty'
          description: The collection of multi-value extended properties defined for the message. Nullable.
        singleValueExtendedProperties:
          type: array
          items:
            $ref: '#/components/schemas/microsoft.graph.singleValueLegacyExtendedProperty'
          description: The collection of single-value extended properties defined for the message. Nullable.
    microsoft.graph.recipient:
      title: recipient
      required:
        - '@odata.type'
      type: object
      properties:
        emailAddress:
          $ref: '#/components/schemas/microsoft.graph.emailAddress'
        '@odata.type':
          type: string
      discriminator:
        propertyName: '@odata.type'
    microsoft.graph.itemBody:
      title: itemBody
      required:
        - '@odata.type'
      type: object
      properties:
        content:
          type: string
          description: The content of the item.
          nullable: true
        contentType:
          $ref: '#/components/schemas/microsoft.graph.bodyType'
        '@odata.type':
          type: string
      description: The body of the message. It can be in HTML or text format. Find out about safe HTML in a message body.
    microsoft.graph.followupFlag:
      title: followupFlag
      required:
        - '@odata.type'
      type: object
      properties:
        completedDateTime:
          $ref: '#/components/schemas/microsoft.graph.dateTimeTimeZone'
        dueDateTime:
          $ref: '#/components/schemas/microsoft.graph.dateTimeTimeZone'
        flagStatus:
          $ref: '#/components/schemas/microsoft.graph.followupFlagStatus'
        startDateTime:
          $ref: '#/components/schemas/microsoft.graph.dateTimeTimeZone'
        '@odata.type':
          type: string
      description: 'The flag value that indicates the status, start date, due date, or completion date for the message.'
    microsoft.graph.importance:
      title: importance
      enum:
        - low
        - normal
        - high
      type: string
      description: 'The importance of the message. The possible values are: low, normal, and high.'
    microsoft.graph.inferenceClassificationType:
      title: inferenceClassificationType
      enum:
        - focused
        - other
      type: string
      description: 'The classification of the message for the user, based on inferred relevance or importance, or on an explicit override. The possible values are: focused or other.'
    microsoft.graph.internetMessageHeader:
      title: internetMessageHeader
      required:
        - '@odata.type'
      type: object
      properties:
        name:
          type: string
          description: Represents the key in a key-value pair.
          nullable: true
        value:
          type: string
          description: The value in a key-value pair.
          nullable: true
        '@odata.type':
          type: string
    microsoft.graph.attachment:
      title: attachment
      required:
        - '@odata.type'
      type: object
      properties:
        id:
          type: string
          description: The unique identifier for an entity. Read-only.
        '@odata.type':
          type: string
        contentType:
          type: string
          description: The MIME type.
          nullable: true
        isInline:
          type: boolean
          description: 'true if the attachment is an inline attachment; otherwise, false.'
        lastModifiedDateTime:
          pattern: '^[0-9]{4,}-(0[1-9]|1[012])-(0[1-9]|[12][0-9]|3[01])T([01][0-9]|2[0-3]):[0-5][0-9]:[0-5][0-9]([.][0-9]{1,12})?(Z|[+-][0-9][0-9]:[0-9][0-9])$'
          type: string
          description: 'The Timestamp type represents date and time information using ISO 8601 format and is always in UTC time. For example, midnight UTC on Jan 1, 2014 is 2014-01-01T00:00:00Z'
          format: date-time
          nullable: true
        name:
          type: string
          description: The attachment's file name.
          nullable: true
        size:
          maximum: 2147483647
          minimum: -2147483648
          type: number
          description: The length of the attachment in bytes.
          format: int32
    microsoft.graph.extension:
      title: extension
      required:
        - '@odata.type'
      type: object
      properties:
        id:
          type: string
          description: The unique identifier for an entity. Read-only.
        '@odata.type':
          type: string
    microsoft.graph.multiValueLegacyExtendedProperty:
      title: multiValueLegacyExtendedProperty
      required:
        - '@odata.type'
      type: object
      properties:
        id:
          type: string
          description: The unique identifier for an entity. Read-only.
        '@odata.type':
          type: string
        value:
          type: array
          items:
            type: string
            nullable: true
          description: A collection of property values.
    microsoft.graph.singleValueLegacyExtendedProperty:
      title: singleValueLegacyExtendedProperty
      required:
        - '@odata.type'
      type: object
      properties:
        id:
          type: string
          description: The unique identifier for an entity. Read-only.
        '@odata.type':
          type: string
        value:
          type: string
          description: A property value.
          nullable: true
    microsoft.graph.emailAddress:
      title: emailAddress
      required:
        - '@odata.type'
      type: object
      properties:
        address:
          type: string
          description: The email address of the person or entity.
          nullable: true
        name:
          type: string
          description: The display name of the person or entity.
          nullable: true
        '@odata.type':
          type: string
      description: The recipient's email address.
    microsoft.graph.bodyType:
      title: bodyType
      enum:
        - text
        - html
      type: string
      description: The type of the content. Possible values are text and html.
    microsoft.graph.dateTimeTimeZone:
      title: dateTimeTimeZone
      required:
        - '@odata.type'
      type: object
      properties:
        dateTime:
          type: string
          description: 'A single point of time in a combined date and time representation ({date}T{time}; for example, 2017-08-29T04:00:00.0000000).'
        timeZone:
          type: string
          description: 'Represents a time zone, for example, ''Pacific Standard Time''. See below for more possible values.'
          nullable: true
        '@odata.type':
          type: string
      description: The date and time that the follow-up was finished.
    microsoft.graph.followupFlagStatus:
      title: followupFlagStatus
      enum:
        - notFlagged
        - complete
        - flagged
      type: string
      description: 'The status for follow-up for an item. Possible values are notFlagged, complete, and flagged.'
  requestBodies:
    sendMailRequestBody:
      description: Action parameters
      content:
        application/json:
          schema:
            type: object
            properties:
              Message:
                $ref: '#/components/schemas/microsoft.graph.message'
              SaveToSentItems:
                type: boolean
                default: false
                nullable: true
      required: true

#else
.GroupBy(p => p.Name, StringComparer.Ordinal)
.Select(g => g.First())
#endif
.ToList();

var returnParameter = operation.GetDefaultReturnParameter();
Expand Down
Loading