microsoftgraph / msgraph-metadata

Microsoft Graph metadata captured and used for generating client library code files.
https://graph.microsoft.com
MIT License
105 stars 31 forks source link

`Delta` functions should have `Deltatoken` query parameter #354

Open peombwa opened 1 year ago

peombwa commented 1 year ago

All Delta functions should have Deltatoken query parameter via CustomQueryOptions as described in the API reference at https://learn.microsoft.com/en-us/graph/delta-query-users?tabs=http#deltalink-request. Deltatoken is used to get to pass the deltaToken from the last response, you'll get changes (additions, deletions, or updates) to resource since the last request.

OData capability annotations allows for:

<Term Name="OperationRestrictions" Type="Capabilities.OperationRestrictionsType" Nullable="false" AppliesTo="Action Function">
    <Annotation Term="Core.Description" String="Restrictions for function or action operation"/>
</Term>
<ComplexType Name="OperationRestrictionsType">
    <Property Name="FilterSegmentSupported" Type="Edm.Boolean" Nullable="false" DefaultValue="true">
        <Annotation Term="Core.Description" String="Bound action or function can be invoked on a collection-valued binding parameter path with a `/$filter(...)` segment"/>
    </Property>
    <Property Name="Permissions" Type="Collection(Capabilities.PermissionType)" Nullable="true">
        <Annotation Term="Core.Description" String="Required permissions. One of the specified sets of scopes is required to invoke an action or function"/>
    </Property>
    <Property Name="CustomHeaders" Type="Collection(Capabilities.CustomParameter)" Nullable="false">
        <Annotation Term="Core.Description" String="Supported or required custom headers"/>
    </Property>
    <Property Name="CustomQueryOptions" Type="Collection(Capabilities.CustomParameter)" Nullable="false">
        <Annotation Term="Core.Description" String="Supported or required custom query options"/>
    </Property>
    <Property Name="ErrorResponses" Type="Collection(Capabilities.HttpResponse)" Nullable="false">
        <Annotation Term="Core.Description" String="Possible error responses returned by the request."/>
    </Property>
</ComplexType>
maisarissi commented 1 year ago

Can we add a XLT transform to fix it? All /delta endpoints should support deltaLink.

darrelmiller commented 1 year ago

OpenAPI.Net.Odata should use the ChangeTracking annotation to know if the query parameter is required. This should be transfer to that repo.

irvinesunday commented 1 year ago

OpenAPI.Net.Odata should use the ChangeTracking annotation to know if the query parameter is required. This should be transfer to that repo.

Isn't the ChangeTracking annotation specifically applied to entity sets to indicate that an entity set supports change tracking, whereas the CustomQueryOptions property of the OperationRestrictions annotation defines a list of supported or required custom query options for a given operation? I believe if we want to list the Deltatoken query parameter for an operation we'd add it to the OperationRestrictions annotation for delta functions, which would then be picked up by the conversion lib. here.

andrueastman commented 1 year ago

According to the link below, it looks like the ChangeTracking annotation can also exist on functions, singletons and nav properties. https://github.com/oasis-tcs/odata-vocabularies/blob/1d1bd57d8280fa1032d4e77b7e9cd7b26d713c66/vocabularies/Org.OData.Capabilities.V1.json#L168

However, the delta functions currently do not seem to have this in the annotations, but current ChangeTracking annotations are on types/nav properties(An open question is if this means that it cascades down to all functions on the property?)

To me it looks like, we may either have to add the ChangeTracking annotation to the delta functions then modify the lib to pick them up or simply add the OperationRestrictions on delta functions as @irvinesunday suggests.

Thoughts? cc @darrelmiller, @irvinesunday

baywet commented 1 year ago

We have to be careful in what we consider here. Microsoft Graph Delta (change tracking) is different from OData delta (I believe graph delta is older than OData delta, and was then retrofitted in the standard, but to avoid breaking people, graph never went back to adopt the standard). here is the documentation on the annotation and the function to use.

With that being said the guidance to Graph users has always been "odata next and delta links are opaque, don't try to parse them, use them as it". That's because in a lot of cases re-building the complete URL correctly is error prone. We often see situations where the client passes the delta/skip token + a bunch of query parameters that are already encoded in the token itself, messing things up.

This is why those two query parameters are not being projected by the conversion library. This is also why we're not exposing those query parameters as parameters/properties in the SDKs. I'm not sure what the scenario is for powershell needing that parameter, but it should probably use the full link instead in the design.

maisarissi commented 1 year ago

I was the one testing PowerShell when encountered this scenario, which is the following:

When calling delta endpoint in PowerShell, the SDK handles all the calls to get the response: https://github.com/microsoftgraph/msgraph-sdk-powershell/pull/1908. So, when there is more than 1 page, PowerShell uses the @odata.nextLink and iterate to get you the full response. However, the issue is with not having access to the latest @odata.deltaLink value. Per the documentation after getting all the pages, the deltaLink should be used to later on get the updates from the last time you called the endpoint and today we don't have it available in PS.

Hope this clarifies @baywet

baywet commented 1 year ago

Thanks for the clarification. The @odata.deltalink is part of what gets projected today to the OpenAPI description, so you should have a property/return value for it (or at least the ability to project it). Otherwise it'd be absent from the dotnet sdk The thing is we use inheritance so avoid generating the property multiple times (for each delta endpoint) maybe that's what's throwing PS generation out of balance?

maisarissi commented 1 year ago

I would assume that we would call something like:

Get-MgUserDelta -DeltaToken $token

Even our code snippets generation logic assumes the same 😆 image

as what you are calling would be: GET https://graph.microsoft.com/v1.0/users/delta?$deltatoken=oEcOySpF_hWYmTIUZBOIfPzcwisr_rPe8o9M54L45qEXQGmvQC6T2dbL-9O7nSU-njKhFiGlAZqewNAThmCVnNxqPu5gOBegrm1CaVZ-ZtFZ2tPOAO98OD9y0ao460

I guess this is why the first ask from @peombwa was to get the deltaToken.

baywet commented 1 year ago

I'm asserting that the cmdlet should instead be

Get-MgUserDelta -DeltaLink $link # value from the odata.deltalink property of the previous call

And that our snippets should be updated accordingly to reflect our recommendation.

peombwa commented 1 year ago

Sorry I missed the previous discussions.

The original request of exposing DeltaToken query parameter was based on in the API reference description/SDK snippets. However, I'm more in favor of just passing the whole @odata.deltaLink value as it keeps the deltaToken opaque as suggested by Vincent.

Is using the @odata.deltaLink value in a request builder currently supported by any of the SDKs? For PowerShell, this will only be possible in v3 as AutoREST does not support this kind of customizability unless done by hand for all delta cmdlets, which doesn't scale.

baywet commented 1 year ago

yeah it's possible in kiota although a bit cumbersome you effectively need to do something like this

var requestBuilder = new UsersDeltaRequestBuilder("deltaLink", client.RequestAdapter);
var firstPage = await requestBuilder.GetAsync();

we're looking into ways to make that simpler.

peombwa commented 1 year ago

Perfect! We'll track the feature request at https://github.com/microsoftgraph/msgraph-sdk-powershell/issues/2062#issuecomment-1644768630 for PowerShell SDK v3.

@irvinesunday, feel free to close the issue as the feature will need to be handled by the SDKs.

maisarissi commented 1 year ago

yeah it's possible in kiota although a bit cumbersome you effectively need to do something like this

We need to update the code snippet logic for those already using Kiota versions. I raised a new issue in the DevX API repo to track this effort: https://github.com/microsoftgraph/microsoft-graph-devx-api/issues/1673

baywet commented 1 year ago

Couple of additions in that space

SharePoint/OneDrive is the only service to have an additional function definition with the token, this creates unnecessary overrides and the second one with the token should be removed.

<Function Name="delta" IsBound="true">
  <Parameter Name="bindingParameter" Type="graph.driveItem" />
  <ReturnType Type="Collection(graph.driveItem)" />
</Function>
<Function Name="delta" IsBound="true">
  <Parameter Name="bindingParameter" Type="graph.driveItem" />
  <Parameter Name="token" Type="Edm.String" Unicode="false" />
  <ReturnType Type="Collection(graph.driveItem)" />
</Function>

We're working to improve access to the request builder with an arbitrary url https://github.com/microsoft/kiota/issues/3199

andrueastman commented 1 year ago

I believe the second function creates a function with a path parameter argument as opposed to a query parameter argument. Removing it would block scenario 3 and 4 where the specific API allows using timestamps or retrieving the current token which are client-side driven scenarios.

In this situation, I think the right thing to do is that the docs examples should probably be fixed so that the examples using the query parameters are replaced with examples using path parameters as much as both do work.

Couple of additions in that space

SharePoint/OneDrive is the only service to have an additional function definition with the token, this creates unnecessary overrides and the second one with the token should be removed.

<Function Name="delta" IsBound="true">
  <Parameter Name="bindingParameter" Type="graph.driveItem" />
  <ReturnType Type="Collection(graph.driveItem)" />
</Function>
<Function Name="delta" IsBound="true">
  <Parameter Name="bindingParameter" Type="graph.driveItem" />
  <Parameter Name="token" Type="Edm.String" Unicode="false" />
  <ReturnType Type="Collection(graph.driveItem)" />
</Function>

We're working to improve access to the request builder with an arbitrary url microsoft/kiota#3199

baywet commented 1 year ago

ah good call! I forgot the ODSP delta implementation was not conform with the rest of the API surface 🤦‍♂️ I retract my previous statement, we need to leave those functions in place, even though it's really confusing to consumers who end up trying to parse the URL and put the delta token there.