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

Delta functions should have Deltatoken query parameter #354

Open
peombwa opened this issue Jun 2, 2023 · 16 comments
Open

Delta functions should have Deltatoken query parameter #354

peombwa opened this issue Jun 2, 2023 · 16 comments
Assignees
Labels
Area: Annotations An issue which requires annotations to be added/modified Area: odata-to-openapi An issue with the conversion of OData to OpenAPI Needs: Directive Issue needs directive Needs: Investigation Issue needs to be investigated further priority:p1 High priority but not blocking.

Comments

@peombwa
Copy link
Member

peombwa commented Jun 2, 2023

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
Copy link

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

@darrelmiller
Copy link
Contributor

darrelmiller commented Jun 15, 2023

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

@irvinesunday
Copy link
Contributor

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 irvinesunday self-assigned this Jul 12, 2023
@andrueastman
Copy link
Member

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

@irvinesunday irvinesunday added Area: odata-to-openapi An issue with the conversion of OData to OpenAPI Area: Annotations An issue which requires annotations to be added/modified labels Jul 17, 2023
@baywet
Copy link
Member

baywet commented Jul 19, 2023

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.

@irvinesunday irvinesunday added Needs: Directive Issue needs directive Needs: Investigation Issue needs to be investigated further labels Jul 20, 2023
@maisarissi
Copy link

maisarissi commented Jul 20, 2023

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: microsoftgraph/msgraph-sdk-powershell#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
Copy link
Member

baywet commented Jul 20, 2023

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
Copy link

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
Copy link
Member

baywet commented Jul 20, 2023

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
Copy link
Member Author

peombwa commented Jul 20, 2023

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
Copy link
Member

baywet commented Jul 20, 2023

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
Copy link
Member Author

peombwa commented Jul 20, 2023

Perfect! We'll track the feature request at microsoftgraph/msgraph-sdk-powershell#2062 (comment) for PowerShell SDK v3.

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

@maisarissi
Copy link

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: microsoftgraph/microsoft-graph-devx-api#1673

@baywet
Copy link
Member

baywet commented Aug 28, 2023

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
Copy link
Member

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
Copy link
Member

baywet commented Aug 29, 2023

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.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Area: Annotations An issue which requires annotations to be added/modified Area: odata-to-openapi An issue with the conversion of OData to OpenAPI Needs: Directive Issue needs directive Needs: Investigation Issue needs to be investigated further priority:p1 High priority but not blocking.
Projects
None yet
Development

No branches or pull requests

6 participants