Skip to content

Commit f22f537

Browse files
committed
docs: justify postgres behaviors by backend parity, not the issue
Review feedback on #1532: the codebase is the source of truth, so comments should argue from the existing mongo/NeDB behavior rather than cite the originating issue. Drops the stale repo_users TODO (implemented on a follow-up branch) and the issue references in code comments, CI config and test names.
1 parent afde6fc commit f22f537

7 files changed

Lines changed: 10 additions & 22 deletions

File tree

.github/workflows/ci.yml

Lines changed: 3 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -22,10 +22,9 @@ jobs:
2222
node-version: [22.x, 24.x]
2323
mongodb-version: ['6.0', '7.0', '8.0']
2424

25-
# PostgreSQL service container for the postgres integration tests added in
26-
# issue #1497. A single version (postgres:16) is sufficient for the initial
27-
# CI lane per the issue's "Open Questions"; a broader version matrix can
28-
# follow later.
25+
# PostgreSQL service container for the postgres integration tests. A
26+
# single version (postgres:16) keeps the lane fast; a broader version
27+
# matrix can follow later if needed.
2928
services:
3029
postgres:
3130
image: postgres:16

src/db/postgres/helper.ts

Lines changed: 1 addition & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -130,8 +130,7 @@ export const resetConnection = async (): Promise<void> => {
130130
* IMPORTANT: this function MUST NOT silently return undefined when Postgres is
131131
* the active sink — that would cause express-session to fall back to its
132132
* default in-memory store, which loses sessions on every restart and is unsafe
133-
* in any multi-process deployment. Issue #1497 calls this out as a must-fix
134-
* requirement, so we throw loudly instead.
133+
* in any multi-process deployment. Throw loudly instead.
135134
*/
136135
export const getSessionStore = (): Store => {
137136
const connectionString = getDatabase().connectionString;

src/db/postgres/repo.ts

Lines changed: 1 addition & 10 deletions
Original file line numberDiff line numberDiff line change
@@ -14,11 +14,6 @@
1414
* limitations under the License.
1515
*/
1616

17-
// TODO(#1497-followup): consider normalizing repo permissions into a
18-
// repo_users(repo_id, user, role) join table. JSONB is used for v1 to
19-
// match the mongo/fs shape and minimize migration churn — the issue
20-
// flags this as an open question for a follow-up PR.
21-
2217
import { Repo, RepoQuery } from '../types';
2318
import { query } from './helper';
2419

@@ -100,9 +95,6 @@ export const createRepo = async (repo: Repo): Promise<Repo> => {
10095
* Append a user to one of the JSONB permission arrays. The query is a
10196
* read-modify-write that deduplicates the value, then re-serialises the array
10297
* so the stored shape matches the existing mongo/fs backends exactly.
103-
*
104-
* Crucially: when the last user is later removed, the array stays `[]` rather
105-
* than collapsing to `null` — issue #1497 explicitly requires this.
10698
*/
10799
const addUserToRole = async (
108100
_id: string,
@@ -137,8 +129,7 @@ const removeUserFromRole = async (
137129
role: 'canPush' | 'canAuthorise',
138130
): Promise<void> => {
139131
const lowered = user.toLowerCase();
140-
// The filter expression evaluates to `[]` if the last matching user is
141-
// removed — preserving the empty-array invariant from issue #1497.
132+
// The filter evaluates to `[]` if the last matching user is removed
142133
await query(
143134
`UPDATE repos
144135
SET users = jsonb_set(

src/service/index.ts

Lines changed: 1 addition & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -137,8 +137,7 @@ const corsOptions: cors.CorsOptions = {
137137
// Backend sink types that promise a persistent session store. If one of these
138138
// is active and getSessionStore() returns undefined, express-session would
139139
// silently fall back to MemoryStore — which loses sessions on restart and is
140-
// unsafe in any multi-process deployment. Issue #1497 calls this out as a
141-
// must-fix requirement, so we throw loudly instead.
140+
// unsafe in any multi-process deployment. Throw loudly instead.
142141
const PERSISTENT_SESSION_BACKENDS = new Set(['mongo', 'postgres']);
143142

144143
async function createApp(proxy: Proxy): Promise<Express> {

test/db/postgres/pushes.integration.test.ts

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -145,7 +145,7 @@ describe.runIf(shouldRunPostgresTests)('PostgreSQL Pushes Integration Tests', ()
145145
);
146146
});
147147

148-
it('orders pushes by timestamp DESC (issue #1497 must-fix)', async () => {
148+
it('orders pushes by timestamp DESC', async () => {
149149
const result = await getPushes({});
150150
const ids = result.map((p) => p.id);
151151
expect(ids).toEqual(['push-authorised', 'push-b', 'push-a']);

test/db/postgres/repo.integration.test.ts

Lines changed: 2 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -94,7 +94,7 @@ describe.runIf(shouldRunPostgresTests)('PostgreSQL Repo Integration Tests', () =
9494
});
9595
});
9696

97-
describe('permission JSONB — issue #1497 must-fix', () => {
97+
describe('permission JSONB', () => {
9898
it('starts with empty arrays', async () => {
9999
const created = await createRepo(
100100
createTestRepo({ name: 'perm-start', url: 'https://example.com/ps.git' }),
@@ -127,7 +127,7 @@ describe.runIf(shouldRunPostgresTests)('PostgreSQL Repo Integration Tests', () =
127127
await removeUserCanPush(id, 'bob');
128128

129129
const fromDb = await getRepoById(id);
130-
// This is the core invariant from issue #1497.
130+
// Same behavior as Mongo and NeDB
131131
expect(fromDb?.users.canPush).toEqual([]);
132132
expect(fromDb?.users.canPush).not.toBeNull();
133133
});

test/db/postgres/repo.test.ts

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -157,7 +157,7 @@ describe('PostgreSQL - Repo', async () => {
157157
});
158158
});
159159

160-
describe('add/remove user — empty array invariant (issue #1497)', () => {
160+
describe('add/remove user — empty array invariant', () => {
161161
it('lower-cases user on add', async () => {
162162
mockQuery.mockResolvedValue({ rowCount: 1, rows: [] });
163163
await addUserCanPush('r-1', 'Bob');

0 commit comments

Comments
 (0)