From cbf8554eb3247a059b8d553fc2bbb60c6963af7d Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Denis=20=C4=86ori=C4=87?= Date: Sun, 27 Sep 2026 22:46:11 +0200 Subject: [PATCH 01/18] fix(proxy): avoid duplicating diffs in audit logs MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Signed-off-by: Denis Ćorić --- src/proxy/processors/push-action/getDiff.ts | 2 +- src/proxy/processors/push-action/scanDiff.ts | 1 - test/processors/diffStorage.test.ts | 126 +++++++++++++++++++ 3 files changed, 127 insertions(+), 2 deletions(-) create mode 100644 test/processors/diffStorage.test.ts diff --git a/src/proxy/processors/push-action/getDiff.ts b/src/proxy/processors/push-action/getDiff.ts index 445b119e4..85029c68c 100644 --- a/src/proxy/processors/push-action/getDiff.ts +++ b/src/proxy/processors/push-action/getDiff.ts @@ -49,7 +49,7 @@ const exec = async (_req: Request, action: Action): Promise => { step.log(`Executing "git diff ${commitFrom} ${action.commitTo}" in ${path}`); const revisionRange = `${commitFrom}..${action.commitTo}`; const diff = await git.diff([revisionRange]); - step.log(diff); + step.log(`Generated diff (${Buffer.byteLength(diff, 'utf8')} bytes)`); step.setContent(diff); } catch (error: unknown) { const msg = getErrorMessage(error); diff --git a/src/proxy/processors/push-action/scanDiff.ts b/src/proxy/processors/push-action/scanDiff.ts index a9cfa5d78..48f91a341 100644 --- a/src/proxy/processors/push-action/scanDiff.ts +++ b/src/proxy/processors/push-action/scanDiff.ts @@ -182,7 +182,6 @@ const exec = async (_req: Request, action: Action): Promise => { const diff = steps.find((s) => s.stepName === 'diff')?.content; - step.log(diff); const diffViolations = getDiffViolations(diff, action.project, step); if (diffViolations) { diff --git a/test/processors/diffStorage.test.ts b/test/processors/diffStorage.test.ts new file mode 100644 index 000000000..88309737b --- /dev/null +++ b/test/processors/diffStorage.test.ts @@ -0,0 +1,126 @@ +/** + * Copyright 2026 GitProxy Contributors + * + * Licensed under the Apache License, Version 2.0 (the "License"); + * you may not use this file except in compliance with the License. + * You may obtain a copy of the License at + * + * http://www.apache.org/licenses/LICENSE-2.0 + * + * Unless required by applicable law or agreed to in writing, software + * distributed under the License is distributed on an "AS IS" BASIS, + * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. + * See the License for the specific language governing permissions and + * limitations under the License. + */ + +import { afterEach, beforeEach, describe, expect, it, vi } from 'vitest'; +import type { Request } from 'express'; +import { Action, RequestType } from '../../src/proxy/actions'; +import { SAMPLE_COMMIT } from '../../src/proxy/constants'; +import { exec as getDiff } from '../../src/proxy/processors/push-action/getDiff'; +import { exec as scanDiff } from '../../src/proxy/processors/push-action/scanDiff'; + +const { gitDiff } = vi.hoisted(() => ({ gitDiff: vi.fn() })); + +vi.mock('simple-git', () => ({ default: () => ({ diff: gitDiff }) })); +vi.mock('../../src/config', () => ({ + getCommitConfig: () => ({ diff: { block: { literals: ['BLOCKED_MARKER'] } } }), + getPrivateOrganizations: () => [], +})); + +const request = {} as Request; +const makeDiff = (lines: string[]) => + `diff --git a/example.txt b/example.txt +--- a/example.txt ++++ b/example.txt +@@ -0,0 +1,${lines.length} @@ +${lines.map((line) => `+${line}`).join('\n')} +`; + +const makeAction = () => { + const action = new Action( + 'diff-storage', + RequestType.PUSH, + 'POST', + 1, + 'https://example.com/org/repo.git', + ); + action.proxyGitPath = '/unused'; + action.commitFrom = 'a'.repeat(40); + action.commitTo = 'b'.repeat(40); + action.commitData = [SAMPLE_COMMIT]; + return action; +}; + +describe('diff storage and audit logging', () => { + beforeEach(() => { + gitDiff.mockReset(); + vi.spyOn(console, 'info').mockImplementation(() => {}); + }); + + afterEach(() => vi.restoreAllMocks()); + + it.each([ + ['ordinary', ['reviewable change']], + ['Unicode', ['Ćorić — 日本語 🚀']], + ['large', Array.from({ length: 16384 }, () => 'x'.repeat(64))], + ])('retains a %s diff once for review without logging its contents', async (_name, lines) => { + const diff = makeDiff(lines as string[]); + gitDiff.mockResolvedValue(diff); + const action = await getDiff(request, makeAction()); + await scanDiff(request, action); + + expect(action.error).toBe(false); + expect(action.steps.find((step) => step.stepName === 'diff')?.content).toBe(diff); + const encodedDiff = JSON.stringify(diff).slice(1, -1); + expect(JSON.stringify(action).split(encodedDiff)).toHaveLength(2); + expect(action.steps.flatMap((step) => step.logs).join('\n')).not.toContain(diff); + expect(console.info).toHaveBeenCalledWith( + `diff - Generated diff (${Buffer.byteLength(diff, 'utf8')} bytes)`, + ); + expect(vi.mocked(console.info).mock.calls.flat().join('\n')).not.toContain(diff); + }); + + it('still rejects prohibited content and records the finding without duplicating the diff', async () => { + const diff = makeDiff(['ordinary line', 'BLOCKED_MARKER']); + gitDiff.mockResolvedValue(diff); + const action = await getDiff(request, makeAction()); + await scanDiff(request, action); + + expect(action.error).toBe(true); + expect(action.continue()).toBe(false); + expect(action.errorMessage).toContain('BLOCKED_MARKER'); + expect(action.errorMessage).toContain('example.txt'); + expect(action.steps[0].content).toBe(diff); + expect(action.steps[1].logs).toContain( + 'scanDiff - Diff is blocked via configured literals/patterns/providers.', + ); + expect(JSON.stringify(action).split(JSON.stringify(diff).slice(1, -1))).toHaveLength(2); + expect(vi.mocked(console.info).mock.calls.flat().join('\n')).not.toContain(diff); + }); + + it('preserves a legitimate empty diff and its audit messages', async () => { + gitDiff.mockResolvedValue(''); + const action = await getDiff(request, makeAction()); + await scanDiff(request, action); + + expect(action.error).toBe(false); + expect(action.steps[0].content).toBe(''); + expect(action.steps[0].logs).toContain('diff - Generated diff (0 bytes)'); + expect(action.steps[1].logs).toContain( + 'scanDiff - No commit diff found, but this may be legitimate (empty diff).', + ); + }); + + it('records generation errors without claiming to have generated a diff', async () => { + gitDiff.mockRejectedValue(new Error('Unable to read Git objects')); + const action = await getDiff(request, makeAction()); + + expect(action.error).toBe(true); + expect(action.continue()).toBe(false); + expect(action.steps[0].content).toBeNull(); + expect(action.steps[0].logs).toContain('diff - Unable to read Git objects'); + expect(action.steps[0].logs.join('\n')).not.toContain('Generated diff'); + }); +}); From cc6e851cd70672d801561408b8da5e081a296258 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Denis=20=C4=86ori=C4=87?= Date: Sun, 27 Sep 2026 22:51:38 +0200 Subject: [PATCH 02/18] perf(db): return consistent metadata-only push lists MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Signed-off-by: Denis Ćorić --- src/db/file/pushes.ts | 3 + src/db/mongo/pushes.ts | 28 +-- src/db/pushProjection.ts | 60 +++++ test/db/file/pushStorage.test.ts | 20 ++ test/db/mongo/push.test.ts | 27 +-- test/db/mongo/pushStorage.integration.test.ts | 23 ++ test/db/pushStorage.contract.ts | 226 ++++++++++++++++++ test/testDb.test.ts | 12 +- test/testPush.test.ts | 11 +- 9 files changed, 354 insertions(+), 56 deletions(-) create mode 100644 src/db/pushProjection.ts create mode 100644 test/db/file/pushStorage.test.ts create mode 100644 test/db/mongo/pushStorage.integration.test.ts create mode 100644 test/db/pushStorage.contract.ts diff --git a/src/db/file/pushes.ts b/src/db/file/pushes.ts index 6fb980573..98b6de29c 100644 --- a/src/db/file/pushes.ts +++ b/src/db/file/pushes.ts @@ -20,6 +20,7 @@ import { activityPrimaryStatusFromFlags } from '../../activity/activityPrimarySt import { canonicalRemoteUrl } from '../../activity/canonicalRemoteUrl'; import { Action } from '../../proxy/actions/Action'; import { toClass } from '../helper'; +import { pushListProjection } from '../pushProjection'; import { PushQuery, RepoActivityTabCounts, @@ -154,6 +155,7 @@ export const getPushes = (query: Partial): Promise => { if (!query) query = defaultPushQuery; return new Promise((resolve, reject) => { db.find(query) + .projection(pushListProjection) .sort({ timestamp: -1 }) .exec((err, docs) => { // ignore for code coverage as neDB rarely returns errors even for an invalid query @@ -178,6 +180,7 @@ export const getPushesForUserProfile = ( const filter = buildUserProfilePushFilter(emailVariants, profileUsername); return new Promise((resolve, reject) => { db.find(filter) + .projection(pushListProjection) .sort({ timestamp: -1 }) .exec((err, docs) => { /* istanbul ignore if */ diff --git a/src/db/mongo/pushes.ts b/src/db/mongo/pushes.ts index a0ab1f067..9e3602d31 100644 --- a/src/db/mongo/pushes.ts +++ b/src/db/mongo/pushes.ts @@ -19,6 +19,7 @@ import { canonicalRemoteUrl } from '../../activity/canonicalRemoteUrl'; import { connect, findDocuments, findOneDocument } from './helper'; import { Action } from '../../proxy/actions'; import { toClass } from '../helper'; +import { pushListProjection } from '../pushProjection'; import { PushQuery, RepoActivityTabCounts, @@ -120,33 +121,6 @@ const defaultPushQuery: Partial = { type: 'push', }; -/** Fields returned for push list / activity UIs (shared by getPushes and getPushesForUserProfile). */ -const pushListProjection = { - _id: 0, - id: 1, - allowPush: 1, - attestation: 1, - authorised: 1, - blocked: 1, - blockedMessage: 1, - branch: 1, - canceled: 1, - commitData: 1, - commitFrom: 1, - commitTo: 1, - error: 1, - method: 1, - project: 1, - rejected: 1, - rejection: 1, - repo: 1, - repoName: 1, - timestamp: 1, - type: 1, - url: 1, - userEmail: 1, -} as const; - export const getPushes = async ( query: Partial = defaultPushQuery, ): Promise => { diff --git a/src/db/pushProjection.ts b/src/db/pushProjection.ts new file mode 100644 index 000000000..0b08801c1 --- /dev/null +++ b/src/db/pushProjection.ts @@ -0,0 +1,60 @@ +/** + * Copyright 2026 GitProxy Contributors + * + * Licensed under the Apache License, Version 2.0 (the "License"); + * you may not use this file except in compliance with the License. + * You may obtain a copy of the License at + * + * http://www.apache.org/licenses/LICENSE-2.0 + * + * Unless required by applicable law or agreed to in writing, software + * distributed under the License is distributed on an "AS IS" BASIS, + * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. + * See the License for the specific language governing permissions and + * limitations under the License. + */ + +import type { Action } from '../proxy/actions'; + +/** + * List and profile responses contain metadata only. Keep every diff representation + * (steps, lastStep, and the proposed top-level diff) out of this allowlist. + * Detail reads and audit writes retain the complete record. + */ +export const pushListProjection = { + _id: 0, + actionType: 1, + allowPush: 1, + attestation: 1, + author: 1, + authorised: 1, + autoApproved: 1, + autoRejected: 1, + blocked: 1, + blockedMessage: 1, + branch: 1, + canceled: 1, + commitData: 1, + commitFrom: 1, + commitTo: 1, + error: 1, + errorMessage: 1, + id: 1, + legacyId: 1, + message: 1, + method: 1, + project: 1, + protocol: 1, + pusherVerified: 1, + rejected: 1, + rejection: 1, + repo: 1, + repoName: 1, + tagData: 1, + tags: 1, + timestamp: 1, + type: 1, + url: 1, + user: 1, + userEmail: 1, +} as const satisfies Partial>; diff --git a/test/db/file/pushStorage.test.ts b/test/db/file/pushStorage.test.ts new file mode 100644 index 000000000..7f22986df --- /dev/null +++ b/test/db/file/pushStorage.test.ts @@ -0,0 +1,20 @@ +/** + * Copyright 2026 GitProxy Contributors + * + * Licensed under the Apache License, Version 2.0 (the "License"); + * you may not use this file except in compliance with the License. + * You may obtain a copy of the License at + * + * http://www.apache.org/licenses/LICENSE-2.0 + * + * Unless required by applicable law or agreed to in writing, software + * distributed under the License is distributed on an "AS IS" BASIS, + * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. + * See the License for the specific language governing permissions and + * limitations under the License. + */ + +import * as pushes from '../../../src/db/file/pushes'; +import { definePushStorageContract } from '../pushStorage.contract'; + +definePushStorageContract('NeDB push storage', pushes); diff --git a/test/db/mongo/push.test.ts b/test/db/mongo/push.test.ts index 97f2f183e..33d1b9efc 100644 --- a/test/db/mongo/push.test.ts +++ b/test/db/mongo/push.test.ts @@ -16,6 +16,7 @@ import { describe, it, expect, afterEach, vi, beforeEach } from 'vitest'; import { Action } from '../../../src/proxy/actions'; +import { pushListProjection } from '../../../src/db/pushProjection'; const mockFindOne = vi.fn(); const mockDeleteOne = vi.fn(); @@ -103,31 +104,7 @@ describe('MongoDB Push Handler', async () => { type: 'push', }, { - projection: { - _id: 0, - id: 1, - allowPush: 1, - attestation: 1, - authorised: 1, - blocked: 1, - blockedMessage: 1, - branch: 1, - canceled: 1, - commitData: 1, - commitFrom: 1, - commitTo: 1, - error: 1, - method: 1, - project: 1, - rejected: 1, - rejection: 1, - repo: 1, - repoName: 1, - timestamp: 1, - type: 1, - url: 1, - userEmail: 1, - }, + projection: pushListProjection, sort: { timestamp: -1, }, diff --git a/test/db/mongo/pushStorage.integration.test.ts b/test/db/mongo/pushStorage.integration.test.ts new file mode 100644 index 000000000..595925a9f --- /dev/null +++ b/test/db/mongo/pushStorage.integration.test.ts @@ -0,0 +1,23 @@ +/** + * Copyright 2026 GitProxy Contributors + * + * Licensed under the Apache License, Version 2.0 (the "License"); + * you may not use this file except in compliance with the License. + * You may obtain a copy of the License at + * + * http://www.apache.org/licenses/LICENSE-2.0 + * + * Unless required by applicable law or agreed to in writing, software + * distributed under the License is distributed on an "AS IS" BASIS, + * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. + * See the License for the specific language governing permissions and + * limitations under the License. + */ + +import { describe } from 'vitest'; +import * as pushes from '../../../src/db/mongo/pushes'; +import { definePushStorageContract } from '../pushStorage.contract'; + +describe.runIf(process.env.RUN_MONGO_TESTS === 'true')('MongoDB storage contract', () => { + definePushStorageContract('MongoDB push storage', pushes); +}); diff --git a/test/db/pushStorage.contract.ts b/test/db/pushStorage.contract.ts new file mode 100644 index 000000000..e78308ee0 --- /dev/null +++ b/test/db/pushStorage.contract.ts @@ -0,0 +1,226 @@ +/** + * Copyright 2026 GitProxy Contributors + * + * Licensed under the Apache License, Version 2.0 (the "License"); + * you may not use this file except in compliance with the License. + * You may obtain a copy of the License at + * + * http://www.apache.org/licenses/LICENSE-2.0 + * + * Unless required by applicable law or agreed to in writing, software + * distributed under the License is distributed on an "AS IS" BASIS, + * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. + * See the License for the specific language governing permissions and + * limitations under the License. + */ + +import { afterEach, beforeEach, describe, expect, it } from 'vitest'; +import { Action, PushType, RequestType, Step } from '../../src/proxy/actions'; +import { canonicalRemoteUrl } from '../../src/activity/canonicalRemoteUrl'; +import type { Sink } from '../../src/db/types'; + +type PushStorage = Pick< + Sink, + | 'writeAudit' + | 'getPush' + | 'getPushes' + | 'getPushesForUserProfile' + | 'deletePush' + | 'authorise' + | 'reject' + | 'cancel' + | 'getRepoPushRollupsByCanonicalUrl' +>; + +const ids = ['storage-contract-1', 'storage-contract-2', 'storage-contract-3']; +const url = 'https://example.com/storage-contract/repo.git'; +const diff = 'diff --git a/private.txt b/private.txt\n+private payload\n'; +const attestation = { + answers: [{ label: 'Reviewed', checked: true }], + timestamp: new Date('2026-01-01T00:00:00Z'), + reviewer: { username: 'Reviewer', email: 'reviewer@example.com' }, +}; +const rejection = { + reason: 'Needs changes', + timestamp: new Date('2026-01-02T00:00:00Z'), + reviewer: attestation.reviewer, +}; + +const makePush = (id = ids[0], content = diff): Action => { + const action = new Action(id, RequestType.PUSH, 'POST', 100, url); + action.blocked = true; + action.user = 'Contributor'; + action.userEmail = 'author@example.com'; + action.branch = 'refs/heads/main'; + action.commitFrom = 'a'.repeat(40); + action.commitTo = 'b'.repeat(40); + const step = new Step('diff', false, null, false, null, content); + // Historical pushes include the raw diff in logs and sometimes in lastStep too. + step.logs = [`diff - ${content}`]; + action.addStep(step); + return action; +}; + +/** Exercise real sinks with one shared contract; mocks cannot validate database projections. */ +export const definePushStorageContract = (name: string, sink: PushStorage): void => { + describe(name, () => { + const cleanup = async () => { + for (const id of ids) await sink.deletePush(id); + }; + beforeEach(cleanup); + afterEach(cleanup); + + it('keeps legacy audit content on detail reads while excluding every diff copy from lists', async () => { + const action = makePush(); + // Future top-level diff records from #1693 must also remain outside list responses. + Object.assign(action, { diff, pluginPrivateData: diff }); + await sink.writeAudit(action); + + const summaries = await sink.getPushes({ type: 'push' }); + const summary = summaries.find((push) => push.id === action.id)!; + expect(summary).toMatchObject({ + id: action.id, + url, + user: 'Contributor', + userEmail: 'author@example.com', + commitFrom: action.commitFrom, + commitTo: action.commitTo, + blocked: true, + }); + for (const field of ['steps', 'lastStep', 'diff', '_id', 'pluginPrivateData']) { + expect(summary).not.toHaveProperty(field); + } + expect(JSON.stringify(summary)).not.toContain('private payload'); + + const detail = await sink.getPush(action.id); + expect(detail?.steps[0].content).toBe(diff); + expect(detail?.steps[0].logs).toEqual([`diff - ${diff}`]); + expect(detail?.lastStep?.content).toBe(diff); + }); + + it('returns equally small summaries for small and multi-megabyte diffs', async () => { + await sink.writeAudit(makePush(ids[0], 'small diff')); + await sink.writeAudit(makePush(ids[1], 'private payload\n'.repeat(131072))); + + const summaries = (await sink.getPushes({ type: 'push' })).filter((push) => + ids.includes(push.id), + ); + expect(summaries).toHaveLength(2); + const sizes = summaries.map((push) => Buffer.byteLength(JSON.stringify(push))); + expect(sizes[0]).toBe(sizes[1]); + expect(sizes[0]).toBeLessThan(2000); + expect((await sink.getPush(ids[1]))?.steps[0].content.length).toBeGreaterThan(2_000_000); + }); + + it('preserves tag metadata used by the activity table', async () => { + const action = makePush(); + action.actionType = PushType.TAG; + action.tags = ['refs/tags/v1']; + action.tagData = [ + { + type: 'tag', + tagName: 'v1', + tagger: 'Tagger', + taggerEmail: 'tagger@example.com', + message: 'Release', + }, + ]; + action.steps = []; + delete action.lastStep; + await sink.writeAudit(action); + + expect( + (await sink.getPushes({ type: 'push' })).find((push) => push.id === action.id), + ).toMatchObject({ actionType: PushType.TAG, tags: action.tags, tagData: action.tagData }); + expect((await sink.getPush(action.id))?.steps).toEqual([]); + }); + + it('preserves filtering and newest-first ordering', async () => { + for (let i = 0; i < ids.length; i++) { + const action = makePush(ids[i]); + action.timestamp = 100 + i; + action.authorised = i === 1; + await sink.writeAudit(action); + } + expect( + (await sink.getPushes({ authorised: false })) + .filter((push) => ids.includes(push.id)) + .map((push) => push.id), + ).toEqual([ids[2], ids[0]]); + expect( + (await sink.getPushes({ authorised: true })) + .filter((push) => ids.includes(push.id)) + .map((push) => push.id), + ).toEqual([ids[1]]); + }); + + it('projects profile activity for both authors and reviewers', async () => { + const authored = makePush(ids[0]); + const reviewed = makePush(ids[1]); + reviewed.userEmail = 'someone-else@example.com'; + reviewed.attestation = attestation; + reviewed.timestamp = 200; + const unrelated = makePush(ids[2]); + unrelated.userEmail = 'unrelated@example.com'; + for (const action of [authored, reviewed, unrelated]) await sink.writeAudit(action); + + const summaries = await sink.getPushesForUserProfile(['author@example.com'], 'reviewer'); + expect(summaries.filter((push) => ids.includes(push.id)).map((push) => push.id)).toEqual([ + ids[1], + ids[0], + ]); + expect(JSON.stringify(summaries)).not.toContain('private payload'); + expect(summaries.find((push) => push.id === reviewed.id)?.attestation?.reviewer).toEqual( + attestation.reviewer, + ); + const reviewOnly = await sink.getPushesForUserProfile([], 'REVIEWER'); + expect(reviewOnly.filter((push) => ids.includes(push.id)).map((push) => push.id)).toEqual([ + ids[1], + ]); + }); + + it('preserves full legacy records through review decisions and cancellation', async () => { + await sink.writeAudit(makePush()); + await sink.authorise(ids[0], attestation); + expect(await sink.getPush(ids[0])).toMatchObject({ + authorised: true, + canceled: false, + rejected: false, + }); + await sink.reject(ids[0], rejection); + expect(await sink.getPush(ids[0])).toMatchObject({ + authorised: false, + canceled: false, + rejected: true, + }); + await sink.cancel(ids[0]); + + const detail = await sink.getPush(ids[0]); + expect(detail).toMatchObject({ authorised: false, canceled: true, rejected: false }); + expect(detail?.steps[0].content).toBe(diff); + expect(detail?.steps[0].logs).toEqual([`diff - ${diff}`]); + expect(detail?.attestation?.reviewer).toEqual(attestation.reviewer); + expect(detail?.rejection?.reason).toBe(rejection.reason); + }); + + it('keeps activity counts and timestamps correct after updates and deletion', async () => { + const action = makePush(); + await sink.writeAudit(action); + const key = canonicalRemoteUrl(url); + const pending = await sink.getRepoPushRollupsByCanonicalUrl(); + expect(pending.tabCounts.get(key)?.pending).toBe(1); + expect(pending.latestPendingReviewAtMs.get(key)).toBe(action.timestamp); + + await sink.authorise(action.id, attestation); + const approved = await sink.getRepoPushRollupsByCanonicalUrl(); + expect(approved.tabCounts.get(key)?.pending).toBe(0); + expect(approved.tabCounts.get(key)?.approved).toBe(1); + expect(approved.latestPendingReviewAtMs.has(key)).toBe(false); + expect(approved.latestPushAtMs.get(key)).toBe(action.timestamp); + + await sink.deletePush(action.id); + expect((await sink.getRepoPushRollupsByCanonicalUrl()).tabCounts.has(key)).toBe(false); + expect(await sink.getPush(action.id)).toBeNull(); + }); + }); +}; diff --git a/test/testDb.test.ts b/test/testDb.test.ts index d2d11c174..be5c4a9d3 100644 --- a/test/testDb.test.ts +++ b/test/testDb.test.ts @@ -583,8 +583,16 @@ describe('Database clients', () => { it('should be able to create a push', async () => { await db.writeAudit(TEST_PUSH); const pushes = await db.getPushes({}); - const cleanPushes = cleanResponseData(TEST_PUSH, pushes); - expect(cleanPushes).toContainEqual(TEST_PUSH); + expect(pushes).toContainEqual( + expect.objectContaining({ + id: TEST_PUSH.id, + user: TEST_PUSH.user, + userEmail: TEST_PUSH.userEmail, + }), + ); + expect(pushes.find((push) => push.id === TEST_PUSH.id)).not.toHaveProperty('steps'); + const detail = await db.getPush(TEST_PUSH.id); + expect(cleanResponseData(TEST_PUSH, detail!)).toEqual(TEST_PUSH); }, 20000); it('should be able to delete a push', async () => { diff --git a/test/testPush.test.ts b/test/testPush.test.ts index bde48cfe7..dbf7aec5c 100644 --- a/test/testPush.test.ts +++ b/test/testPush.test.ts @@ -498,8 +498,15 @@ describe('Push API', () => { const push = res.body.find((p: Action) => p.id === TEST_PUSH.id); expect(push).toBeDefined(); - // Check that all values in push are in TEST_PUSH, except for _id - expect(push).toMatchObject(TEST_PUSH); + expect(push).toMatchObject({ + id: TEST_PUSH.id, + url: TEST_PUSH.url, + user: TEST_PUSH.user, + userEmail: TEST_PUSH.userEmail, + pusherVerified: true, + }); + expect(push).not.toHaveProperty('steps'); + expect(push).not.toHaveProperty('lastStep'); expect(push.canceled).toBe(false); }); From 627577fa37c6139c87a03ff4cf0438c369194244 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Denis=20=C4=86ori=C4=87?= Date: Sun, 27 Sep 2026 23:02:58 +0200 Subject: [PATCH 03/18] test(e2e): verify compact push audits through approval MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Signed-off-by: Denis Ćorić --- test/e2e/push.test.ts | 31 +++++++++++++++++++++++++++++++ 1 file changed, 31 insertions(+) diff --git a/test/e2e/push.test.ts b/test/e2e/push.test.ts index 4da4ed974..29c3c93f1 100644 --- a/test/e2e/push.test.ts +++ b/test/e2e/push.test.ts @@ -20,6 +20,7 @@ import { mintUpstreamToken, testConfig } from './setup'; import fs from 'fs'; import path from 'path'; import os from 'os'; +import type { Action } from '../../src/proxy/actions'; describe('Git Proxy E2E - Repository Push Tests', () => { const tempDir: string = path.join(os.tmpdir(), 'git-proxy-push-e2e-tests', Date.now().toString()); @@ -730,9 +731,39 @@ describe('Git Proxy E2E - Repository Push Tests', () => { console.log('[TEST] Step 7: Approving push as authorized approver...'); const approverCookie = await login(approverUser.username, approverUser.password); + // Reviewing a push needs the full diff; the dashboard list only needs metadata. + const detailUrl = `${testConfig.gitProxyUiUrl}/api/v1/push/${pushId}`; + const reviewHeaders = { Cookie: approverCookie }; + const detailResponse = await fetch(detailUrl, { headers: reviewHeaders }); + expect(detailResponse.status).toBe(200); + const pendingPush: Action = await detailResponse.json(); + const diff = pendingPush.steps.find((step) => step.stepName === 'diff')?.content; + expect(diff).toEqual(expect.stringContaining('Approved Workflow Test')); + expect(diff).toContain('approved-workflow-test.txt'); + const encodedDiff = JSON.stringify(diff).slice(1, -1); + expect(JSON.stringify(pendingPush).split(encodedDiff)).toHaveLength(2); + expect(pendingPush.authorised).toBe(false); + + const listResponse = await fetch(`${testConfig.gitProxyUiUrl}/api/v1/push`, { + headers: reviewHeaders, + }); + expect(listResponse.status).toBe(200); + const pushes: Action[] = await listResponse.json(); + const summary = pushes.find((push) => push.id === pushId); + expect(summary).toMatchObject({ id: pushId, authorised: false }); + for (const field of ['steps', 'lastStep', 'diff']) { + expect(summary).not.toHaveProperty(field); + } + await approvePush(approverCookie, pushId!, defaultQuestions); console.log(`[TEST] SUCCESS: Push ${pushId} approved by ${approverUser.username}`); + const approvedResponse = await fetch(detailUrl, { headers: reviewHeaders }); + expect(approvedResponse.status).toBe(200); + const approvedPush: Action = await approvedResponse.json(); + expect(approvedPush.authorised).toBe(true); + expect(approvedPush.steps).toEqual(pendingPush.steps); + // Step 8: Re-push after approval (should succeed) console.log('[TEST] Step 8: Re-pushing after approval...'); let finalPushOutput = ''; From c33df0f30cd03712549da65c9c79509d5c9e24d4 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Denis=20=C4=86ori=C4=87?= Date: Sun, 27 Sep 2026 23:03:03 +0200 Subject: [PATCH 04/18] docs: explain diff storage and push list responses MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Signed-off-by: Denis Ćorić --- website/docs/architecture/processors.md | 4 +++- 1 file changed, 3 insertions(+), 1 deletion(-) diff --git a/website/docs/architecture/processors.md b/website/docs/architecture/processors.md index 4e1874a8d..6b7032be4 100644 --- a/website/docs/architecture/processors.md +++ b/website/docs/architecture/processors.md @@ -173,7 +173,7 @@ Source: [/src/proxy/processors/push-action/preReceive.ts](https://github.com/fin Executes `git diff` to obtain the diff for the given revision range. If there are no commits (possibly due to a malformed push), the push is blocked. -The data extracted in this step is later used in [`scanDiff`](#scandiff). +The full diff is stored in this step's `content` for review and later used in [`scanDiff`](#scandiff). The processors log the diff's byte count and scan findings without logging another copy of the full diff. Source: [/src/proxy/processors/push-action/getDiff.ts](https://github.com/finos/git-proxy/blob/main/src/proxy/processors/push-action/getDiff.ts) @@ -244,6 +244,8 @@ Source: [/src/proxy/processors/push-action/blockForAuth.ts](https://github.com/f This action runs after a chain has been executed. It stores in the database the entire `Action` object along with the list of `steps` that the action has gone through and their associated logs or error messages that occurred during processing of the chain. +Push list and profile activity responses return metadata without processor steps, `lastStep`, or diff content. Use `GET /api/v1/push/:id` to retrieve the full audit record and diff for review. Existing stored records remain readable and are not rewritten or truncated. + Note: **`audit` writes all actions** (push, pull, default/unclassified) to the DB. An action object (or entry in the pushes table) might look like this: From 4134790b997ae1076f718c15f555543d3646bc90 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Denis=20=C4=86ori=C4=87?= Date: Sun, 4 Oct 2026 15:57:57 +0200 Subject: [PATCH 05/18] perf(db): store repeated final audit steps once --- src/db/file/pushes.ts | 5 +- src/db/mongo/pushes.ts | 9 ++- src/db/pushStorage.ts | 42 ++++++++++ test/db/file/pushStorage.test.ts | 2 +- test/db/mongo/pushStorage.integration.test.ts | 5 +- test/db/pushStorage.contract.ts | 24 +++++- test/db/pushStorage.test.ts | 77 +++++++++++++++++++ 7 files changed, 155 insertions(+), 9 deletions(-) create mode 100644 src/db/pushStorage.ts create mode 100644 test/db/pushStorage.test.ts diff --git a/src/db/file/pushes.ts b/src/db/file/pushes.ts index 98b6de29c..e2b4217ce 100644 --- a/src/db/file/pushes.ts +++ b/src/db/file/pushes.ts @@ -21,6 +21,7 @@ import { canonicalRemoteUrl } from '../../activity/canonicalRemoteUrl'; import { Action } from '../../proxy/actions/Action'; import { toClass } from '../helper'; import { pushListProjection } from '../pushProjection'; +import { compactPush, restorePush } from '../pushStorage'; import { PushQuery, RepoActivityTabCounts, @@ -208,7 +209,7 @@ export const getPush = async (id: string): Promise => { if (!doc) { resolve(null); } else { - resolve(toClass(doc, Action.prototype)); + resolve(restorePush(doc)); } } }); @@ -232,7 +233,7 @@ export const deletePush = async (id: string): Promise => { export const writeAudit = async (action: Action): Promise => { return new Promise((resolve, reject) => { const options = { multi: false, upsert: true }; - db.update({ id: action.id }, action, options, (err) => { + db.update({ id: action.id }, compactPush(action), options, (err) => { // ignore for code coverage as neDB rarely returns errors even for an invalid query /* istanbul ignore if */ if (err) { diff --git a/src/db/mongo/pushes.ts b/src/db/mongo/pushes.ts index 9e3602d31..84bdd89cf 100644 --- a/src/db/mongo/pushes.ts +++ b/src/db/mongo/pushes.ts @@ -18,8 +18,8 @@ import { activityPrimaryStatusFromFlags } from '../../activity/activityPrimarySt import { canonicalRemoteUrl } from '../../activity/canonicalRemoteUrl'; import { connect, findDocuments, findOneDocument } from './helper'; import { Action } from '../../proxy/actions'; -import { toClass } from '../helper'; import { pushListProjection } from '../pushProjection'; +import { compactPush, restorePush } from '../pushStorage'; import { PushQuery, RepoActivityTabCounts, @@ -143,7 +143,7 @@ export const getPushesForUserProfile = async ( export const getPush = async (id: string): Promise => { const doc = await findOneDocument(collectionName, { id }); - return doc ? (toClass(doc, Action.prototype) as Action) : null; + return doc ? restorePush(doc) : null; }; export const deletePush = async function (id: string): Promise { @@ -152,14 +152,15 @@ export const deletePush = async function (id: string): Promise { }; export const writeAudit = async (action: Action): Promise => { - const data = JSON.parse(JSON.stringify(action)); + const data = JSON.parse(JSON.stringify(compactPush(action))); const options = { upsert: true }; const collection = await connect(collectionName); delete data._id; if (typeof data.id !== 'string') { throw new Error('Invalid id'); } - await collection.updateOne({ id: data.id }, { $set: data }, options); + const unset = data._lastStepIndex === undefined ? { _lastStepIndex: '' } : { lastStep: '' }; + await collection.updateOne({ id: data.id }, { $set: data, $unset: unset }, options); }; export const authorise = async ( diff --git a/src/db/pushStorage.ts b/src/db/pushStorage.ts new file mode 100644 index 000000000..5c4aef453 --- /dev/null +++ b/src/db/pushStorage.ts @@ -0,0 +1,42 @@ +/** + * Copyright 2026 GitProxy Contributors + * + * Licensed under the Apache License, Version 2.0 (the "License"); + * you may not use this file except in compliance with the License. + * You may obtain a copy of the License at + * + * http://www.apache.org/licenses/LICENSE-2.0 + * + * Unless required by applicable law or agreed to in writing, software + * distributed under the License is distributed on an "AS IS" BASIS, + * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. + * See the License for the specific language governing permissions and + * limitations under the License. + */ + +import { isEqual } from 'lodash'; +import { Action } from '../proxy/actions'; +import { toClass } from './helper'; + +type StoredPush = Action & { _lastStepIndex?: number }; + +export const compactPush = (action: Action) => { + const stored: Partial = { ...action }; + delete stored._lastStepIndex; + const index = action.steps?.length - 1; + if (action.lastStep && index >= 0 && isEqual(action.lastStep, action.steps[index])) { + stored._lastStepIndex = index; + delete stored.lastStep; + } + return stored; +}; + +export const restorePush = (doc: unknown): Action => { + const action: StoredPush = toClass(doc, Action.prototype); + const index = action._lastStepIndex; + if (index !== undefined && Number.isInteger(index) && index >= 0 && !action.lastStep) { + action.lastStep = action.steps?.[index]; + } + delete action._lastStepIndex; + return action; +}; diff --git a/test/db/file/pushStorage.test.ts b/test/db/file/pushStorage.test.ts index 7f22986df..2716e9b2c 100644 --- a/test/db/file/pushStorage.test.ts +++ b/test/db/file/pushStorage.test.ts @@ -17,4 +17,4 @@ import * as pushes from '../../../src/db/file/pushes'; import { definePushStorageContract } from '../pushStorage.contract'; -definePushStorageContract('NeDB push storage', pushes); +definePushStorageContract('NeDB push storage', pushes, (id) => pushes.db.findOneAsync({ id })); diff --git a/test/db/mongo/pushStorage.integration.test.ts b/test/db/mongo/pushStorage.integration.test.ts index 595925a9f..cb03165bb 100644 --- a/test/db/mongo/pushStorage.integration.test.ts +++ b/test/db/mongo/pushStorage.integration.test.ts @@ -17,7 +17,10 @@ import { describe } from 'vitest'; import * as pushes from '../../../src/db/mongo/pushes'; import { definePushStorageContract } from '../pushStorage.contract'; +import { connect } from '../../../src/db/mongo/helper'; describe.runIf(process.env.RUN_MONGO_TESTS === 'true')('MongoDB storage contract', () => { - definePushStorageContract('MongoDB push storage', pushes); + definePushStorageContract('MongoDB push storage', pushes, async (id) => + (await connect('pushes')).findOne({ id }), + ); }); diff --git a/test/db/pushStorage.contract.ts b/test/db/pushStorage.contract.ts index e78308ee0..67bceff10 100644 --- a/test/db/pushStorage.contract.ts +++ b/test/db/pushStorage.contract.ts @@ -62,7 +62,11 @@ const makePush = (id = ids[0], content = diff): Action => { }; /** Exercise real sinks with one shared contract; mocks cannot validate database projections. */ -export const definePushStorageContract = (name: string, sink: PushStorage): void => { +export const definePushStorageContract = ( + name: string, + sink: PushStorage, + readStored: (id: string) => Promise, +): void => { describe(name, () => { const cleanup = async () => { for (const id of ids) await sink.deletePush(id); @@ -70,6 +74,24 @@ export const definePushStorageContract = (name: string, sink: PushStorage): void beforeEach(cleanup); afterEach(cleanup); + it('stores a diff in the final step once without changing the detail response', async () => { + const action = makePush(); + action.steps[0].logs = ['diff - Generated diff']; + await sink.writeAudit(action); + const stored = await readStored(action.id); + expect(stored).not.toHaveProperty('lastStep'); + expect(JSON.stringify(stored).match(/private payload/g)).toHaveLength(1); + const detail = await sink.getPush(action.id); + expect(detail?.lastStep).toEqual(action.lastStep); + expect(detail).not.toHaveProperty('_lastStepIndex'); + + action.lastStep = new Step('failedAfterDiff'); + action.lastStep.setError('Preserve a separate diagnostic'); + await sink.writeAudit(action); + expect(await readStored(action.id)).not.toHaveProperty('_lastStepIndex'); + expect((await sink.getPush(action.id))?.lastStep).toEqual(action.lastStep); + }); + it('keeps legacy audit content on detail reads while excluding every diff copy from lists', async () => { const action = makePush(); // Future top-level diff records from #1693 must also remain outside list responses. diff --git a/test/db/pushStorage.test.ts b/test/db/pushStorage.test.ts new file mode 100644 index 000000000..93692ca75 --- /dev/null +++ b/test/db/pushStorage.test.ts @@ -0,0 +1,77 @@ +/** + * Copyright 2026 GitProxy Contributors + * + * Licensed under the Apache License, Version 2.0 (the "License"); + * you may not use this file except in compliance with the License. + * You may obtain a copy of the License at + * + * http://www.apache.org/licenses/LICENSE-2.0 + * + * Unless required by applicable law or agreed to in writing, software + * distributed under the License is distributed on an "AS IS" BASIS, + * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. + * See the License for the specific language governing permissions and + * limitations under the License. + */ + +import { describe, expect, it } from 'vitest'; +import { Action, RequestType, Step } from '../../src/proxy/actions'; +import { compactPush, restorePush } from '../../src/db/pushStorage'; + +const makePush = () => + new Action('compact', RequestType.PUSH, 'POST', 1, 'https://example.com/a/b'); + +describe('push storage representation', () => { + it('stores a repeated last step once and restores the detail API shape', () => { + const action = makePush(); + const step = new Step('diff'); + step.content = 'unique diff payload'; + step.logs = ['Generated diff']; + action.addStep(step); + + const stored = compactPush(action); + expect(JSON.stringify(stored).match(/unique diff payload/g)).toHaveLength(1); + expect(stored).not.toHaveProperty('lastStep'); + expect(action.lastStep).toBe(step); + expect(action).not.toHaveProperty('_lastStepIndex'); + + const restored = restorePush(Object.assign(makePush(), JSON.parse(JSON.stringify(stored)))); + expect(restored).toEqual(action); + expect(restored.getLastStep()).toBe(restored.steps[0]); + expect(restored).not.toHaveProperty('_lastStepIndex'); + expect(compactPush(restored)).toEqual(stored); + }); + + it('compacts structurally equal steps read back from a legacy document', () => { + const action = makePush(); + action.addStep(new Step('diff')); + const legacy = JSON.parse(JSON.stringify(action)); + expect(compactPush(legacy)).not.toHaveProperty('lastStep'); + }); + + it('preserves a last step containing information absent from the steps array', () => { + const action = makePush(); + action.addStep(new Step('diff')); + action.lastStep = Object.assign(new Step('diff'), action.steps[0], { + logs: ['additional diagnostic'], + }); + expect(compactPush(action).lastStep).toEqual(action.lastStep); + expect(restorePush(action)).toEqual(action); + }); + + it('preserves a legacy missing lastStep and empty steps', () => { + const action = makePush(); + expect(restorePush(action)).not.toHaveProperty('lastStep'); + action.addStep(new Step('diff')); + delete action.lastStep; + expect(compactPush(action)).not.toHaveProperty('_lastStepIndex'); + expect(restorePush(action)).not.toHaveProperty('lastStep'); + }); + + it.each([-1, 0.5, 999])('does not fabricate a step for an invalid index %s', (index) => { + const action = makePush(); + const restored = restorePush(Object.assign(action, { _lastStepIndex: index })); + expect(restored.lastStep).toBeUndefined(); + expect(restored).not.toHaveProperty('_lastStepIndex'); + }); +}); From e341b9ed2d684fadc0071c5ff659c54d5baf8c32 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Denis=20=C4=86ori=C4=87?= Date: Sun, 4 Oct 2026 16:04:10 +0200 Subject: [PATCH 06/18] perf(db): update file repository rollups on push changes --- src/db/file/pushes.ts | 129 ++++++------------------ src/db/repoActivity.ts | 162 +++++++++++++++++++++++++++++++ test/db/file/pushStorage.test.ts | 13 +++ test/db/pushStorage.contract.ts | 50 ++++++++++ test/db/repoActivity.test.ts | 84 ++++++++++++++++ 5 files changed, 340 insertions(+), 98 deletions(-) create mode 100644 src/db/repoActivity.ts create mode 100644 test/db/repoActivity.test.ts diff --git a/src/db/file/pushes.ts b/src/db/file/pushes.ts index e2b4217ce..d63d196bd 100644 --- a/src/db/file/pushes.ts +++ b/src/db/file/pushes.ts @@ -16,18 +16,17 @@ import _ from 'lodash'; import Datastore from '@seald-io/nedb'; -import { activityPrimaryStatusFromFlags } from '../../activity/activityPrimaryStatus'; -import { canonicalRemoteUrl } from '../../activity/canonicalRemoteUrl'; import { Action } from '../../proxy/actions/Action'; import { toClass } from '../helper'; import { pushListProjection } from '../pushProjection'; import { compactPush, restorePush } from '../pushStorage'; +import { PushQuery, RepoPushRollupsByCanonicalUrl } from '../types'; import { - PushQuery, - RepoActivityTabCounts, - RepoPushRollupsByCanonicalUrl, - emptyRepoActivityTabCounts, -} from '../types'; + RepoActivityIndex, + pushActivity, + pushActivityProjection, + type PushActivityRow, +} from '../repoActivity'; import { CompletedAttestation, Rejection } from '../../proxy/processors/types'; import { handleErrorAndLog } from '../../utils/errors'; import { buildUserProfilePushFilter } from '../userProfilePushQuery'; @@ -51,98 +50,29 @@ try { } db.setAutocompactionInterval(COMPACTION_INTERVAL); -function bumpCount( - m: Map, - canonicalKey: string, - tab: keyof RepoActivityTabCounts, -): void { - if (!canonicalKey) { - return; - } - let row = m.get(canonicalKey); - if (!row) { - row = emptyRepoActivityTabCounts(); - m.set(canonicalKey, row); - } - row[tab] += 1; -} - -function bumpMaxTimestampMs( - m: Map, - canonicalKey: string, - timestamp: unknown, -): void { - if (!canonicalKey) { - return; - } - const ts = typeof timestamp === 'number' ? timestamp : NaN; - if (!Number.isFinite(ts)) { - return; - } - const prev = m.get(canonicalKey); - if (prev === undefined || ts > prev) { - m.set(canonicalKey, ts); - } -} - -type PushActivityProjection = { - url?: string; - error?: boolean; - rejected?: boolean; - canceled?: boolean; - authorised?: boolean; - blocked?: boolean; - allowPush?: boolean; - timestamp?: number; -}; - -export const getRepoPushRollupsByCanonicalUrl = (): Promise => { - return new Promise((resolve, reject) => { - db.find( - { type: 'push' }, - { - url: 1, - error: 1, - rejected: 1, - canceled: 1, - authorised: 1, - blocked: 1, - allowPush: 1, - timestamp: 1, - }, - (err, docs: PushActivityProjection[]) => { - if (err) { - reject(err); - return; - } - const tabCounts = new Map(); - const latestPendingReviewAtMs = new Map(); - const latestPushAtMs = new Map(); - for (const doc of docs) { - const url = typeof doc.url === 'string' ? doc.url : ''; - const key = canonicalRemoteUrl(url); - if (!key) { - continue; +let activityIndex: RepoActivityIndex | undefined; +let activityIndexLoad: Promise | undefined; + +export const getRepoPushRollupsByCanonicalUrl = + async (): Promise => { + if (!activityIndexLoad) { + activityIndexLoad = new Promise((resolve, reject) => { + db.find({}, pushActivityProjection, (err, docs: PushActivityRow[]) => { + if (err) { + reject(err); + return; } - const tab = activityPrimaryStatusFromFlags({ - error: doc.error === true, - rejected: doc.rejected === true, - canceled: doc.canceled === true, - authorised: doc.authorised === true, - blocked: doc.blocked === true, - allowPush: doc.allowPush === true, - }); - bumpCount(tabCounts, key, tab); - bumpMaxTimestampMs(latestPushAtMs, key, doc.timestamp); - if (tab === 'pending') { - bumpMaxTimestampMs(latestPendingReviewAtMs, key, doc.timestamp); - } - } - resolve({ tabCounts, latestPendingReviewAtMs, latestPushAtMs }); - }, - ); - }); -}; + activityIndex = new RepoActivityIndex(); + for (const doc of docs) activityIndex.set(doc.id, pushActivity(doc)); + resolve(activityIndex); + }); + }).catch((error: unknown) => { + activityIndexLoad = undefined; + throw error; + }); + } + return (await activityIndexLoad).snapshot(); + }; const defaultPushQuery: Partial = { error: false, @@ -224,6 +154,7 @@ export const deletePush = async (id: string): Promise => { if (err) { reject(err); } else { + activityIndex?.set(id, null); resolve(); } }); @@ -232,6 +163,7 @@ export const deletePush = async (id: string): Promise => { export const writeAudit = async (action: Action): Promise => { return new Promise((resolve, reject) => { + const contribution = pushActivity(action); const options = { multi: false, upsert: true }; db.update({ id: action.id }, compactPush(action), options, (err) => { // ignore for code coverage as neDB rarely returns errors even for an invalid query @@ -239,6 +171,7 @@ export const writeAudit = async (action: Action): Promise => { if (err) { reject(err); } else { + activityIndex?.set(action.id, contribution); resolve(); } }); diff --git a/src/db/repoActivity.ts b/src/db/repoActivity.ts new file mode 100644 index 000000000..4333c183b --- /dev/null +++ b/src/db/repoActivity.ts @@ -0,0 +1,162 @@ +/** + * Copyright 2026 GitProxy Contributors + * + * Licensed under the Apache License, Version 2.0 (the "License"); + * you may not use this file except in compliance with the License. + * You may obtain a copy of the License at + * + * http://www.apache.org/licenses/LICENSE-2.0 + * + * Unless required by applicable law or agreed to in writing, software + * distributed under the License is distributed on an "AS IS" BASIS, + * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. + * See the License for the specific language governing permissions and + * limitations under the License. + */ + +import { isEqual } from 'lodash'; +import { + activityPrimaryStatusFromFlags, + type ActivityPrimaryStatusInput, + type ActivityStatusTab, +} from '../activity/activityPrimaryStatus'; +import { canonicalRemoteUrl } from '../activity/canonicalRemoteUrl'; +import { + emptyRepoActivityTabCounts, + type RepoActivityTabCounts, + type RepoPushRollupsByCanonicalUrl, +} from './types'; + +export type PushActivityRow = ActivityPrimaryStatusInput & { + id: string; + type?: string; + url?: string; + timestamp?: number; +}; + +export type PushActivity = { + key: string; + tab: ActivityStatusTab; + timestamp: number | null; +}; + +export type RepoActivitySummary = { + key: string; + counts: RepoActivityTabCounts; + latestPush: number | null; + latestPending: number | null; +}; + +export const pushActivityProjection = { + _id: 0, + id: 1, + type: 1, + url: 1, + timestamp: 1, + error: 1, + rejected: 1, + canceled: 1, + authorised: 1, + blocked: 1, + allowPush: 1, +} as const; + +export const pushActivity = (row: PushActivityRow): PushActivity | null => { + const key = row.type === 'push' && typeof row.url === 'string' ? canonicalRemoteUrl(row.url) : ''; + if (!key) return null; + return { + key, + tab: activityPrimaryStatusFromFlags(row), + timestamp: + typeof row.timestamp === 'number' && Number.isFinite(row.timestamp) ? row.timestamp : null, + }; +}; + +export const emptyRepoSummary = (key: string): RepoActivitySummary => ({ + key, + counts: emptyRepoActivityTabCounts(), + latestPush: null, + latestPending: null, +}); + +const maxTimestamp = (previous: number | null, next: number | null): number | null => + next === null ? previous : previous === null ? next : Math.max(previous, next); + +export const addActivity = (summary: RepoActivitySummary, activity: PushActivity): void => { + summary.counts[activity.tab]++; + summary.latestPush = maxTimestamp(summary.latestPush, activity.timestamp); + if (activity.tab === 'pending') { + summary.latestPending = maxTimestamp(summary.latestPending, activity.timestamp); + } +}; + +export const repoRollups = ( + summaries: Iterable, +): RepoPushRollupsByCanonicalUrl => { + const result: RepoPushRollupsByCanonicalUrl = { + tabCounts: new Map(), + latestPendingReviewAtMs: new Map(), + latestPushAtMs: new Map(), + }; + for (const summary of summaries) { + if (!Object.values(summary.counts).some((count) => count > 0)) continue; + result.tabCounts.set(summary.key, { ...summary.counts }); + if (summary.latestPush !== null) result.latestPushAtMs.set(summary.key, summary.latestPush); + if (summary.latestPending !== null) + result.latestPendingReviewAtMs.set(summary.key, summary.latestPending); + } + return result; +}; + +export class RepoActivityIndex { + private readonly pushes = new Map(); + private readonly repositories = new Map>(); + private readonly summaries = new Map(); + + set(id: string, activity: PushActivity | null): void { + const previous = this.pushes.get(id); + if (isEqual(previous ?? null, activity)) return; + if (previous) { + this.pushes.delete(id); + const rows = this.repositories.get(previous.key); + const summary = this.summaries.get(previous.key); + if (rows && summary) { + rows.delete(id); + summary.counts[previous.tab]--; + if (!rows.size) { + this.repositories.delete(previous.key); + this.summaries.delete(previous.key); + } else if ( + previous.timestamp !== null && + (previous.timestamp === summary.latestPush || + previous.timestamp === summary.latestPending) + ) { + summary.latestPush = null; + summary.latestPending = null; + for (const row of rows.values()) { + summary.latestPush = maxTimestamp(summary.latestPush, row.timestamp); + if (row.tab === 'pending') + summary.latestPending = maxTimestamp(summary.latestPending, row.timestamp); + } + } + } + } + if (activity) { + this.pushes.set(id, activity); + let rows = this.repositories.get(activity.key); + let summary = this.summaries.get(activity.key); + if (!rows || !summary) { + rows = new Map(); + summary = emptyRepoSummary(activity.key); + this.repositories.set(activity.key, rows); + this.summaries.set(activity.key, summary); + } + rows.set(id, activity); + addActivity(summary, activity); + } + } + + snapshot(): RepoPushRollupsByCanonicalUrl { + return repoRollups(this.summaries.values()); + } +} diff --git a/test/db/file/pushStorage.test.ts b/test/db/file/pushStorage.test.ts index 2716e9b2c..a5a446106 100644 --- a/test/db/file/pushStorage.test.ts +++ b/test/db/file/pushStorage.test.ts @@ -16,5 +16,18 @@ import * as pushes from '../../../src/db/file/pushes'; import { definePushStorageContract } from '../pushStorage.contract'; +import { expect, it, vi } from 'vitest'; definePushStorageContract('NeDB push storage', pushes, (id) => pushes.db.findOneAsync({ id })); + +it('serves repeated repository reads without querying push history again', async () => { + await pushes.getRepoPushRollupsByCanonicalUrl(); + const find = vi.spyOn(pushes.db, 'find'); + try { + await pushes.getRepoPushRollupsByCanonicalUrl(); + await pushes.getRepoPushRollupsByCanonicalUrl(); + expect(find).not.toHaveBeenCalled(); + } finally { + find.mockRestore(); + } +}); diff --git a/test/db/pushStorage.contract.ts b/test/db/pushStorage.contract.ts index 67bceff10..96928fbbe 100644 --- a/test/db/pushStorage.contract.ts +++ b/test/db/pushStorage.contract.ts @@ -244,5 +244,55 @@ export const definePushStorageContract = ( expect((await sink.getRepoPushRollupsByCanonicalUrl()).tabCounts.has(key)).toBe(false); expect(await sink.getPush(action.id)).toBeNull(); }); + + it('handles retries, concurrent writes, and removal of the latest pending push', async () => { + const key = canonicalRemoteUrl(url); + const actions = ids.map((id, index) => { + const action = makePush(id); + action.timestamp = 100 + index; + return action; + }); + await Promise.all(actions.map((action) => sink.writeAudit(action))); + await Promise.all(actions.map((action) => sink.writeAudit(action))); + expect((await sink.getRepoPushRollupsByCanonicalUrl()).tabCounts.get(key)?.pending).toBe(3); + await Promise.all([sink.authorise(ids[2], attestation), sink.reject(ids[0], rejection)]); + const reviewed = await sink.getRepoPushRollupsByCanonicalUrl(); + expect(reviewed.tabCounts.get(key)).toEqual({ + pending: 1, + approved: 1, + rejected: 1, + canceled: 0, + error: 0, + }); + expect(reviewed.latestPushAtMs.get(key)).toBe(102); + expect(reviewed.latestPendingReviewAtMs.get(key)).toBe(101); + await sink.deletePush(ids[2]); + await sink.deletePush(ids[2]); + expect((await sink.getRepoPushRollupsByCanonicalUrl()).latestPushAtMs.get(key)).toBe(101); + }); + + it('moves activity when a stored push changes its canonical repository or request type', async () => { + const action = makePush(); + await sink.writeAudit(action); + await sink.getRepoPushRollupsByCanonicalUrl(); + action.url = 'git@EXAMPLE.com:storage-contract/REPO.git'; + await sink.writeAudit(action); + expect( + (await sink.getRepoPushRollupsByCanonicalUrl()).tabCounts.get(canonicalRemoteUrl(url)) + ?.pending, + ).toBe(1); + action.url = 'https://example.com/storage-contract/moved.git'; + await sink.writeAudit(action); + const moved = await sink.getRepoPushRollupsByCanonicalUrl(); + expect(moved.tabCounts.has(canonicalRemoteUrl(url))).toBe(false); + expect(moved.tabCounts.get(canonicalRemoteUrl(action.url))?.pending).toBe(1); + action.type = RequestType.PULL; + await sink.writeAudit(action); + expect( + (await sink.getRepoPushRollupsByCanonicalUrl()).tabCounts.has( + canonicalRemoteUrl(action.url), + ), + ).toBe(false); + }); }); }; diff --git a/test/db/repoActivity.test.ts b/test/db/repoActivity.test.ts new file mode 100644 index 000000000..bf7438ac6 --- /dev/null +++ b/test/db/repoActivity.test.ts @@ -0,0 +1,84 @@ +/** + * Copyright 2026 GitProxy Contributors + * + * Licensed under the Apache License, Version 2.0 (the "License"); + * you may not use this file except in compliance with the License. + * You may obtain a copy of the License at + * + * http://www.apache.org/licenses/LICENSE-2.0 + * + * Unless required by applicable law or agreed to in writing, software + * distributed under the License is distributed on an "AS IS" BASIS, + * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. + * See the License for the specific language governing permissions and + * limitations under the License. + */ + +import { describe, expect, it } from 'vitest'; +import { RepoActivityIndex, pushActivity, type PushActivity } from '../../src/db/repoActivity'; + +describe('repository activity index', () => { + it('updates counts once, falls back to older timestamps, and removes empty repositories', () => { + const index = new RepoActivityIndex(); + const old: PushActivity = { key: 'example.com/a/b', tab: 'pending', timestamp: 10 }; + index.set('old', old); + index.set('new', { ...old, timestamp: 20 }); + index.set('old', old); + expect(index.snapshot().tabCounts.get(old.key)?.pending).toBe(2); + index.set('new', { ...old, tab: 'approved', timestamp: 20 }); + expect(index.snapshot().latestPendingReviewAtMs.get(old.key)).toBe(10); + expect(index.snapshot().latestPushAtMs.get(old.key)).toBe(20); + index.set('new', null); + expect(index.snapshot().latestPushAtMs.get(old.key)).toBe(10); + index.set('old', null); + expect(index.snapshot().tabCounts.size).toBe(0); + expect(index.snapshot().latestPushAtMs.size).toBe(0); + }); + + it('moves contributions between repositories and isolates returned snapshots', () => { + const index = new RepoActivityIndex(); + index.set('push', { key: 'old', tab: 'pending', timestamp: 0 }); + const first = index.snapshot(); + const counts = first.tabCounts.get('old'); + if (counts) counts.pending = 42; + expect(index.snapshot().tabCounts.get('old')?.pending).toBe(1); + index.set('push', { key: 'new', tab: 'error', timestamp: -1 }); + expect(index.snapshot().tabCounts.has('old')).toBe(false); + expect(index.snapshot().tabCounts.get('new')?.error).toBe(1); + expect(index.snapshot().latestPushAtMs.get('new')).toBe(-1); + }); + + it('uses the Activity status precedence and canonical remote identity', () => { + expect( + pushActivity({ + id: 'push', + type: 'push', + url: 'git@Example.com:ORG/REPO.git', + error: true, + rejected: true, + canceled: true, + authorised: true, + blocked: true, + timestamp: 10, + }), + ).toEqual({ key: 'example.com/org/repo', tab: 'error', timestamp: 10 }); + expect(pushActivity({ id: 'pull', type: 'pull', url: 'https://example.com/a/b' })).toBeNull(); + expect(pushActivity({ id: 'empty', type: 'push', url: ' ' })).toBeNull(); + }); + + it.each([undefined, NaN, Infinity, -Infinity])( + 'counts pushes without using invalid timestamp %s', + (timestamp) => { + const activity = pushActivity({ + id: 'push', + type: 'push', + url: 'https://example.com/a/b', + timestamp, + }); + const index = new RepoActivityIndex(); + index.set('push', activity); + expect(index.snapshot().tabCounts.get('example.com/a/b')?.pending).toBe(1); + expect(index.snapshot().latestPushAtMs.size).toBe(0); + }, + ); +}); From f7952a27c6ccbb7292022ac4f685b51852077a85 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Denis=20=C4=86ori=C4=87?= Date: Sun, 4 Oct 2026 16:15:48 +0200 Subject: [PATCH 07/18] perf(db): refresh MongoDB activity for changed repositories --- src/db/mongo/helper.ts | 6 +- src/db/mongo/pushes.ts | 151 ++++--------- src/db/mongo/repoActivity.ts | 204 ++++++++++++++++++ src/db/pushStorage.ts | 4 +- test/db/mongo/push.test.ts | 53 ++++- test/db/mongo/pushStorage.integration.test.ts | 200 ++++++++++++++++- test/db/mongo/pushes.test.ts | 13 +- test/setup-integration.ts | 2 +- 8 files changed, 512 insertions(+), 121 deletions(-) create mode 100644 src/db/mongo/repoActivity.ts diff --git a/src/db/mongo/helper.ts b/src/db/mongo/helper.ts index 6abc33d21..289f02503 100644 --- a/src/db/mongo/helper.ts +++ b/src/db/mongo/helper.ts @@ -32,7 +32,9 @@ export const resetConnection = async (): Promise => { export const getDb = (): Db | null => _db; -export const connect = async (collectionName: string): Promise => { +export const connect = async ( + collectionName: string, +): Promise> => { //retrieve config at point of use (rather than import) const dbConfig = getDatabase(); const connectionString = dbConfig.connectionString; @@ -53,7 +55,7 @@ export const connect = async (collectionName: string): Promise => { _db = _client.db(); } - return _db.collection(collectionName); + return _db.collection(collectionName); }; export const findDocuments = async ( diff --git a/src/db/mongo/pushes.ts b/src/db/mongo/pushes.ts index 84bdd89cf..7af77664b 100644 --- a/src/db/mongo/pushes.ts +++ b/src/db/mongo/pushes.ts @@ -14,105 +14,18 @@ * limitations under the License. */ -import { activityPrimaryStatusFromFlags } from '../../activity/activityPrimaryStatus'; -import { canonicalRemoteUrl } from '../../activity/canonicalRemoteUrl'; -import { connect, findDocuments, findOneDocument } from './helper'; +import { findDocuments, findOneDocument } from './helper'; +import { activityCollections, activityState, deletePushWithActivity } from './repoActivity'; +export { getRepoPushRollupsByCanonicalUrl } from './repoActivity'; import { Action } from '../../proxy/actions'; import { pushListProjection } from '../pushProjection'; import { compactPush, restorePush } from '../pushStorage'; -import { - PushQuery, - RepoActivityTabCounts, - RepoPushRollupsByCanonicalUrl, - emptyRepoActivityTabCounts, -} from '../types'; +import { PushQuery } from '../types'; import { CompletedAttestation, Rejection } from '../../proxy/processors/types'; import { buildUserProfilePushFilter } from '../userProfilePushQuery'; const collectionName = 'pushes'; -function bumpCount( - m: Map, - canonicalKey: string, - tab: keyof RepoActivityTabCounts, -): void { - if (!canonicalKey) { - return; - } - let row = m.get(canonicalKey); - if (!row) { - row = emptyRepoActivityTabCounts(); - m.set(canonicalKey, row); - } - row[tab] += 1; -} - -function bumpMaxTimestampMs( - m: Map, - canonicalKey: string, - timestamp: unknown, -): void { - if (!canonicalKey) { - return; - } - const ts = typeof timestamp === 'number' ? timestamp : NaN; - if (!Number.isFinite(ts)) { - return; - } - const prev = m.get(canonicalKey); - if (prev === undefined || ts > prev) { - m.set(canonicalKey, ts); - } -} - -/** - * Scan all push rows: tab counts and max timestamps per canonical remote URL (matches Activity UI). - * Uses a tight projection and in-Node canonicalization for DocumentDB compatibility. - */ -export const getRepoPushRollupsByCanonicalUrl = - async (): Promise => { - const collection = await connect(collectionName); - const cursor = collection.find( - { type: 'push' }, - { - projection: { - url: 1, - error: 1, - rejected: 1, - canceled: 1, - authorised: 1, - blocked: 1, - allowPush: 1, - timestamp: 1, - }, - }, - ); - const tabCounts = new Map(); - const latestPendingReviewAtMs = new Map(); - const latestPushAtMs = new Map(); - for await (const doc of cursor) { - const url = typeof doc.url === 'string' ? doc.url : ''; - const key = canonicalRemoteUrl(url); - if (!key) { - continue; - } - const tab = activityPrimaryStatusFromFlags({ - error: doc.error === true, - rejected: doc.rejected === true, - canceled: doc.canceled === true, - authorised: doc.authorised === true, - blocked: doc.blocked === true, - allowPush: doc.allowPush === true, - }); - bumpCount(tabCounts, key, tab); - bumpMaxTimestampMs(latestPushAtMs, key, doc.timestamp); - if (tab === 'pending') { - bumpMaxTimestampMs(latestPendingReviewAtMs, key, doc.timestamp); - } - } - return { tabCounts, latestPendingReviewAtMs, latestPushAtMs }; - }; - const defaultPushQuery: Partial = { error: false, blocked: true, @@ -124,10 +37,14 @@ const defaultPushQuery: Partial = { export const getPushes = async ( query: Partial = defaultPushQuery, ): Promise => { - return findDocuments(collectionName, query, { - projection: pushListProjection, - sort: { timestamp: -1 }, - }); + return findDocuments( + collectionName, + { ...query, '_activity.deleted': { $ne: true } }, + { + projection: pushListProjection, + sort: { timestamp: -1 }, + }, + ); }; export const getPushesForUserProfile = async ( @@ -135,32 +52,56 @@ export const getPushesForUserProfile = async ( profileUsername: string, ): Promise => { const filter = buildUserProfilePushFilter(emailVariants, profileUsername); - return findDocuments(collectionName, filter, { - projection: pushListProjection, - sort: { timestamp: -1 }, - }); + return findDocuments( + collectionName, + { ...filter, '_activity.deleted': { $ne: true } }, + { + projection: pushListProjection, + sort: { timestamp: -1 }, + }, + ); }; export const getPush = async (id: string): Promise => { - const doc = await findOneDocument(collectionName, { id }); + const doc = await findOneDocument(collectionName, { + id, + '_activity.deleted': { $ne: true }, + }); return doc ? restorePush(doc) : null; }; export const deletePush = async function (id: string): Promise { - const collection = await connect(collectionName); - await collection.deleteOne({ id }); + await deletePushWithActivity(id); }; export const writeAudit = async (action: Action): Promise => { const data = JSON.parse(JSON.stringify(compactPush(action))); const options = { upsert: true }; - const collection = await connect(collectionName); + const { pushes: collection } = await activityCollections(); delete data._id; if (typeof data.id !== 'string') { throw new Error('Invalid id'); } - const unset = data._lastStepIndex === undefined ? { _lastStepIndex: '' } : { lastStep: '' }; - await collection.updateOne({ id: data.id }, { $set: data, $unset: unset }, options); + const unset: Record = + data._lastStepIndex === undefined ? { _lastStepIndex: '' } : { lastStep: '' }; + const activity = activityState(action); + await collection.updateOne( + { id: data.id }, + { + $set: { + ...data, + '_activity.key': activity.key, + '_activity.tab': activity.tab, + '_activity.timestamp': activity.timestamp, + '_activity.revision': activity.revision, + '_activity.dirty': true, + '_activity.deleted': false, + }, + $addToSet: { '_activity.keys': activity.key }, + $unset: unset, + }, + options, + ); }; export const authorise = async ( diff --git a/src/db/mongo/repoActivity.ts b/src/db/mongo/repoActivity.ts new file mode 100644 index 000000000..671ba02be --- /dev/null +++ b/src/db/mongo/repoActivity.ts @@ -0,0 +1,204 @@ +/** + * Copyright 2026 GitProxy Contributors + * + * Licensed under the Apache License, Version 2.0 (the "License"); + * you may not use this file except in compliance with the License. + * You may obtain a copy of the License at + * + * http://www.apache.org/licenses/LICENSE-2.0 + * + * Unless required by applicable law or agreed to in writing, software + * distributed under the License is distributed on an "AS IS" BASIS, + * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. + * See the License for the specific language governing permissions and + * limitations under the License. + */ + +import { randomUUID } from 'node:crypto'; +import type { Db } from 'mongodb'; +import { connect, getDb } from './helper'; +import { + addActivity, + emptyRepoSummary, + pushActivity, + pushActivityProjection, + repoRollups, + RepoActivityIndex, + type PushActivity, + type PushActivityRow, + type RepoActivitySummary, +} from '../repoActivity'; +import type { RepoPushRollupsByCanonicalUrl } from '../types'; + +export type ActivityState = PushActivity & { + revision: string; + keys: string[]; + dirty: boolean; + deleted: boolean; +}; + +type ActivityDocument = PushActivityRow & { _activity?: ActivityState }; +type StoredSummary = RepoActivitySummary & { revision: string; ready: boolean }; +const initialized = new WeakMap>(); + +export const activityState = (row: PushActivityRow): ActivityState => { + const activity = pushActivity(row) ?? { key: '', tab: 'pending', timestamp: null }; + return { ...activity, revision: randomUUID(), keys: [activity.key], dirty: true, deleted: false }; +}; + +export const activityCollections = async () => { + const pushes = await connect('pushes'); + const summaries = await connect('repoPushActivity'); + const database = getDb(); + if (!database) throw new Error('MongoDB connection is not available'); + let initialization = initialized.get(database); + if (!initialization) { + initialization = Promise.all([ + pushes.createIndex({ id: 1 }, { unique: true }), + pushes.createIndex({ '_activity.dirty': 1 }), + pushes.createIndex({ + '_activity.key': 1, + '_activity.deleted': 1, + '_activity.tab': 1, + '_activity.timestamp': 1, + }), + summaries.createIndex({ key: 1 }, { unique: true }), + ]) + .then(() => undefined) + .catch((error: unknown) => { + initialized.delete(database); + throw error; + }); + initialized.set(database, initialization); + } + await initialization; + return { pushes, summaries }; +}; + +const backfillActivity = async (): Promise => { + const { pushes } = await activityCollections(); + const cursor = pushes.find( + { '_activity.dirty': { $exists: false } }, + { projection: { ...pushActivityProjection, _id: 1 } }, + ); + for await (const row of cursor) { + await pushes.updateOne( + { _id: row._id, '_activity.dirty': { $exists: false } }, + { $set: { _activity: activityState(row) } }, + ); + } +}; + +const refreshRepository = async (key: string): Promise => { + const { pushes, summaries } = await activityCollections(); + const revision = randomUUID(); + // A newer refresh fences out stale snapshots, including work resumed after a crash. + await summaries.updateOne({ key }, { $set: { revision, ready: false } }, { upsert: true }); + const summary = emptyRepoSummary(key); + const cursor = pushes.find( + { '_activity.key': key, '_activity.deleted': false }, + { projection: { _id: 0, '_activity.key': 1, '_activity.tab': 1, '_activity.timestamp': 1 } }, + ); + for await (const row of cursor) { + if (row._activity) addActivity(summary, row._activity); + } + const result = await summaries.updateOne( + { key, revision }, + { $set: { ...summary, ready: true } }, + ); + return result.matchedCount === 1; +}; + +const scanActivity = async (): Promise => { + const { pushes } = await activityCollections(); + const index = new RepoActivityIndex(); + for await (const row of pushes.find( + { '_activity.deleted': { $ne: true } }, + { projection: pushActivityProjection }, + )) { + index.set(row.id, pushActivity(row)); + } + return index.snapshot(); +}; + +export const getRepoPushRollupsByCanonicalUrl = + async (): Promise => { + await backfillActivity(); + const { pushes, summaries } = await activityCollections(); + for (let attempt = 0; attempt < 3; attempt++) { + const dirty = await pushes + .find({ '_activity.dirty': true }, { projection: { _id: 1, _activity: 1 } }) + .toArray(); + const unfinished = await summaries + .find({ ready: false }, { projection: { key: 1 } }) + .toArray(); + if (!dirty.length && !unfinished.length) { + const rows = await summaries.find({}).toArray(); + if (rows.every((row) => row.ready)) return repoRollups(rows); + continue; + } + const keys = new Set(unfinished.map((row) => row.key)); + for (const row of dirty) { + for (const key of row._activity?.keys ?? []) if (key) keys.add(key); + } + const refreshed = new Set(); + for (const key of keys) { + if (await refreshRepository(key)) refreshed.add(key); + } + for (const row of dirty) { + const activity = row._activity; + if (!activity || activity.keys.some((key) => key && !refreshed.has(key))) continue; + // Clear only the exact revision summarized above. Concurrent writes remain dirty. + const filter = { _id: row._id, '_activity.revision': activity.revision }; + if (activity.deleted) { + await pushes.deleteOne(filter); + } else { + await pushes.updateOne(filter, { + $set: { '_activity.dirty': false, '_activity.keys': [activity.key] }, + }); + } + } + } + // Bound request latency under continuous writes; the source rows remain authoritative. + return scanActivity(); + }; + +export const deletePushWithActivity = async (id: string): Promise => { + const { pushes } = await activityCollections(); + for (let attempt = 0; attempt < 3; attempt++) { + const row = await pushes.findOne( + { id }, + { + projection: { ...pushActivityProjection, _id: 1, _activity: 1 }, + }, + ); + if (!row || row._activity?.deleted) return; + const previous = row._activity; + const activity = previous ?? activityState(row); + const result = await pushes.replaceOne( + { + _id: row._id, + '_activity.revision': previous ? previous.revision : { $exists: false }, + }, + { + id, + _activity: { ...activity, revision: randomUUID(), dirty: true, deleted: true }, + }, + ); + if (result.matchedCount === 1) return; + } + throw new Error(`Push ${id} changed repeatedly during deletion; retry the request`); +}; + +export const rebuildRepoPushRollups = async (): Promise => { + const { pushes, summaries } = await activityCollections(); + await backfillActivity(); + await summaries.updateMany({}, { $set: { ready: false } }); + await pushes.updateMany( + {}, + { + $set: { '_activity.dirty': true, '_activity.revision': randomUUID() }, + }, + ); + return getRepoPushRollupsByCanonicalUrl(); +}; diff --git a/src/db/pushStorage.ts b/src/db/pushStorage.ts index 5c4aef453..d9e99f0ec 100644 --- a/src/db/pushStorage.ts +++ b/src/db/pushStorage.ts @@ -18,11 +18,12 @@ import { isEqual } from 'lodash'; import { Action } from '../proxy/actions'; import { toClass } from './helper'; -type StoredPush = Action & { _lastStepIndex?: number }; +type StoredPush = Action & { _lastStepIndex?: number; _activity?: unknown }; export const compactPush = (action: Action) => { const stored: Partial = { ...action }; delete stored._lastStepIndex; + delete stored._activity; const index = action.steps?.length - 1; if (action.lastStep && index >= 0 && isEqual(action.lastStep, action.steps[index])) { stored._lastStepIndex = index; @@ -38,5 +39,6 @@ export const restorePush = (doc: unknown): Action => { action.lastStep = action.steps?.[index]; } delete action._lastStepIndex; + delete action._activity; return action; }; diff --git a/test/db/mongo/push.test.ts b/test/db/mongo/push.test.ts index 33d1b9efc..bb8926809 100644 --- a/test/db/mongo/push.test.ts +++ b/test/db/mongo/push.test.ts @@ -20,11 +20,15 @@ import { pushListProjection } from '../../../src/db/pushProjection'; const mockFindOne = vi.fn(); const mockDeleteOne = vi.fn(); +const mockReplaceOne = vi.fn(); +const mockDatabase = {}; const mockUpdateOne = vi.fn(); const mockFind = vi.fn(); const mockConnect = vi.fn(() => ({ findOne: mockFindOne, + replaceOne: mockReplaceOne, + createIndex: vi.fn().mockResolvedValue('index'), deleteOne: mockDeleteOne, updateOne: mockUpdateOne, find: mockFind, @@ -35,6 +39,7 @@ const mockFindOneDocument = vi.fn(); vi.mock('../../../src/db/mongo/helper', () => ({ connect: mockConnect, + getDb: () => mockDatabase, findDocuments: mockFindDocuments, findOneDocument: mockFindOneDocument, })); @@ -102,6 +107,7 @@ describe('MongoDB Push Handler', async () => { allowPush: false, authorised: false, type: 'push', + '_activity.deleted': { $ne: true }, }, { projection: pushListProjection, @@ -122,7 +128,7 @@ describe('MongoDB Push Handler', async () => { expect(mockFindDocuments).toHaveBeenCalledWith( 'pushes', - customQuery, + { ...customQuery, '_activity.deleted': { $ne: true } }, expect.objectContaining({ projection: expect.any(Object), }), @@ -168,7 +174,10 @@ describe('MongoDB Push Handler', async () => { const result = await getPush(TEST_PUSH.id); - expect(mockFindOneDocument).toHaveBeenCalledWith('pushes', { id: TEST_PUSH.id }); + expect(mockFindOneDocument).toHaveBeenCalledWith('pushes', { + id: TEST_PUSH.id, + '_activity.deleted': { $ne: true }, + }); expect(mockToClass).toHaveBeenCalledWith(TEST_PUSH, Action.prototype); expect(result).toBeTruthy(); }); @@ -178,7 +187,10 @@ describe('MongoDB Push Handler', async () => { const result = await getPush('non-existent-id'); - expect(mockFindOneDocument).toHaveBeenCalledWith('pushes', { id: 'non-existent-id' }); + expect(mockFindOneDocument).toHaveBeenCalledWith('pushes', { + id: 'non-existent-id', + '_activity.deleted': { $ne: true }, + }); expect(result).toBeNull(); expect(mockToClass).not.toHaveBeenCalled(); }); @@ -186,12 +198,20 @@ describe('MongoDB Push Handler', async () => { describe('deletePush', () => { it('should delete a push by id', async () => { - mockDeleteOne.mockResolvedValue({ deletedCount: 1 }); + mockFindOne.mockResolvedValue({ ...TEST_PUSH, _id: 'stored-push' }); + mockReplaceOne.mockResolvedValue({ matchedCount: 1 }); await deletePush(TEST_PUSH.id); expect(mockConnect).toHaveBeenCalledWith('pushes'); - expect(mockDeleteOne).toHaveBeenCalledWith({ id: TEST_PUSH.id }); + expect(mockReplaceOne).toHaveBeenCalledWith( + { _id: 'stored-push', '_activity.revision': { $exists: false } }, + { + id: TEST_PUSH.id, + _activity: expect.objectContaining({ deleted: true, dirty: true, key: 'example.com/' }), + }, + ); + expect(mockDeleteOne).not.toHaveBeenCalled(); }); }); @@ -211,6 +231,8 @@ describe('MongoDB Push Handler', async () => { allowPush: TEST_PUSH.allowPush, authorised: TEST_PUSH.authorised, }), + $unset: { _lastStepIndex: '' }, + $addToSet: { '_activity.keys': 'example.com/' }, }, { upsert: true }, ); @@ -235,7 +257,10 @@ describe('MongoDB Push Handler', async () => { const attestation = { signature: 'test-sig' }; const result = await authorise(TEST_PUSH.id, attestation); - expect(mockFindOneDocument).toHaveBeenCalledWith('pushes', { id: TEST_PUSH.id }); + expect(mockFindOneDocument).toHaveBeenCalledWith('pushes', { + id: TEST_PUSH.id, + '_activity.deleted': { $ne: true }, + }); expect(mockConnect).toHaveBeenCalledWith('pushes'); expect(mockUpdateOne).toHaveBeenCalledWith( { id: TEST_PUSH.id }, @@ -246,6 +271,8 @@ describe('MongoDB Push Handler', async () => { rejected: false, attestation: attestation, }), + $unset: { _lastStepIndex: '' }, + $addToSet: { '_activity.keys': 'example.com/' }, }, { upsert: true }, ); @@ -270,7 +297,10 @@ describe('MongoDB Push Handler', async () => { const rejection = { signature: 'test-sig' }; const result = await reject(TEST_PUSH.id, rejection); - expect(mockFindOneDocument).toHaveBeenCalledWith('pushes', { id: TEST_PUSH.id }); + expect(mockFindOneDocument).toHaveBeenCalledWith('pushes', { + id: TEST_PUSH.id, + '_activity.deleted': { $ne: true }, + }); expect(mockConnect).toHaveBeenCalledWith('pushes'); expect(mockUpdateOne).toHaveBeenCalledWith( { id: TEST_PUSH.id }, @@ -281,6 +311,8 @@ describe('MongoDB Push Handler', async () => { rejected: true, rejection: rejection, }), + $unset: { _lastStepIndex: '' }, + $addToSet: { '_activity.keys': 'example.com/' }, }, { upsert: true }, ); @@ -304,7 +336,10 @@ describe('MongoDB Push Handler', async () => { const result = await cancel(TEST_PUSH.id); - expect(mockFindOneDocument).toHaveBeenCalledWith('pushes', { id: TEST_PUSH.id }); + expect(mockFindOneDocument).toHaveBeenCalledWith('pushes', { + id: TEST_PUSH.id, + '_activity.deleted': { $ne: true }, + }); expect(mockConnect).toHaveBeenCalledWith('pushes'); expect(mockUpdateOne).toHaveBeenCalledWith( { id: TEST_PUSH.id }, @@ -314,6 +349,8 @@ describe('MongoDB Push Handler', async () => { canceled: true, rejected: false, }), + $unset: { _lastStepIndex: '' }, + $addToSet: { '_activity.keys': 'example.com/' }, }, { upsert: true }, ); diff --git a/test/db/mongo/pushStorage.integration.test.ts b/test/db/mongo/pushStorage.integration.test.ts index cb03165bb..361047239 100644 --- a/test/db/mongo/pushStorage.integration.test.ts +++ b/test/db/mongo/pushStorage.integration.test.ts @@ -14,13 +14,209 @@ * limitations under the License. */ -import { describe } from 'vitest'; +import { afterEach, describe, expect, it, vi } from 'vitest'; +import { Collection } from 'mongodb'; import * as pushes from '../../../src/db/mongo/pushes'; import { definePushStorageContract } from '../pushStorage.contract'; -import { connect } from '../../../src/db/mongo/helper'; +import { connect, resetConnection } from '../../../src/db/mongo/helper'; +import { rebuildRepoPushRollups } from '../../../src/db/mongo/repoActivity'; +import { Action, RequestType, Step } from '../../../src/proxy/actions'; describe.runIf(process.env.RUN_MONGO_TESTS === 'true')('MongoDB storage contract', () => { definePushStorageContract('MongoDB push storage', pushes, async (id) => (await connect('pushes')).findOne({ id }), ); + + const key = 'example.com/recovery/repo'; + const makePush = (id = 'recovery') => { + const action = new Action(id, RequestType.PUSH, 'POST', 123, `https://${key}.git`); + action.blocked = true; + const step = new Step('diff'); + step.content = 'preserved payload'; + action.addStep(step); + return action; + }; + + afterEach(() => vi.restoreAllMocks()); + + it('backfills historical records without changing their audit data', async () => { + const action = makePush(); + const collection = await connect('pushes'); + await collection.insertOne(JSON.parse(JSON.stringify(action))); + expect((await pushes.getRepoPushRollupsByCanonicalUrl()).tabCounts.get(key)?.pending).toBe(1); + expect(await pushes.getPush(action.id)).toMatchObject(action); + expect(await collection.findOne({ id: action.id })).toMatchObject({ + steps: action.steps, + lastStep: action.lastStep, + _activity: { dirty: false }, + }); + }); + + it('does not scan push history on unchanged repository reads', async () => { + await pushes.writeAudit(makePush()); + await pushes.getRepoPushRollupsByCanonicalUrl(); + const find = vi.spyOn(Collection.prototype, 'find'); + await pushes.getRepoPushRollupsByCanonicalUrl(); + const queries = find.mock.calls + .filter((_, index) => find.mock.contexts[index].collectionName === 'pushes') + .map(([query]) => query); + expect(queries).toEqual([ + { '_activity.dirty': { $exists: false } }, + { '_activity.dirty': true }, + ]); + const collection = await connect('pushes'); + for (const query of queries) { + const plan = await collection.find(query).explain('executionStats'); + expect(plan.executionStats.totalDocsExamined).toBe(0); + } + }); + + it('keeps a concurrent approval dirty until a newer refresh includes it', async () => { + await pushes.writeAudit(makePush()); + const original = Collection.prototype.updateOne; + let interleaved = false; + vi.spyOn(Collection.prototype, 'updateOne').mockImplementation( + async function (filter, update, options) { + if ( + !interleaved && + this.collectionName === 'repoPushActivity' && + update.$set?.ready === true + ) { + interleaved = true; + await pushes.authorise('recovery'); + } + return original.call(this, filter, update, options); + }, + ); + const rollups = await pushes.getRepoPushRollupsByCanonicalUrl(); + expect(interleaved).toBe(true); + expect(rollups.tabCounts.get(key)?.approved).toBe(1); + expect(rollups.tabCounts.get(key)?.pending).toBe(0); + expect(rollups.latestPendingReviewAtMs.has(key)).toBe(false); + }); + + it('prevents an older refresh from overwriting a newer completed refresh', async () => { + await pushes.writeAudit(makePush()); + const original = Collection.prototype.updateOne; + let interleaved = false; + vi.spyOn(Collection.prototype, 'updateOne').mockImplementation( + async function (filter, update, options) { + if ( + !interleaved && + this.collectionName === 'repoPushActivity' && + update.$set?.ready === true + ) { + interleaved = true; + await pushes.authorise('recovery'); + expect( + (await pushes.getRepoPushRollupsByCanonicalUrl()).tabCounts.get(key)?.approved, + ).toBe(1); + } + return original.call(this, filter, update, options); + }, + ); + expect((await pushes.getRepoPushRollupsByCanonicalUrl()).tabCounts.get(key)?.approved).toBe(1); + const stored = await (await connect('repoPushActivity')).findOne({ key }); + expect(stored).toMatchObject({ ready: true, counts: { approved: 1, pending: 0 } }); + }); + + it('recovers an interrupted refresh after reconnecting', async () => { + await pushes.writeAudit(makePush()); + const original = Collection.prototype.updateOne; + const update = vi + .spyOn(Collection.prototype, 'updateOne') + .mockImplementation(async function (filter, update, options) { + if (this.collectionName === 'repoPushActivity' && update.$set?.ready === true) { + throw new Error('interrupted refresh'); + } + return original.call(this, filter, update, options); + }); + await expect(pushes.getRepoPushRollupsByCanonicalUrl()).rejects.toThrow('interrupted refresh'); + update.mockRestore(); + await resetConnection(); + expect((await pushes.getRepoPushRollupsByCanonicalUrl()).tabCounts.get(key)?.pending).toBe(1); + }); + + it('deletes the audit immediately and recovers its pending counter update after a restart', async () => { + await pushes.writeAudit(makePush()); + await pushes.getRepoPushRollupsByCanonicalUrl(); + await pushes.deletePush('recovery'); + expect(await pushes.getPush('recovery')).toBeNull(); + expect(await pushes.getPushes({})).toEqual([]); + const tombstone = await (await connect('pushes')).findOne({ id: 'recovery' }); + expect(tombstone).toMatchObject({ _activity: { deleted: true, dirty: true } }); + expect(JSON.stringify(tombstone)).not.toContain('preserved payload'); + await resetConnection(); + expect((await pushes.getRepoPushRollupsByCanonicalUrl()).tabCounts.has(key)).toBe(false); + expect(await (await connect('pushes')).findOne({ id: 'recovery' })).toBeNull(); + }); + + it('rebuilds missing derived summaries from the source records', async () => { + await pushes.writeAudit(makePush()); + await pushes.getRepoPushRollupsByCanonicalUrl(); + await (await connect('repoPushActivity')).deleteMany({}); + expect((await rebuildRepoPushRollups()).tabCounts.get(key)?.pending).toBe(1); + }); + + it('counts concurrent retries of one push only once', async () => { + await Promise.all(Array.from({ length: 10 }, () => pushes.writeAudit(makePush()))); + expect(await (await connect('pushes')).countDocuments({ id: 'recovery' })).toBe(1); + expect((await pushes.getRepoPushRollupsByCanonicalUrl()).tabCounts.get(key)?.pending).toBe(1); + }); + + it('falls back to the source records after three refreshes under continuous writes', async () => { + await pushes.writeAudit(makePush()); + const original = Collection.prototype.updateOne; + let refreshes = 0; + const update = vi + .spyOn(Collection.prototype, 'updateOne') + .mockImplementation(async function (filter, update, options) { + if (this.collectionName === 'repoPushActivity' && update.$set?.ready === true) { + refreshes++; + if (refreshes % 2) await pushes.authorise('recovery'); + else await pushes.cancel('recovery'); + } + return original.call(this, filter, update, options); + }); + const rollups = await pushes.getRepoPushRollupsByCanonicalUrl(); + expect(refreshes).toBe(3); + expect(rollups.tabCounts.get(key)?.approved).toBe(1); + update.mockRestore(); + expect((await pushes.getRepoPushRollupsByCanonicalUrl()).tabCounts.get(key)?.approved).toBe(1); + }); + + it('does not purge a push recreated while its deletion is being summarized', async () => { + await pushes.writeAudit(makePush()); + await pushes.getRepoPushRollupsByCanonicalUrl(); + await pushes.deletePush('recovery'); + const original = Collection.prototype.deleteOne; + let recreated = false; + vi.spyOn(Collection.prototype, 'deleteOne').mockImplementation( + async function (filter, options) { + if (!recreated && this.collectionName === 'pushes') { + recreated = true; + await pushes.writeAudit(makePush()); + } + return original.call(this, filter, options); + }, + ); + expect((await pushes.getRepoPushRollupsByCanonicalUrl()).tabCounts.get(key)?.pending).toBe(1); + expect((await pushes.getPush('recovery'))?.steps[0].content).toBe('preserved payload'); + }); + + it('keeps previous counts and audit data when an audit write fails', async () => { + await pushes.writeAudit(makePush()); + await pushes.getRepoPushRollupsByCanonicalUrl(); + const original = Collection.prototype.updateOne; + const update = vi + .spyOn(Collection.prototype, 'updateOne') + .mockImplementation(async function (filter, update, options) { + if (this.collectionName === 'pushes') throw new Error('write failed'); + return original.call(this, filter, update, options); + }); + await expect(pushes.authorise('recovery')).rejects.toThrow('write failed'); + update.mockRestore(); + expect((await pushes.getPush('recovery'))?.authorised).toBe(false); + expect((await pushes.getRepoPushRollupsByCanonicalUrl()).tabCounts.get(key)?.pending).toBe(1); + }); }); diff --git a/test/db/mongo/pushes.test.ts b/test/db/mongo/pushes.test.ts index 33eab5ace..975675d8d 100644 --- a/test/db/mongo/pushes.test.ts +++ b/test/db/mongo/pushes.test.ts @@ -20,11 +20,14 @@ const mockFindOneDocument = vi.fn(); const mockUpdateOne = vi.fn(); const mockConnect = vi.fn(() => ({ updateOne: mockUpdateOne, + createIndex: vi.fn().mockResolvedValue('index'), })); +const mockDatabase = {}; vi.mock('../../../src/db/mongo/helper', () => ({ connect: mockConnect, findOneDocument: mockFindOneDocument, + getDb: () => mockDatabase, })); describe('MongoDB - Pushes', async () => { @@ -63,7 +66,10 @@ describe('MongoDB - Pushes', async () => { const result = await reject(pushId, rejection); expect(result).toEqual({ message: `reject ${pushId}` }); - expect(mockFindOneDocument).toHaveBeenCalledWith('pushes', { id: pushId }); + expect(mockFindOneDocument).toHaveBeenCalledWith('pushes', { + id: pushId, + '_activity.deleted': { $ne: true }, + }); expect(mockConnect).toHaveBeenCalledWith('pushes'); const [query, update, options] = mockUpdateOne.mock.calls[0]; @@ -97,7 +103,10 @@ describe('MongoDB - Pushes', async () => { }; await expect(reject(pushId, rejection)).rejects.toThrow(`push ${pushId} not found`); - expect(mockFindOneDocument).toHaveBeenCalledWith('pushes', { id: pushId }); + expect(mockFindOneDocument).toHaveBeenCalledWith('pushes', { + id: pushId, + '_activity.deleted': { $ne: true }, + }); }); }); }); diff --git a/test/setup-integration.ts b/test/setup-integration.ts index c220668b5..e6ac415cf 100644 --- a/test/setup-integration.ts +++ b/test/setup-integration.ts @@ -20,7 +20,7 @@ import { resetConnection } from '../src/db/mongo/helper'; import { invalidateCache } from '../src/config'; const DEFAULT_TEST_DB_NAME = 'git-proxy-test'; -const COLLECTIONS = ['repos', 'users', 'pushes', 'user_session', 'migrations']; +const COLLECTIONS = ['repos', 'users', 'pushes', 'repoPushActivity', 'user_session', 'migrations']; let client: MongoClient | null = null; From 6f7c8c83317dc2ccc7ec23603317e0b4335d90d6 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Denis=20=C4=86ori=C4=87?= Date: Sun, 4 Oct 2026 16:16:42 +0200 Subject: [PATCH 08/18] test(db): cover rollup recovery and report integration coverage --- .github/workflows/ci.yml | 4 +-- test/db/file/repoActivity.test.ts | 56 +++++++++++++++++++++++++++++++ 2 files changed, 58 insertions(+), 2 deletions(-) create mode 100644 test/db/file/repoActivity.test.ts diff --git a/.github/workflows/ci.yml b/.github/workflows/ci.yml index 116a584bd..db635183a 100644 --- a/.github/workflows/ci.yml +++ b/.github/workflows/ci.yml @@ -71,12 +71,12 @@ jobs: env: RUN_MONGO_TESTS: 'true' GIT_PROXY_MONGO_CONNECTION_STRING: mongodb://localhost:27017/git-proxy-test - run: npm run test:integration + run: npm run test:integration -- --coverage --coverage.reporter=lcovonly --coverage.reportsDirectory=coverage/integration - name: Upload test coverage report uses: codecov/codecov-action@303a32d7a59b442fa8d48b6a1cc6825c09c847a5 # v7.1.1 with: - files: ./coverage/lcov.info,./coverage-cli/lcov.info + files: ./coverage/lcov.info,./coverage/integration/lcov.info,./coverage-cli/lcov.info token: ${{ secrets.CODECOV_TOKEN }} - name: Build frontend diff --git a/test/db/file/repoActivity.test.ts b/test/db/file/repoActivity.test.ts new file mode 100644 index 000000000..ac0af6eda --- /dev/null +++ b/test/db/file/repoActivity.test.ts @@ -0,0 +1,56 @@ +/** + * Copyright 2026 GitProxy Contributors + * + * Licensed under the Apache License, Version 2.0 (the "License"); + * you may not use this file except in compliance with the License. + * You may obtain a copy of the License at + * + * http://www.apache.org/licenses/LICENSE-2.0 + * + * Unless required by applicable law or agreed to in writing, software + * distributed under the License is distributed on an "AS IS" BASIS, + * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. + * See the License for the specific language governing permissions and + * limitations under the License. + */ + +import { afterEach, describe, expect, it, vi } from 'vitest'; +import { Action, RequestType } from '../../../src/proxy/actions'; + +describe('file repository rollup initialization', () => { + afterEach(() => vi.restoreAllMocks()); + + it('loads historical rows once and applies writes after initialization', async () => { + vi.resetModules(); + const sink = await import('../../../src/db/file/pushes'); + const action = new Action('history', RequestType.PUSH, 'POST', 123, 'https://example.com/a/b'); + action.blocked = true; + await sink.db.insertAsync(action); + expect( + (await sink.getRepoPushRollupsByCanonicalUrl()).tabCounts.get('example.com/a/b')?.pending, + ).toBe(1); + await sink.authorise(action.id); + const find = vi.spyOn(sink.db, 'find'); + const rollups = await sink.getRepoPushRollupsByCanonicalUrl(); + expect(rollups.tabCounts.get('example.com/a/b')?.approved).toBe(1); + expect(rollups.latestPendingReviewAtMs.has('example.com/a/b')).toBe(false); + expect(find).not.toHaveBeenCalled(); + }); + + it('does not change a warm index when a database write fails', async () => { + vi.resetModules(); + const sink = await import('../../../src/db/file/pushes'); + const action = new Action('failure', RequestType.PUSH, 'POST', 123, 'https://example.com/a/b'); + await sink.writeAudit(action); + await sink.getRepoPushRollupsByCanonicalUrl(); + const update = vi.spyOn(sink.db, 'update').mockImplementationOnce(() => { + throw new Error('write failed'); + }); + await expect(sink.authorise(action.id)).rejects.toThrow('write failed'); + update.mockRestore(); + expect( + (await sink.getRepoPushRollupsByCanonicalUrl()).tabCounts.get('example.com/a/b')?.pending, + ).toBe(1); + expect((await sink.getPush(action.id))?.authorised).toBe(false); + }); +}); From 425d44a70b58bf462df462596d9cbfcb22f390d8 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Denis=20=C4=86ori=C4=87?= Date: Sun, 4 Oct 2026 16:17:19 +0200 Subject: [PATCH 09/18] docs: describe push storage and activity rollup recovery --- website/docs/architecture/processors.md | 4 +++- website/docs/deployment.mdx | 11 +++++++++++ 2 files changed, 14 insertions(+), 1 deletion(-) diff --git a/website/docs/architecture/processors.md b/website/docs/architecture/processors.md index 6b7032be4..fef219f4a 100644 --- a/website/docs/architecture/processors.md +++ b/website/docs/architecture/processors.md @@ -244,7 +244,9 @@ Source: [/src/proxy/processors/push-action/blockForAuth.ts](https://github.com/f This action runs after a chain has been executed. It stores in the database the entire `Action` object along with the list of `steps` that the action has gone through and their associated logs or error messages that occurred during processing of the chain. -Push list and profile activity responses return metadata without processor steps, `lastStep`, or diff content. Use `GET /api/v1/push/:id` to retrieve the full audit record and diff for review. Existing stored records remain readable and are not rewritten or truncated. +Push list and profile activity responses return metadata without processor steps, `lastStep`, or diff content. Use `GET /api/v1/push/:id` to retrieve the full audit record and diff for review. When `lastStep` exactly matches the final entry in `steps`, the database stores a reference and restores `lastStep` on detail reads. A distinct `lastStep` is preserved. Historical diff content and logs remain readable and are not truncated. + +Repository activity counts use derived summaries. NeDB builds an in-memory index on the first repository read and updates it after successful push writes and deletions. MongoDB records activity metadata alongside each push and refreshes summaries for changed repositories on the next repository read. Unchanged repository reads do not scan push history. The first read after an upgrade backfills historical activity metadata; it does not rewrite historical audit content. Under continuous concurrent changes, MongoDB bounds refresh retries and falls back to a metadata-only scan for that request. Note: **`audit` writes all actions** (push, pull, default/unclassified) to the DB. diff --git a/website/docs/deployment.mdx b/website/docs/deployment.mdx index c257c6803..de050a795 100644 --- a/website/docs/deployment.mdx +++ b/website/docs/deployment.mdx @@ -286,6 +286,17 @@ Switch from the default file-based database to MongoDB for production use: - Configure replica sets for high availability - Schedule regular backups of audit data (`mongodump`) +Repository activity summaries are derived from the push audit records. GitProxy creates the required indexes, including a unique push ID index, and backfills activity metadata on the first repository read after upgrade. Use the same GitProxy version across instances during this upgrade: older writers do not update the activity revisions. Standalone MongoDB is supported; summary updates do not require transactions or a replica set. + +Interrupted summary refreshes recover on the next repository read. Deleting a push removes its audit payload immediately; a small internal deletion marker remains until its repository summary is refreshed. If restoring only the `pushes` collection or repairing summaries manually, rebuild the derived data from a source checkout with the deployment's configuration: + +```bash +CONFIG_FILE=/path/to/proxy.config.json npx tsx -e \ + "import { rebuildRepoPushRollups } from './src/db/mongo/repoActivity'; import { resetConnection } from './src/db/mongo/helper'; rebuildRepoPushRollups().finally(resetConnection)" +``` + +For NeDB, restarting GitProxy rebuilds the in-memory summaries from the saved push records. + ### Health Checks GitProxy provides health check endpoints on both servers: From 25a27e1c936fd41697e700ab40d68a05c7c2622f Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Denis=20=C4=86ori=C4=87?= Date: Sun, 4 Oct 2026 16:27:38 +0200 Subject: [PATCH 10/18] fix(ui): refresh activity after review decisions --- cypress/e2e/docker/pushActions.cy.js | 7 ++-- src/ui/views/PushDetails/PushDetails.tsx | 43 ++++++++++++------------ 2 files changed, 26 insertions(+), 24 deletions(-) diff --git a/cypress/e2e/docker/pushActions.cy.js b/cypress/e2e/docker/pushActions.cy.js index 02faf46f8..64590ebb2 100644 --- a/cypress/e2e/docker/pushActions.cy.js +++ b/cypress/e2e/docker/pushActions.cy.js @@ -62,7 +62,8 @@ describe('Push Actions (Approve, Reject, Cancel)', () => { it('should approve a pending push via attestation dialog', function () { cy.login(approverUser.username, approverUser.password); - cy.visit(`/dashboard/push/${this.pushId}`); + cy.visit('/dashboard/push'); + cy.get(`tr[aria-label="Open activity ${this.pushId}"]`).click(); // Verify push is Pending cy.get('[data-testid="push-status"]').should('contain', 'Pending'); @@ -95,9 +96,11 @@ describe('Push Actions (Approve, Reject, Cancel)', () => { // Should navigate back to push list cy.url().should('include', '/dashboard/push'); cy.url().should('not.include', this.pushId); + cy.get(`tr[aria-label="Open activity ${this.pushId}"]`).should('not.exist'); + cy.contains('a', 'Approved').click(); + cy.get(`tr[aria-label="Open activity ${this.pushId}"]`).click(); // Verify push is now Approved by revisiting its detail page - cy.visit(`/dashboard/push/${this.pushId}`); cy.get('[data-testid="push-status"]').should('contain', 'Approved'); // Action buttons should no longer be visible for an approved push diff --git a/src/ui/views/PushDetails/PushDetails.tsx b/src/ui/views/PushDetails/PushDetails.tsx index f54ed0dee..1e49d47e0 100644 --- a/src/ui/views/PushDetails/PushDetails.tsx +++ b/src/ui/views/PushDetails/PushDetails.tsx @@ -42,7 +42,10 @@ import { sortRepoViews } from '../../services/repo'; import { useRepoViewsListQuery } from '../../query/useRepoViewsListQuery'; import { usePushQuery } from '../../query/usePushQuery'; import { usePushPermissionsQuery } from '../../query/usePushPermissionsQuery'; -import { useMutation } from '@tanstack/react-query'; +import { useMutation, useQueryClient } from '@tanstack/react-query'; +import { pushQueryKeys } from '../../query/pushQueryKeys'; +import { repoQueryKeys } from '../../query/repoQueryKeys'; +import { userQueryKeys } from '../../query/userQueryKeys'; import { buildRepoDisplayIndex, resolveActivityRepoDisplay, @@ -130,6 +133,7 @@ function CanceledPushBanner({ push }: { push: PushActionView }): React.ReactElem const PushDetails = () => { const { id } = useParams<{ id: string }>(); const navigate = useNavigate(); + const queryClient = useQueryClient(); const [message, setMessage] = useState(''); const { data: repoListRaw } = useRepoViewsListQuery(true); const registeredRepos = useMemo( @@ -170,40 +174,35 @@ const PushDetails = () => { setMessage(result.message || 'Something went wrong...'); }; + const handleActionSuccess = async (result: ServiceResult) => { + if (!result.success) { + handleActionFailure(result); + return; + } + await Promise.all([ + queryClient.invalidateQueries({ queryKey: pushQueryKeys.all }), + queryClient.invalidateQueries({ queryKey: repoQueryKeys.list() }), + queryClient.invalidateQueries({ queryKey: userQueryKeys.all }), + ]); + navigate('/dashboard/push/'); + }; + const authoriseMutation = useMutation({ mutationFn: (attestationData: Array<{ label: string; checked: boolean }>) => authorisePush(id!, attestationData), - onSuccess: (result) => { - if (result.success) { - navigate('/dashboard/push/'); - } else { - handleActionFailure(result); - } - }, + onSuccess: handleActionSuccess, onError: () => setMessage('Something went wrong...'), }); const rejectMutation = useMutation({ mutationFn: (reason: string) => rejectPush(id!, reason), - onSuccess: (result) => { - if (result.success) { - navigate('/dashboard/push/'); - } else { - handleActionFailure(result); - } - }, + onSuccess: handleActionSuccess, onError: () => setMessage('Something went wrong...'), }); const cancelMutation = useMutation({ mutationFn: () => cancelPush(id!), - onSuccess: (result) => { - if (result.success) { - navigate('/dashboard/push/'); - } else { - handleActionFailure(result); - } - }, + onSuccess: handleActionSuccess, onError: () => setMessage('Something went wrong...'), }); From c63f0e97b5f043176d684b47ccd915b5df499d7c Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Denis=20=C4=86ori=C4=87?= Date: Sun, 4 Oct 2026 16:32:57 +0200 Subject: [PATCH 11/18] fix(ui): parse serialized review timestamps --- cypress/e2e/autoApproved.cy.js | 1 + cypress/e2e/docker/pushActions.cy.js | 16 ++++++++++++---- .../PushDetails/components/AttestationInfo.tsx | 5 ++++- .../PushDetails/components/RejectionInfo.tsx | 5 ++++- 4 files changed, 21 insertions(+), 6 deletions(-) diff --git a/cypress/e2e/autoApproved.cy.js b/cypress/e2e/autoApproved.cy.js index 628e09971..263bd3ef5 100644 --- a/cypress/e2e/autoApproved.cy.js +++ b/cypress/e2e/autoApproved.cy.js @@ -69,6 +69,7 @@ describe('Auto-Approved Push Test', () => { cy.wait('@getPush'); cy.contains('Auto-approved by system').should('be.visible'); + cy.contains('Invalid DateTime').should('not.exist'); cy.contains('approved this contribution').should('not.exist'); }); diff --git a/cypress/e2e/docker/pushActions.cy.js b/cypress/e2e/docker/pushActions.cy.js index 64590ebb2..9aa38e728 100644 --- a/cypress/e2e/docker/pushActions.cy.js +++ b/cypress/e2e/docker/pushActions.cy.js @@ -102,6 +102,7 @@ describe('Push Actions (Approve, Reject, Cancel)', () => { // Verify push is now Approved by revisiting its detail page cy.get('[data-testid="push-status"]').should('contain', 'Approved'); + cy.contains('Invalid DateTime').should('not.exist'); // Action buttons should no longer be visible for an approved push cy.get('[data-testid="push-cancel-btn"]').should('not.exist'); @@ -118,7 +119,8 @@ describe('Push Actions (Approve, Reject, Cancel)', () => { it('should reject a pending push', function () { cy.login(approverUser.username, approverUser.password); - cy.visit(`/dashboard/push/${this.pushId}`); + cy.visit('/dashboard/push'); + cy.get(`tr[aria-label="Open activity ${this.pushId}"]`).click(); // Verify push is Pending cy.get('[data-testid="push-status"]').should('contain', 'Pending'); @@ -143,8 +145,11 @@ describe('Push Actions (Approve, Reject, Cancel)', () => { cy.url().should('not.include', this.pushId); // Verify push is now Rejected - cy.visit(`/dashboard/push/${this.pushId}`); + cy.get(`tr[aria-label="Open activity ${this.pushId}"]`).should('not.exist'); + cy.contains('a', 'Rejected').click(); + cy.get(`tr[aria-label="Open activity ${this.pushId}"]`).click(); cy.get('[data-testid="push-status"]').should('contain', 'Rejected'); + cy.contains('Invalid DateTime').should('not.exist'); // Action buttons should no longer be visible cy.get('[data-testid="push-cancel-btn"]').should('not.exist'); @@ -162,7 +167,8 @@ describe('Push Actions (Approve, Reject, Cancel)', () => { it('should cancel a pending push', function () { // Cancel can be done by the push author cy.login(testUser.username, testUser.password); - cy.visit(`/dashboard/push/${this.pushId}`); + cy.visit('/dashboard/push'); + cy.get(`tr[aria-label="Open activity ${this.pushId}"]`).click(); // Verify push is Pending cy.get('[data-testid="push-status"]').should('contain', 'Pending'); @@ -174,7 +180,9 @@ describe('Push Actions (Approve, Reject, Cancel)', () => { cy.url().should('include', '/dashboard/push'); // Verify push is now Canceled - cy.visit(`/dashboard/push/${this.pushId}`); + cy.get(`tr[aria-label="Open activity ${this.pushId}"]`).should('not.exist'); + cy.contains('a', 'Canceled').click(); + cy.get(`tr[aria-label="Open activity ${this.pushId}"]`).click(); cy.get('[data-testid="push-status"]').should('contain', 'Canceled'); // Action buttons should no longer be visible diff --git a/src/ui/views/PushDetails/components/AttestationInfo.tsx b/src/ui/views/PushDetails/components/AttestationInfo.tsx index 9f2a0d0e1..98a32fd89 100644 --- a/src/ui/views/PushDetails/components/AttestationInfo.tsx +++ b/src/ui/views/PushDetails/components/AttestationInfo.tsx @@ -30,7 +30,10 @@ const AttestationInfo = ({ push }: AttestationInfoProps) => { } const att = push.attestation; - const ts = DateTime.fromMillis(Number(att.timestamp)); + const numericTimestamp = Number(att.timestamp); + const ts = Number.isFinite(numericTimestamp) + ? DateTime.fromMillis(numericTimestamp) + : DateTime.fromISO(String(att.timestamp)); const tsTitle = ts.toFormat('cccc, MMMM d yyyy, h:mm:ss a'); const presetDisplayName = att.reviewer.displayName?.trim() || undefined; diff --git a/src/ui/views/PushDetails/components/RejectionInfo.tsx b/src/ui/views/PushDetails/components/RejectionInfo.tsx index 3f78b7cc9..01bc80d2c 100644 --- a/src/ui/views/PushDetails/components/RejectionInfo.tsx +++ b/src/ui/views/PushDetails/components/RejectionInfo.tsx @@ -30,7 +30,10 @@ const RejectionInfo = ({ push }: RejectionInfoProps) => { } const rej = push.rejection; - const ts = DateTime.fromMillis(Number(rej.timestamp)); + const numericTimestamp = Number(rej.timestamp); + const ts = Number.isFinite(numericTimestamp) + ? DateTime.fromMillis(numericTimestamp) + : DateTime.fromISO(String(rej.timestamp)); const tsTitle = ts.toFormat('cccc, MMMM d yyyy, h:mm:ss a'); return ( From 044a56116b251403c36252afcd3cb664ecafbfaa Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Denis=20=C4=86ori=C4=87?= Date: Sun, 4 Oct 2026 18:15:08 +0200 Subject: [PATCH 12/18] test(db): cover file activity initialization recovery MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Signed-off-by: Denis Ćorić --- test/db/file/repoActivity.test.ts | 27 +++++++++++++++++++++++++++ 1 file changed, 27 insertions(+) diff --git a/test/db/file/repoActivity.test.ts b/test/db/file/repoActivity.test.ts index ac0af6eda..b48f62bee 100644 --- a/test/db/file/repoActivity.test.ts +++ b/test/db/file/repoActivity.test.ts @@ -20,6 +20,33 @@ import { Action, RequestType } from '../../../src/proxy/actions'; describe('file repository rollup initialization', () => { afterEach(() => vi.restoreAllMocks()); + it('retries a failed history load and shares the successful index between readers', async () => { + vi.resetModules(); + const sink = await import('../../../src/db/file/pushes'); + const action = new Action('history', RequestType.PUSH, 'POST', 123, 'https://example.com/a/b'); + await sink.db.insertAsync(action); + const failure = new Error('history read failed'); + const original = sink.db.find.bind(sink.db); + const find = vi + .spyOn(sink.db, 'find') + .mockImplementationOnce( + (query: unknown, projection: unknown, callback?: (error: Error, rows: never[]) => void) => { + queueMicrotask(() => callback?.(failure, [])); + return original(query, projection); + }, + ); + await expect(sink.getRepoPushRollupsByCanonicalUrl()).rejects.toThrow(failure); + const readers = await Promise.all([ + sink.getRepoPushRollupsByCanonicalUrl(), + sink.getRepoPushRollupsByCanonicalUrl(), + ]); + for (const rollups of readers) { + expect(rollups.tabCounts.get('example.com/a/b')?.pending).toBe(1); + expect(rollups.latestPushAtMs.get('example.com/a/b')).toBe(123); + } + expect(find).toHaveBeenCalledTimes(2); + }); + it('loads historical rows once and applies writes after initialization', async () => { vi.resetModules(); const sink = await import('../../../src/db/file/pushes'); From 773cc10548494fc3eeb2b80b9e419645ee92e13c Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Denis=20=C4=86ori=C4=87?= Date: Sun, 4 Oct 2026 18:15:24 +0200 Subject: [PATCH 13/18] test(db): cover MongoDB activity recovery and deletion races MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Signed-off-by: Denis Ćorić --- test/db/mongo/pushStorage.integration.test.ts | 104 ++++++++++++++++++ 1 file changed, 104 insertions(+) diff --git a/test/db/mongo/pushStorage.integration.test.ts b/test/db/mongo/pushStorage.integration.test.ts index 361047239..dacae91fc 100644 --- a/test/db/mongo/pushStorage.integration.test.ts +++ b/test/db/mongo/pushStorage.integration.test.ts @@ -19,6 +19,7 @@ import { Collection } from 'mongodb'; import * as pushes from '../../../src/db/mongo/pushes'; import { definePushStorageContract } from '../pushStorage.contract'; import { connect, resetConnection } from '../../../src/db/mongo/helper'; +import * as mongoHelper from '../../../src/db/mongo/helper'; import { rebuildRepoPushRollups } from '../../../src/db/mongo/repoActivity'; import { Action, RequestType, Step } from '../../../src/proxy/actions'; @@ -39,6 +40,109 @@ describe.runIf(process.env.RUN_MONGO_TESTS === 'true')('MongoDB storage contract afterEach(() => vi.restoreAllMocks()); + it('retries initialization after index creation fails', async () => { + const createIndex = vi + .spyOn(Collection.prototype, 'createIndex') + .mockRejectedValueOnce(new Error('index creation failed')); + await expect(pushes.writeAudit(makePush())).rejects.toThrow('index creation failed'); + expect(await (await connect('pushes')).findOne({ id: 'recovery' })).toBeNull(); + await pushes.writeAudit(makePush()); + expect(createIndex).toHaveBeenCalledTimes(8); + expect((await pushes.getRepoPushRollupsByCanonicalUrl()).tabCounts.get(key)?.pending).toBe(1); + }); + + it('rejects initialization if the connection disappears and succeeds on retry', async () => { + vi.spyOn(mongoHelper, 'getDb').mockReturnValueOnce(null); + await expect(pushes.writeAudit(makePush())).rejects.toThrow( + 'MongoDB connection is not available', + ); + await pushes.writeAudit(makePush()); + expect((await pushes.getRepoPushRollupsByCanonicalUrl()).tabCounts.get(key)?.pending).toBe(1); + }); + + it('deletes a historical push before backfill and tolerates repeated deletion', async () => { + await (await connect('pushes')).insertOne(JSON.parse(JSON.stringify(makePush()))); + await pushes.deletePush('recovery'); + const replace = vi.spyOn(Collection.prototype, 'replaceOne'); + await pushes.deletePush('recovery'); + expect(replace).not.toHaveBeenCalled(); + expect(await pushes.getPush('recovery')).toBeNull(); + expect((await pushes.getRepoPushRollupsByCanonicalUrl()).tabCounts.size).toBe(0); + expect(await (await connect('pushes')).findOne({ id: 'recovery' })).toBeNull(); + }); + + it.each([1, 3])('handles %i concurrent updates during deletion', async (conflicts) => { + await pushes.writeAudit(makePush()); + const original = Collection.prototype.replaceOne; + let attempts = 0; + const replace = vi + .spyOn(Collection.prototype, 'replaceOne') + .mockImplementation(async function (filter, replacement, options) { + if (this.collectionName === 'pushes' && attempts++ < conflicts) { + await pushes.authorise('recovery'); + } + return original.call(this, filter, replacement, options); + }); + if (conflicts === 1) { + await pushes.deletePush('recovery'); + expect(attempts).toBe(2); + expect(await pushes.getPush('recovery')).toBeNull(); + } else { + await expect(pushes.deletePush('recovery')).rejects.toThrow( + 'Push recovery changed repeatedly during deletion; retry the request', + ); + expect(attempts).toBe(3); + expect((await pushes.getPush('recovery'))?.authorised).toBe(true); + expect((await pushes.getRepoPushRollupsByCanonicalUrl()).tabCounts.get(key)?.approved).toBe( + 1, + ); + replace.mockRestore(); + await pushes.deletePush('recovery'); + } + expect((await pushes.getRepoPushRollupsByCanonicalUrl()).tabCounts.size).toBe(0); + }); + + it('backfills non-push records without adding repository activity or leaving them dirty', async () => { + const action = new Action('pull', RequestType.PULL, 'GET', 123, `https://${key}`); + const collection = await connect('pushes'); + await collection.insertOne(JSON.parse(JSON.stringify(action))); + expect((await pushes.getRepoPushRollupsByCanonicalUrl()).tabCounts.size).toBe(0); + expect(await collection.findOne({ id: action.id })).toMatchObject({ + _activity: { key: '', dirty: false }, + }); + expect(await pushes.getPush(action.id)).toMatchObject(action); + }); + + it('retries when a summary becomes unfinished during an otherwise clean read', async () => { + await pushes.writeAudit(makePush()); + await pushes.getRepoPushRollupsByCanonicalUrl(); + const summaries = await connect('repoPushActivity'); + const original = Collection.prototype.find; + let interleaved = false; + vi.spyOn(Collection.prototype, 'find').mockImplementation(function (filter, options) { + const cursor = original.call(this, filter, options); + if ( + !interleaved && + this.collectionName === 'repoPushActivity' && + !Object.keys(filter).length + ) { + interleaved = true; + const toArray = cursor.toArray.bind(cursor); + vi.spyOn(cursor, 'toArray').mockImplementationOnce(async () => { + await pushes.authorise('recovery'); + await summaries.updateOne({ key }, { $set: { ready: false } }); + return toArray(); + }); + } + return cursor; + }); + const rollups = await pushes.getRepoPushRollupsByCanonicalUrl(); + expect(interleaved).toBe(true); + expect(rollups.tabCounts.get(key)?.approved).toBe(1); + expect(rollups.tabCounts.get(key)?.pending).toBe(0); + expect(await summaries.findOne({ key })).toMatchObject({ ready: true }); + }); + it('backfills historical records without changing their audit data', async () => { const action = makePush(); const collection = await connect('pushes'); From cf021fe14d642a13320615f8c35949f6e649342d Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Denis=20=C4=86ori=C4=87?= Date: Tue, 6 Oct 2026 12:09:36 +0200 Subject: [PATCH 14/18] refactor: separate repository activity from audit storage MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Signed-off-by: Denis Ćorić --- cypress/e2e/autoApproved.cy.js | 1 - cypress/e2e/docker/pushActions.cy.js | 23 +- src/db/file/pushes.ts | 129 ++++++-- src/db/mongo/helper.ts | 6 +- src/db/mongo/pushes.ts | 151 ++++++--- src/db/mongo/repoActivity.ts | 204 ------------ src/db/repoActivity.ts | 162 ---------- src/ui/views/PushDetails/PushDetails.tsx | 43 +-- .../components/AttestationInfo.tsx | 5 +- .../PushDetails/components/RejectionInfo.tsx | 5 +- test/db/file/pushStorage.test.ts | 13 - test/db/file/repoActivity.test.ts | 83 ----- test/db/mongo/push.test.ts | 49 +-- test/db/mongo/pushStorage.integration.test.ts | 304 +----------------- test/db/mongo/pushes.test.ts | 13 +- test/db/repoActivity.test.ts | 84 ----- test/setup-integration.ts | 2 +- website/docs/architecture/processors.md | 2 - website/docs/deployment.mdx | 11 - 19 files changed, 248 insertions(+), 1042 deletions(-) delete mode 100644 src/db/mongo/repoActivity.ts delete mode 100644 src/db/repoActivity.ts delete mode 100644 test/db/file/repoActivity.test.ts delete mode 100644 test/db/repoActivity.test.ts diff --git a/cypress/e2e/autoApproved.cy.js b/cypress/e2e/autoApproved.cy.js index 263bd3ef5..628e09971 100644 --- a/cypress/e2e/autoApproved.cy.js +++ b/cypress/e2e/autoApproved.cy.js @@ -69,7 +69,6 @@ describe('Auto-Approved Push Test', () => { cy.wait('@getPush'); cy.contains('Auto-approved by system').should('be.visible'); - cy.contains('Invalid DateTime').should('not.exist'); cy.contains('approved this contribution').should('not.exist'); }); diff --git a/cypress/e2e/docker/pushActions.cy.js b/cypress/e2e/docker/pushActions.cy.js index 9aa38e728..02faf46f8 100644 --- a/cypress/e2e/docker/pushActions.cy.js +++ b/cypress/e2e/docker/pushActions.cy.js @@ -62,8 +62,7 @@ describe('Push Actions (Approve, Reject, Cancel)', () => { it('should approve a pending push via attestation dialog', function () { cy.login(approverUser.username, approverUser.password); - cy.visit('/dashboard/push'); - cy.get(`tr[aria-label="Open activity ${this.pushId}"]`).click(); + cy.visit(`/dashboard/push/${this.pushId}`); // Verify push is Pending cy.get('[data-testid="push-status"]').should('contain', 'Pending'); @@ -96,13 +95,10 @@ describe('Push Actions (Approve, Reject, Cancel)', () => { // Should navigate back to push list cy.url().should('include', '/dashboard/push'); cy.url().should('not.include', this.pushId); - cy.get(`tr[aria-label="Open activity ${this.pushId}"]`).should('not.exist'); - cy.contains('a', 'Approved').click(); - cy.get(`tr[aria-label="Open activity ${this.pushId}"]`).click(); // Verify push is now Approved by revisiting its detail page + cy.visit(`/dashboard/push/${this.pushId}`); cy.get('[data-testid="push-status"]').should('contain', 'Approved'); - cy.contains('Invalid DateTime').should('not.exist'); // Action buttons should no longer be visible for an approved push cy.get('[data-testid="push-cancel-btn"]').should('not.exist'); @@ -119,8 +115,7 @@ describe('Push Actions (Approve, Reject, Cancel)', () => { it('should reject a pending push', function () { cy.login(approverUser.username, approverUser.password); - cy.visit('/dashboard/push'); - cy.get(`tr[aria-label="Open activity ${this.pushId}"]`).click(); + cy.visit(`/dashboard/push/${this.pushId}`); // Verify push is Pending cy.get('[data-testid="push-status"]').should('contain', 'Pending'); @@ -145,11 +140,8 @@ describe('Push Actions (Approve, Reject, Cancel)', () => { cy.url().should('not.include', this.pushId); // Verify push is now Rejected - cy.get(`tr[aria-label="Open activity ${this.pushId}"]`).should('not.exist'); - cy.contains('a', 'Rejected').click(); - cy.get(`tr[aria-label="Open activity ${this.pushId}"]`).click(); + cy.visit(`/dashboard/push/${this.pushId}`); cy.get('[data-testid="push-status"]').should('contain', 'Rejected'); - cy.contains('Invalid DateTime').should('not.exist'); // Action buttons should no longer be visible cy.get('[data-testid="push-cancel-btn"]').should('not.exist'); @@ -167,8 +159,7 @@ describe('Push Actions (Approve, Reject, Cancel)', () => { it('should cancel a pending push', function () { // Cancel can be done by the push author cy.login(testUser.username, testUser.password); - cy.visit('/dashboard/push'); - cy.get(`tr[aria-label="Open activity ${this.pushId}"]`).click(); + cy.visit(`/dashboard/push/${this.pushId}`); // Verify push is Pending cy.get('[data-testid="push-status"]').should('contain', 'Pending'); @@ -180,9 +171,7 @@ describe('Push Actions (Approve, Reject, Cancel)', () => { cy.url().should('include', '/dashboard/push'); // Verify push is now Canceled - cy.get(`tr[aria-label="Open activity ${this.pushId}"]`).should('not.exist'); - cy.contains('a', 'Canceled').click(); - cy.get(`tr[aria-label="Open activity ${this.pushId}"]`).click(); + cy.visit(`/dashboard/push/${this.pushId}`); cy.get('[data-testid="push-status"]').should('contain', 'Canceled'); // Action buttons should no longer be visible diff --git a/src/db/file/pushes.ts b/src/db/file/pushes.ts index d63d196bd..e2b4217ce 100644 --- a/src/db/file/pushes.ts +++ b/src/db/file/pushes.ts @@ -16,17 +16,18 @@ import _ from 'lodash'; import Datastore from '@seald-io/nedb'; +import { activityPrimaryStatusFromFlags } from '../../activity/activityPrimaryStatus'; +import { canonicalRemoteUrl } from '../../activity/canonicalRemoteUrl'; import { Action } from '../../proxy/actions/Action'; import { toClass } from '../helper'; import { pushListProjection } from '../pushProjection'; import { compactPush, restorePush } from '../pushStorage'; -import { PushQuery, RepoPushRollupsByCanonicalUrl } from '../types'; import { - RepoActivityIndex, - pushActivity, - pushActivityProjection, - type PushActivityRow, -} from '../repoActivity'; + PushQuery, + RepoActivityTabCounts, + RepoPushRollupsByCanonicalUrl, + emptyRepoActivityTabCounts, +} from '../types'; import { CompletedAttestation, Rejection } from '../../proxy/processors/types'; import { handleErrorAndLog } from '../../utils/errors'; import { buildUserProfilePushFilter } from '../userProfilePushQuery'; @@ -50,29 +51,98 @@ try { } db.setAutocompactionInterval(COMPACTION_INTERVAL); -let activityIndex: RepoActivityIndex | undefined; -let activityIndexLoad: Promise | undefined; - -export const getRepoPushRollupsByCanonicalUrl = - async (): Promise => { - if (!activityIndexLoad) { - activityIndexLoad = new Promise((resolve, reject) => { - db.find({}, pushActivityProjection, (err, docs: PushActivityRow[]) => { - if (err) { - reject(err); - return; +function bumpCount( + m: Map, + canonicalKey: string, + tab: keyof RepoActivityTabCounts, +): void { + if (!canonicalKey) { + return; + } + let row = m.get(canonicalKey); + if (!row) { + row = emptyRepoActivityTabCounts(); + m.set(canonicalKey, row); + } + row[tab] += 1; +} + +function bumpMaxTimestampMs( + m: Map, + canonicalKey: string, + timestamp: unknown, +): void { + if (!canonicalKey) { + return; + } + const ts = typeof timestamp === 'number' ? timestamp : NaN; + if (!Number.isFinite(ts)) { + return; + } + const prev = m.get(canonicalKey); + if (prev === undefined || ts > prev) { + m.set(canonicalKey, ts); + } +} + +type PushActivityProjection = { + url?: string; + error?: boolean; + rejected?: boolean; + canceled?: boolean; + authorised?: boolean; + blocked?: boolean; + allowPush?: boolean; + timestamp?: number; +}; + +export const getRepoPushRollupsByCanonicalUrl = (): Promise => { + return new Promise((resolve, reject) => { + db.find( + { type: 'push' }, + { + url: 1, + error: 1, + rejected: 1, + canceled: 1, + authorised: 1, + blocked: 1, + allowPush: 1, + timestamp: 1, + }, + (err, docs: PushActivityProjection[]) => { + if (err) { + reject(err); + return; + } + const tabCounts = new Map(); + const latestPendingReviewAtMs = new Map(); + const latestPushAtMs = new Map(); + for (const doc of docs) { + const url = typeof doc.url === 'string' ? doc.url : ''; + const key = canonicalRemoteUrl(url); + if (!key) { + continue; } - activityIndex = new RepoActivityIndex(); - for (const doc of docs) activityIndex.set(doc.id, pushActivity(doc)); - resolve(activityIndex); - }); - }).catch((error: unknown) => { - activityIndexLoad = undefined; - throw error; - }); - } - return (await activityIndexLoad).snapshot(); - }; + const tab = activityPrimaryStatusFromFlags({ + error: doc.error === true, + rejected: doc.rejected === true, + canceled: doc.canceled === true, + authorised: doc.authorised === true, + blocked: doc.blocked === true, + allowPush: doc.allowPush === true, + }); + bumpCount(tabCounts, key, tab); + bumpMaxTimestampMs(latestPushAtMs, key, doc.timestamp); + if (tab === 'pending') { + bumpMaxTimestampMs(latestPendingReviewAtMs, key, doc.timestamp); + } + } + resolve({ tabCounts, latestPendingReviewAtMs, latestPushAtMs }); + }, + ); + }); +}; const defaultPushQuery: Partial = { error: false, @@ -154,7 +224,6 @@ export const deletePush = async (id: string): Promise => { if (err) { reject(err); } else { - activityIndex?.set(id, null); resolve(); } }); @@ -163,7 +232,6 @@ export const deletePush = async (id: string): Promise => { export const writeAudit = async (action: Action): Promise => { return new Promise((resolve, reject) => { - const contribution = pushActivity(action); const options = { multi: false, upsert: true }; db.update({ id: action.id }, compactPush(action), options, (err) => { // ignore for code coverage as neDB rarely returns errors even for an invalid query @@ -171,7 +239,6 @@ export const writeAudit = async (action: Action): Promise => { if (err) { reject(err); } else { - activityIndex?.set(action.id, contribution); resolve(); } }); diff --git a/src/db/mongo/helper.ts b/src/db/mongo/helper.ts index 289f02503..6abc33d21 100644 --- a/src/db/mongo/helper.ts +++ b/src/db/mongo/helper.ts @@ -32,9 +32,7 @@ export const resetConnection = async (): Promise => { export const getDb = (): Db | null => _db; -export const connect = async ( - collectionName: string, -): Promise> => { +export const connect = async (collectionName: string): Promise => { //retrieve config at point of use (rather than import) const dbConfig = getDatabase(); const connectionString = dbConfig.connectionString; @@ -55,7 +53,7 @@ export const connect = async ( _db = _client.db(); } - return _db.collection(collectionName); + return _db.collection(collectionName); }; export const findDocuments = async ( diff --git a/src/db/mongo/pushes.ts b/src/db/mongo/pushes.ts index 7af77664b..84bdd89cf 100644 --- a/src/db/mongo/pushes.ts +++ b/src/db/mongo/pushes.ts @@ -14,18 +14,105 @@ * limitations under the License. */ -import { findDocuments, findOneDocument } from './helper'; -import { activityCollections, activityState, deletePushWithActivity } from './repoActivity'; -export { getRepoPushRollupsByCanonicalUrl } from './repoActivity'; +import { activityPrimaryStatusFromFlags } from '../../activity/activityPrimaryStatus'; +import { canonicalRemoteUrl } from '../../activity/canonicalRemoteUrl'; +import { connect, findDocuments, findOneDocument } from './helper'; import { Action } from '../../proxy/actions'; import { pushListProjection } from '../pushProjection'; import { compactPush, restorePush } from '../pushStorage'; -import { PushQuery } from '../types'; +import { + PushQuery, + RepoActivityTabCounts, + RepoPushRollupsByCanonicalUrl, + emptyRepoActivityTabCounts, +} from '../types'; import { CompletedAttestation, Rejection } from '../../proxy/processors/types'; import { buildUserProfilePushFilter } from '../userProfilePushQuery'; const collectionName = 'pushes'; +function bumpCount( + m: Map, + canonicalKey: string, + tab: keyof RepoActivityTabCounts, +): void { + if (!canonicalKey) { + return; + } + let row = m.get(canonicalKey); + if (!row) { + row = emptyRepoActivityTabCounts(); + m.set(canonicalKey, row); + } + row[tab] += 1; +} + +function bumpMaxTimestampMs( + m: Map, + canonicalKey: string, + timestamp: unknown, +): void { + if (!canonicalKey) { + return; + } + const ts = typeof timestamp === 'number' ? timestamp : NaN; + if (!Number.isFinite(ts)) { + return; + } + const prev = m.get(canonicalKey); + if (prev === undefined || ts > prev) { + m.set(canonicalKey, ts); + } +} + +/** + * Scan all push rows: tab counts and max timestamps per canonical remote URL (matches Activity UI). + * Uses a tight projection and in-Node canonicalization for DocumentDB compatibility. + */ +export const getRepoPushRollupsByCanonicalUrl = + async (): Promise => { + const collection = await connect(collectionName); + const cursor = collection.find( + { type: 'push' }, + { + projection: { + url: 1, + error: 1, + rejected: 1, + canceled: 1, + authorised: 1, + blocked: 1, + allowPush: 1, + timestamp: 1, + }, + }, + ); + const tabCounts = new Map(); + const latestPendingReviewAtMs = new Map(); + const latestPushAtMs = new Map(); + for await (const doc of cursor) { + const url = typeof doc.url === 'string' ? doc.url : ''; + const key = canonicalRemoteUrl(url); + if (!key) { + continue; + } + const tab = activityPrimaryStatusFromFlags({ + error: doc.error === true, + rejected: doc.rejected === true, + canceled: doc.canceled === true, + authorised: doc.authorised === true, + blocked: doc.blocked === true, + allowPush: doc.allowPush === true, + }); + bumpCount(tabCounts, key, tab); + bumpMaxTimestampMs(latestPushAtMs, key, doc.timestamp); + if (tab === 'pending') { + bumpMaxTimestampMs(latestPendingReviewAtMs, key, doc.timestamp); + } + } + return { tabCounts, latestPendingReviewAtMs, latestPushAtMs }; + }; + const defaultPushQuery: Partial = { error: false, blocked: true, @@ -37,14 +124,10 @@ const defaultPushQuery: Partial = { export const getPushes = async ( query: Partial = defaultPushQuery, ): Promise => { - return findDocuments( - collectionName, - { ...query, '_activity.deleted': { $ne: true } }, - { - projection: pushListProjection, - sort: { timestamp: -1 }, - }, - ); + return findDocuments(collectionName, query, { + projection: pushListProjection, + sort: { timestamp: -1 }, + }); }; export const getPushesForUserProfile = async ( @@ -52,56 +135,32 @@ export const getPushesForUserProfile = async ( profileUsername: string, ): Promise => { const filter = buildUserProfilePushFilter(emailVariants, profileUsername); - return findDocuments( - collectionName, - { ...filter, '_activity.deleted': { $ne: true } }, - { - projection: pushListProjection, - sort: { timestamp: -1 }, - }, - ); + return findDocuments(collectionName, filter, { + projection: pushListProjection, + sort: { timestamp: -1 }, + }); }; export const getPush = async (id: string): Promise => { - const doc = await findOneDocument(collectionName, { - id, - '_activity.deleted': { $ne: true }, - }); + const doc = await findOneDocument(collectionName, { id }); return doc ? restorePush(doc) : null; }; export const deletePush = async function (id: string): Promise { - await deletePushWithActivity(id); + const collection = await connect(collectionName); + await collection.deleteOne({ id }); }; export const writeAudit = async (action: Action): Promise => { const data = JSON.parse(JSON.stringify(compactPush(action))); const options = { upsert: true }; - const { pushes: collection } = await activityCollections(); + const collection = await connect(collectionName); delete data._id; if (typeof data.id !== 'string') { throw new Error('Invalid id'); } - const unset: Record = - data._lastStepIndex === undefined ? { _lastStepIndex: '' } : { lastStep: '' }; - const activity = activityState(action); - await collection.updateOne( - { id: data.id }, - { - $set: { - ...data, - '_activity.key': activity.key, - '_activity.tab': activity.tab, - '_activity.timestamp': activity.timestamp, - '_activity.revision': activity.revision, - '_activity.dirty': true, - '_activity.deleted': false, - }, - $addToSet: { '_activity.keys': activity.key }, - $unset: unset, - }, - options, - ); + const unset = data._lastStepIndex === undefined ? { _lastStepIndex: '' } : { lastStep: '' }; + await collection.updateOne({ id: data.id }, { $set: data, $unset: unset }, options); }; export const authorise = async ( diff --git a/src/db/mongo/repoActivity.ts b/src/db/mongo/repoActivity.ts deleted file mode 100644 index 671ba02be..000000000 --- a/src/db/mongo/repoActivity.ts +++ /dev/null @@ -1,204 +0,0 @@ -/** - * Copyright 2026 GitProxy Contributors - * - * Licensed under the Apache License, Version 2.0 (the "License"); - * you may not use this file except in compliance with the License. - * You may obtain a copy of the License at - * - * http://www.apache.org/licenses/LICENSE-2.0 - * - * Unless required by applicable law or agreed to in writing, software - * distributed under the License is distributed on an "AS IS" BASIS, - * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. - * See the License for the specific language governing permissions and - * limitations under the License. - */ - -import { randomUUID } from 'node:crypto'; -import type { Db } from 'mongodb'; -import { connect, getDb } from './helper'; -import { - addActivity, - emptyRepoSummary, - pushActivity, - pushActivityProjection, - repoRollups, - RepoActivityIndex, - type PushActivity, - type PushActivityRow, - type RepoActivitySummary, -} from '../repoActivity'; -import type { RepoPushRollupsByCanonicalUrl } from '../types'; - -export type ActivityState = PushActivity & { - revision: string; - keys: string[]; - dirty: boolean; - deleted: boolean; -}; - -type ActivityDocument = PushActivityRow & { _activity?: ActivityState }; -type StoredSummary = RepoActivitySummary & { revision: string; ready: boolean }; -const initialized = new WeakMap>(); - -export const activityState = (row: PushActivityRow): ActivityState => { - const activity = pushActivity(row) ?? { key: '', tab: 'pending', timestamp: null }; - return { ...activity, revision: randomUUID(), keys: [activity.key], dirty: true, deleted: false }; -}; - -export const activityCollections = async () => { - const pushes = await connect('pushes'); - const summaries = await connect('repoPushActivity'); - const database = getDb(); - if (!database) throw new Error('MongoDB connection is not available'); - let initialization = initialized.get(database); - if (!initialization) { - initialization = Promise.all([ - pushes.createIndex({ id: 1 }, { unique: true }), - pushes.createIndex({ '_activity.dirty': 1 }), - pushes.createIndex({ - '_activity.key': 1, - '_activity.deleted': 1, - '_activity.tab': 1, - '_activity.timestamp': 1, - }), - summaries.createIndex({ key: 1 }, { unique: true }), - ]) - .then(() => undefined) - .catch((error: unknown) => { - initialized.delete(database); - throw error; - }); - initialized.set(database, initialization); - } - await initialization; - return { pushes, summaries }; -}; - -const backfillActivity = async (): Promise => { - const { pushes } = await activityCollections(); - const cursor = pushes.find( - { '_activity.dirty': { $exists: false } }, - { projection: { ...pushActivityProjection, _id: 1 } }, - ); - for await (const row of cursor) { - await pushes.updateOne( - { _id: row._id, '_activity.dirty': { $exists: false } }, - { $set: { _activity: activityState(row) } }, - ); - } -}; - -const refreshRepository = async (key: string): Promise => { - const { pushes, summaries } = await activityCollections(); - const revision = randomUUID(); - // A newer refresh fences out stale snapshots, including work resumed after a crash. - await summaries.updateOne({ key }, { $set: { revision, ready: false } }, { upsert: true }); - const summary = emptyRepoSummary(key); - const cursor = pushes.find( - { '_activity.key': key, '_activity.deleted': false }, - { projection: { _id: 0, '_activity.key': 1, '_activity.tab': 1, '_activity.timestamp': 1 } }, - ); - for await (const row of cursor) { - if (row._activity) addActivity(summary, row._activity); - } - const result = await summaries.updateOne( - { key, revision }, - { $set: { ...summary, ready: true } }, - ); - return result.matchedCount === 1; -}; - -const scanActivity = async (): Promise => { - const { pushes } = await activityCollections(); - const index = new RepoActivityIndex(); - for await (const row of pushes.find( - { '_activity.deleted': { $ne: true } }, - { projection: pushActivityProjection }, - )) { - index.set(row.id, pushActivity(row)); - } - return index.snapshot(); -}; - -export const getRepoPushRollupsByCanonicalUrl = - async (): Promise => { - await backfillActivity(); - const { pushes, summaries } = await activityCollections(); - for (let attempt = 0; attempt < 3; attempt++) { - const dirty = await pushes - .find({ '_activity.dirty': true }, { projection: { _id: 1, _activity: 1 } }) - .toArray(); - const unfinished = await summaries - .find({ ready: false }, { projection: { key: 1 } }) - .toArray(); - if (!dirty.length && !unfinished.length) { - const rows = await summaries.find({}).toArray(); - if (rows.every((row) => row.ready)) return repoRollups(rows); - continue; - } - const keys = new Set(unfinished.map((row) => row.key)); - for (const row of dirty) { - for (const key of row._activity?.keys ?? []) if (key) keys.add(key); - } - const refreshed = new Set(); - for (const key of keys) { - if (await refreshRepository(key)) refreshed.add(key); - } - for (const row of dirty) { - const activity = row._activity; - if (!activity || activity.keys.some((key) => key && !refreshed.has(key))) continue; - // Clear only the exact revision summarized above. Concurrent writes remain dirty. - const filter = { _id: row._id, '_activity.revision': activity.revision }; - if (activity.deleted) { - await pushes.deleteOne(filter); - } else { - await pushes.updateOne(filter, { - $set: { '_activity.dirty': false, '_activity.keys': [activity.key] }, - }); - } - } - } - // Bound request latency under continuous writes; the source rows remain authoritative. - return scanActivity(); - }; - -export const deletePushWithActivity = async (id: string): Promise => { - const { pushes } = await activityCollections(); - for (let attempt = 0; attempt < 3; attempt++) { - const row = await pushes.findOne( - { id }, - { - projection: { ...pushActivityProjection, _id: 1, _activity: 1 }, - }, - ); - if (!row || row._activity?.deleted) return; - const previous = row._activity; - const activity = previous ?? activityState(row); - const result = await pushes.replaceOne( - { - _id: row._id, - '_activity.revision': previous ? previous.revision : { $exists: false }, - }, - { - id, - _activity: { ...activity, revision: randomUUID(), dirty: true, deleted: true }, - }, - ); - if (result.matchedCount === 1) return; - } - throw new Error(`Push ${id} changed repeatedly during deletion; retry the request`); -}; - -export const rebuildRepoPushRollups = async (): Promise => { - const { pushes, summaries } = await activityCollections(); - await backfillActivity(); - await summaries.updateMany({}, { $set: { ready: false } }); - await pushes.updateMany( - {}, - { - $set: { '_activity.dirty': true, '_activity.revision': randomUUID() }, - }, - ); - return getRepoPushRollupsByCanonicalUrl(); -}; diff --git a/src/db/repoActivity.ts b/src/db/repoActivity.ts deleted file mode 100644 index 4333c183b..000000000 --- a/src/db/repoActivity.ts +++ /dev/null @@ -1,162 +0,0 @@ -/** - * Copyright 2026 GitProxy Contributors - * - * Licensed under the Apache License, Version 2.0 (the "License"); - * you may not use this file except in compliance with the License. - * You may obtain a copy of the License at - * - * http://www.apache.org/licenses/LICENSE-2.0 - * - * Unless required by applicable law or agreed to in writing, software - * distributed under the License is distributed on an "AS IS" BASIS, - * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. - * See the License for the specific language governing permissions and - * limitations under the License. - */ - -import { isEqual } from 'lodash'; -import { - activityPrimaryStatusFromFlags, - type ActivityPrimaryStatusInput, - type ActivityStatusTab, -} from '../activity/activityPrimaryStatus'; -import { canonicalRemoteUrl } from '../activity/canonicalRemoteUrl'; -import { - emptyRepoActivityTabCounts, - type RepoActivityTabCounts, - type RepoPushRollupsByCanonicalUrl, -} from './types'; - -export type PushActivityRow = ActivityPrimaryStatusInput & { - id: string; - type?: string; - url?: string; - timestamp?: number; -}; - -export type PushActivity = { - key: string; - tab: ActivityStatusTab; - timestamp: number | null; -}; - -export type RepoActivitySummary = { - key: string; - counts: RepoActivityTabCounts; - latestPush: number | null; - latestPending: number | null; -}; - -export const pushActivityProjection = { - _id: 0, - id: 1, - type: 1, - url: 1, - timestamp: 1, - error: 1, - rejected: 1, - canceled: 1, - authorised: 1, - blocked: 1, - allowPush: 1, -} as const; - -export const pushActivity = (row: PushActivityRow): PushActivity | null => { - const key = row.type === 'push' && typeof row.url === 'string' ? canonicalRemoteUrl(row.url) : ''; - if (!key) return null; - return { - key, - tab: activityPrimaryStatusFromFlags(row), - timestamp: - typeof row.timestamp === 'number' && Number.isFinite(row.timestamp) ? row.timestamp : null, - }; -}; - -export const emptyRepoSummary = (key: string): RepoActivitySummary => ({ - key, - counts: emptyRepoActivityTabCounts(), - latestPush: null, - latestPending: null, -}); - -const maxTimestamp = (previous: number | null, next: number | null): number | null => - next === null ? previous : previous === null ? next : Math.max(previous, next); - -export const addActivity = (summary: RepoActivitySummary, activity: PushActivity): void => { - summary.counts[activity.tab]++; - summary.latestPush = maxTimestamp(summary.latestPush, activity.timestamp); - if (activity.tab === 'pending') { - summary.latestPending = maxTimestamp(summary.latestPending, activity.timestamp); - } -}; - -export const repoRollups = ( - summaries: Iterable, -): RepoPushRollupsByCanonicalUrl => { - const result: RepoPushRollupsByCanonicalUrl = { - tabCounts: new Map(), - latestPendingReviewAtMs: new Map(), - latestPushAtMs: new Map(), - }; - for (const summary of summaries) { - if (!Object.values(summary.counts).some((count) => count > 0)) continue; - result.tabCounts.set(summary.key, { ...summary.counts }); - if (summary.latestPush !== null) result.latestPushAtMs.set(summary.key, summary.latestPush); - if (summary.latestPending !== null) - result.latestPendingReviewAtMs.set(summary.key, summary.latestPending); - } - return result; -}; - -export class RepoActivityIndex { - private readonly pushes = new Map(); - private readonly repositories = new Map>(); - private readonly summaries = new Map(); - - set(id: string, activity: PushActivity | null): void { - const previous = this.pushes.get(id); - if (isEqual(previous ?? null, activity)) return; - if (previous) { - this.pushes.delete(id); - const rows = this.repositories.get(previous.key); - const summary = this.summaries.get(previous.key); - if (rows && summary) { - rows.delete(id); - summary.counts[previous.tab]--; - if (!rows.size) { - this.repositories.delete(previous.key); - this.summaries.delete(previous.key); - } else if ( - previous.timestamp !== null && - (previous.timestamp === summary.latestPush || - previous.timestamp === summary.latestPending) - ) { - summary.latestPush = null; - summary.latestPending = null; - for (const row of rows.values()) { - summary.latestPush = maxTimestamp(summary.latestPush, row.timestamp); - if (row.tab === 'pending') - summary.latestPending = maxTimestamp(summary.latestPending, row.timestamp); - } - } - } - } - if (activity) { - this.pushes.set(id, activity); - let rows = this.repositories.get(activity.key); - let summary = this.summaries.get(activity.key); - if (!rows || !summary) { - rows = new Map(); - summary = emptyRepoSummary(activity.key); - this.repositories.set(activity.key, rows); - this.summaries.set(activity.key, summary); - } - rows.set(id, activity); - addActivity(summary, activity); - } - } - - snapshot(): RepoPushRollupsByCanonicalUrl { - return repoRollups(this.summaries.values()); - } -} diff --git a/src/ui/views/PushDetails/PushDetails.tsx b/src/ui/views/PushDetails/PushDetails.tsx index 1e49d47e0..f54ed0dee 100644 --- a/src/ui/views/PushDetails/PushDetails.tsx +++ b/src/ui/views/PushDetails/PushDetails.tsx @@ -42,10 +42,7 @@ import { sortRepoViews } from '../../services/repo'; import { useRepoViewsListQuery } from '../../query/useRepoViewsListQuery'; import { usePushQuery } from '../../query/usePushQuery'; import { usePushPermissionsQuery } from '../../query/usePushPermissionsQuery'; -import { useMutation, useQueryClient } from '@tanstack/react-query'; -import { pushQueryKeys } from '../../query/pushQueryKeys'; -import { repoQueryKeys } from '../../query/repoQueryKeys'; -import { userQueryKeys } from '../../query/userQueryKeys'; +import { useMutation } from '@tanstack/react-query'; import { buildRepoDisplayIndex, resolveActivityRepoDisplay, @@ -133,7 +130,6 @@ function CanceledPushBanner({ push }: { push: PushActionView }): React.ReactElem const PushDetails = () => { const { id } = useParams<{ id: string }>(); const navigate = useNavigate(); - const queryClient = useQueryClient(); const [message, setMessage] = useState(''); const { data: repoListRaw } = useRepoViewsListQuery(true); const registeredRepos = useMemo( @@ -174,35 +170,40 @@ const PushDetails = () => { setMessage(result.message || 'Something went wrong...'); }; - const handleActionSuccess = async (result: ServiceResult) => { - if (!result.success) { - handleActionFailure(result); - return; - } - await Promise.all([ - queryClient.invalidateQueries({ queryKey: pushQueryKeys.all }), - queryClient.invalidateQueries({ queryKey: repoQueryKeys.list() }), - queryClient.invalidateQueries({ queryKey: userQueryKeys.all }), - ]); - navigate('/dashboard/push/'); - }; - const authoriseMutation = useMutation({ mutationFn: (attestationData: Array<{ label: string; checked: boolean }>) => authorisePush(id!, attestationData), - onSuccess: handleActionSuccess, + onSuccess: (result) => { + if (result.success) { + navigate('/dashboard/push/'); + } else { + handleActionFailure(result); + } + }, onError: () => setMessage('Something went wrong...'), }); const rejectMutation = useMutation({ mutationFn: (reason: string) => rejectPush(id!, reason), - onSuccess: handleActionSuccess, + onSuccess: (result) => { + if (result.success) { + navigate('/dashboard/push/'); + } else { + handleActionFailure(result); + } + }, onError: () => setMessage('Something went wrong...'), }); const cancelMutation = useMutation({ mutationFn: () => cancelPush(id!), - onSuccess: handleActionSuccess, + onSuccess: (result) => { + if (result.success) { + navigate('/dashboard/push/'); + } else { + handleActionFailure(result); + } + }, onError: () => setMessage('Something went wrong...'), }); diff --git a/src/ui/views/PushDetails/components/AttestationInfo.tsx b/src/ui/views/PushDetails/components/AttestationInfo.tsx index 98a32fd89..9f2a0d0e1 100644 --- a/src/ui/views/PushDetails/components/AttestationInfo.tsx +++ b/src/ui/views/PushDetails/components/AttestationInfo.tsx @@ -30,10 +30,7 @@ const AttestationInfo = ({ push }: AttestationInfoProps) => { } const att = push.attestation; - const numericTimestamp = Number(att.timestamp); - const ts = Number.isFinite(numericTimestamp) - ? DateTime.fromMillis(numericTimestamp) - : DateTime.fromISO(String(att.timestamp)); + const ts = DateTime.fromMillis(Number(att.timestamp)); const tsTitle = ts.toFormat('cccc, MMMM d yyyy, h:mm:ss a'); const presetDisplayName = att.reviewer.displayName?.trim() || undefined; diff --git a/src/ui/views/PushDetails/components/RejectionInfo.tsx b/src/ui/views/PushDetails/components/RejectionInfo.tsx index 01bc80d2c..3f78b7cc9 100644 --- a/src/ui/views/PushDetails/components/RejectionInfo.tsx +++ b/src/ui/views/PushDetails/components/RejectionInfo.tsx @@ -30,10 +30,7 @@ const RejectionInfo = ({ push }: RejectionInfoProps) => { } const rej = push.rejection; - const numericTimestamp = Number(rej.timestamp); - const ts = Number.isFinite(numericTimestamp) - ? DateTime.fromMillis(numericTimestamp) - : DateTime.fromISO(String(rej.timestamp)); + const ts = DateTime.fromMillis(Number(rej.timestamp)); const tsTitle = ts.toFormat('cccc, MMMM d yyyy, h:mm:ss a'); return ( diff --git a/test/db/file/pushStorage.test.ts b/test/db/file/pushStorage.test.ts index a5a446106..2716e9b2c 100644 --- a/test/db/file/pushStorage.test.ts +++ b/test/db/file/pushStorage.test.ts @@ -16,18 +16,5 @@ import * as pushes from '../../../src/db/file/pushes'; import { definePushStorageContract } from '../pushStorage.contract'; -import { expect, it, vi } from 'vitest'; definePushStorageContract('NeDB push storage', pushes, (id) => pushes.db.findOneAsync({ id })); - -it('serves repeated repository reads without querying push history again', async () => { - await pushes.getRepoPushRollupsByCanonicalUrl(); - const find = vi.spyOn(pushes.db, 'find'); - try { - await pushes.getRepoPushRollupsByCanonicalUrl(); - await pushes.getRepoPushRollupsByCanonicalUrl(); - expect(find).not.toHaveBeenCalled(); - } finally { - find.mockRestore(); - } -}); diff --git a/test/db/file/repoActivity.test.ts b/test/db/file/repoActivity.test.ts deleted file mode 100644 index b48f62bee..000000000 --- a/test/db/file/repoActivity.test.ts +++ /dev/null @@ -1,83 +0,0 @@ -/** - * Copyright 2026 GitProxy Contributors - * - * Licensed under the Apache License, Version 2.0 (the "License"); - * you may not use this file except in compliance with the License. - * You may obtain a copy of the License at - * - * http://www.apache.org/licenses/LICENSE-2.0 - * - * Unless required by applicable law or agreed to in writing, software - * distributed under the License is distributed on an "AS IS" BASIS, - * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. - * See the License for the specific language governing permissions and - * limitations under the License. - */ - -import { afterEach, describe, expect, it, vi } from 'vitest'; -import { Action, RequestType } from '../../../src/proxy/actions'; - -describe('file repository rollup initialization', () => { - afterEach(() => vi.restoreAllMocks()); - - it('retries a failed history load and shares the successful index between readers', async () => { - vi.resetModules(); - const sink = await import('../../../src/db/file/pushes'); - const action = new Action('history', RequestType.PUSH, 'POST', 123, 'https://example.com/a/b'); - await sink.db.insertAsync(action); - const failure = new Error('history read failed'); - const original = sink.db.find.bind(sink.db); - const find = vi - .spyOn(sink.db, 'find') - .mockImplementationOnce( - (query: unknown, projection: unknown, callback?: (error: Error, rows: never[]) => void) => { - queueMicrotask(() => callback?.(failure, [])); - return original(query, projection); - }, - ); - await expect(sink.getRepoPushRollupsByCanonicalUrl()).rejects.toThrow(failure); - const readers = await Promise.all([ - sink.getRepoPushRollupsByCanonicalUrl(), - sink.getRepoPushRollupsByCanonicalUrl(), - ]); - for (const rollups of readers) { - expect(rollups.tabCounts.get('example.com/a/b')?.pending).toBe(1); - expect(rollups.latestPushAtMs.get('example.com/a/b')).toBe(123); - } - expect(find).toHaveBeenCalledTimes(2); - }); - - it('loads historical rows once and applies writes after initialization', async () => { - vi.resetModules(); - const sink = await import('../../../src/db/file/pushes'); - const action = new Action('history', RequestType.PUSH, 'POST', 123, 'https://example.com/a/b'); - action.blocked = true; - await sink.db.insertAsync(action); - expect( - (await sink.getRepoPushRollupsByCanonicalUrl()).tabCounts.get('example.com/a/b')?.pending, - ).toBe(1); - await sink.authorise(action.id); - const find = vi.spyOn(sink.db, 'find'); - const rollups = await sink.getRepoPushRollupsByCanonicalUrl(); - expect(rollups.tabCounts.get('example.com/a/b')?.approved).toBe(1); - expect(rollups.latestPendingReviewAtMs.has('example.com/a/b')).toBe(false); - expect(find).not.toHaveBeenCalled(); - }); - - it('does not change a warm index when a database write fails', async () => { - vi.resetModules(); - const sink = await import('../../../src/db/file/pushes'); - const action = new Action('failure', RequestType.PUSH, 'POST', 123, 'https://example.com/a/b'); - await sink.writeAudit(action); - await sink.getRepoPushRollupsByCanonicalUrl(); - const update = vi.spyOn(sink.db, 'update').mockImplementationOnce(() => { - throw new Error('write failed'); - }); - await expect(sink.authorise(action.id)).rejects.toThrow('write failed'); - update.mockRestore(); - expect( - (await sink.getRepoPushRollupsByCanonicalUrl()).tabCounts.get('example.com/a/b')?.pending, - ).toBe(1); - expect((await sink.getPush(action.id))?.authorised).toBe(false); - }); -}); diff --git a/test/db/mongo/push.test.ts b/test/db/mongo/push.test.ts index bb8926809..231db5405 100644 --- a/test/db/mongo/push.test.ts +++ b/test/db/mongo/push.test.ts @@ -20,15 +20,11 @@ import { pushListProjection } from '../../../src/db/pushProjection'; const mockFindOne = vi.fn(); const mockDeleteOne = vi.fn(); -const mockReplaceOne = vi.fn(); -const mockDatabase = {}; const mockUpdateOne = vi.fn(); const mockFind = vi.fn(); const mockConnect = vi.fn(() => ({ findOne: mockFindOne, - replaceOne: mockReplaceOne, - createIndex: vi.fn().mockResolvedValue('index'), deleteOne: mockDeleteOne, updateOne: mockUpdateOne, find: mockFind, @@ -39,7 +35,6 @@ const mockFindOneDocument = vi.fn(); vi.mock('../../../src/db/mongo/helper', () => ({ connect: mockConnect, - getDb: () => mockDatabase, findDocuments: mockFindDocuments, findOneDocument: mockFindOneDocument, })); @@ -107,7 +102,6 @@ describe('MongoDB Push Handler', async () => { allowPush: false, authorised: false, type: 'push', - '_activity.deleted': { $ne: true }, }, { projection: pushListProjection, @@ -128,7 +122,7 @@ describe('MongoDB Push Handler', async () => { expect(mockFindDocuments).toHaveBeenCalledWith( 'pushes', - { ...customQuery, '_activity.deleted': { $ne: true } }, + customQuery, expect.objectContaining({ projection: expect.any(Object), }), @@ -174,10 +168,7 @@ describe('MongoDB Push Handler', async () => { const result = await getPush(TEST_PUSH.id); - expect(mockFindOneDocument).toHaveBeenCalledWith('pushes', { - id: TEST_PUSH.id, - '_activity.deleted': { $ne: true }, - }); + expect(mockFindOneDocument).toHaveBeenCalledWith('pushes', { id: TEST_PUSH.id }); expect(mockToClass).toHaveBeenCalledWith(TEST_PUSH, Action.prototype); expect(result).toBeTruthy(); }); @@ -187,10 +178,7 @@ describe('MongoDB Push Handler', async () => { const result = await getPush('non-existent-id'); - expect(mockFindOneDocument).toHaveBeenCalledWith('pushes', { - id: 'non-existent-id', - '_activity.deleted': { $ne: true }, - }); + expect(mockFindOneDocument).toHaveBeenCalledWith('pushes', { id: 'non-existent-id' }); expect(result).toBeNull(); expect(mockToClass).not.toHaveBeenCalled(); }); @@ -198,20 +186,12 @@ describe('MongoDB Push Handler', async () => { describe('deletePush', () => { it('should delete a push by id', async () => { - mockFindOne.mockResolvedValue({ ...TEST_PUSH, _id: 'stored-push' }); - mockReplaceOne.mockResolvedValue({ matchedCount: 1 }); + mockDeleteOne.mockResolvedValue({ deletedCount: 1 }); await deletePush(TEST_PUSH.id); expect(mockConnect).toHaveBeenCalledWith('pushes'); - expect(mockReplaceOne).toHaveBeenCalledWith( - { _id: 'stored-push', '_activity.revision': { $exists: false } }, - { - id: TEST_PUSH.id, - _activity: expect.objectContaining({ deleted: true, dirty: true, key: 'example.com/' }), - }, - ); - expect(mockDeleteOne).not.toHaveBeenCalled(); + expect(mockDeleteOne).toHaveBeenCalledWith({ id: TEST_PUSH.id }); }); }); @@ -232,7 +212,6 @@ describe('MongoDB Push Handler', async () => { authorised: TEST_PUSH.authorised, }), $unset: { _lastStepIndex: '' }, - $addToSet: { '_activity.keys': 'example.com/' }, }, { upsert: true }, ); @@ -257,10 +236,7 @@ describe('MongoDB Push Handler', async () => { const attestation = { signature: 'test-sig' }; const result = await authorise(TEST_PUSH.id, attestation); - expect(mockFindOneDocument).toHaveBeenCalledWith('pushes', { - id: TEST_PUSH.id, - '_activity.deleted': { $ne: true }, - }); + expect(mockFindOneDocument).toHaveBeenCalledWith('pushes', { id: TEST_PUSH.id }); expect(mockConnect).toHaveBeenCalledWith('pushes'); expect(mockUpdateOne).toHaveBeenCalledWith( { id: TEST_PUSH.id }, @@ -272,7 +248,6 @@ describe('MongoDB Push Handler', async () => { attestation: attestation, }), $unset: { _lastStepIndex: '' }, - $addToSet: { '_activity.keys': 'example.com/' }, }, { upsert: true }, ); @@ -297,10 +272,7 @@ describe('MongoDB Push Handler', async () => { const rejection = { signature: 'test-sig' }; const result = await reject(TEST_PUSH.id, rejection); - expect(mockFindOneDocument).toHaveBeenCalledWith('pushes', { - id: TEST_PUSH.id, - '_activity.deleted': { $ne: true }, - }); + expect(mockFindOneDocument).toHaveBeenCalledWith('pushes', { id: TEST_PUSH.id }); expect(mockConnect).toHaveBeenCalledWith('pushes'); expect(mockUpdateOne).toHaveBeenCalledWith( { id: TEST_PUSH.id }, @@ -312,7 +284,6 @@ describe('MongoDB Push Handler', async () => { rejection: rejection, }), $unset: { _lastStepIndex: '' }, - $addToSet: { '_activity.keys': 'example.com/' }, }, { upsert: true }, ); @@ -336,10 +307,7 @@ describe('MongoDB Push Handler', async () => { const result = await cancel(TEST_PUSH.id); - expect(mockFindOneDocument).toHaveBeenCalledWith('pushes', { - id: TEST_PUSH.id, - '_activity.deleted': { $ne: true }, - }); + expect(mockFindOneDocument).toHaveBeenCalledWith('pushes', { id: TEST_PUSH.id }); expect(mockConnect).toHaveBeenCalledWith('pushes'); expect(mockUpdateOne).toHaveBeenCalledWith( { id: TEST_PUSH.id }, @@ -350,7 +318,6 @@ describe('MongoDB Push Handler', async () => { rejected: false, }), $unset: { _lastStepIndex: '' }, - $addToSet: { '_activity.keys': 'example.com/' }, }, { upsert: true }, ); diff --git a/test/db/mongo/pushStorage.integration.test.ts b/test/db/mongo/pushStorage.integration.test.ts index dacae91fc..cb03165bb 100644 --- a/test/db/mongo/pushStorage.integration.test.ts +++ b/test/db/mongo/pushStorage.integration.test.ts @@ -14,313 +14,13 @@ * limitations under the License. */ -import { afterEach, describe, expect, it, vi } from 'vitest'; -import { Collection } from 'mongodb'; +import { describe } from 'vitest'; import * as pushes from '../../../src/db/mongo/pushes'; import { definePushStorageContract } from '../pushStorage.contract'; -import { connect, resetConnection } from '../../../src/db/mongo/helper'; -import * as mongoHelper from '../../../src/db/mongo/helper'; -import { rebuildRepoPushRollups } from '../../../src/db/mongo/repoActivity'; -import { Action, RequestType, Step } from '../../../src/proxy/actions'; +import { connect } from '../../../src/db/mongo/helper'; describe.runIf(process.env.RUN_MONGO_TESTS === 'true')('MongoDB storage contract', () => { definePushStorageContract('MongoDB push storage', pushes, async (id) => (await connect('pushes')).findOne({ id }), ); - - const key = 'example.com/recovery/repo'; - const makePush = (id = 'recovery') => { - const action = new Action(id, RequestType.PUSH, 'POST', 123, `https://${key}.git`); - action.blocked = true; - const step = new Step('diff'); - step.content = 'preserved payload'; - action.addStep(step); - return action; - }; - - afterEach(() => vi.restoreAllMocks()); - - it('retries initialization after index creation fails', async () => { - const createIndex = vi - .spyOn(Collection.prototype, 'createIndex') - .mockRejectedValueOnce(new Error('index creation failed')); - await expect(pushes.writeAudit(makePush())).rejects.toThrow('index creation failed'); - expect(await (await connect('pushes')).findOne({ id: 'recovery' })).toBeNull(); - await pushes.writeAudit(makePush()); - expect(createIndex).toHaveBeenCalledTimes(8); - expect((await pushes.getRepoPushRollupsByCanonicalUrl()).tabCounts.get(key)?.pending).toBe(1); - }); - - it('rejects initialization if the connection disappears and succeeds on retry', async () => { - vi.spyOn(mongoHelper, 'getDb').mockReturnValueOnce(null); - await expect(pushes.writeAudit(makePush())).rejects.toThrow( - 'MongoDB connection is not available', - ); - await pushes.writeAudit(makePush()); - expect((await pushes.getRepoPushRollupsByCanonicalUrl()).tabCounts.get(key)?.pending).toBe(1); - }); - - it('deletes a historical push before backfill and tolerates repeated deletion', async () => { - await (await connect('pushes')).insertOne(JSON.parse(JSON.stringify(makePush()))); - await pushes.deletePush('recovery'); - const replace = vi.spyOn(Collection.prototype, 'replaceOne'); - await pushes.deletePush('recovery'); - expect(replace).not.toHaveBeenCalled(); - expect(await pushes.getPush('recovery')).toBeNull(); - expect((await pushes.getRepoPushRollupsByCanonicalUrl()).tabCounts.size).toBe(0); - expect(await (await connect('pushes')).findOne({ id: 'recovery' })).toBeNull(); - }); - - it.each([1, 3])('handles %i concurrent updates during deletion', async (conflicts) => { - await pushes.writeAudit(makePush()); - const original = Collection.prototype.replaceOne; - let attempts = 0; - const replace = vi - .spyOn(Collection.prototype, 'replaceOne') - .mockImplementation(async function (filter, replacement, options) { - if (this.collectionName === 'pushes' && attempts++ < conflicts) { - await pushes.authorise('recovery'); - } - return original.call(this, filter, replacement, options); - }); - if (conflicts === 1) { - await pushes.deletePush('recovery'); - expect(attempts).toBe(2); - expect(await pushes.getPush('recovery')).toBeNull(); - } else { - await expect(pushes.deletePush('recovery')).rejects.toThrow( - 'Push recovery changed repeatedly during deletion; retry the request', - ); - expect(attempts).toBe(3); - expect((await pushes.getPush('recovery'))?.authorised).toBe(true); - expect((await pushes.getRepoPushRollupsByCanonicalUrl()).tabCounts.get(key)?.approved).toBe( - 1, - ); - replace.mockRestore(); - await pushes.deletePush('recovery'); - } - expect((await pushes.getRepoPushRollupsByCanonicalUrl()).tabCounts.size).toBe(0); - }); - - it('backfills non-push records without adding repository activity or leaving them dirty', async () => { - const action = new Action('pull', RequestType.PULL, 'GET', 123, `https://${key}`); - const collection = await connect('pushes'); - await collection.insertOne(JSON.parse(JSON.stringify(action))); - expect((await pushes.getRepoPushRollupsByCanonicalUrl()).tabCounts.size).toBe(0); - expect(await collection.findOne({ id: action.id })).toMatchObject({ - _activity: { key: '', dirty: false }, - }); - expect(await pushes.getPush(action.id)).toMatchObject(action); - }); - - it('retries when a summary becomes unfinished during an otherwise clean read', async () => { - await pushes.writeAudit(makePush()); - await pushes.getRepoPushRollupsByCanonicalUrl(); - const summaries = await connect('repoPushActivity'); - const original = Collection.prototype.find; - let interleaved = false; - vi.spyOn(Collection.prototype, 'find').mockImplementation(function (filter, options) { - const cursor = original.call(this, filter, options); - if ( - !interleaved && - this.collectionName === 'repoPushActivity' && - !Object.keys(filter).length - ) { - interleaved = true; - const toArray = cursor.toArray.bind(cursor); - vi.spyOn(cursor, 'toArray').mockImplementationOnce(async () => { - await pushes.authorise('recovery'); - await summaries.updateOne({ key }, { $set: { ready: false } }); - return toArray(); - }); - } - return cursor; - }); - const rollups = await pushes.getRepoPushRollupsByCanonicalUrl(); - expect(interleaved).toBe(true); - expect(rollups.tabCounts.get(key)?.approved).toBe(1); - expect(rollups.tabCounts.get(key)?.pending).toBe(0); - expect(await summaries.findOne({ key })).toMatchObject({ ready: true }); - }); - - it('backfills historical records without changing their audit data', async () => { - const action = makePush(); - const collection = await connect('pushes'); - await collection.insertOne(JSON.parse(JSON.stringify(action))); - expect((await pushes.getRepoPushRollupsByCanonicalUrl()).tabCounts.get(key)?.pending).toBe(1); - expect(await pushes.getPush(action.id)).toMatchObject(action); - expect(await collection.findOne({ id: action.id })).toMatchObject({ - steps: action.steps, - lastStep: action.lastStep, - _activity: { dirty: false }, - }); - }); - - it('does not scan push history on unchanged repository reads', async () => { - await pushes.writeAudit(makePush()); - await pushes.getRepoPushRollupsByCanonicalUrl(); - const find = vi.spyOn(Collection.prototype, 'find'); - await pushes.getRepoPushRollupsByCanonicalUrl(); - const queries = find.mock.calls - .filter((_, index) => find.mock.contexts[index].collectionName === 'pushes') - .map(([query]) => query); - expect(queries).toEqual([ - { '_activity.dirty': { $exists: false } }, - { '_activity.dirty': true }, - ]); - const collection = await connect('pushes'); - for (const query of queries) { - const plan = await collection.find(query).explain('executionStats'); - expect(plan.executionStats.totalDocsExamined).toBe(0); - } - }); - - it('keeps a concurrent approval dirty until a newer refresh includes it', async () => { - await pushes.writeAudit(makePush()); - const original = Collection.prototype.updateOne; - let interleaved = false; - vi.spyOn(Collection.prototype, 'updateOne').mockImplementation( - async function (filter, update, options) { - if ( - !interleaved && - this.collectionName === 'repoPushActivity' && - update.$set?.ready === true - ) { - interleaved = true; - await pushes.authorise('recovery'); - } - return original.call(this, filter, update, options); - }, - ); - const rollups = await pushes.getRepoPushRollupsByCanonicalUrl(); - expect(interleaved).toBe(true); - expect(rollups.tabCounts.get(key)?.approved).toBe(1); - expect(rollups.tabCounts.get(key)?.pending).toBe(0); - expect(rollups.latestPendingReviewAtMs.has(key)).toBe(false); - }); - - it('prevents an older refresh from overwriting a newer completed refresh', async () => { - await pushes.writeAudit(makePush()); - const original = Collection.prototype.updateOne; - let interleaved = false; - vi.spyOn(Collection.prototype, 'updateOne').mockImplementation( - async function (filter, update, options) { - if ( - !interleaved && - this.collectionName === 'repoPushActivity' && - update.$set?.ready === true - ) { - interleaved = true; - await pushes.authorise('recovery'); - expect( - (await pushes.getRepoPushRollupsByCanonicalUrl()).tabCounts.get(key)?.approved, - ).toBe(1); - } - return original.call(this, filter, update, options); - }, - ); - expect((await pushes.getRepoPushRollupsByCanonicalUrl()).tabCounts.get(key)?.approved).toBe(1); - const stored = await (await connect('repoPushActivity')).findOne({ key }); - expect(stored).toMatchObject({ ready: true, counts: { approved: 1, pending: 0 } }); - }); - - it('recovers an interrupted refresh after reconnecting', async () => { - await pushes.writeAudit(makePush()); - const original = Collection.prototype.updateOne; - const update = vi - .spyOn(Collection.prototype, 'updateOne') - .mockImplementation(async function (filter, update, options) { - if (this.collectionName === 'repoPushActivity' && update.$set?.ready === true) { - throw new Error('interrupted refresh'); - } - return original.call(this, filter, update, options); - }); - await expect(pushes.getRepoPushRollupsByCanonicalUrl()).rejects.toThrow('interrupted refresh'); - update.mockRestore(); - await resetConnection(); - expect((await pushes.getRepoPushRollupsByCanonicalUrl()).tabCounts.get(key)?.pending).toBe(1); - }); - - it('deletes the audit immediately and recovers its pending counter update after a restart', async () => { - await pushes.writeAudit(makePush()); - await pushes.getRepoPushRollupsByCanonicalUrl(); - await pushes.deletePush('recovery'); - expect(await pushes.getPush('recovery')).toBeNull(); - expect(await pushes.getPushes({})).toEqual([]); - const tombstone = await (await connect('pushes')).findOne({ id: 'recovery' }); - expect(tombstone).toMatchObject({ _activity: { deleted: true, dirty: true } }); - expect(JSON.stringify(tombstone)).not.toContain('preserved payload'); - await resetConnection(); - expect((await pushes.getRepoPushRollupsByCanonicalUrl()).tabCounts.has(key)).toBe(false); - expect(await (await connect('pushes')).findOne({ id: 'recovery' })).toBeNull(); - }); - - it('rebuilds missing derived summaries from the source records', async () => { - await pushes.writeAudit(makePush()); - await pushes.getRepoPushRollupsByCanonicalUrl(); - await (await connect('repoPushActivity')).deleteMany({}); - expect((await rebuildRepoPushRollups()).tabCounts.get(key)?.pending).toBe(1); - }); - - it('counts concurrent retries of one push only once', async () => { - await Promise.all(Array.from({ length: 10 }, () => pushes.writeAudit(makePush()))); - expect(await (await connect('pushes')).countDocuments({ id: 'recovery' })).toBe(1); - expect((await pushes.getRepoPushRollupsByCanonicalUrl()).tabCounts.get(key)?.pending).toBe(1); - }); - - it('falls back to the source records after three refreshes under continuous writes', async () => { - await pushes.writeAudit(makePush()); - const original = Collection.prototype.updateOne; - let refreshes = 0; - const update = vi - .spyOn(Collection.prototype, 'updateOne') - .mockImplementation(async function (filter, update, options) { - if (this.collectionName === 'repoPushActivity' && update.$set?.ready === true) { - refreshes++; - if (refreshes % 2) await pushes.authorise('recovery'); - else await pushes.cancel('recovery'); - } - return original.call(this, filter, update, options); - }); - const rollups = await pushes.getRepoPushRollupsByCanonicalUrl(); - expect(refreshes).toBe(3); - expect(rollups.tabCounts.get(key)?.approved).toBe(1); - update.mockRestore(); - expect((await pushes.getRepoPushRollupsByCanonicalUrl()).tabCounts.get(key)?.approved).toBe(1); - }); - - it('does not purge a push recreated while its deletion is being summarized', async () => { - await pushes.writeAudit(makePush()); - await pushes.getRepoPushRollupsByCanonicalUrl(); - await pushes.deletePush('recovery'); - const original = Collection.prototype.deleteOne; - let recreated = false; - vi.spyOn(Collection.prototype, 'deleteOne').mockImplementation( - async function (filter, options) { - if (!recreated && this.collectionName === 'pushes') { - recreated = true; - await pushes.writeAudit(makePush()); - } - return original.call(this, filter, options); - }, - ); - expect((await pushes.getRepoPushRollupsByCanonicalUrl()).tabCounts.get(key)?.pending).toBe(1); - expect((await pushes.getPush('recovery'))?.steps[0].content).toBe('preserved payload'); - }); - - it('keeps previous counts and audit data when an audit write fails', async () => { - await pushes.writeAudit(makePush()); - await pushes.getRepoPushRollupsByCanonicalUrl(); - const original = Collection.prototype.updateOne; - const update = vi - .spyOn(Collection.prototype, 'updateOne') - .mockImplementation(async function (filter, update, options) { - if (this.collectionName === 'pushes') throw new Error('write failed'); - return original.call(this, filter, update, options); - }); - await expect(pushes.authorise('recovery')).rejects.toThrow('write failed'); - update.mockRestore(); - expect((await pushes.getPush('recovery'))?.authorised).toBe(false); - expect((await pushes.getRepoPushRollupsByCanonicalUrl()).tabCounts.get(key)?.pending).toBe(1); - }); }); diff --git a/test/db/mongo/pushes.test.ts b/test/db/mongo/pushes.test.ts index 975675d8d..33eab5ace 100644 --- a/test/db/mongo/pushes.test.ts +++ b/test/db/mongo/pushes.test.ts @@ -20,14 +20,11 @@ const mockFindOneDocument = vi.fn(); const mockUpdateOne = vi.fn(); const mockConnect = vi.fn(() => ({ updateOne: mockUpdateOne, - createIndex: vi.fn().mockResolvedValue('index'), })); -const mockDatabase = {}; vi.mock('../../../src/db/mongo/helper', () => ({ connect: mockConnect, findOneDocument: mockFindOneDocument, - getDb: () => mockDatabase, })); describe('MongoDB - Pushes', async () => { @@ -66,10 +63,7 @@ describe('MongoDB - Pushes', async () => { const result = await reject(pushId, rejection); expect(result).toEqual({ message: `reject ${pushId}` }); - expect(mockFindOneDocument).toHaveBeenCalledWith('pushes', { - id: pushId, - '_activity.deleted': { $ne: true }, - }); + expect(mockFindOneDocument).toHaveBeenCalledWith('pushes', { id: pushId }); expect(mockConnect).toHaveBeenCalledWith('pushes'); const [query, update, options] = mockUpdateOne.mock.calls[0]; @@ -103,10 +97,7 @@ describe('MongoDB - Pushes', async () => { }; await expect(reject(pushId, rejection)).rejects.toThrow(`push ${pushId} not found`); - expect(mockFindOneDocument).toHaveBeenCalledWith('pushes', { - id: pushId, - '_activity.deleted': { $ne: true }, - }); + expect(mockFindOneDocument).toHaveBeenCalledWith('pushes', { id: pushId }); }); }); }); diff --git a/test/db/repoActivity.test.ts b/test/db/repoActivity.test.ts deleted file mode 100644 index bf7438ac6..000000000 --- a/test/db/repoActivity.test.ts +++ /dev/null @@ -1,84 +0,0 @@ -/** - * Copyright 2026 GitProxy Contributors - * - * Licensed under the Apache License, Version 2.0 (the "License"); - * you may not use this file except in compliance with the License. - * You may obtain a copy of the License at - * - * http://www.apache.org/licenses/LICENSE-2.0 - * - * Unless required by applicable law or agreed to in writing, software - * distributed under the License is distributed on an "AS IS" BASIS, - * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. - * See the License for the specific language governing permissions and - * limitations under the License. - */ - -import { describe, expect, it } from 'vitest'; -import { RepoActivityIndex, pushActivity, type PushActivity } from '../../src/db/repoActivity'; - -describe('repository activity index', () => { - it('updates counts once, falls back to older timestamps, and removes empty repositories', () => { - const index = new RepoActivityIndex(); - const old: PushActivity = { key: 'example.com/a/b', tab: 'pending', timestamp: 10 }; - index.set('old', old); - index.set('new', { ...old, timestamp: 20 }); - index.set('old', old); - expect(index.snapshot().tabCounts.get(old.key)?.pending).toBe(2); - index.set('new', { ...old, tab: 'approved', timestamp: 20 }); - expect(index.snapshot().latestPendingReviewAtMs.get(old.key)).toBe(10); - expect(index.snapshot().latestPushAtMs.get(old.key)).toBe(20); - index.set('new', null); - expect(index.snapshot().latestPushAtMs.get(old.key)).toBe(10); - index.set('old', null); - expect(index.snapshot().tabCounts.size).toBe(0); - expect(index.snapshot().latestPushAtMs.size).toBe(0); - }); - - it('moves contributions between repositories and isolates returned snapshots', () => { - const index = new RepoActivityIndex(); - index.set('push', { key: 'old', tab: 'pending', timestamp: 0 }); - const first = index.snapshot(); - const counts = first.tabCounts.get('old'); - if (counts) counts.pending = 42; - expect(index.snapshot().tabCounts.get('old')?.pending).toBe(1); - index.set('push', { key: 'new', tab: 'error', timestamp: -1 }); - expect(index.snapshot().tabCounts.has('old')).toBe(false); - expect(index.snapshot().tabCounts.get('new')?.error).toBe(1); - expect(index.snapshot().latestPushAtMs.get('new')).toBe(-1); - }); - - it('uses the Activity status precedence and canonical remote identity', () => { - expect( - pushActivity({ - id: 'push', - type: 'push', - url: 'git@Example.com:ORG/REPO.git', - error: true, - rejected: true, - canceled: true, - authorised: true, - blocked: true, - timestamp: 10, - }), - ).toEqual({ key: 'example.com/org/repo', tab: 'error', timestamp: 10 }); - expect(pushActivity({ id: 'pull', type: 'pull', url: 'https://example.com/a/b' })).toBeNull(); - expect(pushActivity({ id: 'empty', type: 'push', url: ' ' })).toBeNull(); - }); - - it.each([undefined, NaN, Infinity, -Infinity])( - 'counts pushes without using invalid timestamp %s', - (timestamp) => { - const activity = pushActivity({ - id: 'push', - type: 'push', - url: 'https://example.com/a/b', - timestamp, - }); - const index = new RepoActivityIndex(); - index.set('push', activity); - expect(index.snapshot().tabCounts.get('example.com/a/b')?.pending).toBe(1); - expect(index.snapshot().latestPushAtMs.size).toBe(0); - }, - ); -}); diff --git a/test/setup-integration.ts b/test/setup-integration.ts index e6ac415cf..c220668b5 100644 --- a/test/setup-integration.ts +++ b/test/setup-integration.ts @@ -20,7 +20,7 @@ import { resetConnection } from '../src/db/mongo/helper'; import { invalidateCache } from '../src/config'; const DEFAULT_TEST_DB_NAME = 'git-proxy-test'; -const COLLECTIONS = ['repos', 'users', 'pushes', 'repoPushActivity', 'user_session', 'migrations']; +const COLLECTIONS = ['repos', 'users', 'pushes', 'user_session', 'migrations']; let client: MongoClient | null = null; diff --git a/website/docs/architecture/processors.md b/website/docs/architecture/processors.md index fef219f4a..20e39d80c 100644 --- a/website/docs/architecture/processors.md +++ b/website/docs/architecture/processors.md @@ -246,8 +246,6 @@ This action runs after a chain has been executed. It stores in the database the Push list and profile activity responses return metadata without processor steps, `lastStep`, or diff content. Use `GET /api/v1/push/:id` to retrieve the full audit record and diff for review. When `lastStep` exactly matches the final entry in `steps`, the database stores a reference and restores `lastStep` on detail reads. A distinct `lastStep` is preserved. Historical diff content and logs remain readable and are not truncated. -Repository activity counts use derived summaries. NeDB builds an in-memory index on the first repository read and updates it after successful push writes and deletions. MongoDB records activity metadata alongside each push and refreshes summaries for changed repositories on the next repository read. Unchanged repository reads do not scan push history. The first read after an upgrade backfills historical activity metadata; it does not rewrite historical audit content. Under continuous concurrent changes, MongoDB bounds refresh retries and falls back to a metadata-only scan for that request. - Note: **`audit` writes all actions** (push, pull, default/unclassified) to the DB. An action object (or entry in the pushes table) might look like this: diff --git a/website/docs/deployment.mdx b/website/docs/deployment.mdx index de050a795..c257c6803 100644 --- a/website/docs/deployment.mdx +++ b/website/docs/deployment.mdx @@ -286,17 +286,6 @@ Switch from the default file-based database to MongoDB for production use: - Configure replica sets for high availability - Schedule regular backups of audit data (`mongodump`) -Repository activity summaries are derived from the push audit records. GitProxy creates the required indexes, including a unique push ID index, and backfills activity metadata on the first repository read after upgrade. Use the same GitProxy version across instances during this upgrade: older writers do not update the activity revisions. Standalone MongoDB is supported; summary updates do not require transactions or a replica set. - -Interrupted summary refreshes recover on the next repository read. Deleting a push removes its audit payload immediately; a small internal deletion marker remains until its repository summary is refreshed. If restoring only the `pushes` collection or repairing summaries manually, rebuild the derived data from a source checkout with the deployment's configuration: - -```bash -CONFIG_FILE=/path/to/proxy.config.json npx tsx -e \ - "import { rebuildRepoPushRollups } from './src/db/mongo/repoActivity'; import { resetConnection } from './src/db/mongo/helper'; rebuildRepoPushRollups().finally(resetConnection)" -``` - -For NeDB, restarting GitProxy rebuilds the in-memory summaries from the saved push records. - ### Health Checks GitProxy provides health check endpoints on both servers: From c1746a7f80c64879c9cf898dd882a6d3258804b1 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Denis=20=C4=86ori=C4=87?= Date: Tue, 6 Oct 2026 12:11:43 +0200 Subject: [PATCH 15/18] perf(db): compact PostgreSQL push audits and list responses MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Signed-off-by: Denis Ćorić --- .github/workflows/ci.yml | 4 +- src/db/postgres/pushes.ts | 22 ++++--- .../postgres/pushStorage.integration.test.ts | 57 +++++++++++++++++++ test/db/postgres/pushes.test.ts | 40 +++++++++++-- 4 files changed, 106 insertions(+), 17 deletions(-) create mode 100644 test/db/postgres/pushStorage.integration.test.ts diff --git a/.github/workflows/ci.yml b/.github/workflows/ci.yml index 1c4f78002..a01594042 100644 --- a/.github/workflows/ci.yml +++ b/.github/workflows/ci.yml @@ -95,12 +95,12 @@ jobs: env: RUN_POSTGRES_TESTS: 'true' GIT_PROXY_POSTGRES_CONNECTION_STRING: postgresql://postgres:postgres@localhost:5432/git_proxy_test - run: npm run test:integration:postgres + run: npm run test:integration:postgres -- --coverage --coverage.reporter=lcovonly --coverage.reportsDirectory=coverage/postgres - name: Upload test coverage report uses: codecov/codecov-action@303a32d7a59b442fa8d48b6a1cc6825c09c847a5 # v7.1.1 with: - files: ./coverage/lcov.info,./coverage/integration/lcov.info,./coverage-cli/lcov.info + files: ./coverage/lcov.info,./coverage/integration/lcov.info,./coverage/postgres/lcov.info,./coverage-cli/lcov.info token: ${{ secrets.CODECOV_TOKEN }} - name: Build frontend diff --git a/src/db/postgres/pushes.ts b/src/db/postgres/pushes.ts index 97865295d..029455083 100644 --- a/src/db/postgres/pushes.ts +++ b/src/db/postgres/pushes.ts @@ -18,7 +18,8 @@ import { activityPrimaryStatusFromFlags } from '../../activity/activityPrimarySt import { canonicalRemoteUrl } from '../../activity/canonicalRemoteUrl'; import { Action } from '../../proxy/actions'; import { CompletedAttestation, Rejection } from '../../proxy/processors/types'; -import { toClass } from '../helper'; +import { pushListProjection } from '../pushProjection'; +import { compactPush, restorePush } from '../pushStorage'; import { emptyRepoActivityTabCounts, PushQuery, @@ -47,8 +48,14 @@ const FILTER_COLUMNS: Record = { type: 'type', }; -const rowToAction = (row: { data: unknown }): Action => - toClass(row.data, Action.prototype) as Action; +const rowToAction = (row: { data: unknown }): Action => restorePush(row.data); + +const listFields = Object.entries(pushListProjection) + .filter(([, included]) => included) + .map(([field]) => `'${field}'`) + .join(', '); +const listData = `(SELECT COALESCE(jsonb_object_agg(field, data->field), '{}'::jsonb) + FROM unnest(ARRAY[${listFields}]) AS field WHERE data ? field)`; function bumpCount( m: Map, @@ -163,15 +170,12 @@ export const getPushesForUserProfile = async ( } const result = await query<{ data: unknown }>( - `SELECT data - 'steps' AS data FROM pushes WHERE type = 'push' AND ${predicate} ORDER BY timestamp DESC`, + `SELECT ${listData} AS data FROM pushes WHERE type = 'push' AND ${predicate} ORDER BY timestamp DESC`, values, ); return result.rows.map(rowToAction); }; -// List queries drop `steps` from the returned document: it holds the full diff -// (largest part of a push row) and the mongo backend's list projection excludes -// it as well. The push-detail path (`getPush`) still returns the whole document. export const getPushes = async (q: Partial = defaultPushQuery): Promise => { const clauses: string[] = []; const values: unknown[] = []; @@ -184,7 +188,7 @@ export const getPushes = async (q: Partial = defaultPushQuery): Promi const where = clauses.length > 0 ? `WHERE ${clauses.join(' AND ')}` : ''; const result = await query<{ data: unknown }>( - `SELECT data - 'steps' AS data FROM pushes ${where} ORDER BY timestamp DESC`, + `SELECT ${listData} AS data FROM pushes ${where} ORDER BY timestamp DESC`, values, ); return result.rows.map(rowToAction); @@ -207,7 +211,7 @@ const buildAuditUpsert = (action: Action): { text: string; values: unknown[] } = // Round-trip through JSON to drop class identity / mongo-specific _id fields // before persisting (mirrors mongo's `JSON.parse(JSON.stringify(action))`). - const data = JSON.parse(JSON.stringify(action)); + const data = JSON.parse(JSON.stringify(compactPush(action))); delete data._id; return { diff --git a/test/db/postgres/pushStorage.integration.test.ts b/test/db/postgres/pushStorage.integration.test.ts new file mode 100644 index 000000000..3430417e4 --- /dev/null +++ b/test/db/postgres/pushStorage.integration.test.ts @@ -0,0 +1,57 @@ +/** + * Copyright 2026 GitProxy Contributors + * + * Licensed under the Apache License, Version 2.0 (the "License"); + * you may not use this file except in compliance with the License. + * You may obtain a copy of the License at + * + * http://www.apache.org/licenses/LICENSE-2.0 + * + * Unless required by applicable law or agreed to in writing, software + * distributed under the License is distributed on an "AS IS" BASIS, + * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. + * See the License for the specific language governing permissions and + * limitations under the License. + */ + +import { describe, expect, it } from 'vitest'; +import * as pushes from '../../../src/db/postgres/pushes'; +import { definePushStorageContract } from '../pushStorage.contract'; +import { query } from '../../../src/db/postgres/helper'; +import { Action, RequestType, Step } from '../../../src/proxy/actions'; + +describe.runIf(process.env.RUN_POSTGRES_TESTS === 'true')('PostgreSQL storage contract', () => { + definePushStorageContract('PostgreSQL push storage', pushes, async (id) => { + const result = await query<{ data: unknown }>('SELECT data FROM pushes WHERE id = $1', [id]); + return result.rows[0]?.data; + }); + + it('reads historical final steps without rewriting the stored audit', async () => { + const action = new Action('legacy', RequestType.PUSH, 'POST', 123, 'https://example.com/a/b'); + action.addStep(new Step('diff', false, null, false, null, 'legacy diff')); + const data = JSON.parse(JSON.stringify(action)); + await query('INSERT INTO pushes (id, timestamp, type, data) VALUES ($1, $2, $3, $4)', [ + action.id, + action.timestamp, + action.type, + data, + ]); + expect(await pushes.getPush(action.id)).toMatchObject(data); + expect( + (await query('SELECT data FROM pushes WHERE id = $1', [action.id])).rows[0].data, + ).toEqual(data); + const [summary] = await pushes.getPushes({}); + expect(summary).not.toHaveProperty('lastStep'); + expect(summary).not.toHaveProperty('steps'); + }); + + it('preserves explicit null metadata while omitting absent fields', async () => { + const action = new Action('metadata', RequestType.PUSH, 'POST', 123, 'https://example.com/a/b'); + Object.assign(action, { author: null }); + delete action.rejection; + await pushes.writeAudit(action); + const [summary] = await pushes.getPushes({}); + expect(summary).toHaveProperty('author', null); + expect(summary).not.toHaveProperty('rejection'); + }); +}); diff --git a/test/db/postgres/pushes.test.ts b/test/db/postgres/pushes.test.ts index 5a91a5eab..1780557b5 100644 --- a/test/db/postgres/pushes.test.ts +++ b/test/db/postgres/pushes.test.ts @@ -15,6 +15,7 @@ */ import { describe, it, expect, beforeEach, vi } from 'vitest'; +import { Action, RequestType, Step } from '../../../src/proxy/actions'; const mockQuery = vi.fn(); @@ -70,7 +71,7 @@ describe('PostgreSQL - Pushes', async () => { await getPushes({ id: 'x' } as never); const [sql, params] = mockQuery.mock.calls[0]; - expect(sql).not.toContain('WHERE'); + expect(sql).toMatch(/FROM pushes\s+ORDER BY timestamp DESC$/); expect(params).toEqual([]); }); }); @@ -83,6 +84,27 @@ describe('PostgreSQL - Pushes', async () => { }); describe('writeAudit', () => { + it('compacts the final step and restores it on detail reads', async () => { + const action = new Action( + 'compact', + RequestType.PUSH, + 'POST', + 123, + 'https://example.com/a/b', + ); + action.addStep(new Step('diff', false, null, false, null, 'reviewable diff')); + mockQuery.mockResolvedValue({ rowCount: 1, rows: [] }); + await writeAudit(action); + const stored = JSON.parse(mockQuery.mock.calls[0][1][9]); + expect(stored).not.toHaveProperty('lastStep'); + expect(stored._lastStepIndex).toBe(0); + mockQuery.mockResolvedValue({ rowCount: 1, rows: [{ data: stored }] }); + const detail = await getPush(action.id); + expect(detail?.lastStep).toEqual(action.lastStep); + expect(detail).not.toHaveProperty('_lastStepIndex'); + expect(action.lastStep?.content).toBe('reviewable diff'); + }); + it('throws Invalid id when id is not a string', async () => { const action = { id: 42, timestamp: 1 } as unknown as Parameters[0]; await expect(writeAudit(action)).rejects.toThrow('Invalid id'); @@ -222,7 +244,7 @@ describe('PostgreSQL - Pushes', async () => { }); describe('list projection', () => { - it('drops steps from list results but not from the detail view', async () => { + it('selects metadata fields in SQL while detail reads retain the entire document', async () => { mockQuery.mockResolvedValue({ rowCount: 0, rows: [] }); await getPushes({}); @@ -232,9 +254,15 @@ describe('PostgreSQL - Pushes', async () => { const [listSql] = mockQuery.mock.calls[0]; const [profileSql] = mockQuery.mock.calls[1]; const [detailSql] = mockQuery.mock.calls[2]; - expect(listSql).toContain("data - 'steps'"); - expect(profileSql).toContain("data - 'steps'"); - expect(detailSql).not.toContain("data - 'steps'"); + for (const sql of [listSql, profileSql]) { + expect(sql).toContain('jsonb_object_agg'); + expect(sql).toContain("'tagData'"); + expect(sql).toContain("'rejection'"); + expect(sql).not.toContain("'steps'"); + expect(sql).not.toContain("'lastStep'"); + expect(sql).not.toContain("'diff'"); + } + expect(detailSql).toBe('SELECT data FROM pushes WHERE id = $1'); }); }); @@ -247,7 +275,7 @@ describe('PostgreSQL - Pushes', async () => { const [sql, params] = mockQuery.mock.calls[0]; expect(sql).toContain("data->'attestation'->'reviewer'->>'username'"); expect(sql).toMatch(/ORDER BY timestamp DESC/); - expect(sql).not.toContain('userEmail'); + expect(sql).not.toContain("(data->>'userEmail') = ANY"); expect(params).toEqual(['Alice']); }); From d6f44ab0e0bcecf70be0d43d12c106900c25b248 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Denis=20=C4=86ori=C4=87?= Date: Tue, 6 Oct 2026 12:29:25 +0200 Subject: [PATCH 16/18] fix(db): preserve MongoDB unset literal types MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Signed-off-by: Denis Ćorić --- src/db/mongo/pushes.ts | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) diff --git a/src/db/mongo/pushes.ts b/src/db/mongo/pushes.ts index 84bdd89cf..3dd0d193a 100644 --- a/src/db/mongo/pushes.ts +++ b/src/db/mongo/pushes.ts @@ -159,7 +159,8 @@ export const writeAudit = async (action: Action): Promise => { if (typeof data.id !== 'string') { throw new Error('Invalid id'); } - const unset = data._lastStepIndex === undefined ? { _lastStepIndex: '' } : { lastStep: '' }; + const unset: Record = + data._lastStepIndex === undefined ? { _lastStepIndex: '' } : { lastStep: '' }; await collection.updateOne({ id: data.id }, { $set: data, $unset: unset }, options); }; From a47440c3a563541715aa6bcea1c62cea44b6ba4e Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Denis=20=C4=86ori=C4=87?= Date: Tue, 13 Oct 2026 15:04:33 +0200 Subject: [PATCH 17/18] fix(db): trim push summary metadata MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Signed-off-by: Denis Ćorić --- src/db/pushProjection.ts | 4 ---- .../postgres/pushStorage.integration.test.ts | 4 ++-- test/db/pushStorage.contract.ts | 22 +++++++++++++++++-- 3 files changed, 22 insertions(+), 8 deletions(-) diff --git a/src/db/pushProjection.ts b/src/db/pushProjection.ts index 0b08801c1..e3b7905b0 100644 --- a/src/db/pushProjection.ts +++ b/src/db/pushProjection.ts @@ -26,7 +26,6 @@ export const pushListProjection = { actionType: 1, allowPush: 1, attestation: 1, - author: 1, authorised: 1, autoApproved: 1, autoRejected: 1, @@ -38,13 +37,10 @@ export const pushListProjection = { commitFrom: 1, commitTo: 1, error: 1, - errorMessage: 1, id: 1, legacyId: 1, - message: 1, method: 1, project: 1, - protocol: 1, pusherVerified: 1, rejected: 1, rejection: 1, diff --git a/test/db/postgres/pushStorage.integration.test.ts b/test/db/postgres/pushStorage.integration.test.ts index 3430417e4..3f9305f73 100644 --- a/test/db/postgres/pushStorage.integration.test.ts +++ b/test/db/postgres/pushStorage.integration.test.ts @@ -47,11 +47,11 @@ describe.runIf(process.env.RUN_POSTGRES_TESTS === 'true')('PostgreSQL storage co it('preserves explicit null metadata while omitting absent fields', async () => { const action = new Action('metadata', RequestType.PUSH, 'POST', 123, 'https://example.com/a/b'); - Object.assign(action, { author: null }); + Object.assign(action, { blockedMessage: null }); delete action.rejection; await pushes.writeAudit(action); const [summary] = await pushes.getPushes({}); - expect(summary).toHaveProperty('author', null); + expect(summary).toHaveProperty('blockedMessage', null); expect(summary).not.toHaveProperty('rejection'); }); }); diff --git a/test/db/pushStorage.contract.ts b/test/db/pushStorage.contract.ts index 96928fbbe..b4556e3eb 100644 --- a/test/db/pushStorage.contract.ts +++ b/test/db/pushStorage.contract.ts @@ -95,7 +95,13 @@ export const definePushStorageContract = ( it('keeps legacy audit content on detail reads while excluding every diff copy from lists', async () => { const action = makePush(); // Future top-level diff records from #1693 must also remain outside list responses. - Object.assign(action, { diff, pluginPrivateData: diff }); + const detailOnly = { + author: 'audit author', + message: diff, + errorMessage: diff, + protocol: 'http', + }; + Object.assign(action, { diff, pluginPrivateData: diff, ...detailOnly }); await sink.writeAudit(action); const summaries = await sink.getPushes({ type: 'push' }); @@ -109,12 +115,24 @@ export const definePushStorageContract = ( commitTo: action.commitTo, blocked: true, }); - for (const field of ['steps', 'lastStep', 'diff', '_id', 'pluginPrivateData']) { + for (const field of [ + 'steps', + 'lastStep', + 'diff', + '_id', + 'pluginPrivateData', + ...Object.keys(detailOnly), + ]) { expect(summary).not.toHaveProperty(field); } + const profile = await sink.getPushesForUserProfile(['author@example.com'], 'reviewer'); + for (const field of Object.keys(detailOnly)) { + expect(profile.find((push) => push.id === action.id)).not.toHaveProperty(field); + } expect(JSON.stringify(summary)).not.toContain('private payload'); const detail = await sink.getPush(action.id); + expect(detail).toMatchObject(detailOnly); expect(detail?.steps[0].content).toBe(diff); expect(detail?.steps[0].logs).toEqual([`diff - ${diff}`]); expect(detail?.lastStep?.content).toBe(diff); From 2fa28bf61157b31d1964f3d3b89beea7b04287cc Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Denis=20=C4=86ori=C4=87?= Date: Sat, 7 Nov 2026 22:53:52 +0100 Subject: [PATCH 18/18] docs(db): explain compactPush and restorePush --- src/db/pushStorage.ts | 16 ++++++++++++++++ 1 file changed, 16 insertions(+) diff --git a/src/db/pushStorage.ts b/src/db/pushStorage.ts index d9e99f0ec..16dfc1b89 100644 --- a/src/db/pushStorage.ts +++ b/src/db/pushStorage.ts @@ -18,8 +18,17 @@ import { isEqual } from 'lodash'; import { Action } from '../proxy/actions'; import { toClass } from './helper'; +// Internal storage markers that may exist on persisted records but must never +// reach callers of the database layer. type StoredPush = Action & { _lastStepIndex?: number; _activity?: unknown }; +/** + * Prepares a push for storage. Works on a shallow copy, so the caller's Action + * is not modified. Any internal markers already on the input are dropped + * first. When `lastStep` is identical to the final entry in `steps`, the + * duplicate copy is replaced with `_lastStepIndex`, a reference to that entry. + * A `lastStep` that differs from the final step is stored as-is. + */ export const compactPush = (action: Action) => { const stored: Partial = { ...action }; delete stored._lastStepIndex; @@ -32,6 +41,13 @@ export const compactPush = (action: Action) => { return stored; }; +/** + * Reverses compactPush when reading a stored record back into an Action. If + * the record holds a valid `_lastStepIndex` and no `lastStep`, `lastStep` is + * rebuilt from that entry in `steps`. Records that store `lastStep` directly + * (a distinct last step, or historical records) keep it as stored. Internal + * markers are always removed from the returned Action. + */ export const restorePush = (doc: unknown): Action => { const action: StoredPush = toClass(doc, Action.prototype); const index = action._lastStepIndex;