From 7169b401bb98af860a496a89c306d54f2c579e70 Mon Sep 17 00:00:00 2001 From: Douglas Johnson Date: Mon, 21 Apr 2025 13:40:23 -0500 Subject: [PATCH] feat: add a simple in-memory cache for suborg/repo config fetches --- lib/settings.js | 48 ++++++++-- test/unit/lib/settings.test.js | 155 +++++++++++++++++++++++++++++++++ 2 files changed, 197 insertions(+), 6 deletions(-) diff --git a/lib/settings.js b/lib/settings.js index 8f902252..99805bb3 100644 --- a/lib/settings.js +++ b/lib/settings.js @@ -10,7 +10,11 @@ const env = require('./env') const CONFIG_PATH = env.CONFIG_PATH const eta = new Eta({ views: path.join(__dirname) }) const SCOPE = { ORG: 'org', REPO: 'repo' } // Determine if the setting is a org setting or repo setting +const yaml = require('js-yaml'); + class Settings { + static fileCache = {}; + static async syncAll (nop, context, repo, config, ref) { const settings = new Settings(nop, context, repo, config, ref) try { @@ -758,7 +762,7 @@ ${this.results.reduce((x, y) => { } )) { delete subOrgConfigs[key] - } + } } } return subOrgConfigs @@ -788,12 +792,33 @@ ${this.results.reduce((x, y) => { * @param params Params to fetch the file with * @return The parsed YAML file */ - async loadYaml (filePath) { + async loadYaml(filePath) { try { const repo = { owner: this.repo.owner, repo: env.ADMIN_REPO } - const params = Object.assign(repo, { path: filePath, ref: this.ref }) + const params = Object.assign(repo, { + path: filePath, + ref: this.ref + }) + const namespacedFilepath = `${this.repo.owner}/${filePath}`; + + // If the filepath already exists in the fileCache, add the etag to the params + // to check if the file has changed + if (Settings.fileCache[namespacedFilepath]) { + params.headers = { + 'If-None-Match': Settings.fileCache[namespacedFilepath].etag + } + } + const response = await this.github.repos.getContent(params).catch(e => { + if (e.status === 304) { + this.log.debug(`Cache hit for file ${filePath}`) + return { + ...Settings.fileCache[namespacedFilepath], + cached: true + } + } this.log.error(`Error getting settings ${e}`) + throw e }) // Ignore in case path is a folder @@ -808,8 +833,19 @@ ${this.results.reduce((x, y) => { if (typeof response.data.content !== 'string') { return } - const yaml = require('js-yaml') - return yaml.load(Buffer.from(response.data.content, 'base64').toString()) || {} + + const content = yaml.load(Buffer.from(response.data.content, 'base64').toString()) || {} + + // Cache the content, as its either new or changed + if (!response.cached) { + this.log.debug(`Cache miss for file ${filePath}`) + Settings.fileCache[namespacedFilepath] = { + etag: response.headers.etag, + data: response.data + } + } + + return content } catch (e) { if (e.status === 404) { return null @@ -862,7 +898,7 @@ ${this.results.reduce((x, y) => { throw new Error(`Failed to filter repositories for property ${name}: ${error.message}`) } } - + async getSubOrgRepositories (subOrgProperties) { const organizationName = this.repo.owner diff --git a/test/unit/lib/settings.test.js b/test/unit/lib/settings.test.js index 8b2ec618..61f5ae00 100644 --- a/test/unit/lib/settings.test.js +++ b/test/unit/lib/settings.test.js @@ -303,4 +303,159 @@ repository: }) }) // loadConfigs + describe('loadYaml', () => { + let settings; + + beforeEach(() => { + Settings.fileCache = {}; + stubContext = { + octokit: { + repos: { + getContent: jest.fn() + }, + request: jest.fn(), + paginate: jest.fn() + }, + log: { + debug: jest.fn(), + info: jest.fn(), + error: jest.fn() + }, + payload: { + installation: { + id: 123 + } + } + }; + settings = createSettings({}); + }); + + it('should return parsed YAML content when file is fetched successfully', async () => { + // Given + const filePath = 'path/to/file.yml'; + const content = Buffer.from('key: value').toString('base64'); + jest.spyOn(settings.github.repos, 'getContent').mockResolvedValue({ + data: { content }, + headers: { etag: 'etag123' } + }); + + // When + const result = await settings.loadYaml(filePath); + + // Then + expect(result).toEqual({ key: 'value' }); + expect(Settings.fileCache[`${mockRepo.owner}/${filePath}`]).toEqual({ + etag: 'etag123', + data: { content } + }); + }); + + it('should return cached content when file has not changed (304 response)', async () => { + // Given + const filePath = 'path/to/file.yml'; + const content = Buffer.from('key: value').toString('base64'); + Settings.fileCache[`${mockRepo.owner}/${filePath}`] = { etag: 'etag123', data: { content } }; + jest.spyOn(settings.github.repos, 'getContent').mockRejectedValue({ status: 304 }); + + // When + const result = await settings.loadYaml(filePath); + + // Then + expect(result).toEqual({ key: 'value' }); + expect(settings.github.repos.getContent).toHaveBeenCalledWith( + expect.objectContaining({ headers: { 'If-None-Match': 'etag123' } }) + ); + }); + + it('should not return cached content when the cache is for another org', async () => { + // Given + const filePath = 'path/to/file.yml'; + const content = Buffer.from('key: value').toString('base64'); + const wrongContent = Buffer.from('wrong: content').toString('base64'); + Settings.fileCache['another-org/path/to/file.yml'] = { etag: 'etag123', data: { wrongContent } }; + jest.spyOn(settings.github.repos, 'getContent').mockResolvedValue({ + data: { content }, + headers: { etag: 'etag123' } + }); + + // When + const result = await settings.loadYaml(filePath); + + // Then + expect(result).toEqual({ key: 'value' }); + }) + + it('should return null when the file path is a folder', async () => { + // Given + const filePath = 'path/to/folder'; + jest.spyOn(settings.github.repos, 'getContent').mockResolvedValue({ + data: [] + }); + + // When + const result = await settings.loadYaml(filePath); + + // Then + expect(result).toBeNull(); + }); + + it('should return null when the file is a symlink or submodule', async () => { + // Given + const filePath = 'path/to/symlink'; + jest.spyOn(settings.github.repos, 'getContent').mockResolvedValue({ + data: { content: null } + }); + + // When + const result = await settings.loadYaml(filePath); + + // Then + expect(result).toBeUndefined(); + }); + + it('should handle 404 errors gracefully and return null', async () => { + // Given + const filePath = 'path/to/nonexistent.yml'; + jest.spyOn(settings.github.repos, 'getContent').mockRejectedValue({ status: 404 }); + + // When + const result = await settings.loadYaml(filePath); + + // Then + expect(result).toBeNull(); + }); + + it('should throw an error for non-404 exceptions when not in nop mode', async () => { + // Given + const filePath = 'path/to/error.yml'; + jest.spyOn(settings.github.repos, 'getContent').mockRejectedValue(new Error('Unexpected error')); + + // When / Then + await expect(settings.loadYaml(filePath)).rejects.toThrow('Unexpected error'); + }); + + it('should log and append NopCommand for non-404 exceptions in nop mode', async () => { + // Given + const filePath = 'path/to/error.yml'; + settings.nop = true; + jest.spyOn(settings.github.repos, 'getContent').mockRejectedValue(new Error('Unexpected error')); + jest.spyOn(settings, 'appendToResults'); + + // When + const result = await settings.loadYaml(filePath); + + // Then + expect(result).toBeUndefined(); + expect(settings.appendToResults).toHaveBeenCalledWith( + expect.arrayContaining([ + expect.objectContaining({ + type: 'ERROR', + action: expect.objectContaining({ + msg: expect.stringContaining('Unexpected error') + }) + }) + ]) + ); + }); + }); }) // Settings Tests