💡 Advanced caching for team members (#99)

This PR adds eTag checking for team member listing. This should result
in significant reductions in GitHub API calls. This will increase
performance of each sync, while also reducing the amount of times rate
limits are hit for larger orgs.

Author: JoshuaTheMiller

package.json

@@ -5,10 +5,10 @@
   "main": "app.ts",
   "module": "es2015",
   "scripts": {
-    "test:unit": "jest test/unit --coverage --reporters=jest-junit",
+    "test:unit": "jest test/unit",
     "test:int": "jest test/integration",
     "test:all": "jest --coverage --reporters=jest-junit",
-    "test": "npm run test:unit",
+    "test": "jest test/unit",
     "dev": "nodemon",
     "start": "node ./out/app.js",
     "build": "tsc && shx cp ./src/openapi.yaml ./out/openapi.yaml",
@@ -26,10 +26,11 @@
     "@types/swagger-ui-express": "^4.1.3",
     "graphql-tag": "^2.12.6",
     "jest": "^29.7.0",
-    "jest-junit": "^16.0.0",
+    "jest-junit": "^16.0.0",    
     "nodemon": "^3.0.2",
     "shx": "^0.3.4",
     "ts-jest": "^29.2.5",
+    "ts-mockito": "^2.6.1",
     "ts-node": "^10.9.1",
     "typescript": "^5.6.3"
   },

src/integrations/redisCacheClient.ts

@@ -0,0 +1,17 @@
+import { CacheClient } from "../app";
+import { ICacheClient } from "../services/CacheClient";
+
+export class RedisCacheClient implements ICacheClient {
+    constructor(private cacheClient: CacheClient){}
+
+    async set(cacheKey: string, object: string, options: { EX: number; }): Promise<undefined> {
+        await this.cacheClient.set(cacheKey, object, {
+            EX: options.EX            
+        }); 
+    }
+
+    async get(cacheKey: string): Promise<string | null> {
+        return await this.cacheClient.get(cacheKey);
+    }
+
+}

src/services/CacheClient.ts

@@ -0,0 +1,4 @@
+export interface ICacheClient {
+    set(cacheKey: string, object: string, options: { EX: number; }): Promise<undefined>;
+    get(cacheKey: string): Promise<string | undefined | null>;
+}

src/services/gitHub.ts

@@ -1,18 +1,19 @@
 import { Octokit } from "octokit";
 import { createAppAuth } from "@octokit/auth-app";
 import { Config } from "../config";
-import { GitHubClient, InstalledClient, Org } from "./gitHubTypes";
+import { GitHubClient, IInstalledClient, Org } from "./gitHubTypes";
 import { AppConfig } from "./appConfig";
 import yaml from "js-yaml";
 import { throttling } from "@octokit/plugin-throttling";
 import { Log, LoggerToUse } from "../logging";
 import { GitHubClientCache } from "./gitHubCache";
 import { redisClient } from "../app";
 import { InstalledGitHubClient } from "./installedGitHubClient";
+import { RedisCacheClient } from "../integrations/redisCacheClient";
 
 const config = Config();
 
-async function GetOrgClient(installationId: number): Promise<InstalledClient> {
+async function GetOrgClient(installationId: number): Promise<IInstalledClient> {
     // TODO: look further into this... it seems like it would be best if 
     // installation client was generated from the original client, and not
     // created fresh.    
@@ -65,9 +66,10 @@ async function GetOrgClient(installationId: number): Promise<InstalledClient> {
 
     // HACK: gross typing nonsense
     const baseClient = new InstalledGitHubClient(installedOctokit, (orgName.data.account as thisShouldNotBeNeeded).login);
+    const redisCacheClient = new RedisCacheClient(redisClient);
 
     if (Config().AppOptions.RedisHost) {
-        const cachedClient = new GitHubClientCache(baseClient, redisClient, LoggerToUse());
+        const cachedClient = new GitHubClientCache(baseClient, redisCacheClient, LoggerToUse());
         return cachedClient;
     }
 

src/services/gitHubCache.ts

@@ -1,26 +1,31 @@
-import { CacheClient } from "../app";
 import { ILogger } from "../logging";
-import { AddMemberResponse, CopilotAddResponse, GitHubTeamId, InstalledClient, OrgConfigResponse, OrgInvite, OrgRoles, RemoveMemberResponse, Response } from "./gitHubTypes";
-
-export class GitHubClientCache implements InstalledClient {
-    client: InstalledClient;
-    cacheClient: CacheClient;
-    logger:ILogger;
-
-    constructor(client: InstalledClient, cacheClient: CacheClient, logger:ILogger) {
-        this.client = client;
-        this.cacheClient = cacheClient;
-        this.logger = logger;
-    }
+import { AddMemberResponse, CopilotAddResponse, GitHubTeamId, IInstalledClient as IInstalledClient, IRawInstalledGitHubClient, OrgConfigResponse, OrgInvite, OrgRoles, RemoveMemberResponse, Response } from "./gitHubTypes";
+import { ICacheClient } from "./CacheClient";
+
+/**
+ * This class decorates the InstalledClient with additional caching logic. In general, 
+ * this logic should be kept simple. However, in an effort to reduce the bleeding of
+ * complex logic into the concrete InstalledClient implementations, some complexity
+ * may be acceptable here (such as checking for eTag and then retrying a call).
+ * 
+ * Chain of responsibility:
+ * 
+ * eTag checker
+ * if there are no changes, check cache and return
+    * if cache has nothing, call client, cache value, then return
+ * if there are changes, check client, cache changes and etag, return
+ */
+export class GitHubClientCache implements IInstalledClient {
+    constructor(private client: IRawInstalledGitHubClient, private cacheClient: ICacheClient, private logger: ILogger) { }
 
     AddTeamsToCopilotSubscription(teamNames: string[]): Response<CopilotAddResponse[]> {
         return this.client.AddTeamsToCopilotSubscription(teamNames);
     }
-    
+
     ListPendingInvitesForTeam(teamName: string): Response<OrgInvite[]> {
         return this.client.ListPendingInvitesForTeam(teamName);
     }
-    
+
     CancelOrgInvite(invite: OrgInvite): Response<unknown> {
         return this.client.CancelOrgInvite(invite);
     }
@@ -45,36 +50,36 @@ export class GitHubClientCache implements InstalledClient {
         return this.client.AddOrgMember(id);
     }
 
-    async IsUserMember(id: string): Response<boolean> {        
+    async IsUserMember(id: string): Response<boolean> {
         const cacheKey = `github-member-1:${id}-${this.GetCurrentOrgName()}`;
 
-        const result = await this.cacheClient.get(cacheKey);        
+        const result = await this.cacheClient.get(cacheKey);
 
         if (result) {
             this.ReportCacheHit({
                 cacheKey: cacheKey,
                 operation: "IsUserMember",
-                value: result,
+                value: JSON.stringify(result),
                 user: id
             });
 
             return {
                 successful: true,
                 data: Boolean(result)
             }
-        }        
+        }
 
         const actualResult = await this.client.IsUserMember(id);
-        
-        if (actualResult.successful) {          
+
+        if (actualResult.successful) {
             // TODO: switch all cache expirations to application configuration values.  
-            
+
             const userIsMember = actualResult.data;
 
-            if(userIsMember) {
+            if (userIsMember) {
                 await this.cacheClient.set(cacheKey, userIsMember.toString(), {
                     EX: 172800 // Expire every 2 days                    
-                });   
+                });
             }
             else {
                 // If membership check comes back as "not a member," we still want to cache
@@ -84,11 +89,11 @@ export class GitHubClientCache implements InstalledClient {
                 await this.cacheClient.set(cacheKey, userIsMember.toString(), {
                     EX: 1800 // Expire every 30 minutes
                     // It is unlikely that 30 minutes will cause much pain
-                });   
-            }              
+                });
+            }
         }
 
-        return actualResult;    
+        return actualResult;
     }
 
     GetAllTeams(): Response<GitHubTeamId[]> {
@@ -106,18 +111,18 @@ export class GitHubClientCache implements InstalledClient {
     async DoesUserExist(gitHubId: string): Response<string> {
         const cacheKey = `github-user-2:${gitHubId}`;
 
-        const result = await this.cacheClient.get(cacheKey);        
+        const result = await this.cacheClient.get(cacheKey);
 
         if (result) {
             this.ReportCacheHit({
                 operation: "DoesUserExist",
                 user: gitHubId,
                 value: result,
                 cacheKey: cacheKey
-            }); 
+            });
 
             return JSON.parse(result);
-        }        
+        }
 
         const actualResult = await this.client.DoesUserExist(gitHubId);
 
@@ -142,8 +147,85 @@ export class GitHubClientCache implements InstalledClient {
         return actualResult;
     }
 
-    ListCurrentMembersOfGitHubTeam(team: string): Response<string[]> {
-        return this.client.ListCurrentMembersOfGitHubTeam(team);
+    async ListCurrentMembersOfGitHubTeam(team: string): Response<string[]> {
+        const teamSlug = `${this.GetCurrentOrgName()}_${team}`;
+        const eTagCacheKey = `t-e:${teamSlug}`;
+        const teamCacheKey = `t:${teamSlug}`;
+
+        const cachedEtag = await this.cacheClient.get(eTagCacheKey) ?? "";
+
+        const eTagResponse = await this.client.ListMembersOfTeamEtagCheck(team, cachedEtag);        
+
+        if (eTagResponse.successful == false) {            
+            return {
+                successful: false
+            }
+        }        
+
+        this.ReportCacheHit({
+            operation: "eTag-TeamMembers",
+            team: team,
+            value: cachedEtag,
+            cacheKey: eTagCacheKey
+        })
+
+        let newETag = "";
+        let teamMembers: string[] | undefined = undefined;        
+
+        if (eTagResponse.successful == "no_changes") {
+            newETag = eTagResponse.eTag;            
+
+            const cachedTeamMembers = await this.cacheClient.get(teamCacheKey);            
+            
+            if (cachedTeamMembers) {                
+                teamMembers = JSON.parse(cachedTeamMembers);
+                this.ReportCacheHit({
+                    operation: "TeamMembers",
+                    team: team,
+                    value: cachedTeamMembers,
+                    cacheKey: teamCacheKey
+                })
+            }
+            else {                
+                const newTeamMembersResponse = await this.client.ListCurrentMembersOfGitHubTeam(team);
+
+                if (newTeamMembersResponse.successful == false) {                    
+                    return {
+                        successful: false
+                    }
+                }
+
+                await this.cacheClient.set(teamCacheKey, JSON.stringify(newTeamMembersResponse.data), { EX: fourteenDaysInSeconds });
+                teamMembers = newTeamMembersResponse.data;
+            }
+        }
+
+        if (eTagResponse.successful == true) {
+            newETag = eTagResponse.data;
+            const newTeamMembersResponse = await this.client.ListCurrentMembersOfGitHubTeam(team);
+
+            if (newTeamMembersResponse.successful == false) {                
+                return {
+                    successful: false
+                }
+            }
+
+            await this.cacheClient.set(teamCacheKey, JSON.stringify(newTeamMembersResponse.data), { EX: fourteenDaysInSeconds });
+            teamMembers = newTeamMembersResponse.data;
+        }
+
+        await this.cacheClient.set(eTagCacheKey, newETag, { EX: fourteenDaysInSeconds });
+
+        if (teamMembers === undefined) {            
+            return {
+                successful: false
+            }
+        }
+
+        return {
+            successful: true,
+            data: teamMembers
+        };
     }
 
     RemoveTeamMemberAsync(team: string, user: string): RemoveMemberResponse {
@@ -160,28 +242,31 @@ export class GitHubClientCache implements InstalledClient {
 
     GetConfigurationForInstallation(): OrgConfigResponse {
         return this.client.GetConfigurationForInstallation();
-    }    
+    }
 
-    private ReportCacheHit(props: {operation: string, user?: string, team?:string, value: string, cacheKey:string}) {
-        const properties:any = {
+    private ReportCacheHit(props: { operation: string, user?: string, team?: string, value: string, cacheKey: string }) {
+        const properties: any = {
             "Group": "GitHub",
             "Operation": props.operation,
-            "Org": this.GetCurrentOrgName(),                        
+            "Org": this.GetCurrentOrgName(),
             "Value": props.value,
             "CacheKey": props.cacheKey
         };
 
-        if(props.user) {
+        if (props.user) {
             properties["User"] = props.user;
         }
 
-        if(props.team) {
+        if (props.team) {
             properties["Team"] = props.team;
         }
 
         this.logger.ReportEvent({
-            Name:"CacheHit",
+            Name: "CacheHit",
             properties: properties
         });
     }
-}
+}
+
+const twoDaysInSeconds = 172_800;
+const fourteenDaysInSeconds = 1_209_600;