diff --git a/src/cache.ts b/src/cache.ts index ecfe99953..3022b9f72 100644 --- a/src/cache.ts +++ b/src/cache.ts @@ -2,7 +2,7 @@ import { Lru } from "toad-cache"; /* v8 ignore next */ import type { - InstallationAuthOptions, + CacheableInstallationAuthOptions, Cache, CacheData, Permissions, @@ -21,7 +21,7 @@ export function getCache() { export async function get( cache: Cache, - options: InstallationAuthOptions, + options: CacheableInstallationAuthOptions, ): Promise { const cacheKey = optionsToCacheKey(options); const result = await cache.get(cacheKey); @@ -66,7 +66,7 @@ export async function get( } export async function set( cache: Cache, - options: InstallationAuthOptions, + options: CacheableInstallationAuthOptions, data: CacheData, ): Promise { const key = optionsToCacheKey(options); @@ -91,12 +91,13 @@ export async function set( await cache.set(key, value); } -function optionsToCacheKey({ +// TODO: consider baseUrl and appId too, so that we don't incorrectly cache tokens across them +export function optionsToCacheKey({ installationId, permissions = {}, repositoryIds = [], repositoryNames = [], -}: InstallationAuthOptions) { +}: CacheableInstallationAuthOptions) { const permissionsString = Object.keys(permissions) .sort() .map((name) => (permissions[name] === "read" ? name : `${name}!`)) diff --git a/src/get-installation-authentication.ts b/src/get-installation-authentication.ts index 889e5ee27..08758f633 100644 --- a/src/get-installation-authentication.ts +++ b/src/get-installation-authentication.ts @@ -1,4 +1,4 @@ -import { get, set } from "./cache.js"; +import { get, optionsToCacheKey, set } from "./cache.js"; import { getAppAuthentication } from "./get-app-authentication.js"; import { toTokenAuthentication } from "./to-token-authentication.js"; import type { @@ -6,6 +6,7 @@ import type { InstallationAccessTokenAuthentication, RequestInterface, State, + CacheableInstallationAuthOptions, } from "./types.js"; export async function getInstallationAuthentication( @@ -30,16 +31,45 @@ export async function getInstallationAuthentication( return factory(factoryAuthOptions); } - const optionsWithInstallationTokenFromState = Object.assign( - { installationId }, - options, + const request = customRequest || state.request; + + return getInstallationAuthenticationConcurrently( + state, + { ...options, installationId }, + request, ); +} +const pendingPromises = new Map< + string, + Promise +>(); + +function getInstallationAuthenticationConcurrently( + state: State, + options: CacheableInstallationAuthOptions, + request: RequestInterface, +) { + const cacheKey = optionsToCacheKey(options); + if (pendingPromises.has(cacheKey)) { + return pendingPromises.get(cacheKey)!; + } + const promise = getInstallationAuthenticationImpl( + state, + options, + request, + ).finally(() => pendingPromises.delete(cacheKey)); + pendingPromises.set(cacheKey, promise); + return promise; +} + +async function getInstallationAuthenticationImpl( + state: State, + options: CacheableInstallationAuthOptions, + request: RequestInterface, +) { if (!options.refresh) { - const result = await get( - state.cache, - optionsWithInstallationTokenFromState, - ); + const result = await get(state.cache, options); if (result) { const { @@ -54,7 +84,7 @@ export async function getInstallationAuthentication( } = result; return toTokenAuthentication({ - installationId, + installationId: options.installationId, token, createdAt, expiresAt, @@ -68,10 +98,9 @@ export async function getInstallationAuthentication( } const appAuthentication = await getAppAuthentication(state); - const request = customRequest || state.request; const payload = { - installation_id: installationId, + installation_id: options.installationId, mediaType: { previews: ["machine-man"], }, @@ -136,10 +165,10 @@ export async function getInstallationAuthentication( Object.assign(payload, { singleFileName }); } - await set(state.cache, optionsWithInstallationTokenFromState, cacheOptions); + await set(state.cache, options, cacheOptions); const cacheData = { - installationId, + installationId: options.installationId, token, createdAt, expiresAt, diff --git a/src/types.ts b/src/types.ts index 6f7d1d3b2..5134a52ea 100644 --- a/src/types.ts +++ b/src/types.ts @@ -58,6 +58,11 @@ export type InstallationAuthOptionsWithFactory = { [key: string]: unknown; }; +// normalized installation auth options that can be used for caching +export type CacheableInstallationAuthOptions = InstallationAuthOptions & { + installationId: number; +}; + export type OAuthAppAuthOptions = OAuthAppAuth.AppAuthOptions; export type OAuthWebFlowAuthOptions = OAuthAppAuth.WebFlowAuthOptions; export type OAuthDeviceFlowAuthOptions = diff --git a/test/index.test.ts b/test/index.test.ts index 612aec385..a5a9c6bab 100644 --- a/test/index.test.ts +++ b/test/index.test.ts @@ -1,4 +1,4 @@ -import fetchMock, { type MockMatcherFunction } from "fetch-mock"; +import fetchMock from "fetch-mock"; import { request } from "@octokit/request"; import { vi, beforeEach, test, expect } from "vitest"; @@ -6,6 +6,8 @@ import { vi, beforeEach, test, expect } from "vitest"; import { createAppAuth, createOAuthUserAuth } from "../src/index.ts"; import type { FactoryInstallation } from "../src/types.ts"; +type MockMatcherFunction = fetchMock.MockMatcherFunction; + const APP_ID = 1; const PRIVATE_KEY = `-----BEGIN RSA PRIVATE KEY----- MIIEpAIBAAKCAQEA1c7+9z5Pad7OejecsQ0bu3aozN3tihPmljnnudb9G3HECdnH @@ -397,6 +399,111 @@ test("repositoryIds auth option", async () => { }); }); +test("installationId strategy option coalesces concurrent calls with the same options", async () => { + const sandbox = fetchMock + .sandbox() + .post("https://api.github.com/app/installations/123/access_tokens", { + token: "secret123", + expires_at: "1970-01-01T01:00:00.000Z", + permissions: { + metadata: "read", + }, + repository_selection: "all", + }) + .postOnce("https://api.github.com/app/installations/234/access_tokens", { + token: "secret234", + expires_at: "1970-01-01T01:00:00.000Z", + permissions: { + metadata: "read", + }, + repository_selection: "all", + }); + const requestMock = request.defaults({ + headers: { + "user-agent": "test", + }, + request: { + fetch: sandbox, + }, + }); + + const auth = createAppAuth({ + appId: APP_ID, + privateKey: PRIVATE_KEY, + installationId: 123, + request: requestMock, + }); + const unrelatedAuth = createAppAuth({ + appId: APP_ID, + privateKey: PRIVATE_KEY, + installationId: 234, + request: requestMock, + }); + + const authentications = await Promise.all([ + auth({ type: "installation", refresh: true }), + auth({ type: "installation", refresh: true }), + unrelatedAuth({ type: "installation", refresh: true }), + ]); + expect(authentications[0]).toEqual({ + type: "token", + token: "secret123", + tokenType: "installation", + installationId: 123, + permissions: { + metadata: "read", + }, + createdAt: "1970-01-01T00:00:00.000Z", + expiresAt: "1970-01-01T01:00:00.000Z", + repositorySelection: "all", + }); + // same result as the first one + expect(authentications[1]).toBe(authentications[0]); + // and only request was made + expect( + sandbox.calls("https://api.github.com/app/installations/123/access_tokens") + .length, + ).toBe(1); + + // whereas the auth for the unrelated installation got a different token + expect(authentications[2]).toEqual({ + type: "token", + token: "secret234", + tokenType: "installation", + installationId: 234, + permissions: { + metadata: "read", + }, + createdAt: "1970-01-01T00:00:00.000Z", + expiresAt: "1970-01-01T01:00:00.000Z", + repositorySelection: "all", + }); + + // now that the original auth call has resolved, if we do it again it will actually run + const laterAuthentication = await auth({ + type: "installation", + refresh: true, + }); + expect(laterAuthentication).toEqual({ + type: "token", + token: "secret123", + tokenType: "installation", + installationId: 123, + permissions: { + metadata: "read", + }, + createdAt: "1970-01-01T00:00:00.000Z", + expiresAt: "1970-01-01T01:00:00.000Z", + repositorySelection: "all", + }); + + // a fresh request was made + expect( + sandbox.calls("https://api.github.com/app/installations/123/access_tokens") + .length, + ).toBe(2); +}); + test("repositoryNames auth option", async () => { const matchCreateInstallationAccessToken: MockMatcherFunction = ( _url,