PageIterator should not mutate his arguments

[OlivierCuyp]

Bug Report

Prerequisites

• Can you reproduce the problem?
• Are you running the latest version?
• Are you reporting to the correct repository?
• Did you perform a cursory search?

For more information, see the CONTRIBUTING guide.

Description

Impure function has always unexpected behaviour.
PageIterator his mutating the response he is given, he should not.
Here is the mutation:
https://github.com/microsoftgraph/msgraph-sdk-javascript/blob/dev/src/tasks/PageIterator.ts#L127

Expected behavior:
The response passed to the PageIterator should be intact after iteration.
Change the Array.shift method to use Array.slice and keep a cursor index.

Actual behavior:
The response passed to the PageIterator is empty after the iteration.

SDK Version - 3.0.2

• Node (Check, if using Node version of SDK)

Node Version - 16.17.0

[nikithauc]

@OlivierCuyp Thank you for bringing this up.

Can you please provide the steps to reproduce the issue, that is, the use case which is throwing the unexpected behaviour

[OlivierCuyp]

@nikithauc I think, the fix would have been written faster than the example, but here it is:

test.ts

import 'isomorphic-fetch';
import { Client, PageIterator } from '@microsoft/microsoft-graph-client';
import { TokenCredentialAuthenticationProvider } from '@microsoft/microsoft-graph-client/authProviders/azureTokenCredentials';
import { ClientSecretCredential } from '@azure/identity';

const fakeGraphApiResponse = {
  value: [{ foo: 'bar' }]
};

const credential = new ClientSecretCredential('baz', '1', 'hush');
const authProvider = new TokenCredentialAuthenticationProvider(credential, {
  scopes: ['https://graph.microsoft.com/.default']
});

const client = Client.initWithMiddleware({ authProvider });
const pageIterator = new PageIterator(client, fakeGraphApiResponse, () => true);

pageIterator.iterate();

console.log(fakeGraphApiResponse);

Running it gives:

❯ npx ts-node ./test.ts
{ value: [] }

As you can see the fakeGraphApiResponse.value have been mutate and is now empty.
I would have expect that the fakeGraphApiResponse would have been left has it was.
The output should have been:

❯ npx ts-node ./test.ts
{
  value: [{ foo: 'bar' }]
}

[OlivierCuyp]

@nikithauc since it was a quick fix, I made a PR.

[nikithauc]

Fix released in v3.0.3

[NickFenzan]

I apologize if this is not the right place or format for this. I haven't commented before.

The callback for the PageIterator callback is not firing for me for pages after the first. I believe this may be related to the change here.
iterationHelper() checks that the cursor is lower than the collection length and iterates this.cursor until that value exceeds the collection length. It is initialized to 0 in the constructor.

private iterationHelper(): boolean {
...
  while (advance && this.cursor < this.collection.length) {
    const item = this.collection[this.cursor];
    advance = this.callback(item);
    this.cursor++;
  }
...
}

This works for the first page. However, fetchAndUpdateNextPageData() does not reset the cursor. That means that for each subsequent page, the cursor is already at the collection length, effectively skipping the callback while continuing to iterate through the pages.

I believe adding this.cursor = 0; to the end of the fetchAndUpdateNextPageData() function resolves the problem for me

private async fetchAndUpdateNextPageData(): Promise<any> {
...
const response: PageCollection = await graphRequest.get();
  this.collection = response.value;
  this.nextLink = response["@odata.nextLink"];
  this.deltaLink = response["@odata.deltaLink"];
  this.cursor = 0; // Fixes issue for me
}

[sebastienlevert]

This has been fixed in 3.0.4 and merged here : #1046

Can you update to the latest version and test if this solves the issue? Thanks!

[NickFenzan]

Oops, thought I had the most recent version. Yes, that works. Thanks!