Fixed cache to cache image state when no image is available (#670)

* factored get image logic to account for 404

* added comment

* fixed JSON.parse exception when reading from cache

* Prevented undefined user responses from being cached

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

Author: nmetulev

packages/mgt/src/graph/graph.people.ts

@@ -247,7 +247,7 @@ export function getEmailFromGraphEntity(entity: IDynamicPerson): string {
  * @returns {(Promise<Contact[]>)}
  * @memberof Graph
  */
-export async function findContactByEmail(graph: IGraph, email: string): Promise<Contact[]> {
+export async function findContactsByEmail(graph: IGraph, email: string): Promise<Contact[]> {
   const scopes = 'contacts.read';
   let cache: CacheStore<CachePerson>;
   if (peopleCacheEnabled()) {
@@ -271,17 +271,3 @@ export async function findContactByEmail(graph: IGraph, email: string): Promise<
 
   return result ? result.value : null;
 }
-
-/**
- * async promise, returns Graph contact and/or Person associated with the email provided
- * Uses: Graph.findPerson(email) and Graph.findContactByEmail(email)
- *
- * @param {string} email
- * @returns {(Promise<Array<Person | Contact>>)}
- * @memberof Graph
- */
-export function findUserByEmail(graph: IGraph, email: string): Promise<Array<Person | Contact>> {
-  return Promise.all([findPeople(graph, email), findContactByEmail(graph, email)]).then(([people, contacts]) => {
-    return ((people as any[]) || []).concat(contacts || []);
-  });
-}

packages/mgt/src/graph/graph.photos.ts

@@ -11,7 +11,8 @@ import * as MicrosoftGraph from '@microsoft/microsoft-graph-types';
 import { CacheItem, CacheSchema, CacheService, CacheStore } from '../utils/Cache';
 import { prepScopes } from '../utils/GraphHelpers';
 import { blobToBase64 } from '../utils/Utils';
-import { findContactByEmail, findUserByEmail, getEmailFromGraphEntity } from './graph.people';
+import { findContactsByEmail, getEmailFromGraphEntity } from './graph.people';
+import { findUsers } from './graph.user';
 import { IDynamicPerson } from './types';
 
 /**
@@ -76,9 +77,15 @@ export async function getPhotoForResource(graph: IGraph, resource: string, scope
       .middlewareOptions(prepScopes(...scopes))
       .get()) as Response;
 
-    if (!response.ok) {
+    if (response.status === 404) {
+      // 404 means the resource does not have a photo
+      // we still want to cache that state
+      // so we return an object that can be cached
+      return { eTag: null, photo: null };
+    } else if (!response.ok) {
       return null;
     }
+
     const eTag = response.headers.get('eTag');
     const blob = await blobToBase64(await response.blob());
     return { eTag, photo: blob };
@@ -195,55 +202,51 @@ export async function myPhoto(graph: IGraph): Promise<string> {
  * @export
  */
 export async function getPersonImage(graph: IGraph, person: IDynamicPerson) {
-  let image: string;
-  let email: string;
-
-  if ((person as MicrosoftGraph.Person).userPrincipalName) {
-    // try to find a user by userPrincipalName
-    const userPrincipalName = (person as MicrosoftGraph.Person).userPrincipalName;
-    image = await getUserPhoto(graph, userPrincipalName);
-  } else if ('personType' in person && (person as any).personType.subclass === 'PersonalContact') {
+  // handle if contact
+  if ('personType' in person && (person as any).personType.subclass === 'PersonalContact') {
     // if person is a contact, look for them and their photo in contact api
-    email = getEmailFromGraphEntity(person);
-    const contact = await findContactByEmail(graph, email);
+    let email = getEmailFromGraphEntity(person);
+    const contact = await findContactsByEmail(graph, email);
     if (contact && contact.length && contact[0].id) {
-      image = await getContactPhoto(graph, contact[0].id);
+      return await getContactPhoto(graph, contact[0].id);
     }
-  } else if (person.id) {
-    image = await getUserPhoto(graph, person.id);
+
+    return null;
+  }
+
+  // handle if user
+  if ((person as MicrosoftGraph.Person).userPrincipalName || person.id) {
+    // try to find a user by userPrincipalName
+    const id = (person as MicrosoftGraph.Person).userPrincipalName || person.id;
+    return await getUserPhoto(graph, id);
   }
-  if (image) {
-    return image;
+
+  // else assume id is for user and try to get photo
+  if (person.id) {
+    let image = await getUserPhoto(graph, person.id);
+    if (image) {
+      return image;
+    }
   }
 
-  // try to find a user by e-mail
-  email = getEmailFromGraphEntity(person);
+  // let's try to find a person by the email
+  let email = getEmailFromGraphEntity(person);
 
   if (email) {
-    const users = await findUserByEmail(graph, email);
+    // try to find user
+    const users = await findUsers(graph, email, 1);
     if (users && users.length) {
-      // Check for an OrganizationUser
-      const orgUser = users.find(p => {
-        return (p as any).personType && (p as any).personType.subclass === 'OrganizationUser';
-      });
-      if (orgUser) {
-        // Lookup by userId
-        const userId = (users[0] as MicrosoftGraph.Person).scoredEmailAddresses[0].address;
-        image = await getUserPhoto(graph, userId);
-      } else {
-        // Lookup by contactId
-        for (const user of users) {
-          const contactId = user.id;
-          image = await getContactPhoto(graph, contactId);
-          if (image) {
-            break;
-          }
-        }
-      }
+      return await getUserPhoto(graph, users[0].id);
+    }
+
+    // if no user, try to find a contact
+    const contacts = await findContactsByEmail(graph, email);
+    if (contacts && contacts.length) {
+      return await getContactPhoto(graph, contacts[0].id);
     }
   }
 
-  return image;
+  return null;
 }
 
 /**

packages/mgt/src/graph/graph.user.ts

@@ -111,20 +111,23 @@ export async function getMe(graph: IGraph): Promise<User> {
  * @memberof Graph
  */
 export async function getUserWithPhoto(graph: IGraph, userId?: string): Promise<IDynamicPerson> {
-  let person = null as IDynamicPerson;
-  let cache: CacheStore<CacheUser>;
   let photo = null;
-  let user: IDynamicPerson;
+  let user: IDynamicPerson = null;
+
   let cachedPhoto: CachePhoto;
+  let cachedUser: CacheUser;
+
   const resource = userId ? `users/${userId}` : 'me';
   const scopes = userId ? ['user.readbasic.all'] : ['user.read'];
 
   // attempt to get user and photo from cache if enabled
   if (usersCacheEnabled()) {
-    cache = CacheService.getCache<CacheUser>(cacheSchema, userStore);
-    const cachedUser = await cache.getValue(userId || 'me');
+    let cache = CacheService.getCache<CacheUser>(cacheSchema, userStore);
+    cachedUser = await cache.getValue(userId || 'me');
     if (cachedUser && getUserInvalidationTime() > Date.now() - cachedUser.timeCached) {
-      user = JSON.parse(cachedUser.user);
+      user = cachedUser.user ? JSON.parse(cachedUser.user) : null;
+    } else {
+      cachedUser = null;
     }
   }
   if (photosCacheEnabled()) {
@@ -138,12 +141,15 @@ export async function getUserWithPhoto(graph: IGraph, userId?: string): Promise<
           // put current image into the cache to update the timestamp since etag is the same
           storePhotoInCache(userId || 'me', 'users', cachedPhoto);
           photo = cachedPhoto.photo;
+        } else {
+          cachedPhoto = null;
         }
       } catch (e) {}
     }
   }
 
-  if (!photo && !user) {
+  // if both are not in the cache, batch get them
+  if (!cachedPhoto && !cachedUser) {
     let eTag: string;
 
     // batch calls
@@ -170,45 +176,41 @@ export async function getUserWithPhoto(graph: IGraph, userId?: string): Promise<
 
     // store user & photo in their respective cache
     if (usersCacheEnabled()) {
+      let cache = CacheService.getCache<CacheUser>(cacheSchema, userStore);
       cache.putValue(userId || 'me', { user: JSON.stringify(user) });
     }
     if (photosCacheEnabled()) {
       storePhotoInCache(userId || 'me', 'users', { eTag, photo: photo });
     }
-  } else if (!photo) {
-    // get photo from graph
-    const resource = userId ? `users/${userId}` : 'me';
-    const scopes = userId ? ['user.readbasic.all'] : ['user.read'];
+  } else if (!cachedPhoto) {
+    // if only photo or user is not cached, get it individually
     const response = await getPhotoForResource(graph, resource, scopes);
     if (response) {
       if (photosCacheEnabled()) {
         storePhotoInCache(userId || 'me', 'users', { eTag: response.eTag, photo: response.photo });
       }
       photo = response.photo;
     }
-  } else if (!user) {
+  } else if (!cachedUser) {
     // get user from graph
-    const response = userId
-      ? await graph
-          .api(`/users/${userId}`)
-          .middlewareOptions(prepScopes('user.readbasic.all'))
-          .get()
-      : await graph
-          .api('me')
-          .middlewareOptions(prepScopes('user.read'))
-          .get();
+    const response = await graph
+      .api(resource)
+      .middlewareOptions(prepScopes(...scopes))
+      .get();
+
     if (response) {
       if (usersCacheEnabled()) {
+        let cache = CacheService.getCache<CacheUser>(cacheSchema, userStore);
         cache.putValue(userId || 'me', { user: JSON.stringify(response) });
       }
       user = response;
     }
   }
+
   if (user) {
-    person = user;
-    person.personImage = photo;
+    user.personImage = photo;
   }
-  return person;
+  return user;
 }
 
 /**
@@ -229,7 +231,7 @@ export async function getUser(graph: IGraph, userPrincipleName: string): Promise
     // is it stored and is timestamp good?
     if (user && getUserInvalidationTime() > Date.now() - user.timeCached) {
       // return without any worries
-      return JSON.parse(user.user);
+      return user.user ? JSON.parse(user.user) : null;
     }
   }
   // else we must grab it
@@ -271,7 +273,7 @@ export async function getUsersForUserIds(graph: IGraph, userIds: string[]): Prom
       user = await cache.getValue(id);
     }
     if (user && getUserInvalidationTime() > Date.now() - user.timeCached) {
-      peopleDict[id] = JSON.parse(user.user);
+      peopleDict[id] = user.user ? JSON.parse(user.user) : null;
     } else if (id !== '') {
       batch.get(id, `/users/${id}`, ['user.readbasic.all']);
       notInCache.push(id);
@@ -399,14 +401,18 @@ export async function findUsers(graph: IGraph, query: string, top: number = 10):
     }
   }
 
-  const graphResult = await graph
-    .api('users')
-    .filter(
-      `startswith(displayName,'${query}') or startswith(givenName,'${query}') or startswith(surname,'${query}') or startswith(mail,'${query}') or startswith(userPrincipalName,'${query}')`
-    )
-    .top(top)
-    .middlewareOptions(prepScopes(scopes))
-    .get();
+  let graphResult;
+
+  try {
+    graphResult = await graph
+      .api('users')
+      .filter(
+        `startswith(displayName,'${query}') or startswith(givenName,'${query}') or startswith(surname,'${query}') or startswith(mail,'${query}') or startswith(userPrincipalName,'${query}')`
+      )
+      .top(top)
+      .middlewareOptions(prepScopes(scopes))
+      .get();
+  } catch {}
 
   if (usersCacheEnabled() && graphResult) {
     item.results = graphResult.value.map(userStr => JSON.stringify(userStr));