Semantic violation: Certain OData functions have path parameters referenced in query

[peombwa]

Certain OData functions have path parameters referenced in query instead of path. This results in generation error since code generators cannot find the specified path parameters.

Current Representation

'/users/{user-id}/joinedTeams/{team-id}/primaryChannel/microsoft.graph.doesUserHaveAccess(userId=''{userId}'',tenantId=''{tenantId}'',userPrincipalName=''{userPrincipalName}'')':
  get:
    tags:
      - users.Functions
    summary: Invoke function doesUserHaveAccess
    operationId: users.joinedTeams.primaryChannel_doesUserHaveAccess
    parameters:
      - name: user-id
        in: path
        description: 'key: id of user'
        required: true
        schema:
          type: string
        x-ms-docs-key-type: user
      - name: team-id
        in: path
        description: 'key: id of team'
        required: true
        schema:
          type: string
        x-ms-docs-key-type: team
      - name: userId
        in: query
        description: 'Usage: userId=''{userId}'''
        schema:
          type: string
          nullable: true
      - name: tenantId
        in: query
        description: 'Usage: tenantId=''{tenantId}'''
        schema:
          type: string
          nullable: true
      - name: userPrincipalName
        in: query
        description: 'Usage: userPrincipalName=''{userPrincipalName}'''
        schema:
          type: string
          nullable: true
    responses:
      '200': ...
      default:
        $ref: '#/components/responses/error'
    x-ms-docs-operation-type: function

Ideal Representation

'/users/{user-id}/joinedTeams/{team-id}/primaryChannel/microsoft.graph.doesUserHaveAccess(userId=''{userId}'',tenantId=''{tenantId}'',userPrincipalName=''{userPrincipalName}'')':
  get:
    tags:
      - users.Functions
    summary: Invoke function doesUserHaveAccess
    operationId: users.joinedTeams.primaryChannel_doesUserHaveAccess
    parameters:
      - name: user-id
        in: path
        description: 'key: id of user'
        required: true
        schema:
          type: string
        x-ms-docs-key-type: user
      - name: team-id
        in: path
        description: 'key: id of team'
        required: true
        schema:
          type: string
        x-ms-docs-key-type: team
      - name: userId
+         in: path
-         in: query
        description: 'Usage: userId=''{userId}'''
        schema:
          type: string
          nullable: true
      - name: tenantId
+         in: path
-         in: query
        description: 'Usage: tenantId=''{tenantId}'''
        schema:
          type: string
          nullable: true
      - name: userPrincipalName
+         in: path
-         in: query
        description: 'Usage: userPrincipalName=''{userPrincipalName}'''
        schema:
          type: string
          nullable: true
    responses:
      '200': ...
      default:
        $ref: '#/components/responses/error'
    x-ms-docs-operation-type: function

[darrelmiller]

@irvinesunday Do you know why we are generating the above path? JoinedTeams is non-contained and yet we are exploding the navigation property primaryChannel.

@peombwa I am fairly confident that this path does not work. You can't even readbykey on the JoinedTeams so there is no way to get at this function via a path that has user-id in it.

[baywet]

still the parameters should be in path and not in query, this is something we're running into in Kiota based SDK as well.

[irvinesunday]

Since having optional parameters defined either in: path and in: query are both acceptable (see this related issue), I suggest we introduce a convert setting, OptionalParametersInPath and have its default to false. Then we can use this setting in DevX API and Hidi to have our generation append optional parameters to in: path by setting the convert setting value to true. @peombwa @baywet

[baywet]

I don't think this is a functional preference at this point. The query parameter should be described as in path. It could be described as in query if it were aliased, but we've already made changes to have it in path instead. If we were to provide a setting it should be between this (corrected with in path) and the aliased approach. Not just for query and path for this format as it is invalid.

[irvinesunday]

Implicit parameter aliases are permitted as well, where the query parameter can be described in the query, acting as the implicit parameter alias.

doc ref:

[irvinesunday]

@irvinesunday Do you know why we are generating the above path? JoinedTeams is non-contained and yet we are exploding the navigation property primaryChannel.

@peombwa I am fairly confident that this path does not work. You can't even readbykey on the JoinedTeams so there is no way to get at this function via a path that has user-id in it.

@darrelmiller we do not generate this path in our latest version of the conversion lib.

[baywet]

@irvinesunday sure but in that case the parameters should not be in the path anymore for the URI template. The concern is that currently the URL template is invalid. Let's just fix it to have query changed to path in the parameter description, which was the original ask of this issue. And defer additional feature works (option to have it aliased or implicitly aliased) to another issue

[irvinesunday]

@irvinesunday sure but in that case the parameters should not be in the path anymore for the URI template. The concern is that currently the URL template is invalid. Let's just fix it to have query changed to path in the parameter description, which was the original ask of this issue. And defer additional feature works (option to have it aliased or implicitly aliased) to another issue

My concern here is that this will be a breaking change, with regards to this issue which requested that optional parameters be defined in: query which was resolved by this PR

[baywet]

Good catch. What do you think of the following:

• parameter is required, put it in path (both at the URL template level and in parameters description), no aliasing
• parameter is optional, alias it explicitly, and put it in query.

[irvinesunday]

How about:

• When parameter is optional, we use implicit parameter aliasing...

For example:

will be:

[baywet]

The only problem I see with that (and the suggestion I previously suggested) is if a parameter is changed from required to optional, which I don't believe is a breaking change in Graph API lifecycle, we'd be generating a breaking change (the parameter moves from path to query). But maybe that's ok? At this point the only solution would be to explicitly alias all parameters to move them to query, which I don't believe we want because it'd technically make everything optional.

@darrelmiller can you provide some input please?

[darrelmiller]

One clarification that I can't tell from the thread if it is clear. OpenAPI doesn't support optional path parameters. Therefore we must use aliases for optional OData Function parameters. Query parameters can be made required in OpenAPI and so I think the simplest solution would be to make all the parameters aliased. This would give us the ability to use the implicit alias. It is also possible to create URI Templates that differentiate between required and optional parameters. e.g.

?fixed={fixed}{&x}     ?fixed=yes&x=1024

https://www.rfc-editor.org/rfc/rfc6570#section-3.2.9

[baywet]

Thanks for the clarification, for the templating mechanism of the query parameters this is actually handled by kiota but this is surely something we could update.

So implicitly alias all parameters and set them to query at the end. And explicitly alias them if the parameter name was already present in previous path segments. Did we reach an agreement?

[darrelmiller]

Wait. I'm only talking about aliasing parameters that are for a function. I wasn't considering aliasing regular path parameters. Does OData allow aliasing of collection indexing parameters?

[baywet]

I meant all functions parameters as well, not other ones. Thanks for the callout.

[irvinesunday]

Are we going with implicit parameter aliases with the format? /roleManagement/directory/microsoft.graph.roleScheduleInstances?directoryScopeId={directoryScopeId}&appScopeId={appScopeId}&principalId={principalId}&roleDefinitionId={roleDefinitionId}

[baywet]

This approach will be an issue if we have overloads: /roleManagement/directory/microsoft.graph.roleScheduleInstances?directoryScopeId={directoryScopeId}&appScopeId={appScopeId}&principalId={principalId}&roleDefinitionId={roleDefinitionId}

/roleManagement/directory/microsoft.graph.roleScheduleInstances?directoryScopeId={directoryScopeId}&appScopeId={appScopeId}&principalId={principalId}

Will conflict during generation.

But this /roleManagement/directory/microsoft.graph.roleScheduleInstances(directoryScopeId='@directoryScopeIdAlias')?directoryScopeIdAlias={directoryScopeId}&appScopeId={appScopeId}&principalId={principalId}&roleDefinitionId={roleDefinitionId} (apply logic for all parameters)

Would not solve the overload issue either.

This is why I was advocating for

/roleManagement/directory/microsoft.graph.roleScheduleInstances(directoryScopeId='{directoryScopeId}')?appScopeId={appScopeId}&principalId={principalId}&roleDefinitionId={roleDefinitionId} (apply logic for all parameters)

Which will result in roleScheduleInstancesByDirectoryScopeId, and is less likely to conflict with any overload.

[irvinesunday]

@baywet your proposed path structure /roleManagement/directory/microsoft.graph.roleScheduleInstances(directoryScopeId='{directoryScopeId}')?appScopeId={appScopeId}&principalId={principalId}&roleDefinitionId={roleDefinitionId} will violate the principle of implicit parameter aliasing, where parentheses are not to be appended to the function name.

[baywet]

ah, you can't mix implicit and explicit?

[irvinesunday]

Sadly, we're not allowed to.

[baywet]

thanks, didn't know about that aspect. So I guess we're back to square one at this point... From an OData linguo perspective, how would one not pass any value to an optional parameter if we were to put everything in the path?

[darrelmiller]

/roleManagement/directory/microsoft.graph.roleScheduleInstances(directoryScopeId='{directoryScopeId}')?appScopeId={appScopeId}&principalId={principalId}&roleDefinitionId={roleDefinitionId}

I don't think this is allowed because OData doesn't support path parameters for function parameters and implicit parameters at the same time. If we have any function path parameters they need to be all path parameters. We could make just the optional ones aliases though.... I think.

[baywet]

After some more thinking with Darrel, here is what we came up with:

• required parameters should be path parameters in the open API description, unaliased
• optional parameters should be explicitly aliased, and be query parameters in the open API description

The case where function overloads have the same required parameters but vary in optional parameters is a non-existing case as the OData engine would not know which overload to pick when not providing all parameters (impossible to implement on the service side).

A few Open API path items examples

path/foo(param1='{param1}') (param1 required and in path, no optional parameters) path/foo(param1='{param1}',param2='@param2') (param1 required and in path, param2 optional and in query) path/foo(param1='@param1') (no required parameter, param1 optional and in query)

Note : {?param1} or {?param2} will be added to the URL template by kiota when generating the client, not the conversion process.

[irvinesunday]

Thanks for the clarification on this. Question: should the parameter alias be in quotes? I don't see this from the example provided in the specification:

[baywet]

I believe it does, but I might be wrong in this. In the example you're quoting, the ID is integer, hence no quotes. But the quotes could instead be required in the query parameter value, which would be impossible to express as quotes + optional according to RFC 6570...fun.