Skip to content

Commit 27c031e

Browse files
coopernetescoopernetes
authored
perf: replace N+1 hydration with SQL aggregates on active repos and users pages (#253)
## Summary - **Active Repos page**: replaces `pushStore.find(limit=5000)` + per-record hydration with a single `GROUP BY` aggregate (`PushStore.summarizeByRepo()`). At 130 records × 3 child queries × 8ms RTT this was ~3s; now one query. - **Users list**: replaces the per-user `countPushesByStatus()` loop (1 + N×3 queries) with a single `SELECT resolved_user, status, COUNT(*) GROUP BY` via `PushStore.countPushStatusByUser()`. `GET /api/users/{username}` also updated via `countByStatus(query)`. - **Push page status counts**: adds `GET /api/push/counts` returning `Map<String, Long>` from one `GROUP BY status` query. `PushList.tsx` previously fired 7 separate fully-hydrated requests (one per status) just to get `.length`; now one lightweight call. Counts also update when repo/user filters change. The WHERE clause builder in `JdbcPushStore` was extracted into a shared private method reused by `find()` and `countByStatus()`. Default interface implementations on `PushStore` keep `InMemoryPushStore` and `MongoPushStore` correct without changes. ## Test plan - [ ] Existing `PushControllerTest`, `RepoControllerTest` updated and passing - [ ] New `PushControllerTest$Counts` tests cover the `/api/push/counts` endpoint - [ ] Full build + coverage threshold passes locally - [ ] Manual: Active Repos, Users, and Pushes pages load noticeably faster against a remote DB closes #247 🤖 Generated with [Claude Code](https://claude.com/claude-code)
2 parents fa4af73 + 9eb42ac commit 27c031e

9 files changed

Lines changed: 284 additions & 163 deletions

File tree

‎git-proxy-java-core/src/main/java/org/finos/gitproxy/db/PushStore.java‎

Lines changed: 62 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -1,6 +1,8 @@
11
package org.finos.gitproxy.db;
22

3+
import java.util.HashMap;
34
import java.util.List;
5+
import java.util.Map;
46
import java.util.Optional;
57
import org.finos.gitproxy.db.model.Attestation;
68
import org.finos.gitproxy.db.model.PushQuery;
@@ -66,4 +68,64 @@ public interface PushStore {
6668

6769
/** Close resources. Called on shutdown. */
6870
default void close() {}
71+
72+
/**
73+
* Summarise push activity grouped by provider + project + repo_name. Returns one entry per distinct repo with total
74+
* push count. Default implementation is correct but unoptimised; JDBC override uses a SQL aggregate.
75+
*/
76+
default List<RepoPushSummary> summarizeByRepo() {
77+
record Key(String provider, String owner, String repoName) {}
78+
Map<Key, Long> counts = new java.util.LinkedHashMap<>();
79+
for (PushRecord r : find(PushQuery.builder().limit(Integer.MAX_VALUE).build())) {
80+
Key k = new Key(
81+
r.getProvider() != null ? r.getProvider() : "",
82+
r.getProject() != null ? r.getProject() : "",
83+
r.getRepoName() != null ? r.getRepoName() : "");
84+
counts.merge(k, 1L, Long::sum);
85+
}
86+
return counts.entrySet().stream()
87+
.map(e -> new RepoPushSummary(
88+
e.getKey().provider(), e.getKey().owner(), e.getKey().repoName(), e.getValue()))
89+
.toList();
90+
}
91+
92+
/**
93+
* Count push records for a user grouped by status. Accepts the same filters as {@link #find(PushQuery)} except
94+
* {@code status} is ignored — counts are returned for all statuses. Default implementation is correct but
95+
* unoptimised; JDBC override uses a SQL aggregate.
96+
*/
97+
default Map<String, Long> countByStatus(PushQuery query) {
98+
PushQuery noStatus = PushQuery.builder()
99+
.project(query.getProject())
100+
.repoName(query.getRepoName())
101+
.branch(query.getBranch())
102+
.user(query.getUser())
103+
.authorEmail(query.getAuthorEmail())
104+
.commitTo(query.getCommitTo())
105+
.search(query.getSearch())
106+
.limit(Integer.MAX_VALUE)
107+
.build();
108+
Map<String, Long> result = new HashMap<>();
109+
for (PushRecord r : find(noStatus)) {
110+
result.merge(r.getStatus().name(), 1L, Long::sum);
111+
}
112+
return result;
113+
}
114+
115+
/**
116+
* Aggregate push status counts for all users in a single pass, returning a map of username → (status → count).
117+
* Default implementation is correct but unoptimised; JDBC override uses a SQL aggregate.
118+
*/
119+
default Map<String, Map<String, Long>> countPushStatusByUser() {
120+
Map<String, Map<String, Long>> result = new HashMap<>();
121+
for (PushRecord r : find(PushQuery.builder().limit(Integer.MAX_VALUE).build())) {
122+
if (r.getResolvedUser() == null) continue;
123+
result.computeIfAbsent(r.getResolvedUser(), k -> new HashMap<>())
124+
.merge(r.getStatus().name(), 1L, Long::sum);
125+
}
126+
return result;
127+
}
128+
129+
/** Per-repo aggregate returned by {@link #summarizeByRepo()}. */
130+
record RepoPushSummary(String provider, String owner, String repoName, long total) {}
69131
}

‎git-proxy-java-core/src/main/java/org/finos/gitproxy/db/jdbc/JdbcPushStore.java‎

Lines changed: 105 additions & 40 deletions
Original file line numberDiff line numberDiff line change
@@ -3,6 +3,7 @@
33
import java.sql.Timestamp;
44
import java.util.ArrayList;
55
import java.util.HashMap;
6+
import java.util.LinkedHashMap;
67
import java.util.List;
78
import java.util.Map;
89
import java.util.Optional;
@@ -15,6 +16,7 @@
1516
import org.finos.gitproxy.db.model.*;
1617
import org.slf4j.Logger;
1718
import org.slf4j.LoggerFactory;
19+
import org.springframework.jdbc.core.RowCallbackHandler;
1820
import org.springframework.jdbc.core.namedparam.MapSqlParameterSource;
1921
import org.springframework.jdbc.core.namedparam.NamedParameterJdbcTemplate;
2022
import org.springframework.jdbc.datasource.DataSourceTransactionManager;
@@ -81,54 +83,73 @@ public Optional<PushRecord> findById(String id) {
8183

8284
@Override
8385
public List<PushRecord> find(PushQuery query) {
84-
StringBuilder sql = new StringBuilder("SELECT * FROM push_records WHERE 1=1");
8586
MapSqlParameterSource params = new MapSqlParameterSource();
86-
87-
if (query.getStatus() != null) {
88-
sql.append(" AND status = :status");
89-
params.addValue("status", query.getStatus().name());
90-
}
91-
if (query.getProject() != null) {
92-
sql.append(" AND project = :project");
93-
params.addValue("project", query.getProject());
94-
}
95-
if (query.getRepoName() != null) {
96-
sql.append(" AND repo_name = :repoName");
97-
params.addValue("repoName", query.getRepoName());
98-
}
99-
if (query.getBranch() != null) {
100-
sql.append(" AND branch = :branch");
101-
params.addValue("branch", query.getBranch());
102-
}
103-
if (query.getCommitTo() != null) {
104-
sql.append(" AND commit_to = :commitTo");
105-
params.addValue("commitTo", query.getCommitTo());
106-
}
107-
if (query.getUser() != null) {
108-
sql.append(" AND resolved_user = :user");
109-
params.addValue("user", query.getUser());
110-
}
111-
if (query.getAuthorEmail() != null) {
112-
sql.append(" AND author_email = :authorEmail");
113-
params.addValue("authorEmail", query.getAuthorEmail());
114-
}
115-
if (query.getSearch() != null && !query.getSearch().isBlank()) {
116-
sql.append(
117-
" AND (LOWER(provider) LIKE :search OR LOWER(project) LIKE :search OR LOWER(repo_name) LIKE :search)");
118-
params.addValue("search", "%" + query.getSearch().toLowerCase() + "%");
119-
}
120-
121-
sql.append(" ORDER BY timestamp ");
122-
sql.append(query.isNewestFirst() ? "DESC" : "ASC");
123-
sql.append(" LIMIT :limit OFFSET :offset");
87+
String where = buildWhere(query, params);
88+
String sql = "SELECT * FROM push_records" + where
89+
+ " ORDER BY timestamp " + (query.isNewestFirst() ? "DESC" : "ASC")
90+
+ " LIMIT :limit OFFSET :offset";
12491
params.addValue("limit", query.getLimit());
12592
params.addValue("offset", query.getOffset());
12693

127-
List<PushRecord> results = jdbc.query(sql.toString(), params, PushRecordRowMapper.INSTANCE);
94+
List<PushRecord> results = jdbc.query(sql, params, PushRecordRowMapper.INSTANCE);
12895
results.forEach(this::hydrate);
12996
return results;
13097
}
13198

99+
@Override
100+
public List<RepoPushSummary> summarizeByRepo() {
101+
return jdbc.query(
102+
"""
103+
SELECT provider, project, repo_name, COUNT(*) AS total
104+
FROM push_records
105+
GROUP BY provider, project, repo_name
106+
ORDER BY total DESC
107+
""",
108+
(rs, rowNum) -> new RepoPushSummary(
109+
rs.getString("provider"),
110+
rs.getString("project"),
111+
rs.getString("repo_name"),
112+
rs.getLong("total")));
113+
}
114+
115+
@Override
116+
public Map<String, Long> countByStatus(PushQuery query) {
117+
MapSqlParameterSource params = new MapSqlParameterSource();
118+
// Strip status so we get counts for all statuses
119+
PushQuery noStatus = PushQuery.builder()
120+
.project(query.getProject())
121+
.repoName(query.getRepoName())
122+
.branch(query.getBranch())
123+
.user(query.getUser())
124+
.authorEmail(query.getAuthorEmail())
125+
.commitTo(query.getCommitTo())
126+
.search(query.getSearch())
127+
.build();
128+
String where = buildWhere(noStatus, params);
129+
Map<String, Long> result = new LinkedHashMap<>();
130+
jdbc.query(
131+
"SELECT status, COUNT(*) AS total FROM push_records" + where + " GROUP BY status",
132+
params,
133+
(RowCallbackHandler) rs -> result.put(rs.getString("status"), rs.getLong("total")));
134+
return result;
135+
}
136+
137+
@Override
138+
public Map<String, Map<String, Long>> countPushStatusByUser() {
139+
Map<String, Map<String, Long>> result = new LinkedHashMap<>();
140+
jdbc.query("""
141+
SELECT resolved_user, status, COUNT(*) AS total
142+
FROM push_records
143+
WHERE resolved_user IS NOT NULL
144+
GROUP BY resolved_user, status
145+
""", rs -> {
146+
String user = rs.getString("resolved_user");
147+
String status = rs.getString("status");
148+
result.computeIfAbsent(user, k -> new LinkedHashMap<>()).put(status, rs.getLong("total"));
149+
});
150+
return result;
151+
}
152+
132153
@Override
133154
public void delete(String id) {
134155
// CASCADE handles child tables
@@ -179,6 +200,50 @@ public void close() {
179200

180201
// --- Private helpers ---
181202

203+
/**
204+
* Builds a SQL WHERE clause from the query and populates {@code params}. Returns the clause string starting with
205+
* {@code " WHERE 1=1 ..."} (or just {@code " WHERE 1=1"} if no filters are set).
206+
*/
207+
private static String buildWhere(PushQuery query, MapSqlParameterSource params) {
208+
StringBuilder sql = new StringBuilder(" WHERE 1=1");
209+
210+
if (query.getStatus() != null) {
211+
sql.append(" AND status = :status");
212+
params.addValue("status", query.getStatus().name());
213+
}
214+
if (query.getProject() != null) {
215+
sql.append(" AND project = :project");
216+
params.addValue("project", query.getProject());
217+
}
218+
if (query.getRepoName() != null) {
219+
sql.append(" AND repo_name = :repoName");
220+
params.addValue("repoName", query.getRepoName());
221+
}
222+
if (query.getBranch() != null) {
223+
sql.append(" AND branch = :branch");
224+
params.addValue("branch", query.getBranch());
225+
}
226+
if (query.getCommitTo() != null) {
227+
sql.append(" AND commit_to = :commitTo");
228+
params.addValue("commitTo", query.getCommitTo());
229+
}
230+
if (query.getUser() != null) {
231+
sql.append(" AND resolved_user = :user");
232+
params.addValue("user", query.getUser());
233+
}
234+
if (query.getAuthorEmail() != null) {
235+
sql.append(" AND author_email = :authorEmail");
236+
params.addValue("authorEmail", query.getAuthorEmail());
237+
}
238+
if (query.getSearch() != null && !query.getSearch().isBlank()) {
239+
sql.append(
240+
" AND (LOWER(provider) LIKE :search OR LOWER(project) LIKE :search OR LOWER(repo_name) LIKE :search)");
241+
params.addValue("search", "%" + query.getSearch().toLowerCase() + "%");
242+
}
243+
244+
return sql.toString();
245+
}
246+
182247
private PushRecord updateStatus(String id, PushStatus status, Attestation attestation) {
183248
tx.executeWithoutResult(txStatus -> {
184249
int updated = jdbc.update(

‎git-proxy-java-dashboard/frontend/src/api.ts‎

Lines changed: 6 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -34,6 +34,12 @@ export async function fetchPushes(params: URLSearchParams) {
3434
return res.json()
3535
}
3636

37+
export async function fetchPushCounts(params: URLSearchParams): Promise<Record<string, number>> {
38+
const res = await apiFetch('/api/push/counts?' + params)
39+
if (!res.ok) throw new Error('Failed to fetch push counts')
40+
return res.json()
41+
}
42+
3743
export async function fetchPush(id: string) {
3844
const url = id.includes('_') ? `/api/push/by-ref/${id}` : `/api/push/${id}`
3945
const res = await apiFetch(url)

‎git-proxy-java-dashboard/frontend/src/pages/PushList.tsx‎

Lines changed: 19 additions & 15 deletions
Original file line numberDiff line numberDiff line change
@@ -1,6 +1,6 @@
11
import { useCallback, useEffect, useRef, useState } from 'react'
22
import { useNavigate } from 'react-router-dom'
3-
import { approvePush, fetchPushes, rejectPush } from '../api'
3+
import { approvePush, fetchPushCounts, fetchPushes, rejectPush } from '../api'
44
import { StatusBadge } from '../components/StatusBadge'
55
import type { CurrentUser, PushRecord, PushStatus } from '../types'
66

@@ -142,17 +142,16 @@ export function PushList({ currentUser }: PushListProps) {
142142
[currentUser],
143143
)
144144

145-
const loadCounts = useCallback(async () => {
146-
const results: Partial<Record<string, number>> = {}
147-
await Promise.all(
148-
STATUSES.map(async (s) => {
149-
const params = new URLSearchParams({ status: s, limit: '1000' })
150-
const data: PushRecord[] = await fetchPushes(params)
151-
results[s] = data.length
152-
}),
153-
)
154-
setCounts(results)
155-
}, [])
145+
const loadCounts = useCallback(
146+
async (search: string, myOnly: boolean) => {
147+
const params = new URLSearchParams()
148+
if (search) params.set('search', search)
149+
if (myOnly && currentUser?.username) params.set('user', currentUser.username)
150+
const data = await fetchPushCounts(params)
151+
setCounts(data)
152+
},
153+
[currentUser],
154+
)
156155

157156
// Clear selection when leaving PENDING filter
158157
useEffect(() => {
@@ -169,10 +168,15 @@ export function PushList({ currentUser }: PushListProps) {
169168
return () => clearInterval(timer)
170169
}, [filterStatus, filterRepo, myPushesOnly, newestFirst, page, load])
171170

172-
// Load counts once on mount
171+
// Reload counts when filter-relevant state changes, with auto-refresh
173172
useEffect(() => {
174-
void Promise.resolve().then(() => loadCounts())
175-
}, [loadCounts])
173+
void Promise.resolve().then(() => loadCounts(filterRepo, myPushesOnly))
174+
const timer = setInterval(
175+
() => loadCounts(filterRepo, myPushesOnly),
176+
10_000,
177+
)
178+
return () => clearInterval(timer)
179+
}, [filterRepo, myPushesOnly, loadCounts])
176180

177181
function handleRepoChange(value: string) {
178182
setFilterRepo(value)

‎git-proxy-java-dashboard/src/main/java/org/finos/gitproxy/dashboard/controller/PushController.java‎

Lines changed: 21 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -85,6 +85,27 @@ public List<PushRecord> list(
8585
return pushStore.find(query.build());
8686
}
8787

88+
/**
89+
* Count push records grouped by status. Accepts the same filter params as {@link #list} except {@code status} —
90+
* returns counts for all statuses so the caller can populate all filter tabs in one round trip.
91+
*/
92+
@Operation(operationId = "countPushes", summary = "Count push records by status")
93+
@GetMapping("/counts")
94+
public Map<String, Long> counts(
95+
@RequestParam(required = false) String project,
96+
@RequestParam(required = false) String repo,
97+
@RequestParam(required = false) String user,
98+
@RequestParam(required = false) String search) {
99+
100+
PushQuery.PushQueryBuilder query = PushQuery.builder();
101+
if (project != null && !project.isBlank()) query.project(project);
102+
if (repo != null && !repo.isBlank()) query.repoName(repo);
103+
if (user != null && !user.isBlank()) query.user(user);
104+
if (search != null && !search.isBlank()) query.search(search);
105+
106+
return pushStore.countByStatus(query.build());
107+
}
108+
88109
/**
89110
* Look up a push record by its commit reference ({commitFrom}_{commitTo}). Used by the transparent proxy flow where
90111
* we link to a push before it has been saved with a UUID.

0 commit comments

Comments
 (0)