Skip to content

Commit 9eb42ac

Browse files
coopernetesclaude
coopernetes
and
claude
committed
perf: replace N+1 hydration with SQL aggregates on active repos and users pages
Eliminates three N+1 query patterns identified in #247: 1. Active Repos page: replace pushStore.find(limit=5000) + per-record hydration with a single GROUP BY aggregate via PushStore.summarizeByRepo(). JdbcPushStore implements this as one SQL query; default interface implementation falls back to find() for InMemory/Mongo. 2. Users list: replace per-user countPushesByStatus() loop (1 + N×3 queries) with a single aggregate via PushStore.countPushStatusByUser(). UserController.list() loads all push status counts in one query and distributes them to UserSummary objects without touching push records. toDetail() now uses PushStore.countByStatus(PushQuery) instead of fetching and hydrating all push records for the user. 3. Push page status counts: add GET /api/push/counts endpoint that returns a Map<String, Long> from a single GROUP BY query. Accepts the same filter params as the list endpoint (repo, user, search) so tab counts reflect the current filter set. Updates PushList.tsx to call this endpoint instead of firing one request per status value. Also extracts the WHERE clause builder in JdbcPushStore into a shared private method reused by find(), countByStatus(), and the existing query path. closes #247 Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
1 parent 429339e commit 9eb42ac

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)