Skip to content

Commit 5613370

Browse files
DouJohnDouJohn
authored
feat: add a simple in-memory cache for suborg/repo config fetches (#817)
1 parent e61cf05 commit 5613370

File tree

2 files changed

+197
-6
lines changed

2 files changed

+197
-6
lines changed

‎lib/settings.js‎

Lines changed: 42 additions & 6 deletions
Original file line numberDiff line numberDiff line change
@@ -10,7 +10,11 @@ const env = require('./env')
1010
const CONFIG_PATH = env.CONFIG_PATH
1111
const eta = new Eta({ views: path.join(__dirname) })
1212
const SCOPE = { ORG: 'org', REPO: 'repo' } // Determine if the setting is a org setting or repo setting
13+
const yaml = require('js-yaml');
14+
1315
class Settings {
16+
static fileCache = {};
17+
1418
static async syncAll (nop, context, repo, config, ref) {
1519
const settings = new Settings(nop, context, repo, config, ref)
1620
try {
@@ -758,7 +762,7 @@ ${this.results.reduce((x, y) => {
758762
}
759763
)) {
760764
delete subOrgConfigs[key]
761-
}
765+
}
762766
}
763767
}
764768
return subOrgConfigs
@@ -788,12 +792,33 @@ ${this.results.reduce((x, y) => {
788792
* @param params Params to fetch the file with
789793
* @return The parsed YAML file
790794
*/
791-
async loadYaml (filePath) {
795+
async loadYaml(filePath) {
792796
try {
793797
const repo = { owner: this.repo.owner, repo: env.ADMIN_REPO }
794-
const params = Object.assign(repo, { path: filePath, ref: this.ref })
798+
const params = Object.assign(repo, {
799+
path: filePath,
800+
ref: this.ref
801+
})
802+
const namespacedFilepath = `${this.repo.owner}/${filePath}`;
803+
804+
// If the filepath already exists in the fileCache, add the etag to the params
805+
// to check if the file has changed
806+
if (Settings.fileCache[namespacedFilepath]) {
807+
params.headers = {
808+
'If-None-Match': Settings.fileCache[namespacedFilepath].etag
809+
}
810+
}
811+
795812
const response = await this.github.repos.getContent(params).catch(e => {
813+
if (e.status === 304) {
814+
this.log.debug(`Cache hit for file ${filePath}`)
815+
return {
816+
...Settings.fileCache[namespacedFilepath],
817+
cached: true
818+
}
819+
}
796820
this.log.error(`Error getting settings ${e}`)
821+
throw e
797822
})
798823

799824
// Ignore in case path is a folder
@@ -808,8 +833,19 @@ ${this.results.reduce((x, y) => {
808833
if (typeof response.data.content !== 'string') {
809834
return
810835
}
811-
const yaml = require('js-yaml')
812-
return yaml.load(Buffer.from(response.data.content, 'base64').toString()) || {}
836+
837+
const content = yaml.load(Buffer.from(response.data.content, 'base64').toString()) || {}
838+
839+
// Cache the content, as its either new or changed
840+
if (!response.cached) {
841+
this.log.debug(`Cache miss for file ${filePath}`)
842+
Settings.fileCache[namespacedFilepath] = {
843+
etag: response.headers.etag,
844+
data: response.data
845+
}
846+
}
847+
848+
return content
813849
} catch (e) {
814850
if (e.status === 404) {
815851
return null
@@ -862,7 +898,7 @@ ${this.results.reduce((x, y) => {
862898
throw new Error(`Failed to filter repositories for property ${name}: ${error.message}`)
863899
}
864900
}
865-
901+
866902

867903
async getSubOrgRepositories (subOrgProperties) {
868904
const organizationName = this.repo.owner

‎test/unit/lib/settings.test.js‎

Lines changed: 155 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -303,4 +303,159 @@ repository:
303303
})
304304
}) // loadConfigs
305305

306+
describe('loadYaml', () => {
307+
let settings;
308+
309+
beforeEach(() => {
310+
Settings.fileCache = {};
311+
stubContext = {
312+
octokit: {
313+
repos: {
314+
getContent: jest.fn()
315+
},
316+
request: jest.fn(),
317+
paginate: jest.fn()
318+
},
319+
log: {
320+
debug: jest.fn(),
321+
info: jest.fn(),
322+
error: jest.fn()
323+
},
324+
payload: {
325+
installation: {
326+
id: 123
327+
}
328+
}
329+
};
330+
settings = createSettings({});
331+
});
332+
333+
it('should return parsed YAML content when file is fetched successfully', async () => {
334+
// Given
335+
const filePath = 'path/to/file.yml';
336+
const content = Buffer.from('key: value').toString('base64');
337+
jest.spyOn(settings.github.repos, 'getContent').mockResolvedValue({
338+
data: { content },
339+
headers: { etag: 'etag123' }
340+
});
341+
342+
// When
343+
const result = await settings.loadYaml(filePath);
344+
345+
// Then
346+
expect(result).toEqual({ key: 'value' });
347+
expect(Settings.fileCache[`${mockRepo.owner}/${filePath}`]).toEqual({
348+
etag: 'etag123',
349+
data: { content }
350+
});
351+
});
352+
353+
it('should return cached content when file has not changed (304 response)', async () => {
354+
// Given
355+
const filePath = 'path/to/file.yml';
356+
const content = Buffer.from('key: value').toString('base64');
357+
Settings.fileCache[`${mockRepo.owner}/${filePath}`] = { etag: 'etag123', data: { content } };
358+
jest.spyOn(settings.github.repos, 'getContent').mockRejectedValue({ status: 304 });
359+
360+
// When
361+
const result = await settings.loadYaml(filePath);
362+
363+
// Then
364+
expect(result).toEqual({ key: 'value' });
365+
expect(settings.github.repos.getContent).toHaveBeenCalledWith(
366+
expect.objectContaining({ headers: { 'If-None-Match': 'etag123' } })
367+
);
368+
});
369+
370+
it('should not return cached content when the cache is for another org', async () => {
371+
// Given
372+
const filePath = 'path/to/file.yml';
373+
const content = Buffer.from('key: value').toString('base64');
374+
const wrongContent = Buffer.from('wrong: content').toString('base64');
375+
Settings.fileCache['another-org/path/to/file.yml'] = { etag: 'etag123', data: { wrongContent } };
376+
jest.spyOn(settings.github.repos, 'getContent').mockResolvedValue({
377+
data: { content },
378+
headers: { etag: 'etag123' }
379+
});
380+
381+
// When
382+
const result = await settings.loadYaml(filePath);
383+
384+
// Then
385+
expect(result).toEqual({ key: 'value' });
386+
})
387+
388+
it('should return null when the file path is a folder', async () => {
389+
// Given
390+
const filePath = 'path/to/folder';
391+
jest.spyOn(settings.github.repos, 'getContent').mockResolvedValue({
392+
data: []
393+
});
394+
395+
// When
396+
const result = await settings.loadYaml(filePath);
397+
398+
// Then
399+
expect(result).toBeNull();
400+
});
401+
402+
it('should return null when the file is a symlink or submodule', async () => {
403+
// Given
404+
const filePath = 'path/to/symlink';
405+
jest.spyOn(settings.github.repos, 'getContent').mockResolvedValue({
406+
data: { content: null }
407+
});
408+
409+
// When
410+
const result = await settings.loadYaml(filePath);
411+
412+
// Then
413+
expect(result).toBeUndefined();
414+
});
415+
416+
it('should handle 404 errors gracefully and return null', async () => {
417+
// Given
418+
const filePath = 'path/to/nonexistent.yml';
419+
jest.spyOn(settings.github.repos, 'getContent').mockRejectedValue({ status: 404 });
420+
421+
// When
422+
const result = await settings.loadYaml(filePath);
423+
424+
// Then
425+
expect(result).toBeNull();
426+
});
427+
428+
it('should throw an error for non-404 exceptions when not in nop mode', async () => {
429+
// Given
430+
const filePath = 'path/to/error.yml';
431+
jest.spyOn(settings.github.repos, 'getContent').mockRejectedValue(new Error('Unexpected error'));
432+
433+
// When / Then
434+
await expect(settings.loadYaml(filePath)).rejects.toThrow('Unexpected error');
435+
});
436+
437+
it('should log and append NopCommand for non-404 exceptions in nop mode', async () => {
438+
// Given
439+
const filePath = 'path/to/error.yml';
440+
settings.nop = true;
441+
jest.spyOn(settings.github.repos, 'getContent').mockRejectedValue(new Error('Unexpected error'));
442+
jest.spyOn(settings, 'appendToResults');
443+
444+
// When
445+
const result = await settings.loadYaml(filePath);
446+
447+
// Then
448+
expect(result).toBeUndefined();
449+
expect(settings.appendToResults).toHaveBeenCalledWith(
450+
expect.arrayContaining([
451+
expect.objectContaining({
452+
type: 'ERROR',
453+
action: expect.objectContaining({
454+
msg: expect.stringContaining('Unexpected error')
455+
})
456+
})
457+
])
458+
);
459+
});
460+
});
306461
}) // Settings Tests

0 commit comments

Comments
 (0)