msgraph-metadata icon indicating copy to clipboard operation
msgraph-metadata copied to clipboard

`Delta` functions should have `Deltatoken` query parameter

Open peombwa opened this issue 2 years ago • 16 comments

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>

peombwa avatar Jun 02 '23 21:06 peombwa

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

maisarissi avatar Jun 02 '23 22:06 maisarissi

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

darrelmiller avatar Jun 15 '23 13:06 darrelmiller

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.

irvinesunday avatar Jul 12 '23 10:07 irvinesunday

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

andrueastman avatar Jul 12 '23 14:07 andrueastman

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.

baywet avatar Jul 19 '23 17:07 baywet

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

maisarissi avatar Jul 20 '23 15:07 maisarissi

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?

baywet avatar Jul 20 '23 16:07 baywet

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.

maisarissi avatar Jul 20 '23 18:07 maisarissi

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.

baywet avatar Jul 20 '23 18:07 baywet

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.

peombwa avatar Jul 20 '23 19:07 peombwa

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.

baywet avatar Jul 20 '23 19:07 baywet

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.

peombwa avatar Jul 20 '23 23:07 peombwa

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

maisarissi avatar Jul 21 '23 13:07 maisarissi

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

baywet avatar Aug 28 '23 17:08 baywet

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

andrueastman avatar Aug 29 '23 06:08 andrueastman

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.

baywet avatar Aug 29 '23 12:08 baywet