From 36516198c9a07b1c58eee5ffd7380ced8921735a Mon Sep 17 00:00:00 2001
From: Sergei Zharinov <zharinov@users.noreply.github.com>
Date: Mon, 26 Sep 2022 07:51:13 +0300
Subject: [PATCH] fix(github): Don't store raw PR data in long-term cache
(#17967)
---
lib/modules/platform/github/common.ts | 10 +--
lib/modules/platform/github/index.spec.ts | 85 +----------------------
lib/modules/platform/github/index.ts | 7 +-
lib/modules/platform/github/pr.ts | 79 +++++++--------------
lib/modules/platform/github/types.ts | 3 +-
5 files changed, 37 insertions(+), 147 deletions(-)
diff --git a/lib/modules/platform/github/common.ts b/lib/modules/platform/github/common.ts
index 8a039c4629..2b9e146bab 100644
--- a/lib/modules/platform/github/common.ts
+++ b/lib/modules/platform/github/common.ts
@@ -6,12 +6,7 @@ import type { GhPr, GhRestPr } from './types';
/**
* @see https://docs.github.com/en/rest/reference/pulls#list-pull-requests
*/
-export function coerceRestPr(pr: GhRestPr | null | undefined): GhPr | null {
- // istanbul ignore if
- if (!pr) {
- return null;
- }
-
+export function coerceRestPr(pr: GhRestPr): GhPr {
const bodyStruct = pr.bodyStruct ?? getPrBodyStruct(pr.body);
const result: GhPr = {
displayNumber: `Pull Request #${pr.number}`,
@@ -23,7 +18,8 @@ export function coerceRestPr(pr: GhRestPr | null | undefined): GhPr | null {
? PrState.Merged
: pr.state,
bodyStruct,
- updatedAt: pr.updated_at,
+ updated_at: pr.updated_at,
+ node_id: pr.node_id,
};
if (pr.head?.sha) {
diff --git a/lib/modules/platform/github/index.spec.ts b/lib/modules/platform/github/index.spec.ts
index 07a109f2d3..3518e86708 100644
--- a/lib/modules/platform/github/index.spec.ts
+++ b/lib/modules/platform/github/index.spec.ts
@@ -679,64 +679,6 @@ describe('modules/platform/github/index', () => {
]);
});
- describe('Url cleanup', () => {
- type GhRestPrWithUrls = GhRestPr & {
- url: string;
- example_url: string;
- repo: {
- example_url: string;
- };
- };
-
- type PrCache = ApiPageCache<GhRestPrWithUrls>;
-
- const prWithUrls = (): GhRestPrWithUrls => ({
- ...pr1,
- url: 'https://example.com',
- example_url: 'https://example.com',
- _links: { foo: { href: 'https://example.com' } },
- repo: { example_url: 'https://example.com' },
- });
-
- it('removes url data from response', async () => {
- const scope = httpMock.scope(githubApiHost);
- initRepoMock(scope, 'some/repo');
- scope.get(pagePath(1)).reply(200, [prWithUrls()]);
- await github.initRepo({ repository: 'some/repo' });
-
- await github.getPrList();
-
- const repoCache = repository.getCache();
- const prCache = repoCache.platform?.github?.prCache as PrCache;
- expect(prCache).toMatchObject({ items: {} });
-
- const item = prCache.items[1];
- expect(item).toBeDefined();
- expect(item._links).toBeUndefined();
- expect(item.url).toBeUndefined();
- expect(item.example_url).toBeUndefined();
- expect(item.repo.example_url).toBeUndefined();
- });
-
- it('removes url data from existing cache', async () => {
- const scope = httpMock.scope(githubApiHost);
- initRepoMock(scope, 'some/repo');
- scope.get(pagePath(1, 20)).reply(200, []);
- await github.initRepo({ repository: 'some/repo' });
- const repoCache = repository.getCache();
- const prCache: PrCache = { items: { 1: prWithUrls() } };
- repoCache.platform = { github: { prCache } };
-
- await github.getPrList();
-
- const item = prCache.items[1];
- expect(item._links).toBeUndefined();
- expect(item.url).toBeUndefined();
- expect(item.example_url).toBeUndefined();
- expect(item.repo.example_url).toBeUndefined();
- });
- });
-
describe('Body compaction', () => {
type PrCache = ApiPageCache<GhRestPr>;
@@ -762,27 +704,6 @@ describe('modules/platform/github/index', () => {
expect(item.body).toBeUndefined();
expect(item.bodyStruct).toEqual({ hash: hashBody('foo') });
});
-
- it('removes url data from existing cache', async () => {
- const scope = httpMock.scope(githubApiHost);
- initRepoMock(scope, 'some/repo');
- scope.get(pagePath(1)).reply(200, [prWithBody('foo')]);
- await github.initRepo({ repository: 'some/repo' });
- const repoCache = repository.getCache();
- const prCache: PrCache = {
- items: { 1: prWithBody('bar'), 2: prWithBody('baz') },
- };
- repoCache.platform = { github: { prCache } };
-
- await github.getPrList();
-
- expect(prCache.items[2]).toBeUndefined();
-
- const item = prCache.items[1];
- expect(item).toBeDefined();
- expect(item.body).toBeUndefined();
- expect(item.bodyStruct).toEqual({ hash: hashBody('foo') });
- });
});
});
@@ -2456,7 +2377,7 @@ describe('modules/platform/github/index', () => {
sourceRepo: 'some/repo',
state: 'open',
title: 'chore(deps): update dependency jest to v23.6.0',
- updatedAt: '01-09-2022',
+ updated_at: '01-09-2022',
});
});
@@ -2586,7 +2507,7 @@ describe('modules/platform/github/index', () => {
sourceBranch: 'some/branch',
state: 'open',
title: 'Some title',
- updatedAt: '01-09-2022',
+ updated_at: '01-09-2022',
});
});
@@ -2619,7 +2540,7 @@ describe('modules/platform/github/index', () => {
sourceBranch: 'some/branch',
state: 'open',
title: 'Some title',
- updatedAt: '01-09-2022',
+ updated_at: '01-09-2022',
});
});
});
diff --git a/lib/modules/platform/github/index.ts b/lib/modules/platform/github/index.ts
index 082fc58b5e..5a3bcab757 100644
--- a/lib/modules/platform/github/index.ts
+++ b/lib/modules/platform/github/index.ts
@@ -1443,10 +1443,13 @@ export async function createPr({
{ branch: sourceBranch, pr: ghPr.number, draft: draftPR },
'PR created'
);
- const { number, node_id } = ghPr;
+
+ const result = coerceRestPr(ghPr);
+ const { number, node_id } = result;
+
await addLabels(number, labels);
await tryPrAutomerge(number, node_id, platformOptions);
- const result = coerceRestPr(ghPr);
+
cachePr(result);
return result;
}
diff --git a/lib/modules/platform/github/pr.ts b/lib/modules/platform/github/pr.ts
index 76a8bff0db..f1064b875f 100644
--- a/lib/modules/platform/github/pr.ts
+++ b/lib/modules/platform/github/pr.ts
@@ -5,63 +5,43 @@ import { ExternalHostError } from '../../../types/errors/external-host-error';
import { getCache } from '../../../util/cache/repository';
import type { GithubHttp, GithubHttpOptions } from '../../../util/http/github';
import { parseLinkHeader } from '../../../util/url';
-import { getPrBodyStruct } from '../pr-body';
import { ApiCache } from './api-cache';
import { coerceRestPr } from './common';
import type { ApiPageCache, GhPr, GhRestPr } from './types';
-function removeUrlFields(input: unknown): void {
- if (is.plainObject(input)) {
- for (const [key, val] of Object.entries(input)) {
- if ((key === 'url' || key.endsWith('_url')) && is.string(val)) {
- delete input[key];
- } else {
- removeUrlFields(val);
- }
+function isOldCache(prCache: unknown): prCache is ApiPageCache<GhRestPr> {
+ if (
+ is.plainObject(prCache) &&
+ is.plainObject(prCache.items) &&
+ !is.emptyObject(prCache.items)
+ ) {
+ const [item] = Object.values(prCache.items);
+ if (is.plainObject(item) && is.string(item.node_id)) {
+ return true;
}
}
+
+ return false;
}
-function compactPrBodyStructure(input: unknown): void {
- if (is.plainObject(input)) {
- if (!input.bodyStruct && is.string(input.body)) {
- input.bodyStruct = getPrBodyStruct(input.body);
- delete input.body;
+function migrateCache(cache: unknown): void {
+ const items: ApiPageCache<GhPr>['items'] = {};
+ if (isOldCache(cache)) {
+ for (const item of Object.values(cache.items)) {
+ items[item.number] = coerceRestPr(item);
}
+ cache.items = items as never;
}
}
-function massageGhRestPr(ghPr: GhRestPr): GhRestPr {
- removeUrlFields(ghPr);
- delete ghPr.head?.repo?.pushed_at;
- delete ghPr.base?.repo?.pushed_at;
- delete ghPr._links;
-
- compactPrBodyStructure(ghPr);
-
- return ghPr;
-}
-
-function getPrApiCache(): ApiCache<GhRestPr> {
+function getPrApiCache(): ApiCache<GhPr> {
const repoCache = getCache();
repoCache.platform ??= {};
repoCache.platform.github ??= {};
repoCache.platform.github.prCache ??= { items: {} };
- const apiPageCache = repoCache.platform.github
- .prCache as ApiPageCache<GhRestPr>;
-
- const items = Object.values(apiPageCache.items);
-
- const firstItem = items?.[0];
- if (firstItem?.body) {
- apiPageCache.items = {};
- } else if (firstItem?._links) {
- for (const ghPr of items) {
- massageGhRestPr(ghPr);
- }
- }
-
- const prCache = new ApiCache(apiPageCache);
+ const cache = repoCache.platform.github.prCache;
+ migrateCache(cache);
+ const prCache = new ApiCache<GhPr>(cache as ApiPageCache<GhPr>);
return prCache;
}
@@ -97,7 +77,6 @@ export async function getPrCache(
repo: string,
username: string | null
): Promise<Record<number, GhPr>> {
- const prCache: Record<number, GhPr> = {};
const prApiCache = getPrApiCache();
const isInitial = is.emptyArray(prApiCache.getItems());
@@ -134,11 +113,9 @@ export async function getPrCache(
);
}
- for (const ghPr of page) {
- massageGhRestPr(ghPr);
- }
+ const items = page.map(coerceRestPr);
- needNextPageSync = prApiCache.reconcile(page);
+ needNextPageSync = prApiCache.reconcile(items);
needNextPageFetch = !!parseLinkHeader(linkHeader)?.next;
if (pageIdx === 1) {
@@ -161,13 +138,5 @@ export async function getPrCache(
throw new ExternalHostError(err, PlatformId.Github);
}
- const cacheItems = prApiCache.getItems();
- for (const ghPr of cacheItems) {
- const pr = coerceRestPr(ghPr);
- if (pr) {
- prCache[ghPr.number] = pr;
- }
- }
-
- return prCache;
+ return prApiCache.getItems();
}
diff --git a/lib/modules/platform/github/types.ts b/lib/modules/platform/github/types.ts
index e1d66645fc..11e56db517 100644
--- a/lib/modules/platform/github/types.ts
+++ b/lib/modules/platform/github/types.ts
@@ -54,7 +54,8 @@ export interface GhRestPr {
}
export interface GhPr extends Pr {
- updatedAt: string;
+ updated_at: string;
+ node_id: string;
}
export interface UserDetails {
--
GitLab