microsoftgraph / msgraph-beta-sdk-dotnet

The Microsoft Graph Client Beta Library for .NET supports the Microsoft Graph /beta endpoint. (preview)
Other
96 stars 32 forks source link

Filtering ICollectionpages #317

Closed DerGuru closed 3 years ago

DerGuru commented 3 years ago
var request = GraphClient
  .DeviceManagement
  .ManagedDevices
  .Request()
  .Filter(filter)
  .Select(select);
  var ll = new LinkedList<ManagedDevice>();
  do
  {
      var collectionPage = await request.GetAsync(); //second execution brings Bad Request
      foreach (var dev in collectionPage)
      {
          ll.AddLast(dev);
      }
      request = collectionPage.NextPageRequest;
  } while (request != null);

This results in a Bad Request after having downloaded the first 1000 ManagedDevices, so the error comes up, when the GetAsync() of the NextPageRequest is invoked. Interestingly, if I take the url and queryoption, the resulting url works in GraphExplorer, so I assume it is not on the server side. When I remove the filter (commenting it), it also works in code.

Using Graph Beta 4.5.0-preview.

AB#10118

MIchaelMainer commented 3 years ago

This is strange. So, if you remove the filter, it works fine when paging through all the NextPageRequests (you get all of the devices)?

Can you repro and capture this locally using Fiddler? If so, can you share a sanitized capture of the request and response? Otherwise, could you enable logging?

DerGuru commented 3 years ago

I think I found the bug. When the first request returns, the filter is URL-Encoded. When I replace the URL-Encoded filter clause, with the old non-url-encoded one, it works.

andrueastman commented 3 years ago

Thanks @DerGuru.

Are you able to share more information on the nature of the encoded URL(specifically the filter clause) so that we may fix the issue of the URL encoding? I believe encoding the URL shouldn't cause an issue unless we encode a character that shouldn't be encoded.

DerGuru commented 3 years ago

In Code I am replacing the Queryoptions:

public static async Task<LinkedList<TResult>> LoadMultiple<TRequest, TCollectionPage, TAzure, TResult>(this TRequest request, Func<TRequest, Task<TCollectionPage>> invoke, Func<TCollectionPage, TRequest> next, Func<TAzure, TResult> transform = null, Func<TAzure, bool> clientSelector = null)
            where TRequest : IBaseRequest
            where TCollectionPage : ICollectionPage<TAzure>
            where TResult : class
        {
            transform = transform ?? new Func<TAzure, TResult>(x => x as TResult);
            clientSelector = clientSelector ?? new Func<TAzure, bool>(x => true);

            var filter = request.QueryOptions.FirstOrDefault(x => x.Name.EndsWith("filter"));

            LinkedList<TResult> ll = new LinkedList<TResult>();
            do
            {
                var collectionPage = await invoke(request);
                foreach (TResult retVal in collectionPage.CurrentPage
                    .Where(clientSelector)
                    .Select(transform))
                {
                    ll.AddLast(retVal);
                }
                request = next(collectionPage);
                var newFilter = request?.QueryOptions.FirstOrDefault(x => x.Name.EndsWith("filter"));
                if (newFilter != null)
                {
                    var index = request.QueryOptions.IndexOf(newFilter);
                    request.QueryOptions[index] = filter;
                }

            } while (request != null);
            return ll;
        }

Original Filter clause (filter.Value): (deviceType eq 'android' or deviceType eq 'androidEnterprise' or deviceType eq 'androidForWork' or deviceType eq 'iPad' or deviceType eq 'iPhone')

Returned Filter Clause (newFilter.Value) (deviceType+eq+%27android%27+or+deviceType+eq+%27androidEnterprise%27+or+deviceType+eq+%27androidForWork%27+or+deviceType+eq+%27iPad%27+or+deviceType+eq+%27iPhone%27)

The code could be simplified, if the NextPageRequest would be part of ICollectionPage<T>, but that is not the issue here. The above filter clauses are real values, and the code is in production like this.

andrueastman commented 3 years ago

Hey @DerGuru,

Just to confirm, when you run the same query on graph explorer, does the value returned in the @odata.nextLink property have the space characters replaced with + characters?

DerGuru commented 3 years ago

This is, what graph explorer returns. Interestingly, when I "follow the nextlink", it also works.

{
    "@odata.context": "https://graph.microsoft.com/beta/$metadata#deviceManagement/managedDevices",
    "@odata.count": 1000,
    "@odata.nextLink": "https://graph.microsoft.com/beta/deviceManagement/managedDevices?$filter=(deviceType+eq+%27android%27+or+deviceType+eq+%27androidEnterprise%27+or+deviceType+eq+%27androidForWork%27+or+deviceType+eq+%27iPad%27+or+deviceType+eq+%27iPhone%27)&$skiptoken=LastDeviceName%3d%27Andrew%25e2%2580%2599s%2520iPhone%27%2cLastDeviceId%3d%272dcc19dc-a2b9-434a-8013-ac85b69bece3%27",
    "value": [
...
andrueastman commented 3 years ago

Thanks for the information. It looks like since the SDK does do URL encoding of the filter clause, having the + encoded makes the API side interpret it as actually a + in the filter clause rather than it being interpreted as a space character. We will need to investigate this and provide a fix in the SDK.

DerGuru commented 3 years ago

maybe you could replace the + with %20?

karol-pieciukiewicz commented 3 years ago

Hi, the same problem is with "select" clause. For example, this code is working fine:

IGraphServiceUsersCollectionPage iGraphServiceUsersCollectionPage = await _graphServiceClient.Users
                                                        .Request()
                                                        .Select(x => new { x.UserType, x.SignInActivity, x.Id, x.DisplayName, x.Surname })                                                                      
                                                        .Top(50)
                                                        .GetAsync();

var userPageIterator = PageIterator<User>.CreatePageIterator(_graphServiceClient,
                                            iGraphServiceUsersCollectionPage,
                                            entity => { allUsers.Add(entity); return true; });    

and this:

IGraphServiceUsersCollectionPage iGraphServiceUsersCollectionPage = await _graphServiceClient.Users
                                        .Request()
                                        .Select($"id, displayName, surname, signInActivity, userType")
                                        .Top(50)
                                        .GetAsync();

throws exception:

Message: Parsing OData Select and Expand failed: Syntax error: character '+' is not valid at position 3 in 'id,+displayName,+surname,+signInActivity,+userType'.

Generated URL looks like: `https://graph.microsoft.com/beta/users?$select=id%2C%2BdisplayName%2C%2Bsurname%2C%2BsignInActivity%2C%2BuserType&$top=50&$skiptoken=AQABAAAABAAAA`....

andrueastman commented 3 years ago

Hey, @karol-azure-way, I believe your second example should actually be like this.

IGraphServiceUsersCollectionPage iGraphServiceUsersCollectionPage = await _graphServiceClient.Users
                                        .Request()
                                        .Select($"id,displayName,surname,signInActivity,userType")
                                        .Top(50)
                                        .GetAsync();

The space characters aren't really valid in the select clause.

karol-pieciukiewicz commented 3 years ago

@andrueastman sure, it works fine, but I think that this is a general problem with serialization :) That space ' ' is replaced with '+'

andrueastman commented 3 years ago

Are you able to capture a fiddler trace of what is sent out? As seen in the other case, the it is actually the API response that replaces the spaces with '+' characters but the SDK does not do that.

karol-pieciukiewicz commented 3 years ago

@andrueastman I already make it works. I used custom HttpClient (https://docs.microsoft.com/en-us/graph/sdks/customize-client?tabs=csharp) with custom HttpClientHandler which replaces %2B with %20, see code below:

    public class MessageHandler1 : DelegatingHandler
    {
        protected async override Task<HttpResponseMessage> SendAsync(HttpRequestMessage request, CancellationToken cancellationToken)
        {
            var uri = request.RequestUri;
            var newUri = uri.AbsoluteUri.Replace("%2B", "%20");
            request.RequestUri = new Uri(newUri);

            var response = await base.SendAsync(request, cancellationToken);

            return response;
        }
    }

If you still need a trace for these requests, I can catch it.

andrueastman commented 3 years ago

Thanks @karol-azure-way, It would be great to get a trace as well before the workaround so as to confirm the URL sent out when the error occurs when we put the space characters. From earlier tests, the SDK sent out %20 but the API response had + instead.

andrueastman commented 3 years ago

Closing this as this is now resolved via 4.8.0-preview.