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

OData cast paths should follow the same naming convention for tags and operationIds as non-OData cast paths #324

Closed
peombwa opened this issue Dec 22, 2022 · 6 comments · Fixed by #338
Assignees
Labels
priority:p1 High priority but not blocking. Causes major but not critical loss of functionality SLA <=7days type:enhancement Enhancement request targeting an existing experience.
Milestone

Comments

@peombwa
Copy link
Contributor

peombwa commented Dec 22, 2022

To support accurate slicing of an OpenAPI document, OData cast paths should follow the same naming convention for tags and operationIds as their non-OData cast paths counterparts. An As<TypeName> should be added at the end of an operationId.

Assemblies affected

v1.1.0

Actual result

Path Tags OperationId
GET /directoryRoles/{directoryRole-id}/members directoryRoles.directoryObject directoryRoles.ListMembers
GET /directoryRoles/{directoryRole-id}/members/microsoft.graph.application directoryObject.application Get.microsoft.graph.directoryObject.Items.As.microsoft.graph.application-ced0

Expected result

Path Tags OperationId
GET /directoryRoles/{directoryRole-id}/members directoryRoles.directoryObject directoryRoles.ListMembers
GET /directoryRoles/{directoryRole-id}/members/microsoft.graph.application directoryObject.application Get.microsoft.graph.directoryObject.Items.As.microsoft.graph.application-ced0
GET /directoryRoles/{directoryRole-id}/members/microsoft.graph.application directoryRoles.directoryObject directoryRoles.ListMembers.AsApplication
@CarolKigoonya CarolKigoonya added this to the OData:1.3 milestone Jan 24, 2023
@peombwa peombwa added the priority:p1 High priority but not blocking. Causes major but not critical loss of functionality SLA <=7days label Jan 24, 2023
@irvinesunday irvinesunday self-assigned this Jan 30, 2023
@irvinesunday
Copy link
Contributor

How about operation ids of $count segments appended to OData cast paths? Example:

 /directoryRoles/{directoryRole-id}/members/microsoft.graph.device/$count':
  description: Provides operations to count the resources in the collection.
  get:
    summary: Get the number of the resource
    operationId: Get.Count.microsoft.graph.device-e712

Would the operation id: Get.Count.microsoft.graph.device-e712 also need to be updated as Get.Count.AsDevice-e712?

@irvinesunday
Copy link
Contributor

Currently, we don't generate tag names for $count and complex property paths.
What tag naming convention do you think will be most appropriate for:

  1. $count paths. Example is the one mentioned above
  2. Complex property paths. Example:
'/admin/serviceAnnouncement/issues/{serviceHealthIssue-id}/posts':
    get:
      summary: Get posts property value
      description: Collection of historical posts for the service issue.
      operationId: posts.serviceHealthIssuePost.ListServiceHealthIssuePost
      parameters:

@peombwa @baywet @millicentachieng

@baywet
Copy link
Member

baywet commented Feb 1, 2023

I don't have an opinion on the matter either way, kiota doesn't make use of tags or operation IDs. I think Peter's opinion is probably the most relevant here.

@peombwa
Copy link
Contributor Author

peombwa commented Feb 6, 2023

@irvinesunday, good question. We currently don't support /$count in PowerShell. However, we can use the following operationIds and tags for:

  1. /$count paths:
/directoryRoles/{directoryRole-id}/members/$count':
description: Provides operations to count the resources in the collection.
get:
  tags:
    - directoryRoles.directoryObject
  summary: Get the number of the resource
  operationId: directoryRoles.Members.GetCount
  1. /$count on OData cast paths:
/directoryRoles/{directoryRole-id}/members/microsoft.graph.device/$count':
  description: Provides operations to count the resources in the collection.
  get:
    tags:
      - directoryRoles.directoryObject
    summary: Get the number of the resource
    operationId: directoryRoles.Members.GetCount.AsDevice
  1. Complex property paths (if supported):
'/admin/serviceAnnouncement/issues/{serviceHealthIssue-id}/posts':
  get:
    tags:
      - admin.serviceAnnouncement
    summary: Get posts property value
    description: Collection of historical posts for the service issue.
    operationId: admin.serviceAnnouncement.issues.ListPosts

Do we currently generate paths for collection of complex property types? I couldn't find /admin/serviceAnnouncement/issues/{serviceHealthIssue-id}/posts in https://github.com/microsoftgraph/msgraph-metadata/tree/master/openapi/v1.0.

tags should ideally maintain the current naming convention of regular paths for consitency when slicing the API i.e., [EntitySet]/[Singleton].[EntityType/Actions/Functions].

@peombwa
Copy link
Contributor Author

peombwa commented Feb 6, 2023

@irvinesunday , since OperationIds are not used by Kiota, can we standardize the naming of operationIds at the OData -> OpenAPI conversion layer instead of modifying them in DevX API at PowershellFormatter.cs?

@irvinesunday
Copy link
Contributor

@irvinesunday , since OperationIds are not used by Kiota, can we standardize the naming of operationIds at the OData -> OpenAPI conversion layer instead of modifying them in DevX API at PowershellFormatter.cs?

There are other clients who rely on the operation ids. See this issue for example: It is better to have them standardized as they currently in the conversion layer.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
priority:p1 High priority but not blocking. Causes major but not critical loss of functionality SLA <=7days type:enhancement Enhancement request targeting an existing experience.
Projects
None yet
Development

Successfully merging a pull request may close this issue.

5 participants