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

functions with parameters that are collection are poorly converted #122

Closed
baywet opened this issue Oct 25, 2021 · 5 comments · Fixed by #133
Closed

functions with parameters that are collection are poorly converted #122

baywet opened this issue Oct 25, 2021 · 5 comments · Fixed by #133
Assignees
Milestone

Comments

@baywet
Copy link
Member

baywet commented Oct 25, 2021

If a function has a parameter of type Collection(Something), the path will be wrong in the resulting OpenAPI description.

Assemblies affected

latest

Steps to reproduce

<Function Name="getRoleScopeTagsByIds" IsBound="true">
        <Parameter Name="bindingParameter" Type="graph.deviceManagement" />
        <Parameter Name="ids" Type="Collection(Edm.String)" Unicode="false" />
        <ReturnType Type="Collection(graph.roleScopeTag)" />
      </Function>

(taken from latest beta full description)

Expected result

/deviceManagement/microsoft.graph.getRoleScopeTagsByIds(ids='{ids}'):
    get:
      tags:
        - deviceManagement.Functions
      summary: Invoke function getRoleScopeTagsByIds
      operationId: deviceManagement.getRoleScopeTagsByIds
      parameters:
        - name: ids
          in: query
          description: 'Usage: ids={ids}'
          required: true
          schema:
            type: array
            items:
              type: string
      responses:
        '200':
          description: Success
          content:
            application/json:
              schema:
                type: array
                items:
                  anyOf:
                    - $ref: '#/components/schemas/microsoft.graph.roleScopeTag'
                  nullable: true
        default:
          $ref: '#/components/responses/error'
      x-ms-docs-operation-type: function

(note the parameter type, and the parameter in the path)

Actual result

/deviceManagement/microsoft.graph.getRoleScopeTagsByIds(ids=@ids):
    get:
      tags:
        - deviceManagement.Functions
      summary: Invoke function getRoleScopeTagsByIds
      operationId: deviceManagement.getRoleScopeTagsByIds
      parameters:
        - name: ids
          in: query
          description: 'Usage: ids={ids}'
          required: true
          schema:
            type: string
      responses:
        '200':
          description: Success
          content:
            application/json:
              schema:
                type: array
                items:
                  anyOf:
                    - $ref: '#/components/schemas/microsoft.graph.roleScopeTag'
                  nullable: true
        default:
          $ref: '#/components/responses/error'
      x-ms-docs-operation-type: function

Additional detail

*Optional, details of the root cause if known.

@irvinesunday
Copy link
Contributor

irvinesunday commented Nov 9, 2021

@baywet
According to the OData - OpenAPI documentation, collection-valued parameters are represented as parameter aliases.

image

@darrelmiller
Copy link
Member

OData allows function parameters to be passed directly in the last path segment, or in the query string via an alias. So,

https://example.org/getUser(userId='100')

and

https://example.org/getUser(userId=@userId)?@userId='100'

are functionally equivalent.
It appears that the OASIS guidance around converting OData to OpenAPI suggests using the alias option for complex and collection valued parameters. I suspect that is because the OData spec says that complex values should be represented as a JSON element. e.g.

http://host/service/ProductsByColors(colors=@c)?@c=["red","green"]

Sadly the example in our case makes it impossible to tell how the list of Ids is expected to be formatted. https://docs.microsoft.com/en-us/graph/api/intune-shared-devicemanagement-getrolescopetagsbyids?view=graph-rest-beta#request

This appears to be some mixed hybrid with something inside the string value.

GET https://graph.microsoft.com/beta/deviceManagement/getRoleScopeTagsByIds(ids=[
  "Ids value"
])

As far as Graph is concerned, the first OpenAPI is how I expected Graph APIs to work. The second one does accurately describe the aliasing, although the parameter should be @ids and having the type as string is not correct. Whether it should be array of string, or object us unknown because the getRoleScopeTagsByIds docs are unclear.

We should do a survey of Graph APIs that accept lists of ids and see what syntax is actually being used.

@baywet
Copy link
Member Author

baywet commented Nov 9, 2021

thanks for the additional information here. This is only used once in beta today.
So at this point having an @ sign in the URL is normal, and Kiota's namespace/method name generation should account for that, but the type should be an array and the conversion process needs to be fixed on that aspect, correct?
Assuming both fixes are implemented, we then should be ok.

@darrelmiller
Copy link
Member

darrelmiller commented Nov 9, 2021

According to OData the type should serialized as JSON which we use "style":"deepObject" for. But normally it is a JSON object. I've never run into a situation where someone tried to create a parameter value as a JSON array before.

But what OData says, and what a specific Graph API does are two different things.

@darrelmiller
Copy link
Member

I was wrong about using the deepObject. Instead we use a content property and a media type object to indicate that the ids are serialized as JSON.

/deviceManagement/microsoft.graph.getRoleScopeTagsByIds(ids='{ids}'):
    get:
      tags:
        - deviceManagement.Functions
      summary: Invoke function getRoleScopeTagsByIds
      operationId: deviceManagement.getRoleScopeTagsByIds
      parameters:
        - name: ids
          in: query
          description: 'Usage: ids={ids}'
          required: true
          content:
            application/json:
              schema:
                type: array
                items:
                  type: string
      responses:
        '200':
          description: Success
          content:
            application/json:
              schema:
                type: array
                items:
                  anyOf:
                    - $ref: '#/components/schemas/microsoft.graph.roleScopeTag'
                  nullable: true
        default:
          $ref: '#/components/responses/error'
      x-ms-docs-operation-type: function

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging a pull request may close this issue.

3 participants