From 9eb42acf47286aa20df51a5cc37d30e737046b7f Mon Sep 17 00:00:00 2001 From: Thomas Cooper Date: Wed, 13 May 2026 00:43:41 -0400 Subject: [PATCH] perf: replace N+1 hydration with SQL aggregates on active repos and users pages MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit 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 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 --- .../java/org/finos/gitproxy/db/PushStore.java | 62 ++++++++ .../finos/gitproxy/db/jdbc/JdbcPushStore.java | 145 +++++++++++++----- git-proxy-java-dashboard/frontend/src/api.ts | 6 + .../frontend/src/pages/PushList.tsx | 34 ++-- .../dashboard/controller/PushController.java | 21 +++ .../dashboard/controller/RepoController.java | 45 ++---- .../dashboard/controller/UserController.java | 19 +-- .../controller/PushControllerTest.java | 33 ++++ .../controller/RepoControllerTest.java | 82 ++-------- 9 files changed, 284 insertions(+), 163 deletions(-) diff --git a/git-proxy-java-core/src/main/java/org/finos/gitproxy/db/PushStore.java b/git-proxy-java-core/src/main/java/org/finos/gitproxy/db/PushStore.java index 833259dd..4dd94a60 100644 --- a/git-proxy-java-core/src/main/java/org/finos/gitproxy/db/PushStore.java +++ b/git-proxy-java-core/src/main/java/org/finos/gitproxy/db/PushStore.java @@ -1,6 +1,8 @@ package org.finos.gitproxy.db; +import java.util.HashMap; import java.util.List; +import java.util.Map; import java.util.Optional; import org.finos.gitproxy.db.model.Attestation; import org.finos.gitproxy.db.model.PushQuery; @@ -66,4 +68,64 @@ public interface PushStore { /** Close resources. Called on shutdown. */ default void close() {} + + /** + * Summarise push activity grouped by provider + project + repo_name. Returns one entry per distinct repo with total + * push count. Default implementation is correct but unoptimised; JDBC override uses a SQL aggregate. + */ + default List summarizeByRepo() { + record Key(String provider, String owner, String repoName) {} + Map counts = new java.util.LinkedHashMap<>(); + for (PushRecord r : find(PushQuery.builder().limit(Integer.MAX_VALUE).build())) { + Key k = new Key( + r.getProvider() != null ? r.getProvider() : "", + r.getProject() != null ? r.getProject() : "", + r.getRepoName() != null ? r.getRepoName() : ""); + counts.merge(k, 1L, Long::sum); + } + return counts.entrySet().stream() + .map(e -> new RepoPushSummary( + e.getKey().provider(), e.getKey().owner(), e.getKey().repoName(), e.getValue())) + .toList(); + } + + /** + * Count push records for a user grouped by status. Accepts the same filters as {@link #find(PushQuery)} except + * {@code status} is ignored — counts are returned for all statuses. Default implementation is correct but + * unoptimised; JDBC override uses a SQL aggregate. + */ + default Map countByStatus(PushQuery query) { + PushQuery noStatus = PushQuery.builder() + .project(query.getProject()) + .repoName(query.getRepoName()) + .branch(query.getBranch()) + .user(query.getUser()) + .authorEmail(query.getAuthorEmail()) + .commitTo(query.getCommitTo()) + .search(query.getSearch()) + .limit(Integer.MAX_VALUE) + .build(); + Map result = new HashMap<>(); + for (PushRecord r : find(noStatus)) { + result.merge(r.getStatus().name(), 1L, Long::sum); + } + return result; + } + + /** + * Aggregate push status counts for all users in a single pass, returning a map of username → (status → count). + * Default implementation is correct but unoptimised; JDBC override uses a SQL aggregate. + */ + default Map> countPushStatusByUser() { + Map> result = new HashMap<>(); + for (PushRecord r : find(PushQuery.builder().limit(Integer.MAX_VALUE).build())) { + if (r.getResolvedUser() == null) continue; + result.computeIfAbsent(r.getResolvedUser(), k -> new HashMap<>()) + .merge(r.getStatus().name(), 1L, Long::sum); + } + return result; + } + + /** Per-repo aggregate returned by {@link #summarizeByRepo()}. */ + record RepoPushSummary(String provider, String owner, String repoName, long total) {} } diff --git a/git-proxy-java-core/src/main/java/org/finos/gitproxy/db/jdbc/JdbcPushStore.java b/git-proxy-java-core/src/main/java/org/finos/gitproxy/db/jdbc/JdbcPushStore.java index cb3d3145..c61e503a 100644 --- a/git-proxy-java-core/src/main/java/org/finos/gitproxy/db/jdbc/JdbcPushStore.java +++ b/git-proxy-java-core/src/main/java/org/finos/gitproxy/db/jdbc/JdbcPushStore.java @@ -3,6 +3,7 @@ import java.sql.Timestamp; import java.util.ArrayList; import java.util.HashMap; +import java.util.LinkedHashMap; import java.util.List; import java.util.Map; import java.util.Optional; @@ -15,6 +16,7 @@ import org.finos.gitproxy.db.model.*; import org.slf4j.Logger; import org.slf4j.LoggerFactory; +import org.springframework.jdbc.core.RowCallbackHandler; import org.springframework.jdbc.core.namedparam.MapSqlParameterSource; import org.springframework.jdbc.core.namedparam.NamedParameterJdbcTemplate; import org.springframework.jdbc.datasource.DataSourceTransactionManager; @@ -81,54 +83,73 @@ public Optional findById(String id) { @Override public List find(PushQuery query) { - StringBuilder sql = new StringBuilder("SELECT * FROM push_records WHERE 1=1"); MapSqlParameterSource params = new MapSqlParameterSource(); - - if (query.getStatus() != null) { - sql.append(" AND status = :status"); - params.addValue("status", query.getStatus().name()); - } - if (query.getProject() != null) { - sql.append(" AND project = :project"); - params.addValue("project", query.getProject()); - } - if (query.getRepoName() != null) { - sql.append(" AND repo_name = :repoName"); - params.addValue("repoName", query.getRepoName()); - } - if (query.getBranch() != null) { - sql.append(" AND branch = :branch"); - params.addValue("branch", query.getBranch()); - } - if (query.getCommitTo() != null) { - sql.append(" AND commit_to = :commitTo"); - params.addValue("commitTo", query.getCommitTo()); - } - if (query.getUser() != null) { - sql.append(" AND resolved_user = :user"); - params.addValue("user", query.getUser()); - } - if (query.getAuthorEmail() != null) { - sql.append(" AND author_email = :authorEmail"); - params.addValue("authorEmail", query.getAuthorEmail()); - } - if (query.getSearch() != null && !query.getSearch().isBlank()) { - sql.append( - " AND (LOWER(provider) LIKE :search OR LOWER(project) LIKE :search OR LOWER(repo_name) LIKE :search)"); - params.addValue("search", "%" + query.getSearch().toLowerCase() + "%"); - } - - sql.append(" ORDER BY timestamp "); - sql.append(query.isNewestFirst() ? "DESC" : "ASC"); - sql.append(" LIMIT :limit OFFSET :offset"); + String where = buildWhere(query, params); + String sql = "SELECT * FROM push_records" + where + + " ORDER BY timestamp " + (query.isNewestFirst() ? "DESC" : "ASC") + + " LIMIT :limit OFFSET :offset"; params.addValue("limit", query.getLimit()); params.addValue("offset", query.getOffset()); - List results = jdbc.query(sql.toString(), params, PushRecordRowMapper.INSTANCE); + List results = jdbc.query(sql, params, PushRecordRowMapper.INSTANCE); results.forEach(this::hydrate); return results; } + @Override + public List summarizeByRepo() { + return jdbc.query( + """ + SELECT provider, project, repo_name, COUNT(*) AS total + FROM push_records + GROUP BY provider, project, repo_name + ORDER BY total DESC + """, + (rs, rowNum) -> new RepoPushSummary( + rs.getString("provider"), + rs.getString("project"), + rs.getString("repo_name"), + rs.getLong("total"))); + } + + @Override + public Map countByStatus(PushQuery query) { + MapSqlParameterSource params = new MapSqlParameterSource(); + // Strip status so we get counts for all statuses + PushQuery noStatus = PushQuery.builder() + .project(query.getProject()) + .repoName(query.getRepoName()) + .branch(query.getBranch()) + .user(query.getUser()) + .authorEmail(query.getAuthorEmail()) + .commitTo(query.getCommitTo()) + .search(query.getSearch()) + .build(); + String where = buildWhere(noStatus, params); + Map result = new LinkedHashMap<>(); + jdbc.query( + "SELECT status, COUNT(*) AS total FROM push_records" + where + " GROUP BY status", + params, + (RowCallbackHandler) rs -> result.put(rs.getString("status"), rs.getLong("total"))); + return result; + } + + @Override + public Map> countPushStatusByUser() { + Map> result = new LinkedHashMap<>(); + jdbc.query(""" + SELECT resolved_user, status, COUNT(*) AS total + FROM push_records + WHERE resolved_user IS NOT NULL + GROUP BY resolved_user, status + """, rs -> { + String user = rs.getString("resolved_user"); + String status = rs.getString("status"); + result.computeIfAbsent(user, k -> new LinkedHashMap<>()).put(status, rs.getLong("total")); + }); + return result; + } + @Override public void delete(String id) { // CASCADE handles child tables @@ -179,6 +200,50 @@ public void close() { // --- Private helpers --- + /** + * Builds a SQL WHERE clause from the query and populates {@code params}. Returns the clause string starting with + * {@code " WHERE 1=1 ..."} (or just {@code " WHERE 1=1"} if no filters are set). + */ + private static String buildWhere(PushQuery query, MapSqlParameterSource params) { + StringBuilder sql = new StringBuilder(" WHERE 1=1"); + + if (query.getStatus() != null) { + sql.append(" AND status = :status"); + params.addValue("status", query.getStatus().name()); + } + if (query.getProject() != null) { + sql.append(" AND project = :project"); + params.addValue("project", query.getProject()); + } + if (query.getRepoName() != null) { + sql.append(" AND repo_name = :repoName"); + params.addValue("repoName", query.getRepoName()); + } + if (query.getBranch() != null) { + sql.append(" AND branch = :branch"); + params.addValue("branch", query.getBranch()); + } + if (query.getCommitTo() != null) { + sql.append(" AND commit_to = :commitTo"); + params.addValue("commitTo", query.getCommitTo()); + } + if (query.getUser() != null) { + sql.append(" AND resolved_user = :user"); + params.addValue("user", query.getUser()); + } + if (query.getAuthorEmail() != null) { + sql.append(" AND author_email = :authorEmail"); + params.addValue("authorEmail", query.getAuthorEmail()); + } + if (query.getSearch() != null && !query.getSearch().isBlank()) { + sql.append( + " AND (LOWER(provider) LIKE :search OR LOWER(project) LIKE :search OR LOWER(repo_name) LIKE :search)"); + params.addValue("search", "%" + query.getSearch().toLowerCase() + "%"); + } + + return sql.toString(); + } + private PushRecord updateStatus(String id, PushStatus status, Attestation attestation) { tx.executeWithoutResult(txStatus -> { int updated = jdbc.update( diff --git a/git-proxy-java-dashboard/frontend/src/api.ts b/git-proxy-java-dashboard/frontend/src/api.ts index dd4ed343..e604358e 100644 --- a/git-proxy-java-dashboard/frontend/src/api.ts +++ b/git-proxy-java-dashboard/frontend/src/api.ts @@ -34,6 +34,12 @@ export async function fetchPushes(params: URLSearchParams) { return res.json() } +export async function fetchPushCounts(params: URLSearchParams): Promise> { + const res = await apiFetch('/api/push/counts?' + params) + if (!res.ok) throw new Error('Failed to fetch push counts') + return res.json() +} + export async function fetchPush(id: string) { const url = id.includes('_') ? `/api/push/by-ref/${id}` : `/api/push/${id}` const res = await apiFetch(url) diff --git a/git-proxy-java-dashboard/frontend/src/pages/PushList.tsx b/git-proxy-java-dashboard/frontend/src/pages/PushList.tsx index a812cfad..5b431688 100644 --- a/git-proxy-java-dashboard/frontend/src/pages/PushList.tsx +++ b/git-proxy-java-dashboard/frontend/src/pages/PushList.tsx @@ -1,6 +1,6 @@ import { useCallback, useEffect, useRef, useState } from 'react' import { useNavigate } from 'react-router-dom' -import { approvePush, fetchPushes, rejectPush } from '../api' +import { approvePush, fetchPushCounts, fetchPushes, rejectPush } from '../api' import { StatusBadge } from '../components/StatusBadge' import type { CurrentUser, PushRecord, PushStatus } from '../types' @@ -142,17 +142,16 @@ export function PushList({ currentUser }: PushListProps) { [currentUser], ) - const loadCounts = useCallback(async () => { - const results: Partial> = {} - await Promise.all( - STATUSES.map(async (s) => { - const params = new URLSearchParams({ status: s, limit: '1000' }) - const data: PushRecord[] = await fetchPushes(params) - results[s] = data.length - }), - ) - setCounts(results) - }, []) + const loadCounts = useCallback( + async (search: string, myOnly: boolean) => { + const params = new URLSearchParams() + if (search) params.set('search', search) + if (myOnly && currentUser?.username) params.set('user', currentUser.username) + const data = await fetchPushCounts(params) + setCounts(data) + }, + [currentUser], + ) // Clear selection when leaving PENDING filter useEffect(() => { @@ -169,10 +168,15 @@ export function PushList({ currentUser }: PushListProps) { return () => clearInterval(timer) }, [filterStatus, filterRepo, myPushesOnly, newestFirst, page, load]) - // Load counts once on mount + // Reload counts when filter-relevant state changes, with auto-refresh useEffect(() => { - void Promise.resolve().then(() => loadCounts()) - }, [loadCounts]) + void Promise.resolve().then(() => loadCounts(filterRepo, myPushesOnly)) + const timer = setInterval( + () => loadCounts(filterRepo, myPushesOnly), + 10_000, + ) + return () => clearInterval(timer) + }, [filterRepo, myPushesOnly, loadCounts]) function handleRepoChange(value: string) { setFilterRepo(value) diff --git a/git-proxy-java-dashboard/src/main/java/org/finos/gitproxy/dashboard/controller/PushController.java b/git-proxy-java-dashboard/src/main/java/org/finos/gitproxy/dashboard/controller/PushController.java index a488d66c..d6ae0c97 100644 --- a/git-proxy-java-dashboard/src/main/java/org/finos/gitproxy/dashboard/controller/PushController.java +++ b/git-proxy-java-dashboard/src/main/java/org/finos/gitproxy/dashboard/controller/PushController.java @@ -85,6 +85,27 @@ public List list( return pushStore.find(query.build()); } + /** + * Count push records grouped by status. Accepts the same filter params as {@link #list} except {@code status} — + * returns counts for all statuses so the caller can populate all filter tabs in one round trip. + */ + @Operation(operationId = "countPushes", summary = "Count push records by status") + @GetMapping("/counts") + public Map counts( + @RequestParam(required = false) String project, + @RequestParam(required = false) String repo, + @RequestParam(required = false) String user, + @RequestParam(required = false) String search) { + + PushQuery.PushQueryBuilder query = PushQuery.builder(); + if (project != null && !project.isBlank()) query.project(project); + if (repo != null && !repo.isBlank()) query.repoName(repo); + if (user != null && !user.isBlank()) query.user(user); + if (search != null && !search.isBlank()) query.search(search); + + return pushStore.countByStatus(query.build()); + } + /** * Look up a push record by its commit reference ({commitFrom}_{commitTo}). Used by the transparent proxy flow where * we link to a push before it has been saved with a UUID. diff --git a/git-proxy-java-dashboard/src/main/java/org/finos/gitproxy/dashboard/controller/RepoController.java b/git-proxy-java-dashboard/src/main/java/org/finos/gitproxy/dashboard/controller/RepoController.java index 36bf6709..62bcab7f 100644 --- a/git-proxy-java-dashboard/src/main/java/org/finos/gitproxy/dashboard/controller/RepoController.java +++ b/git-proxy-java-dashboard/src/main/java/org/finos/gitproxy/dashboard/controller/RepoController.java @@ -14,10 +14,9 @@ import org.finos.gitproxy.db.FetchStore; import org.finos.gitproxy.db.FetchStore.RepoFetchSummary; import org.finos.gitproxy.db.PushStore; +import org.finos.gitproxy.db.PushStore.RepoPushSummary; import org.finos.gitproxy.db.UrlRuleRegistry; import org.finos.gitproxy.db.model.AccessRule; -import org.finos.gitproxy.db.model.PushQuery; -import org.finos.gitproxy.db.model.PushRecord; import org.finos.gitproxy.provider.ProviderRegistry; import org.springframework.beans.factory.annotation.Autowired; import org.springframework.http.HttpStatus; @@ -127,27 +126,20 @@ public List> activeRepos() { // Keyed by "provider|owner|repoName" Map> byRepo = new HashMap<>(); - // Aggregate push records. Use the stored provider ID where available; fall back to - // parsing the upstream URL for records created before the provider field was populated. - List pushRecords = - pushStore.find(PushQuery.builder().limit(5000).build()); - for (PushRecord pr : pushRecords) { - String provider = (pr.getProvider() != null && !pr.getProvider().isBlank()) - ? pr.getProvider() - : providerFromUrl(pr.getUpstreamUrl()); - String owner = pr.getProject(); // project = owner (see PushRecordMapper) - String key = provider + "|" + owner + "|" + pr.getRepoName(); - byRepo.computeIfAbsent(key, k -> { - Map entry = new HashMap<>(); - entry.put("provider", provider); - entry.put("owner", owner); - entry.put("repoName", pr.getRepoName()); - entry.put("pushCount", 0L); - entry.put("fetchCount", 0L); - entry.put("blockedFetchCount", 0L); - return entry; + // Aggregate push records using a single SQL GROUP BY query. + for (RepoPushSummary summary : pushStore.summarizeByRepo()) { + String key = summary.provider() + "|" + summary.owner() + "|" + summary.repoName(); + Map entry = byRepo.computeIfAbsent(key, k -> { + Map e = new HashMap<>(); + e.put("provider", summary.provider()); + e.put("owner", summary.owner()); + e.put("repoName", summary.repoName()); + e.put("pushCount", 0L); + e.put("fetchCount", 0L); + e.put("blockedFetchCount", 0L); + return e; }); - byRepo.get(key).merge("pushCount", 1L, (a, b) -> (long) a + (long) b); + entry.put("pushCount", summary.total()); } // Merge fetch summaries @@ -175,13 +167,4 @@ public List> activeRepos() { .reversed()) .collect(Collectors.toList()); } - - private static String providerFromUrl(String url) { - if (url == null || url.isBlank()) return "unknown"; - try { - return new java.net.URI(url).getHost(); - } catch (Exception e) { - return "unknown"; - } - } } diff --git a/git-proxy-java-dashboard/src/main/java/org/finos/gitproxy/dashboard/controller/UserController.java b/git-proxy-java-dashboard/src/main/java/org/finos/gitproxy/dashboard/controller/UserController.java index 967bd7af..083338b2 100644 --- a/git-proxy-java-dashboard/src/main/java/org/finos/gitproxy/dashboard/controller/UserController.java +++ b/git-proxy-java-dashboard/src/main/java/org/finos/gitproxy/dashboard/controller/UserController.java @@ -4,10 +4,8 @@ import io.swagger.v3.oas.annotations.tags.Tag; import java.util.List; import java.util.Map; -import java.util.stream.Collectors; import org.finos.gitproxy.db.PushStore; import org.finos.gitproxy.db.model.PushQuery; -import org.finos.gitproxy.db.model.PushRecord; import org.finos.gitproxy.user.EmailConflictException; import org.finos.gitproxy.user.LockedByConfigException; import org.finos.gitproxy.user.ReadOnlyUserStore; @@ -37,7 +35,10 @@ public class UserController { @Operation(operationId = "listUsers", summary = "List all users") @GetMapping public List list() { - return userStore.findAll().stream().map(this::toSummary).toList(); + Map> allPushCounts = pushStore.countPushStatusByUser(); + return userStore.findAll().stream() + .map(u -> toSummary(u, allPushCounts.getOrDefault(u.getUsername(), Map.of()))) + .toList(); } @Operation(operationId = "getUser", summary = "Get user details") @@ -206,14 +207,13 @@ public ResponseEntity removeIdentity( return ResponseEntity.noContent().build(); } - private UserSummary toSummary(UserEntry u) { + private UserSummary toSummary(UserEntry u, Map pushCounts) { String primaryEmail = u.getEmails().isEmpty() ? null : u.getEmails().get(0); List scmProviders = u.getScmIdentities().stream() .map(id -> id.getProvider()) .filter(p -> !"proxy".equals(p)) .distinct() .toList(); - Map pushCounts = countPushesByStatus(u.getUsername()); return new UserSummary(u.getUsername(), primaryEmail, scmProviders, pushCounts); } @@ -238,16 +238,11 @@ private UserDetail toDetail(UserEntry u) { .toList(); } - Map pushCounts = countPushesByStatus(u.getUsername()); + Map pushCounts = pushStore.countByStatus( + PushQuery.builder().user(u.getUsername()).build()); return new UserDetail(u.getUsername(), emails, scmIdentities, pushCounts); } - private Map countPushesByStatus(String username) { - List pushes = - pushStore.find(PushQuery.builder().user(username).limit(10000).build()); - return pushes.stream().collect(Collectors.groupingBy(p -> p.getStatus().name(), Collectors.counting())); - } - public record UserSummary( String username, String primaryEmail, List scmProviders, Map pushCounts) {} diff --git a/git-proxy-java-dashboard/src/test/java/org/finos/gitproxy/dashboard/controller/PushControllerTest.java b/git-proxy-java-dashboard/src/test/java/org/finos/gitproxy/dashboard/controller/PushControllerTest.java index be73934a..a4307e96 100644 --- a/git-proxy-java-dashboard/src/test/java/org/finos/gitproxy/dashboard/controller/PushControllerTest.java +++ b/git-proxy-java-dashboard/src/test/java/org/finos/gitproxy/dashboard/controller/PushControllerTest.java @@ -548,4 +548,37 @@ void admin_cancelOwnPush_allowed() { assertEquals(HttpStatus.OK, controller.cancel("p1", null).getStatusCode()); } } + + // ── GET /api/push/counts ────────────────────────────────────────────────────── + + @Nested + class Counts { + @Test + void noFilters_delegatesToStoreWithNoStatusFilter() { + when(pushStore.countByStatus(any())).thenReturn(Map.of("PENDING", 3L, "APPROVED", 1L)); + + var result = controller.counts(null, null, null, null); + + assertEquals(Map.of("PENDING", 3L, "APPROVED", 1L), result); + verify(pushStore).countByStatus(argThat(q -> q.getStatus() == null && q.getUser() == null)); + } + + @Test + void userFilter_passedToQuery() { + when(pushStore.countByStatus(any())).thenReturn(Map.of()); + + controller.counts(null, null, "alice", null); + + verify(pushStore).countByStatus(argThat(q -> "alice".equals(q.getUser()))); + } + + @Test + void searchFilter_passedToQuery() { + when(pushStore.countByStatus(any())).thenReturn(Map.of()); + + controller.counts(null, null, null, "myrepo"); + + verify(pushStore).countByStatus(argThat(q -> "myrepo".equals(q.getSearch()))); + } + } } diff --git a/git-proxy-java-dashboard/src/test/java/org/finos/gitproxy/dashboard/controller/RepoControllerTest.java b/git-proxy-java-dashboard/src/test/java/org/finos/gitproxy/dashboard/controller/RepoControllerTest.java index e6bb5bb7..f9e075b7 100644 --- a/git-proxy-java-dashboard/src/test/java/org/finos/gitproxy/dashboard/controller/RepoControllerTest.java +++ b/git-proxy-java-dashboard/src/test/java/org/finos/gitproxy/dashboard/controller/RepoControllerTest.java @@ -1,7 +1,6 @@ package org.finos.gitproxy.dashboard.controller; import static org.junit.jupiter.api.Assertions.assertEquals; -import static org.mockito.ArgumentMatchers.any; import static org.mockito.Mockito.mock; import static org.mockito.Mockito.verify; import static org.mockito.Mockito.when; @@ -11,9 +10,9 @@ import org.finos.gitproxy.db.FetchStore; import org.finos.gitproxy.db.FetchStore.RepoFetchSummary; import org.finos.gitproxy.db.PushStore; +import org.finos.gitproxy.db.PushStore.RepoPushSummary; import org.finos.gitproxy.db.UrlRuleRegistry; import org.finos.gitproxy.db.model.AccessRule; -import org.finos.gitproxy.db.model.PushRecord; import org.finos.gitproxy.provider.GitProxyProvider; import org.finos.gitproxy.provider.ProviderRegistry; import org.junit.jupiter.api.Nested; @@ -160,26 +159,21 @@ class ActiveRepos { @Test void empty_returnsEmptyList() { - when(pushStore.find(any())).thenReturn(List.of()); + when(pushStore.summarizeByRepo()).thenReturn(List.of()); when(fetchStore.summarizeByRepo()).thenReturn(List.of()); assertEquals(List.of(), controller.activeRepos()); } @Test - void pushRecords_aggregatedByRepo() { - var push = PushRecord.builder() - .upstreamUrl("https://github.com/acme/myrepo.git") - .project("acme") - .repoName("myrepo") - .build(); - when(pushStore.find(any())).thenReturn(List.of(push, push)); // two pushes to same repo + void pushSummary_appearsInResults() { + when(pushStore.summarizeByRepo()).thenReturn(List.of(new RepoPushSummary("github", "acme", "myrepo", 2L))); when(fetchStore.summarizeByRepo()).thenReturn(List.of()); var result = controller.activeRepos(); assertEquals(1, result.size()); - assertEquals("github.com", result.get(0).get("provider")); + assertEquals("github", result.get(0).get("provider")); assertEquals("acme", result.get(0).get("owner")); assertEquals("myrepo", result.get(0).get("repoName")); assertEquals(2L, result.get(0).get("pushCount")); @@ -188,14 +182,9 @@ void pushRecords_aggregatedByRepo() { @Test void fetchSummaries_mergedWithPushData() { - var push = PushRecord.builder() - .upstreamUrl("https://github.com/acme/myrepo.git") - .project("acme") - .repoName("myrepo") - .build(); - var fetchSummary = new RepoFetchSummary("github.com", "acme", "myrepo", 10L, 2L); - when(pushStore.find(any())).thenReturn(List.of(push)); - when(fetchStore.summarizeByRepo()).thenReturn(List.of(fetchSummary)); + when(pushStore.summarizeByRepo()).thenReturn(List.of(new RepoPushSummary("github", "acme", "myrepo", 1L))); + when(fetchStore.summarizeByRepo()) + .thenReturn(List.of(new RepoFetchSummary("github", "acme", "myrepo", 10L, 2L))); var result = controller.activeRepos(); @@ -207,34 +196,27 @@ void fetchSummaries_mergedWithPushData() { @Test void fetchOnly_repo_appearsInResults() { - when(pushStore.find(any())).thenReturn(List.of()); + when(pushStore.summarizeByRepo()).thenReturn(List.of()); when(fetchStore.summarizeByRepo()) - .thenReturn(List.of(new RepoFetchSummary("gitlab.com", "org", "repo", 5L, 0L))); + .thenReturn(List.of(new RepoFetchSummary("gitlab", "org", "repo", 5L, 0L))); var result = controller.activeRepos(); assertEquals(1, result.size()); - assertEquals("gitlab.com", result.get(0).get("provider")); + assertEquals("gitlab", result.get(0).get("provider")); assertEquals(0L, result.get(0).get("pushCount")); assertEquals(5L, result.get(0).get("fetchCount")); } @Test void sortedByTotalActivityDescending() { - var busy = PushRecord.builder() - .upstreamUrl("https://github.com/acme/busy.git") - .project("acme") - .repoName("busy") - .build(); - var quiet = PushRecord.builder() - .upstreamUrl("https://github.com/acme/quiet.git") - .project("acme") - .repoName("quiet") - .build(); - // busy gets 3 pushes + 10 fetches = 13; quiet gets 1 push + 0 fetches = 1 - when(pushStore.find(any())).thenReturn(List.of(busy, busy, busy, quiet)); + when(pushStore.summarizeByRepo()) + .thenReturn(List.of( + new RepoPushSummary("github", "acme", "busy", 3L), + new RepoPushSummary("github", "acme", "quiet", 1L))); + // busy also has 10 fetches = 13 total; quiet has 1 push + 0 fetches = 1 when(fetchStore.summarizeByRepo()) - .thenReturn(List.of(new RepoFetchSummary("github.com", "acme", "busy", 10L, 0L))); + .thenReturn(List.of(new RepoFetchSummary("github", "acme", "busy", 10L, 0L))); var result = controller.activeRepos(); @@ -242,35 +224,5 @@ void sortedByTotalActivityDescending() { assertEquals("busy", result.get(0).get("repoName")); assertEquals("quiet", result.get(1).get("repoName")); } - - @Test - void nullUpstreamUrl_usesUnknownProvider() { - var push = PushRecord.builder() - .upstreamUrl(null) - .project("acme") - .repoName("myrepo") - .build(); - when(pushStore.find(any())).thenReturn(List.of(push)); - when(fetchStore.summarizeByRepo()).thenReturn(List.of()); - - var result = controller.activeRepos(); - - assertEquals("unknown", result.get(0).get("provider")); - } - - @Test - void malformedUpstreamUrl_usesUnknownProvider() { - var push = PushRecord.builder() - .upstreamUrl("not a valid url ::::") - .project("acme") - .repoName("myrepo") - .build(); - when(pushStore.find(any())).thenReturn(List.of(push)); - when(fetchStore.summarizeByRepo()).thenReturn(List.of()); - - var result = controller.activeRepos(); - - assertEquals("unknown", result.get(0).get("provider")); - } } }