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

Required parameters in a method are used across HTTP methods executor. #4148

Closed
andrueastman opened this issue Feb 7, 2024 · 2 comments · Fixed by #4154
Closed

Required parameters in a method are used across HTTP methods executor. #4148

andrueastman opened this issue Feb 7, 2024 · 2 comments · Fixed by #4154
Assignees
Labels
generator Issues or improvements relater to generation capabilities. type:bug A broken experience WIP
Milestone

Comments

@andrueastman
Copy link
Member

Follow up to #3989

Taking a look at the url template
{+baseurl}/groups/{group%2Did}/members/$ref?@id={%40id}{&%24count,%24filter,%24orderby,%24search,%24skip,%24top} generated from the description below.

  '/groups/{group-id}/members/$ref':
    description: Provides operations to manage the collection of group entities.
    get:
      tags:
        - groups.directoryObject
      summary: Get ref of members from groups
      description: 'The members of this group, who can be users, devices, other groups, or service principals. Supports the List members, Add member, and Remove member operations. Nullable. Supports $expand including nested $select. For example, /groups?$filter=startsWith(displayName,''Role'')&$select=id,displayName&$expand=members($select=id,userPrincipalName,displayName).'
      externalDocs:
        description: Find more info here
        url: https://learn.microsoft.com/graph/api/group-list-members?view=graph-rest-1.0
      operationId: groups.ListRefMembers
      parameters:
        - name: ConsistencyLevel
          in: header
          description: 'Indicates the requested consistency level. Documentation URL: https://docs.microsoft.com/graph/aad-advanced-queries'
          schema:
            type: string
          examples:
            example-1:
              description: $search and $count queries require the client to set the ConsistencyLevel HTTP header to 'eventual'.
              value: eventual
        - $ref: '#/components/parameters/top'
        - $ref: '#/components/parameters/skip'
        - $ref: '#/components/parameters/search'
        - $ref: '#/components/parameters/filter'
        - $ref: '#/components/parameters/count'
        - name: $orderby
          in: query
          description: Order items by property values
          style: form
          explode: false
          schema:
            uniqueItems: true
            type: array
            items:
              enum:
                - id
                - id desc
                - deletedDateTime
                - deletedDateTime desc
              type: string
      responses:
        2XX:
          $ref: '#/components/responses/StringCollectionResponse'
        4XX:
          $ref: '#/components/responses/error'
        5XX:
          $ref: '#/components/responses/error'
      x-ms-pageable:
        nextLinkName: '@odata.nextLink'
        operationName: listMore
      x-ms-docs-operation-type: operation
    post:
      tags:
        - groups.directoryObject
      summary: Add members
      description: Add a member to a security or Microsoft 365 group through the members navigation property. The following table shows the types of members that can be added to either security groups or Microsoft 365 groups.
      externalDocs:
        description: Find more info here
        url: https://learn.microsoft.com/graph/api/group-post-members?view=graph-rest-1.0
      operationId: groups.CreateRefMembers
      requestBody:
        $ref: '#/components/requestBodies/refPostBody'
      responses:
        '204':
          description: Success
        4XX:
          $ref: '#/components/responses/error'
        5XX:
          $ref: '#/components/responses/error'
      x-ms-docs-operation-type: operation
    delete:
      tags:
        - groups.directoryObject
      summary: Remove member
      description: Remove a member from a group via the members navigation property. You can't remove a member from groups with dynamic memberships.
      externalDocs:
        description: Find more info here
        url: https://learn.microsoft.com/graph/api/group-delete-members?view=graph-rest-1.0
      operationId: groups.DeleteRefMembers
      parameters:
        - name: If-Match
          in: header
          description: ETag
          schema:
            type: string
        - name: '@id'
          in: query
          description: The delete Uri
          required: true
          schema:
            type: string
      responses:
        '204':
          description: Success
        4XX:
          $ref: '#/components/responses/error'
        5XX:
          $ref: '#/components/responses/error'
      x-ms-docs-operation-type: operation
    parameters:
      - name: group-id
        in: path
        description: The unique identifier of group
        required: true
        schema:
          type: string
        x-ms-docs-key-type: group

From the yaml description we conclude that

  • The @id parameter is only required for the DELETE method
  • The template is used for all methods thus making a GET request will generate the url https://graph.microsoft.com/v1.0/groups/groupId/members/$ref?@id= which will result in an API error as seen here.

To avoid the issue, the required parameters should be added to the template if they are in all methods or introduce secondary templates for such methods.

@andrueastman andrueastman changed the title Required parameters in a method are used across other methods. Required parameters in a method are used across HTTP methods executor. Feb 7, 2024
@baywet baywet added type:bug A broken experience generator Issues or improvements relater to generation capabilities. labels Feb 7, 2024
@baywet baywet added this to the Kiota v1.12 milestone Feb 7, 2024
@baywet
Copy link
Member

baywet commented Feb 7, 2024

Thanks for reporting this issue.
I think this is another instance where the history of the project starts showing up...
When we transitioned from concatenating strings to URI templates, we originally had a single URI template per request builder. Both for simplicity reasons and to avoid duplication of information.
Because at the time we translated all query parameters as optional, differences in query parameters between operations didn't matter.
But this clearly shows to be a design mistake.

Learning from the optimization work in the TypeScript world I've done over the past month, I think we should take this as an opportunity to have defaults and overrides, in order to save sizes when possible.

Currently, most languages besides TypeScript are generated looking this way:

at the constructor level

public MessageItemRequestBuilder(string rawUrl, IRequestAdapter requestAdapter) : base(requestAdapter, "{+baseurl}/users/{user%2Did}/messages/{message%2Did}{?includeHiddenMessages*,%24select,%24expand}", rawUrl) {
        }

At the request generator level

public RequestInformation ToDeleteRequestInformation(Action<RequestConfiguration<DefaultQueryParameters>> requestConfiguration = default) {
            var requestInfo = new RequestInformation(Method.DELETE, UrlTemplate, PathParameters);
            requestInfo.Configure(requestConfiguration);
            requestInfo.Headers.TryAdd("Accept", "application/json");
            return requestInfo;
        }

This UrlTemplate property was assigned by the constructor.

We could still keep the same logic, but have another template in the generator when it differs.
As for the template to use in the constructor, we could use the one that has the most occurrences among operations, if a tie, alphabetically.

Thoughts?

@andrueastman
Copy link
Member Author

e most occurrences among operations

+1 on the suggestion here. We should do this.

This should result in small changes in the generated code with only the parameter passed to the request generator level changing.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
generator Issues or improvements relater to generation capabilities. type:bug A broken experience WIP
Projects
Archived in project
Development

Successfully merging a pull request may close this issue.

2 participants