Fixed batch calls with more than 20 requests, added retry logic (#472)

* fixed batch calls with more than 20 requests, and added retry logic for throttling

* updated retry-after parsing

Co-authored-by: Shane Weaver <[email protected]>

Author: nmetulev

src/components/mgt-login/mgt-login.ts

@@ -14,6 +14,7 @@ import { MgtFlyout } from '../sub-components/mgt-flyout/mgt-flyout';
 import { MgtTemplatedComponent } from '../templatedComponent';
 import { styles } from './mgt-login-css';
 
+import { getUserWithPhoto } from '../../graph/graph.user';
 import '../../styles/fabric-icon-font';
 import '../mgt-person/mgt-person';
 import { PersonViewType } from '../mgt-person/mgt-person';
@@ -172,13 +173,11 @@ export class MgtLogin extends MgtTemplatedComponent {
     const provider = Providers.globalProvider;
     if (provider && !this.userDetails) {
       if (provider.state === ProviderState.SignedIn) {
-        const batch = provider.graph.forComponent(this).createBatch();
-        batch.get('me', 'me', ['user.read']);
-        batch.get('photo', 'me/photo/$value', ['user.read']);
-        const response = await batch.execute();
+        this.userDetails = await getUserWithPhoto(provider.graph.forComponent(this));
 
-        this._image = response.photo;
-        this.userDetails = response.me;
+        if (this.userDetails.personImage) {
+          this._image = this.userDetails.personImage;
+        }
       } else {
         this.userDetails = null;
       }

src/components/mgt-teams-channel-picker/mgt-teams-channel-picker.graph.ts

@@ -19,7 +19,7 @@ export async function getAllMyTeams(graph: IGraph): Promise<Team[]> {
   const teams = await graph
     .api('/me/joinedTeams')
     .middlewareOptions(prepScopes('User.Read.All'))
-    .select(['displayName', 'id'])
+    .select(['displayName', 'id', 'isArchived'])
     .get();
   return teams ? teams.value : null;
 }

src/components/mgt-teams-channel-picker/mgt-teams-channel-picker.ts

@@ -535,7 +535,7 @@ export class MgtTeamsChannelPicker extends MgtTemplatedComponent {
   }
 
   /**
-   * Quieries Microsoft Graph for Teams & respective channels then sets to items list
+   * Queries Microsoft Graph for Teams & respective channels then sets to items list
    *
    * @protected
    * @memberof MgtTeamsChannelPicker
@@ -547,27 +547,34 @@ export class MgtTeamsChannelPicker extends MgtTemplatedComponent {
       const graph = provider.graph.forComponent(this);
 
       teams = await getAllMyTeams(graph);
+      teams = teams.filter(t => !t.isArchived);
 
       const batch = provider.graph.createBatch();
 
-      for (const [i, team] of teams.entries()) {
-        batch.get(`${i}`, `teams/${team.id}/channels`, ['group.read.all']);
+      for (const team of teams) {
+        batch.get(team.id, `teams/${team.id}/channels`, ['group.read.all']);
       }
-      const response = await batch.execute();
-      this.items = teams.map(t => {
-        return {
-          item: t
-        };
-      });
-      for (const [i] of teams.entries()) {
-        if (response[i] && response[i].value) {
-          this.items[i].channels = response[i].value.map(c => {
+
+      const responses = await batch.executeAll();
+
+      for (const team of teams) {
+        const response = responses.get(team.id);
+
+        if (response && response.content && response.content.value) {
+          team.channels = response.content.value.map(c => {
             return {
               item: c
             };
           });
         }
       }
+
+      this.items = teams.map(t => {
+        return {
+          channels: t.channels as DropdownItem[],
+          item: t
+        };
+      });
     }
     this.filterList();
     this.resetFocusState();

src/graph/graph.presence.ts

@@ -66,7 +66,13 @@ export async function getUsersPresenceByPeople(graph: IGraph, people?: IDynamicP
   }
 
   try {
-    const peoplePresence = await batch.execute();
+    const peoplePresence = {};
+    const response = await batch.executeAll();
+
+    for (const r of response.values()) {
+      peoplePresence[r.id] = r.content;
+    }
+
     return peoplePresence;
   } catch (_) {
     try {

src/graph/graph.user.ts

@@ -43,11 +43,15 @@ export async function getUserWithPhoto(graph: IGraph, userId?: string): Promise<
     batch.get('user', 'me', ['user.read']);
     batch.get('photo', 'me/photo/$value', ['user.read']);
   }
-  const response = await batch.execute();
+  const response = await batch.executeAll();
 
-  if (response && response.user) {
-    person = response.user;
-    person.personImage = response.photo;
+  const photoResponse = response.get('photo');
+  const userDetailsResponse = response.get('user');
+
+  if (userDetailsResponse.content) {
+    person = userDetailsResponse.content;
+
+    person.personImage = photoResponse.content;
   }
 
   return person;
@@ -90,14 +94,14 @@ export async function getUsersForUserIds(graph: IGraph, userIds: string[]): Prom
   }
 
   try {
-    const response = await batch.execute();
+    const responses = await batch.executeAll();
     const people = [];
 
     // iterate over userIds to ensure the order of ids
     for (const id of userIds) {
-      const person = response[id];
-      if (person && person.id) {
-        people.push(person);
+      const response = responses.get(id);
+      if (response && response.content) {
+        people.push(response.content);
       }
     }
 
@@ -135,12 +139,12 @@ export async function getUsersForPeopleQueries(graph: IGraph, peopleQueries: str
   }
 
   try {
-    const response = await batch.execute();
+    const responses = await batch.executeAll();
 
     for (const personQuery of peopleQueries) {
-      const person = response[personQuery];
-      if (person && person.value && person.value.length) {
-        people.push(person.value[0]);
+      const response = responses.get(personQuery);
+      if (response && response.content && response.content.value && response.content.value.length > 0) {
+        people.push(response.content.value[0]);
       }
     }
 

src/utils/Batch.ts

@@ -8,9 +8,10 @@
 import { BatchRequestContent, MiddlewareOptions } from '@microsoft/microsoft-graph-client';
 import { IGraph } from '../IGraph';
 import { prepScopes } from './GraphHelpers';
+import { delay } from './Utils';
 
 /**
- * Method to reduce repetitive requests to the Graph utilized in Batch
+ * Represents a request to be executed in a batch
  *
  * @class BatchRequest
  */
@@ -21,6 +22,7 @@ export class BatchRequest {
    * @type {string}
    * @memberof BatchRequest
    */
+
   public resource: string;
   /**
    * method passed to be requested
@@ -29,15 +31,71 @@ export class BatchRequest {
    * @memberof BatchRequest
    */
   public method: string;
-  constructor(resource: string, method: string) {
+
+  /**
+   * The index of the requests as it was added to the queue
+   * Use this value if you need to sort the responses
+   * in the order they were added
+   *
+   * @type {number}
+   * @memberof BatchRequest
+   */
+  public index: number;
+
+  /**
+   * The id of the requests
+   *
+   * @type {string}
+   * @memberof BatchRequest
+   */
+  public id: string;
+
+  constructor(index, id, resource: string, method: string) {
     if (resource.charAt(0) !== '/') {
       resource = '/' + resource;
     }
     this.resource = resource;
     this.method = method;
+    this.index = index;
+    this.id = id;
   }
 }
 
+/**
+ * Represents a response from a request executed in a batch
+ *
+ * @export
+ * @class BatchResponse
+ */
+// tslint:disable-next-line:max-classes-per-file
+export class BatchResponse {
+  /**
+   * The index of the requests as it was added to the queue
+   * Use this value if you need to sort the responses
+   * in the order they were added
+   *
+   * @type {number}
+   * @memberof BatchResponse
+   */
+  public index: number;
+
+  /**
+   * The id of the requests
+   *
+   * @type {string}
+   * @memberof BatchResponse
+   */
+  public id: string;
+
+  /**
+   * The content of the response
+   *
+   * @type {*}
+   * @memberof BatchResponse
+   */
+  public content: any;
+}
+
 /**
  * Method to reduce repetitive requests to the Graph
  *
@@ -49,12 +107,33 @@ export class Batch {
   // this doesn't really mater what it is as long as it's a root base url
   // otherwise a Request assumes the current path and that could change the relative path
   private static baseUrl = 'https://graph.microsoft.com';
-  private requests: Map<string, BatchRequest> = new Map<string, BatchRequest>();
-  private scopes: string[] = [];
+
+  private allRequests: BatchRequest[];
+  private requestsQueue: number[];
+  private scopes: string[];
+  private retryAfter: number;
+
   private graph: IGraph;
 
+  private nextIndex: number;
+
   constructor(graph: IGraph) {
     this.graph = graph;
+    this.allRequests = [];
+    this.requestsQueue = [];
+    this.scopes = [];
+    this.nextIndex = 0;
+    this.retryAfter = 0;
+  }
+
+  /**
+   * Get whether there are requests that have not been executed
+   *
+   * @readonly
+   * @memberof Batch
+   */
+  public get hasRequests() {
+    return this.requestsQueue.length > 0;
   }
 
   /**
@@ -66,31 +145,44 @@ export class Batch {
    * @memberof Batch
    */
   public get(id: string, resource: string, scopes?: string[]) {
-    const request = new BatchRequest(resource, 'GET');
-    this.requests.set(id, request);
+    const index = this.nextIndex++;
+    const request = new BatchRequest(index, id, resource, 'GET');
+    this.allRequests.push(request);
+    this.requestsQueue.push(index);
     if (scopes) {
       this.scopes = this.scopes.concat(scopes);
     }
   }
 
   /**
-   * Promise to handle Graph request response
+   * Execute the next set of requests.
+   * This will execute up to 20 requests at a time
    *
-   * @returns {Promise<any>}
+   * @returns {Promise<Map<string, BatchResponse>>}
    * @memberof Batch
    */
-  public async execute(): Promise<any> {
-    const responses = {};
-    if (!this.requests.size) {
+  public async executeNext(): Promise<Map<string, BatchResponse>> {
+    const responses: Map<string, BatchResponse> = new Map();
+
+    if (this.retryAfter) {
+      await delay(this.retryAfter * 1000);
+      this.retryAfter = 0;
+    }
+
+    if (!this.hasRequests) {
       return responses;
     }
 
+    // batch can support up to 20 requests
+    const nextBatch = this.requestsQueue.splice(0, 20);
+
     const batchRequestContent = new BatchRequestContent();
-    for (const request of this.requests) {
+
+    for (const request of nextBatch.map(i => this.allRequests[i])) {
       batchRequestContent.addRequest({
-        id: request[0],
-        request: new Request(Batch.baseUrl + request[1].resource, {
-          method: request[1].method
+        id: request.index.toString(),
+        request: new Request(Batch.baseUrl + request.resource, {
+          method: request.method
         })
       });
     }
@@ -101,13 +193,47 @@ export class Batch {
     const batchRequestBody = await batchRequestContent.getContent();
     const batchResponse = await batchRequest.post(batchRequestBody);
 
-    for (const response of batchResponse.responses) {
-      if (response.status !== 200) {
-        response[response.id] = null;
-      } else if (response.headers['Content-Type'].includes('image/jpeg')) {
-        responses[response.id] = 'data:image/jpeg;base64,' + response.body;
+    for (const r of batchResponse.responses) {
+      const response = new BatchResponse();
+      const request = this.allRequests[r.id];
+
+      response.id = request.id;
+      response.index = request.index;
+
+      if (r.status !== 200) {
+        if (r.status === 429) {
+          // this request was throttled
+          // add request back to queue and set retry wait time
+          this.requestsQueue.unshift(r.id);
+          this.retryAfter = Math.max(this.retryAfter, parseInt(r.headers['Retry-After'], 10) || 1);
+        }
+        continue;
+      } else if (r.headers['Content-Type'].includes('image/jpeg')) {
+        response.content = 'data:image/jpeg;base64,' + r.body;
       } else {
-        responses[response.id] = response.body;
+        response.content = r.body;
+      }
+
+      responses.set(request.id, response);
+    }
+
+    return responses;
+  }
+
+  /**
+   * Execute all requests, up to 20 at a time until
+   * all requests have been executed
+   *
+   * @returns {Promise<Map<string, BatchResponse>>}
+   * @memberof Batch
+   */
+  public async executeAll(): Promise<Map<string, BatchResponse>> {
+    const responses: Map<string, BatchResponse> = new Map();
+
+    while (this.hasRequests) {
+      const r = await this.executeNext();
+      for (const [key, value] of r) {
+        responses.set(key, value);
       }
     }