diff --git a/.gitignore b/.gitignore index f28c5e0..91817f9 100644 --- a/.gitignore +++ b/.gitignore @@ -2,6 +2,9 @@ docs/* # Metric documents are thesis-canonical and belong in the deliverable. Specs, ADRs # and internal notes stay local, which the docs/* rule above already handles. !docs/metrics/ +# The metric-adding procedure is referenced by the thesis and by future contributors, +# not an internal note, so it ships like docs/metrics/ rather than staying local. +!docs/adding-a-metric.md .claude/ .agents/ .idea diff --git a/dev-analytics/src/main/java/com/juliashtal/devanalytics/ai/client/OllamaLlmClient.java b/dev-analytics/src/main/java/com/juliashtal/devanalytics/ai/client/OllamaLlmClient.java index 290ec06..dd45337 100644 --- a/dev-analytics/src/main/java/com/juliashtal/devanalytics/ai/client/OllamaLlmClient.java +++ b/dev-analytics/src/main/java/com/juliashtal/devanalytics/ai/client/OllamaLlmClient.java @@ -1,6 +1,7 @@ package com.juliashtal.devanalytics.ai.client; import com.fasterxml.jackson.annotation.JsonInclude; +import com.fasterxml.jackson.annotation.JsonProperty; import lombok.Data; import lombok.extern.slf4j.Slf4j; import org.springframework.beans.factory.annotation.Value; @@ -24,14 +25,17 @@ public class OllamaLlmClient implements LlmClient { private final RestTemplate restTemplate; private final int seed; + private final String keepAlive; public OllamaLlmClient( @Value("${ai.ollama.base-url:http://localhost:11434}") String baseUrl, @Value("${ai.ollama.num-predict:1024}") int numPredict, - @Value("${ai.ollama.seed:42}") int seed) { + @Value("${ai.ollama.seed:42}") int seed, + @Value("${ai.ollama.keep-alive:0}") String keepAlive) { this.baseUrl = baseUrl; this.numPredict = numPredict; this.seed = seed; + this.keepAlive = keepAlive; SimpleClientHttpRequestFactory factory = new SimpleClientHttpRequestFactory(); factory.setConnectTimeout(5_000); factory.setReadTimeout(300_000); // 5 minutes — LLM inference can be slow @@ -47,6 +51,7 @@ public String complete(String model, String systemPrompt, String userPrompt, boo req.setStream(false); if (jsonMode) req.setFormat("json"); req.setOptions(Map.of("num_predict", numPredict, "temperature", 0.0, "seed", seed)); + req.setKeepAlive(keepAlive); log.debug("Sending request to Ollama: model={}, promptLength={}, numPredict={}, seed={}", model, userPrompt.length(), numPredict, seed); try { @@ -76,6 +81,9 @@ static class OllamaRequest { private boolean stream; private String format; private Map options; + /** How long Ollama keeps the model resident after this request; {@code "0"} unloads it immediately. */ + @JsonProperty("keep_alive") + private String keepAlive; } @Data diff --git a/dev-analytics/src/main/java/com/juliashtal/devanalytics/metrics/service/AggregateWindowResolver.java b/dev-analytics/src/main/java/com/juliashtal/devanalytics/metrics/service/AggregateWindowResolver.java index b65a28c..31e695b 100644 --- a/dev-analytics/src/main/java/com/juliashtal/devanalytics/metrics/service/AggregateWindowResolver.java +++ b/dev-analytics/src/main/java/com/juliashtal/devanalytics/metrics/service/AggregateWindowResolver.java @@ -7,6 +7,7 @@ import java.time.LocalDate; import java.time.temporal.ChronoUnit; import java.util.ArrayList; +import java.util.Arrays; import java.util.Comparator; import java.util.EnumMap; import java.util.LinkedHashMap; @@ -64,6 +65,27 @@ public enum Reduction { REDUCTIONS = Map.copyOf(m); } + /** + * Fails application startup when a {@code MetricType.aggregatePeriod} metric has no + * {@link Reduction}, naming the type and this class so the gap is fixed before it can hide + * behind a silent default (see {@link #reductionOrThrow}). + */ + public AggregateWindowResolver() { + validateAggregatePeriodCoverage(REDUCTIONS); + } + + static void validateAggregatePeriodCoverage(Map reductions) { + List missing = Arrays.stream(MetricType.values()) + .filter(t -> t.aggregatePeriod) + .filter(t -> !reductions.containsKey(t)) + .toList(); + if (!missing.isEmpty()) { + throw new IllegalStateException( + "AggregateWindowResolver.REDUCTIONS has no Reduction for " + missing + + " — every MetricType.aggregatePeriod metric must be added there."); + } + } + /** One stored window and the value covering it, after cross-repository rows are combined. */ public record WindowValue(LocalDate periodFrom, LocalDate periodTo, double value) {} @@ -100,8 +122,6 @@ public boolean isCount(MetricType type) { * Used where the read side wants a series rather than a single figure. */ public List perWindow(List aggregateRows, MetricType type) { - Reduction reduction = REDUCTIONS.getOrDefault(type, Reduction.MEDIAN); - Map, List> byWindow = new LinkedHashMap<>(); aggregateRows.stream() .sorted(Comparator.comparing(MetricSnapshot::getPeriodFrom) @@ -110,6 +130,14 @@ public List perWindow(List aggregateRows, MetricTyp .computeIfAbsent(List.of(s.getPeriodFrom(), s.getPeriodTo()), k -> new ArrayList<>()) .add(s.getValue())); + // Nothing to combine: the reduction lookup is skipped so a type with no rows never + // trips the missing-reduction guard below, regardless of whether it is declared. + if (byWindow.isEmpty()) { + return List.of(); + } + + Reduction reduction = reductionOrThrow(type); + // Rows sharing a window differ only by repository, so WIDEST_WINDOW can still median them. Reduction crossRepo = reduction == Reduction.WIDEST_WINDOW ? Reduction.MEDIAN : reduction; @@ -127,7 +155,7 @@ public Optional resolve(List aggregateRows, M List windows = perWindow(aggregateRows, type); if (windows.isEmpty()) return Optional.empty(); - Reduction reduction = REDUCTIONS.getOrDefault(type, Reduction.MEDIAN); + Reduction reduction = reductionOrThrow(type); if (reduction == Reduction.WIDEST_WINDOW) { WindowValue widest = windows.stream() @@ -144,6 +172,21 @@ public Optional resolve(List aggregateRows, M return Optional.of(new ResolvedAggregate(value, from, to)); } + /** + * The declared {@link Reduction}, or a loud failure instead of the median it used to fall + * back to silently — a type reaching here with rows to combine but no entry is exactly the + * gap a new calculator can open by writing a period without one. + */ + private Reduction reductionOrThrow(MetricType type) { + Reduction reduction = REDUCTIONS.get(type); + if (reduction == null) { + throw new IllegalStateException( + "No Reduction declared for " + type + " in AggregateWindowResolver.REDUCTIONS " + + "— add one before rows for this type can be combined across windows."); + } + return reduction; + } + private double combine(List values, Reduction reduction) { return switch (reduction) { case SUM -> values.stream().mapToDouble(Double::doubleValue).sum(); diff --git a/dev-analytics/src/main/resources/application.yml b/dev-analytics/src/main/resources/application.yml index 6fc5597..ccfa603 100644 --- a/dev-analytics/src/main/resources/application.yml +++ b/dev-analytics/src/main/resources/application.yml @@ -82,6 +82,7 @@ ai: model: ${OLLAMA_MODEL:llama3.2} num-predict: ${OLLAMA_NUM_PREDICT:1024} seed: ${OLLAMA_SEED:42} + keep-alive: ${OLLAMA_KEEP_ALIVE:0} management: endpoints: diff --git a/dev-analytics/src/test/java/com/juliashtal/devanalytics/ai/client/OllamaLlmClientTest.java b/dev-analytics/src/test/java/com/juliashtal/devanalytics/ai/client/OllamaLlmClientTest.java new file mode 100644 index 0000000..2246343 --- /dev/null +++ b/dev-analytics/src/test/java/com/juliashtal/devanalytics/ai/client/OllamaLlmClientTest.java @@ -0,0 +1,158 @@ +package com.juliashtal.devanalytics.ai.client; + +import com.fasterxml.jackson.databind.JsonNode; +import com.fasterxml.jackson.databind.ObjectMapper; +import com.github.tomakehurst.wiremock.junit5.WireMockRuntimeInfo; +import com.github.tomakehurst.wiremock.junit5.WireMockTest; +import com.github.tomakehurst.wiremock.verification.LoggedRequest; +import org.junit.jupiter.api.Test; +import org.springframework.web.server.ResponseStatusException; + +import java.util.List; + +import static com.github.tomakehurst.wiremock.client.WireMock.*; +import static org.assertj.core.api.Assertions.assertThat; +import static org.assertj.core.api.Assertions.assertThatThrownBy; +import static org.springframework.http.HttpStatus.SERVICE_UNAVAILABLE; + +/** + * The summarization path's only HTTP boundary: what is sent to Ollama, and how a failure or an + * unusable reply is turned into a {@link ResponseStatusException} the caller can show the user. + */ +@WireMockTest +class OllamaLlmClientTest { + + @Test + void complete_success_returnsTheResponseText(WireMockRuntimeInfo wm) { + stubFor(post(urlEqualTo("/api/generate")) + .willReturn(okJson("{\"model\":\"llama3.2\",\"response\":\"hello there\"}"))); + + OllamaLlmClient client = client(wm); + String result = client.complete("llama3.2", "system", "user prompt", false); + + assertThat(result).isEqualTo("hello there"); + } + + @Test + void complete_requestBody_carriesModelPromptsAndOptions(WireMockRuntimeInfo wm) throws Exception { + stubFor(post(urlEqualTo("/api/generate")) + .willReturn(okJson("{\"model\":\"llama3.2\",\"response\":\"ok\"}"))); + + client(wm, 256, 7).complete("llama3.2", "be terse", "summarize this", false); + + JsonNode body = sentBody(); + assertThat(body.get("model").asText()).isEqualTo("llama3.2"); + assertThat(body.get("system").asText()).isEqualTo("be terse"); + assertThat(body.get("prompt").asText()).isEqualTo("summarize this"); + assertThat(body.get("stream").asBoolean()).isFalse(); + assertThat(body.get("options").get("num_predict").asInt()).isEqualTo(256); + assertThat(body.get("options").get("temperature").asDouble()).isEqualTo(0.0); + assertThat(body.get("options").get("seed").asInt()).isEqualTo(7); + } + + @Test + void complete_requestBody_carriesTheKeepAliveProperty(WireMockRuntimeInfo wm) throws Exception { + stubFor(post(urlEqualTo("/api/generate")) + .willReturn(okJson("{\"model\":\"llama3.2\",\"response\":\"ok\"}"))); + + new OllamaLlmClient(wm.getHttpBaseUrl(), 1024, 42, "0") + .complete("llama3.2", "system", "prompt", false); + + assertThat(sentBody().get("keep_alive").asText()).isEqualTo("0"); + } + + @Test + void complete_jsonModeTrue_setsFormatJson(WireMockRuntimeInfo wm) throws Exception { + stubFor(post(urlEqualTo("/api/generate")) + .willReturn(okJson("{\"model\":\"llama3.2\",\"response\":\"{}\"}"))); + + client(wm).complete("llama3.2", "system", "prompt", true); + + assertThat(sentBody().get("format").asText()).isEqualTo("json"); + } + + @Test + void complete_jsonModeFalse_omitsFormatField(WireMockRuntimeInfo wm) throws Exception { + stubFor(post(urlEqualTo("/api/generate")) + .willReturn(okJson("{\"model\":\"llama3.2\",\"response\":\"plain text\"}"))); + + client(wm).complete("llama3.2", "system", "prompt", false); + + assertThat(sentBody().has("format")).isFalse(); + } + + @Test + void complete_blankResponseField_throwsServiceUnavailable(WireMockRuntimeInfo wm) { + stubFor(post(urlEqualTo("/api/generate")) + .willReturn(okJson("{\"model\":\"llama3.2\",\"response\":\"\"}"))); + + assertThatThrownBy(() -> client(wm).complete("llama3.2", "system", "prompt", false)) + .isInstanceOf(ResponseStatusException.class) + .hasMessageContaining("Empty response") + .hasMessageContaining("llama3.2") + .extracting(e -> ((ResponseStatusException) e).getStatusCode()) + .isEqualTo(SERVICE_UNAVAILABLE); + } + + @Test + void complete_missingResponseField_throwsServiceUnavailable(WireMockRuntimeInfo wm) { + // No "response" key at all — the field binds to null, same guard as a blank string. + stubFor(post(urlEqualTo("/api/generate")) + .willReturn(okJson("{\"model\":\"llama3.2\"}"))); + + assertThatThrownBy(() -> client(wm).complete("llama3.2", "system", "prompt", false)) + .isInstanceOf(ResponseStatusException.class) + .extracting(e -> ((ResponseStatusException) e).getStatusCode()) + .isEqualTo(SERVICE_UNAVAILABLE); + } + + @Test + void complete_serverError_wrapsAsServiceUnavailableNamingTheBaseUrl(WireMockRuntimeInfo wm) { + stubFor(post(urlEqualTo("/api/generate")).willReturn(aResponse().withStatus(500))); + + assertThatThrownBy(() -> client(wm).complete("llama3.2", "system", "prompt", false)) + .isInstanceOf(ResponseStatusException.class) + .hasMessageContaining(wm.getHttpBaseUrl()) + .extracting(e -> ((ResponseStatusException) e).getStatusCode()) + .isEqualTo(SERVICE_UNAVAILABLE); + } + + @Test + void complete_unreachableHost_wrapsAsServiceUnavailable() { + // Exercises the same catch-all path a real read timeout would: nothing answers, the + // client cannot distinguish "slow" from "down", and both fail the same way. + OllamaLlmClient client = new OllamaLlmClient("http://127.0.0.1:1", 1024, 42, "0"); + + assertThatThrownBy(() -> client.complete("llama3.2", "system", "prompt", false)) + .isInstanceOf(ResponseStatusException.class) + .hasMessageContaining("AI model is unavailable") + .extracting(e -> ((ResponseStatusException) e).getStatusCode()) + .isEqualTo(SERVICE_UNAVAILABLE); + } + + @Test + void complete_invalidJsonResponse_wrapsAsServiceUnavailable(WireMockRuntimeInfo wm) { + stubFor(post(urlEqualTo("/api/generate")) + .willReturn(aResponse().withStatus(200) + .withHeader("Content-Type", "application/json") + .withBody("not json"))); + + assertThatThrownBy(() -> client(wm).complete("llama3.2", "system", "prompt", false)) + .isInstanceOf(ResponseStatusException.class) + .extracting(e -> ((ResponseStatusException) e).getStatusCode()) + .isEqualTo(SERVICE_UNAVAILABLE); + } + + private static JsonNode sentBody() throws Exception { + List requests = findAll(postRequestedFor(urlEqualTo("/api/generate"))); + return new ObjectMapper().readTree(requests.get(requests.size() - 1).getBodyAsString()); + } + + private static OllamaLlmClient client(WireMockRuntimeInfo wm) { + return client(wm, 1024, 42); + } + + private static OllamaLlmClient client(WireMockRuntimeInfo wm, int numPredict, int seed) { + return new OllamaLlmClient(wm.getHttpBaseUrl(), numPredict, seed, "0"); + } +} diff --git a/dev-analytics/src/test/java/com/juliashtal/devanalytics/auth/AuthServiceTest.java b/dev-analytics/src/test/java/com/juliashtal/devanalytics/auth/AuthServiceTest.java new file mode 100644 index 0000000..8ffd99d --- /dev/null +++ b/dev-analytics/src/test/java/com/juliashtal/devanalytics/auth/AuthServiceTest.java @@ -0,0 +1,290 @@ +package com.juliashtal.devanalytics.auth; + +import com.juliashtal.devanalytics.auth.model.AuthResponse; +import com.juliashtal.devanalytics.auth.model.RefreshToken; +import com.juliashtal.devanalytics.auth.model.request.LoginRequest; +import com.juliashtal.devanalytics.auth.model.request.RegisterRequest; +import com.juliashtal.devanalytics.auth.service.RefreshTokenService; +import com.juliashtal.devanalytics.invite.InviteService; +import com.juliashtal.devanalytics.security.model.CustomUserDetails; +import com.juliashtal.devanalytics.security.service.JwtService; +import com.juliashtal.devanalytics.user.model.User; +import com.juliashtal.devanalytics.user.model.UserCommitEmail; +import com.juliashtal.devanalytics.user.repository.UserCommitEmailRepository; +import com.juliashtal.devanalytics.user.repository.UserRepository; +import org.junit.jupiter.api.BeforeEach; +import org.junit.jupiter.api.Test; +import org.junit.jupiter.api.extension.ExtendWith; +import org.mockito.ArgumentCaptor; +import org.mockito.InjectMocks; +import org.mockito.Mock; +import org.mockito.junit.jupiter.MockitoExtension; +import org.mockito.junit.jupiter.MockitoSettings; +import org.mockito.quality.Strictness; +import org.springframework.security.authentication.AuthenticationManager; +import org.springframework.security.authentication.BadCredentialsException; +import org.springframework.security.core.Authentication; +import org.springframework.security.crypto.password.PasswordEncoder; + +import java.util.Optional; + +import static org.assertj.core.api.Assertions.assertThat; +import static org.assertj.core.api.Assertions.assertThatThrownBy; +import static org.mockito.ArgumentMatchers.any; +import static org.mockito.ArgumentMatchers.anyString; +import static org.mockito.ArgumentMatchers.eq; +import static org.mockito.Mockito.never; +import static org.mockito.Mockito.times; +import static org.mockito.Mockito.verify; +import static org.mockito.Mockito.when; + +/** + * Covers what {@link AuthServiceRegistrationRoleTest} does not: duplicate-account rejection, + * commit-email seeding, login, token refresh, and logout. + */ +@ExtendWith(MockitoExtension.class) +@MockitoSettings(strictness = Strictness.LENIENT) +class AuthServiceTest { + + @Mock private UserRepository userRepository; + @Mock private UserCommitEmailRepository commitEmailRepository; + @Mock private PasswordEncoder passwordEncoder; + @Mock private AuthenticationManager authenticationManager; + @Mock private JwtService jwtService; + @Mock private RefreshTokenService refreshTokenService; + @Mock private InviteService inviteService; + + @InjectMocks private AuthService authService; + + private RegisterRequest registerRequest() { + RegisterRequest r = new RegisterRequest(); + r.setUsername("someone"); + r.setEmail("someone@example.com"); + r.setPassword("correct horse battery staple"); + return r; + } + + // ------------------------------------------------------------------ + // register + // ------------------------------------------------------------------ + + @Test + void register_usernameAlreadyTaken_throwsAndNeverSaves() { + when(userRepository.findByUsername("someone")).thenReturn(Optional.of(new User())); + + assertThatThrownBy(() -> authService.register(registerRequest())) + .isInstanceOf(IllegalArgumentException.class) + .hasMessageContaining("Username already taken"); + + verify(userRepository, never()).save(any()); + } + + @Test + void register_emailAlreadyTaken_throwsAndNeverSaves() { + when(userRepository.findByUsername(anyString())).thenReturn(Optional.empty()); + when(userRepository.findByEmail("someone@example.com")).thenReturn(Optional.of(new User())); + + assertThatThrownBy(() -> authService.register(registerRequest())) + .isInstanceOf(IllegalArgumentException.class) + .hasMessageContaining("Email already taken"); + + verify(userRepository, never()).save(any()); + } + + @Test + void register_blankInviteToken_neverValidatesOrRedeemsAnInvite() { + RegisterRequest req = registerRequest(); + req.setInviteToken(" "); + when(userRepository.findByUsername(anyString())).thenReturn(Optional.empty()); + when(userRepository.findByEmail(anyString())).thenReturn(Optional.empty()); + when(passwordEncoder.encode(anyString())).thenReturn("hashed"); + when(commitEmailRepository.findByEmail(anyString())).thenReturn(Optional.empty()); + when(userRepository.count()).thenReturn(1L); + when(userRepository.save(any(User.class))).thenAnswer(inv -> { + User u = inv.getArgument(0); + u.setId(1L); + return u; + }); + + authService.register(req); + + verify(inviteService, never()).validateInvite(anyString()); + verify(inviteService, never()).redeemInvite(anyString(), any()); + } + + @Test + void register_withInvite_redeemsItAfterSaving() { + RegisterRequest req = registerRequest(); + req.setInviteToken("invite-token"); + when(userRepository.findByUsername(anyString())).thenReturn(Optional.empty()); + when(userRepository.findByEmail(anyString())).thenReturn(Optional.empty()); + when(passwordEncoder.encode(anyString())).thenReturn("hashed"); + when(commitEmailRepository.findByEmail(anyString())).thenReturn(Optional.empty()); + when(inviteService.validateInvite("invite-token")) + .thenReturn(new com.juliashtal.devanalytics.invite.model.InviteInfoDto( + "someone@example.com", "DEVELOPER", null)); + when(userRepository.save(any(User.class))).thenAnswer(inv -> { + User u = inv.getArgument(0); + u.setId(7L); + return u; + }); + + authService.register(req); + + ArgumentCaptor captor = ArgumentCaptor.forClass(User.class); + verify(inviteService).redeemInvite(eq("invite-token"), captor.capture()); + assertThat(captor.getValue().getId()).isEqualTo(7L); + } + + // ------------------------------------------------------------------ + // seedCommitEmail (private, exercised through register) + // ------------------------------------------------------------------ + + private void stubRegisterPrerequisites() { + when(userRepository.findByUsername(anyString())).thenReturn(Optional.empty()); + when(userRepository.findByEmail(anyString())).thenReturn(Optional.empty()); + when(passwordEncoder.encode(anyString())).thenReturn("hashed"); + when(userRepository.count()).thenReturn(1L); + } + + @Test + void register_blankEmail_skipsSeedingACommitEmail() { + RegisterRequest req = registerRequest(); + req.setEmail(" "); + stubRegisterPrerequisites(); + when(userRepository.save(any(User.class))).thenAnswer(inv -> { + User u = inv.getArgument(0); + u.setId(1L); + return u; + }); + + authService.register(req); + + verify(commitEmailRepository, never()).findByEmail(anyString()); + verify(commitEmailRepository, never()).save(any()); + } + + @Test + void register_emailAlreadyDeclaredByAnotherAccount_skipsSeedingWithoutFailing() { + stubRegisterPrerequisites(); + when(commitEmailRepository.findByEmail("someone@example.com")) + .thenReturn(Optional.of(new UserCommitEmail())); + when(userRepository.save(any(User.class))).thenAnswer(inv -> { + User u = inv.getArgument(0); + u.setId(1L); + return u; + }); + + authService.register(registerRequest()); + + verify(commitEmailRepository, never()).save(any()); + } + + @Test + void register_newEmail_seedsItAsTheFirstCommitEmailLowercasedAndTrimmed() { + RegisterRequest req = registerRequest(); + req.setEmail(" Someone@Example.com "); + when(userRepository.findByUsername(anyString())).thenReturn(Optional.empty()); + when(userRepository.findByEmail(anyString())).thenReturn(Optional.empty()); + when(passwordEncoder.encode(anyString())).thenReturn("hashed"); + when(userRepository.count()).thenReturn(1L); + when(commitEmailRepository.findByEmail("someone@example.com")).thenReturn(Optional.empty()); + when(userRepository.save(any(User.class))).thenAnswer(inv -> { + User u = inv.getArgument(0); + u.setId(1L); + return u; + }); + + authService.register(req); + + ArgumentCaptor captor = ArgumentCaptor.forClass(UserCommitEmail.class); + verify(commitEmailRepository).save(captor.capture()); + assertThat(captor.getValue().getEmail()).isEqualTo("someone@example.com"); + } + + // ------------------------------------------------------------------ + // login + // ------------------------------------------------------------------ + + @Test + void login_validCredentials_returnsTokensFromTheAuthenticatedPrincipal() { + LoginRequest req = new LoginRequest(); + req.setUsernameOrEmail("someone"); + req.setPassword("correct horse battery staple"); + + User user = new User(); + user.setId(1L); + user.setUsername("someone"); + CustomUserDetails principal = new CustomUserDetails(user); + + Authentication auth = org.mockito.Mockito.mock(Authentication.class); + when(auth.getPrincipal()).thenReturn(principal); + when(authenticationManager.authenticate(any())).thenReturn(auth); + when(jwtService.generateAccessToken(principal)).thenReturn("access-token"); + when(jwtService.getAccessTokenExpirationMs()).thenReturn(900_000L); + + RefreshToken refreshToken = new RefreshToken(); + refreshToken.setToken("refresh-token"); + when(refreshTokenService.createRefreshToken(user)).thenReturn(refreshToken); + + AuthResponse response = authService.login(req); + + assertThat(response.getAccessToken()).isEqualTo("access-token"); + assertThat(response.getRefreshToken()).isEqualTo("refresh-token"); + assertThat(response.getExpiresIn()).isEqualTo(900L); + } + + @Test + void login_badCredentials_rewrapsWithAGenericMessage() { + LoginRequest req = new LoginRequest(); + req.setUsernameOrEmail("someone"); + req.setPassword("wrong"); + when(authenticationManager.authenticate(any())) + .thenThrow(new BadCredentialsException("no such user: someone")); + + // The rewritten message must not leak whether the account exists. + assertThatThrownBy(() -> authService.login(req)) + .isInstanceOf(BadCredentialsException.class) + .hasMessage("Invalid credentials"); + } + + // ------------------------------------------------------------------ + // refreshToken + // ------------------------------------------------------------------ + + @Test + void refreshToken_valid_rotatesAndReturnsNewTokens() { + User user = new User(); + user.setId(1L); + user.setUsername("someone"); + + RefreshToken oldToken = new RefreshToken(); + oldToken.setToken("old"); + RefreshToken newToken = new RefreshToken(); + newToken.setToken("new"); + newToken.setUser(user); + + when(refreshTokenService.verifyToken("old")).thenReturn(oldToken); + when(refreshTokenService.rotateToken(oldToken)).thenReturn(newToken); + when(jwtService.generateAccessToken(any(CustomUserDetails.class))).thenReturn("access-token"); + when(jwtService.getAccessTokenExpirationMs()).thenReturn(60_000L); + + AuthResponse response = authService.refreshToken("old"); + + assertThat(response.getAccessToken()).isEqualTo("access-token"); + assertThat(response.getRefreshToken()).isEqualTo("new"); + assertThat(response.getExpiresIn()).isEqualTo(60L); + } + + // ------------------------------------------------------------------ + // logout + // ------------------------------------------------------------------ + + @Test + void logout_revokesRefreshTokensAndBumpsTokenVersion() { + authService.logout(1L); + + verify(refreshTokenService).revokeAllUserTokens(1L); + verify(userRepository, times(1)).incrementTokenVersion(1L); + } +} diff --git a/dev-analytics/src/test/java/com/juliashtal/devanalytics/datasource/DataSourceControllerTest.java b/dev-analytics/src/test/java/com/juliashtal/devanalytics/datasource/DataSourceControllerTest.java index 2920bf6..4635075 100644 --- a/dev-analytics/src/test/java/com/juliashtal/devanalytics/datasource/DataSourceControllerTest.java +++ b/dev-analytics/src/test/java/com/juliashtal/devanalytics/datasource/DataSourceControllerTest.java @@ -1,9 +1,16 @@ package com.juliashtal.devanalytics.datasource; +import com.fasterxml.jackson.databind.ObjectMapper; import com.juliashtal.devanalytics.config.SecurityConfig; import com.juliashtal.devanalytics.datasource.controller.DataSourceController; +import com.juliashtal.devanalytics.datasource.model.DataSourceConfig; +import com.juliashtal.devanalytics.datasource.model.DataSourceType; +import com.juliashtal.devanalytics.datasource.model.SyncJobEntity; import com.juliashtal.devanalytics.datasource.model.SyncJobStatus; +import com.juliashtal.devanalytics.datasource.model.dto.CreateDataSourceRequest; +import com.juliashtal.devanalytics.datasource.model.dto.DataSourceResponseDto; import com.juliashtal.devanalytics.datasource.model.dto.SyncJobSummaryDto; +import com.juliashtal.devanalytics.datasource.model.dto.UpdateDataSourceRequest; import com.juliashtal.devanalytics.datasource.service.AsyncDataSourceCollectService; import com.juliashtal.devanalytics.datasource.service.DataSourceService; import com.juliashtal.devanalytics.datasource.service.SyncJobTracker; @@ -19,28 +26,35 @@ import org.springframework.boot.test.autoconfigure.web.servlet.WebMvcTest; import org.springframework.boot.test.mock.mockito.MockBean; import org.springframework.context.annotation.Import; +import org.springframework.http.MediaType; import org.springframework.security.test.context.support.WithMockUser; import org.springframework.test.web.servlet.MockMvc; import java.time.Instant; import java.util.List; import java.util.NoSuchElementException; +import java.util.Optional; +import static org.mockito.ArgumentMatchers.any; +import static org.mockito.ArgumentMatchers.eq; +import static org.mockito.Mockito.verify; import static org.mockito.Mockito.when; -import static org.springframework.test.web.servlet.request.MockMvcRequestBuilders.get; +import static org.springframework.test.web.servlet.request.MockMvcRequestBuilders.*; import static org.springframework.test.web.servlet.result.MockMvcResultMatchers.*; /** - * Controller slice tests for the sync-history endpoint in {@link DataSourceController}. - * {@link SecurityConfig} + {@link JwtAuthFilter} are imported so that the real security - * filter chain enforces the 401 contract; authenticated tests use {@code @WithMockUser} - * + {@code MockedStatic} to short-circuit the CustomUserDetails cast. + * Controller slice tests for {@link DataSourceController}: CRUD, collection triggering, and + * the status/history reads. {@link SecurityConfig} + {@link JwtAuthFilter} are imported so that + * the real security filter chain enforces the 401 contract; authenticated tests use + * {@code @WithMockUser} + {@code MockedStatic} to short-circuit the + * CustomUserDetails cast. */ @WebMvcTest(DataSourceController.class) @Import({SecurityConfig.class, JwtAuthFilter.class}) class DataSourceControllerTest { @Autowired MockMvc mockMvc; + @Autowired ObjectMapper objectMapper; @MockBean DataSourceService dataSourceService; @MockBean AsyncDataSourceCollectService asyncCollectService; @@ -57,6 +71,11 @@ private SyncJobSummaryDto completedJob() { 42, null); } + private static DataSourceResponseDto responseDto(long id) { + return new DataSourceResponseDto(id, DataSourceType.GITHUB, "GitHub personal", + "https://github.com", null, true, null, Instant.now(), null, true, 3L); + } + @Test @WithMockUser void getSyncHistory_owner_returns200WithJobList() throws Exception { @@ -104,4 +123,278 @@ void getSyncHistory_unauthenticated_returns401() throws Exception { mockMvc.perform(get("/api/datasources/5/sync-history").param("limit", "5")) .andExpect(status().isUnauthorized()); } + + @Test + @WithMockUser + void create_valid_returns201WithLocationAndBody() throws Exception { + try (MockedStatic su = Mockito.mockStatic(SecurityUtils.class)) { + su.when(SecurityUtils::getCurrentUserId).thenReturn(1L); + + CreateDataSourceRequest req = new CreateDataSourceRequest(); + req.setType(DataSourceType.GITHUB); + req.setName("GitHub personal"); + when(dataSourceService.create(eq(1L), any())).thenReturn(responseDto(5L)); + + mockMvc.perform(post("/api/datasources") + .contentType(MediaType.APPLICATION_JSON) + .content(objectMapper.writeValueAsString(req))) + .andExpect(status().isCreated()) + .andExpect(header().string("Location", "/api/datasources/5")) + .andExpect(jsonPath("$.id").value(5)) + .andExpect(jsonPath("$.name").value("GitHub personal")); + } + } + + @Test + void create_unauthenticated_returns401() throws Exception { + CreateDataSourceRequest req = new CreateDataSourceRequest(); + req.setType(DataSourceType.GITHUB); + req.setName("GitHub personal"); + + mockMvc.perform(post("/api/datasources") + .contentType(MediaType.APPLICATION_JSON) + .content(objectMapper.writeValueAsString(req))) + .andExpect(status().isUnauthorized()); + } + + @Test + @WithMockUser + void create_missingRequiredFields_returns400() throws Exception { + CreateDataSourceRequest req = new CreateDataSourceRequest(); // type and name both @NotNull/@NotBlank + + mockMvc.perform(post("/api/datasources") + .contentType(MediaType.APPLICATION_JSON) + .content(objectMapper.writeValueAsString(req))) + .andExpect(status().isBadRequest()); + } + + @Test + @WithMockUser + void list_returnsCallersDataSources() throws Exception { + try (MockedStatic su = Mockito.mockStatic(SecurityUtils.class)) { + su.when(SecurityUtils::getCurrentUserId).thenReturn(1L); + when(dataSourceService.listForUser(1L)).thenReturn(List.of(responseDto(5L))); + + mockMvc.perform(get("/api/datasources")) + .andExpect(status().isOk()) + .andExpect(jsonPath("$[0].id").value(5)); + } + } + + @Test + @WithMockUser + void get_owner_returns200WithConfig() throws Exception { + try (MockedStatic su = Mockito.mockStatic(SecurityUtils.class)) { + su.when(SecurityUtils::getCurrentUserId).thenReturn(1L); + DataSourceConfig cfg = new DataSourceConfig(); + cfg.setId(5L); + cfg.setType(DataSourceType.GITHUB); + cfg.setName("GitHub personal"); + when(dataSourceService.getForUser(1L, 5L)).thenReturn(cfg); + + mockMvc.perform(get("/api/datasources/5")) + .andExpect(status().isOk()) + .andExpect(jsonPath("$.id").value(5)) + .andExpect(jsonPath("$.name").value("GitHub personal")); + } + } + + @Test + @WithMockUser + void update_owner_returns200WithUpdatedConfig() throws Exception { + try (MockedStatic su = Mockito.mockStatic(SecurityUtils.class)) { + su.when(SecurityUtils::getCurrentUserId).thenReturn(1L); + UpdateDataSourceRequest req = new UpdateDataSourceRequest(); + req.setName("Renamed"); + DataSourceConfig updated = new DataSourceConfig(); + updated.setId(5L); + updated.setName("Renamed"); + when(dataSourceService.update(eq(1L), eq(5L), any())).thenReturn(updated); + + mockMvc.perform(put("/api/datasources/5") + .contentType(MediaType.APPLICATION_JSON) + .content(objectMapper.writeValueAsString(req))) + .andExpect(status().isOk()) + .andExpect(jsonPath("$.name").value("Renamed")); + } + } + + @Test + @WithMockUser + void delete_owner_returns204() throws Exception { + try (MockedStatic su = Mockito.mockStatic(SecurityUtils.class)) { + su.when(SecurityUtils::getCurrentUserId).thenReturn(1L); + + mockMvc.perform(delete("/api/datasources/5")) + .andExpect(status().isNoContent()); + + verify(dataSourceService).delete(1L, 5L); + } + } + + @Test + @WithMockUser + void collect_owner_returns202AndTriggersAsyncCollection() throws Exception { + try (MockedStatic su = Mockito.mockStatic(SecurityUtils.class)) { + su.when(SecurityUtils::getCurrentUserId).thenReturn(1L); + DataSourceConfig cfg = new DataSourceConfig(); + cfg.setId(5L); + when(dataSourceService.getForUser(1L, 5L)).thenReturn(cfg); + + mockMvc.perform(post("/api/datasources/5/collect")) + .andExpect(status().isAccepted()); + + verify(asyncCollectService).collectAsync(1L, 5L); + } + } + + @Test + @WithMockUser + void collectStatus_inMemoryRunning_returns200WithLiveState() throws Exception { + try (MockedStatic su = Mockito.mockStatic(SecurityUtils.class)) { + su.when(SecurityUtils::getCurrentUserId).thenReturn(1L); + DataSourceConfig cfg = new DataSourceConfig(); + cfg.setId(5L); + when(dataSourceService.getForUser(1L, 5L)).thenReturn(cfg); + + SyncJobTracker.JobState state = new SyncJobTracker.JobState(); + state.phase = "commits"; + state.phaseNumber = 1; + state.totalPhases = 3; + state.phaseTotal = 100; + state.phaseProcessed.set(10); + state.totalProcessed.set(10); + when(syncJobTracker.getState(5L)).thenReturn(Optional.of(state)); + when(syncJobTracker.phaseEtaSeconds(state)).thenReturn(30L); + when(syncJobTracker.overallEtaSeconds(state)).thenReturn(90L); + + mockMvc.perform(get("/api/datasources/5/collect/status")) + .andExpect(status().isOk()) + .andExpect(jsonPath("$.running").value(true)) + .andExpect(jsonPath("$.phase").value("commits")) + .andExpect(jsonPath("$.phaseEtaSeconds").value(30)) + .andExpect(jsonPath("$.overallEtaSeconds").value(90)); + } + } + + @Test + @WithMockUser + void collectStatus_noInMemoryButPersistedCompleted_returns200FromEntity() throws Exception { + try (MockedStatic su = Mockito.mockStatic(SecurityUtils.class)) { + su.when(SecurityUtils::getCurrentUserId).thenReturn(1L); + DataSourceConfig cfg = new DataSourceConfig(); + cfg.setId(5L); + when(dataSourceService.getForUser(1L, 5L)).thenReturn(cfg); + when(syncJobTracker.getState(5L)).thenReturn(Optional.empty()); + + SyncJobEntity entity = new SyncJobEntity(); + entity.setStatus(SyncJobStatus.COMPLETED); + entity.setStartedAt(Instant.now().minusSeconds(60)); + entity.setCompletedAt(Instant.now()); + entity.setTotalProcessed(200); + entity.setResult("Synced 200 items"); + when(syncJobTracker.findLatestPersisted(5L)).thenReturn(Optional.of(entity)); + + mockMvc.perform(get("/api/datasources/5/collect/status")) + .andExpect(status().isOk()) + .andExpect(jsonPath("$.running").value(false)) + .andExpect(jsonPath("$.result").value("Synced 200 items")) + .andExpect(jsonPath("$.totalProcessed").value(200)); + } + } + + @Test + @WithMockUser + void collectStatus_persistedInterrupted_replacesResultWithInterruptedMessage() throws Exception { + try (MockedStatic su = Mockito.mockStatic(SecurityUtils.class)) { + su.when(SecurityUtils::getCurrentUserId).thenReturn(1L); + DataSourceConfig cfg = new DataSourceConfig(); + cfg.setId(5L); + when(dataSourceService.getForUser(1L, 5L)).thenReturn(cfg); + when(syncJobTracker.getState(5L)).thenReturn(Optional.empty()); + + SyncJobEntity entity = new SyncJobEntity(); + entity.setStatus(SyncJobStatus.INTERRUPTED); + entity.setStartedAt(Instant.now().minusSeconds(60)); + entity.setResult("this should be overridden"); + when(syncJobTracker.findLatestPersisted(5L)).thenReturn(Optional.of(entity)); + + mockMvc.perform(get("/api/datasources/5/collect/status")) + .andExpect(status().isOk()) + .andExpect(jsonPath("$.result").value("Job was interrupted by a server restart")); + } + } + + @Test + @WithMockUser + void collectStatus_persistedFailed_includesErrorAndNullResult() throws Exception { + try (MockedStatic su = Mockito.mockStatic(SecurityUtils.class)) { + su.when(SecurityUtils::getCurrentUserId).thenReturn(1L); + DataSourceConfig cfg = new DataSourceConfig(); + cfg.setId(5L); + when(dataSourceService.getForUser(1L, 5L)).thenReturn(cfg); + when(syncJobTracker.getState(5L)).thenReturn(Optional.empty()); + + SyncJobEntity entity = new SyncJobEntity(); + entity.setStatus(SyncJobStatus.FAILED); + entity.setStartedAt(Instant.now().minusSeconds(60)); + entity.setCompletedAt(Instant.now()); + entity.setError("GitHub API rate limit exceeded"); + when(syncJobTracker.findLatestPersisted(5L)).thenReturn(Optional.of(entity)); + + mockMvc.perform(get("/api/datasources/5/collect/status")) + .andExpect(status().isOk()) + .andExpect(jsonPath("$.result").doesNotExist()) + .andExpect(jsonPath("$.error").value("GitHub API rate limit exceeded")); + } + } + + @Test + @WithMockUser + void collectStatus_neitherInMemoryNorPersisted_returns404() throws Exception { + try (MockedStatic su = Mockito.mockStatic(SecurityUtils.class)) { + su.when(SecurityUtils::getCurrentUserId).thenReturn(1L); + DataSourceConfig cfg = new DataSourceConfig(); + cfg.setId(5L); + when(dataSourceService.getForUser(1L, 5L)).thenReturn(cfg); + when(syncJobTracker.getState(5L)).thenReturn(Optional.empty()); + when(syncJobTracker.findLatestPersisted(5L)).thenReturn(Optional.empty()); + + mockMvc.perform(get("/api/datasources/5/collect/status")) + .andExpect(status().isNotFound()); + } + } + + @Test + @WithMockUser + void activeCollectStatuses_runningAndRecentlyDoneIncluded_staleAndUntrackedExcluded() throws Exception { + try (MockedStatic su = Mockito.mockStatic(SecurityUtils.class)) { + su.when(SecurityUtils::getCurrentUserId).thenReturn(1L); + when(dataSourceService.listForUser(1L)).thenReturn( + List.of(responseDto(1L), responseDto(2L), responseDto(3L), responseDto(4L))); + + SyncJobTracker.JobState running = new SyncJobTracker.JobState(); + running.running = true; + + SyncJobTracker.JobState recentlyDone = new SyncJobTracker.JobState(); + recentlyDone.running = false; + recentlyDone.completedAt = Instant.now().minusSeconds(60); + + SyncJobTracker.JobState staleDone = new SyncJobTracker.JobState(); + staleDone.running = false; + staleDone.completedAt = Instant.now().minusSeconds(600); + + when(syncJobTracker.getState(1L)).thenReturn(Optional.of(running)); + when(syncJobTracker.getState(2L)).thenReturn(Optional.of(recentlyDone)); + when(syncJobTracker.getState(3L)).thenReturn(Optional.of(staleDone)); + when(syncJobTracker.getState(4L)).thenReturn(Optional.empty()); + + mockMvc.perform(get("/api/datasources/collect/status/active")) + .andExpect(status().isOk()) + .andExpect(jsonPath("$.['1']").exists()) + .andExpect(jsonPath("$.['2']").exists()) + .andExpect(jsonPath("$.['3']").doesNotExist()) + .andExpect(jsonPath("$.['4']").doesNotExist()); + } + } } diff --git a/dev-analytics/src/test/java/com/juliashtal/devanalytics/exception/GlobalExceptionHandlerTest.java b/dev-analytics/src/test/java/com/juliashtal/devanalytics/exception/GlobalExceptionHandlerTest.java index 30edc3f..f763c62 100644 --- a/dev-analytics/src/test/java/com/juliashtal/devanalytics/exception/GlobalExceptionHandlerTest.java +++ b/dev-analytics/src/test/java/com/juliashtal/devanalytics/exception/GlobalExceptionHandlerTest.java @@ -1,19 +1,38 @@ package com.juliashtal.devanalytics.exception; import jakarta.servlet.http.HttpServletRequest; +import jakarta.validation.ConstraintViolation; +import jakarta.validation.ConstraintViolationException; +import jakarta.validation.Path; +import org.junit.jupiter.api.BeforeEach; import org.junit.jupiter.api.Test; import org.junit.jupiter.api.extension.ExtendWith; import org.mockito.Mock; import org.mockito.junit.jupiter.MockitoExtension; +import org.springframework.core.MethodParameter; import org.springframework.http.HttpStatus; import org.springframework.http.ResponseEntity; +import org.springframework.http.converter.HttpMessageNotReadableException; +import org.springframework.security.access.AccessDeniedException; +import org.springframework.security.authentication.BadCredentialsException; +import org.springframework.validation.BindingResult; +import org.springframework.validation.FieldError; +import org.springframework.web.bind.MethodArgumentNotValidException; import org.springframework.web.bind.MissingServletRequestParameterException; +import org.springframework.web.servlet.resource.NoResourceFoundException; +import org.springframework.http.HttpMethod; + +import java.util.List; +import java.util.NoSuchElementException; +import java.util.Set; import static org.assertj.core.api.Assertions.assertThat; +import static org.mockito.Mockito.mock; import static org.mockito.Mockito.when; /** - * Unit tests for {@link GlobalExceptionHandler}. + * Unit tests for {@link GlobalExceptionHandler}: one test per handler method, pinning the + * HTTP status and the {@link ApiError} body each exception maps to. * *

The handler does not extend {@code ResponseEntityExceptionHandler}, so Spring's own * request-binding failures only get a status other than 500 if this class declares one.

@@ -25,9 +44,55 @@ class GlobalExceptionHandlerTest { @Mock HttpServletRequest request; + @BeforeEach + void stubPath() { + when(request.getRequestURI()).thenReturn("/api/teams/5/export"); + } + + @Test + void handleValidation_fieldErrors_joinsMessagesAndReturns400() { + BindingResult bindingResult = mock(BindingResult.class); + when(bindingResult.getFieldErrors()).thenReturn(List.of( + new FieldError("obj", "name", "must not be blank"), + new FieldError("obj", "type", "must not be null"))); + MethodArgumentNotValidException ex = + new MethodArgumentNotValidException(mock(MethodParameter.class), bindingResult); + + ResponseEntity response = handler.handleValidation(ex, request); + + assertThat(response.getStatusCode()).isEqualTo(HttpStatus.BAD_REQUEST); + assertThat(response.getBody().getError()).isEqualTo("Validation Error"); + assertThat(response.getBody().getMessage()).isEqualTo("must not be blank; must not be null"); + } + + @Test + void handleConstraintViolation_violations_joinsPathsAndMessagesAndReturns400() { + @SuppressWarnings("unchecked") + ConstraintViolation violation = mock(ConstraintViolation.class); + Path path = mock(Path.class); + when(path.toString()).thenReturn("limit"); + when(violation.getPropertyPath()).thenReturn(path); + when(violation.getMessage()).thenReturn("must be positive"); + ConstraintViolationException ex = new ConstraintViolationException(Set.of(violation)); + + ResponseEntity response = handler.handleConstraintViolation(ex, request); + + assertThat(response.getStatusCode()).isEqualTo(HttpStatus.BAD_REQUEST); + assertThat(response.getBody().getMessage()).isEqualTo("limit: must be positive"); + } + + @Test + void handleNotReadable_malformedBody_returns400WithGenericMessage() { + HttpMessageNotReadableException ex = new HttpMessageNotReadableException("boom"); + + ResponseEntity response = handler.handleNotReadable(ex, request); + + assertThat(response.getStatusCode()).isEqualTo(HttpStatus.BAD_REQUEST); + assertThat(response.getBody().getMessage()).isEqualTo("Malformed or unreadable request body"); + } + @Test void handleMissingParameter_missingRequestParam_returns400NamingTheParameter() { - when(request.getRequestURI()).thenReturn("/api/teams/5/export"); MissingServletRequestParameterException ex = new MissingServletRequestParameterException("from", "LocalDate"); @@ -38,4 +103,168 @@ void handleMissingParameter_missingRequestParam_returns400NamingTheParameter() { assertThat(response.getBody().getMessage()).isEqualTo("Required parameter 'from' is missing"); assertThat(response.getBody().getPath()).isEqualTo("/api/teams/5/export"); } + + @Test + void handleIllegalArgument_returns400WithTheExceptionMessage() { + ResponseEntity response = + handler.handleIllegalArgument(new IllegalArgumentException("bad value"), request); + + assertThat(response.getStatusCode()).isEqualTo(HttpStatus.BAD_REQUEST); + assertThat(response.getBody().getMessage()).isEqualTo("bad value"); + } + + @Test + void handleBadRequest_returns400WithTheExceptionMessage() { + ResponseEntity response = + handler.handleBadRequest(new BadRequestException("malformed"), request); + + assertThat(response.getStatusCode()).isEqualTo(HttpStatus.BAD_REQUEST); + assertThat(response.getBody().getMessage()).isEqualTo("malformed"); + } + + @Test + void handleBadCredentials_returns401WithTheExceptionMessage() { + ResponseEntity response = + handler.handleBadCredentials(new BadCredentialsException("Invalid credentials"), request); + + assertThat(response.getStatusCode()).isEqualTo(HttpStatus.UNAUTHORIZED); + assertThat(response.getBody().getMessage()).isEqualTo("Invalid credentials"); + } + + @Test + void handleAccessDenied_returns403WithAGenericMessage() { + ResponseEntity response = + handler.handleAccessDenied(new AccessDeniedException("denied"), request); + + assertThat(response.getStatusCode()).isEqualTo(HttpStatus.FORBIDDEN); + assertThat(response.getBody().getMessage()).isEqualTo("You do not have permission to access this resource"); + } + + @Test + void handleUnprocessable_returns422WithTheExceptionMessage() { + ResponseEntity response = handler.handleUnprocessable( + new UnprocessableEntityException("no such GitHub login"), request); + + assertThat(response.getStatusCode()).isEqualTo(HttpStatus.UNPROCESSABLE_ENTITY); + assertThat(response.getBody().getMessage()).isEqualTo("no such GitHub login"); + } + + @Test + void handleConflict_returns409WithTheExceptionMessage() { + ResponseEntity response = + handler.handleConflict(new ConflictException("already exists"), request); + + assertThat(response.getStatusCode()).isEqualTo(HttpStatus.CONFLICT); + assertThat(response.getBody().getMessage()).isEqualTo("already exists"); + } + + @Test + void handleForbidden_returns403WithTheExceptionMessage() { + ResponseEntity response = + handler.handleForbidden(new ForbiddenException("not the owner"), request); + + assertThat(response.getStatusCode()).isEqualTo(HttpStatus.FORBIDDEN); + assertThat(response.getBody().getMessage()).isEqualTo("not the owner"); + } + + @Test + void handleNotFound_noSuchElementException_returns404() { + ResponseEntity response = + handler.handleNotFound(new NoSuchElementException("DataSource not found: 5"), request); + + assertThat(response.getStatusCode()).isEqualTo(HttpStatus.NOT_FOUND); + assertThat(response.getBody().getMessage()).isEqualTo("DataSource not found: 5"); + } + + @Test + void handleNotFound_notFoundException_returns404() { + ResponseEntity response = + handler.handleNotFound(new NotFoundException("Team", 5L), request); + + assertThat(response.getStatusCode()).isEqualTo(HttpStatus.NOT_FOUND); + assertThat(response.getBody().getMessage()).isEqualTo("Team not found with id: 5"); + } + + @Test + void handleNoResource_returns404NamingTheRequestUri() { + NoResourceFoundException ex = new NoResourceFoundException(HttpMethod.GET, "/api/nope"); + + ResponseEntity response = handler.handleNoResource(ex, request); + + assertThat(response.getStatusCode()).isEqualTo(HttpStatus.NOT_FOUND); + assertThat(response.getBody().getMessage()).isEqualTo("No endpoint found for /api/teams/5/export"); + } + + @Test + void handleRateLimit_returns429WithRetryAfterHeader() { + ResponseEntity response = + handler.handleRateLimit(new RateLimitExceededException(30L), request); + + assertThat(response.getStatusCode()).isEqualTo(HttpStatus.TOO_MANY_REQUESTS); + assertThat(response.getHeaders().getFirst("Retry-After")).isEqualTo("30"); + assertThat(response.getBody().getMessage()).isEqualTo("Rate limit exceeded. Retry after 30 seconds."); + } + + @Test + void handleExternalService_usesTheExceptionsOwnStatus() { + ExternalServiceException ex = new ExternalServiceException( + "Ollama unavailable", HttpStatus.SERVICE_UNAVAILABLE); + + ResponseEntity response = handler.handleExternalService(ex, request); + + assertThat(response.getStatusCode()).isEqualTo(HttpStatus.SERVICE_UNAVAILABLE); + assertThat(response.getBody().getError()).isEqualTo("Service Unavailable"); + assertThat(response.getBody().getMessage()).isEqualTo("Ollama unavailable"); + } + + @Test + void handleGitHub_returns502WithTheExceptionMessage() { + ResponseEntity response = + handler.handleGitHub(new GitHubException("rate limited"), request); + + assertThat(response.getStatusCode()).isEqualTo(HttpStatus.BAD_GATEWAY); + assertThat(response.getBody().getError()).isEqualTo("GitHub Error"); + assertThat(response.getBody().getMessage()).isEqualTo("rate limited"); + } + + @Test + void handleGit_returns500WithAGenericMessage_neverLeakingTheCause() { + GitException ex = new GitException("clone failed", new RuntimeException("disk full")); + + ResponseEntity response = handler.handleGit(ex, request); + + assertThat(response.getStatusCode()).isEqualTo(HttpStatus.INTERNAL_SERVER_ERROR); + assertThat(response.getBody().getMessage()) + .isEqualTo("A Git operation failed — see server logs for details"); + } + + @Test + void handleJira_returns502WithTheExceptionMessage() { + ResponseEntity response = + handler.handleJira(new JiraException("project not found"), request); + + assertThat(response.getStatusCode()).isEqualTo(HttpStatus.BAD_GATEWAY); + assertThat(response.getBody().getError()).isEqualTo("Jira Error"); + assertThat(response.getBody().getMessage()).isEqualTo("project not found"); + } + + @Test + void handleGeneral_returns500WithAGenericMessage_neverLeakingTheCause() { + ResponseEntity response = + handler.handleGeneral(new GeneralException("internal state invalid"), request); + + assertThat(response.getStatusCode()).isEqualTo(HttpStatus.INTERNAL_SERVER_ERROR); + assertThat(response.getBody().getMessage()) + .isEqualTo("An internal error occurred — see server logs for details"); + } + + @Test + void handleUnexpected_returns500WithAGenericMessage_neverLeakingTheCause() { + ResponseEntity response = + handler.handleUnexpected(new RuntimeException("npe somewhere"), request); + + assertThat(response.getStatusCode()).isEqualTo(HttpStatus.INTERNAL_SERVER_ERROR); + assertThat(response.getBody().getMessage()) + .isEqualTo("An unexpected error occurred — see server logs for details"); + } } diff --git a/dev-analytics/src/test/java/com/juliashtal/devanalytics/metrics/MetricTypeTest.java b/dev-analytics/src/test/java/com/juliashtal/devanalytics/metrics/MetricTypeTest.java index a5b3bd8..602510b 100644 --- a/dev-analytics/src/test/java/com/juliashtal/devanalytics/metrics/MetricTypeTest.java +++ b/dev-analytics/src/test/java/com/juliashtal/devanalytics/metrics/MetricTypeTest.java @@ -48,11 +48,6 @@ void aggregatePeriod_isSubsetOfInAiContext() { .isEmpty(); } - @Test - void totalMetricCount_isTwentyOne() { - assertThat(MetricType.values()).hasSize(21); - } - @Test void everyMetric_declaresADisplayUnit() { // A blank unit would put the export back to needing its own mapping. diff --git a/dev-analytics/src/test/java/com/juliashtal/devanalytics/metrics/calc/AggregateStorageShapeDriftTest.java b/dev-analytics/src/test/java/com/juliashtal/devanalytics/metrics/calc/AggregateStorageShapeDriftTest.java index 6dbb94b..63156d0 100644 --- a/dev-analytics/src/test/java/com/juliashtal/devanalytics/metrics/calc/AggregateStorageShapeDriftTest.java +++ b/dev-analytics/src/test/java/com/juliashtal/devanalytics/metrics/calc/AggregateStorageShapeDriftTest.java @@ -21,6 +21,8 @@ import org.mockito.junit.jupiter.MockitoExtension; import org.mockito.junit.jupiter.MockitoSettings; import org.mockito.quality.Strictness; +import org.springframework.context.annotation.ClassPathScanningCandidateComponentProvider; +import org.springframework.core.type.filter.AssignableTypeFilter; import java.sql.Date; import java.time.Instant; @@ -46,7 +48,9 @@ *

Asserted against a checked-in expected set rather than against * {@code MetricType.aggregatePeriod}, so the two cannot drift unnoticed: adding a calculator that * writes a period fails here until the type is given a reduction in - * {@link AggregateWindowResolver}.

+ * {@link AggregateWindowResolver}. {@link #buildRegistry()} is a hand-written list, which by + * itself would let a new calculator go unexercised; {@link #buildRegistry_includesEveryMetricCalculatorImplementation()} + * closes that gap with a classpath scan.

*/ @ExtendWith(MockitoExtension.class) @MockitoSettings(strictness = Strictness.LENIENT) @@ -191,6 +195,42 @@ void periodStoredTypes_allHaveAReductionDeclared() { .containsExactlyInAnyOrderElementsOf(EXPECTED_PERIOD_STORED); } + /** + * {@link #buildRegistry()} is a hand-written list of calculators; without this check a new + * {@link MetricCalculator} implementation could be added under {@code metrics.calc} and + * never be exercised by the two tests above, silently exempting it from the reduction check. + */ + @Test + void buildRegistry_includesEveryMetricCalculatorImplementation() { + Set> discovered = discoverCalculatorImplementations(); + Set> registered = buildRegistry().all().stream() + .map(Object::getClass) + .collect(Collectors.toSet()); + + assertThat(registered) + .as("a MetricCalculator implementation exists under metrics.calc that " + + "AggregateStorageShapeDriftTest.buildRegistry() does not construct — " + + "add it there so this drift test actually runs it") + .containsExactlyInAnyOrderElementsOf(discovered); + } + + private static Set> discoverCalculatorImplementations() { + ClassPathScanningCandidateComponentProvider scanner = + new ClassPathScanningCandidateComponentProvider(false); + scanner.addIncludeFilter(new AssignableTypeFilter(MetricCalculator.class)); + + return scanner.findCandidateComponents("com.juliashtal.devanalytics.metrics.calc").stream() + .map(bd -> { + try { + return Class.forName(bd.getBeanClassName()); + } catch (ClassNotFoundException e) { + throw new IllegalStateException(e); + } + }) + .filter(c -> !c.isInterface()) + .collect(Collectors.toSet()); + } + // ------------------------------------------------------------------ // harness // ------------------------------------------------------------------ diff --git a/dev-analytics/src/test/java/com/juliashtal/devanalytics/metrics/service/AggregateWindowResolverTest.java b/dev-analytics/src/test/java/com/juliashtal/devanalytics/metrics/service/AggregateWindowResolverTest.java index 4b57e1c..7bc236f 100644 --- a/dev-analytics/src/test/java/com/juliashtal/devanalytics/metrics/service/AggregateWindowResolverTest.java +++ b/dev-analytics/src/test/java/com/juliashtal/devanalytics/metrics/service/AggregateWindowResolverTest.java @@ -8,9 +8,12 @@ import java.time.LocalDate; import java.util.List; +import java.util.Map; import java.util.Optional; import static org.assertj.core.api.Assertions.assertThat; +import static org.assertj.core.api.Assertions.assertThatCode; +import static org.assertj.core.api.Assertions.assertThatThrownBy; import static org.assertj.core.api.Assertions.within; /** @@ -164,6 +167,54 @@ void everyAggregatePeriodType_isDeclaredPeriodStored() { .filter(t -> t.aggregatePeriod).toList()); } + // ------------------------------------------------------------------ + // R-NF-14: a missing reduction rule fails loudly instead of defaulting to MEDIAN + // ------------------------------------------------------------------ + + @Test + void perWindow_typeWithNoDeclaredReduction_throwsInsteadOfDefaultingToMedian() { + assertThatThrownBy(() -> resolver.perWindow( + List.of(window(W1_FROM, W1_TO, 10.0)), MetricType.DAILY_COMMITS_COUNT)) + .isInstanceOf(IllegalStateException.class) + .hasMessageContaining("DAILY_COMMITS_COUNT"); + } + + @Test + void resolve_typeWithNoDeclaredReduction_throwsInsteadOfDefaultingToMedian() { + assertThatThrownBy(() -> resolver.resolve( + List.of(window(W1_FROM, W1_TO, 10.0)), MetricType.DAILY_COMMITS_COUNT)) + .isInstanceOf(IllegalStateException.class) + .hasMessageContaining("DAILY_COMMITS_COUNT"); + } + + /** + * A DAILY-shape context metric is still passed through {@code resolve}/{@code perWindow} + * with an empty row list by callers that iterate every context type unconditionally + * (e.g. {@code AiContextBuilderService.buildTeamContext}); the missing-reduction guard must + * not fire when there is nothing to combine. + */ + @Test + void perWindow_noRowsForATypeWithNoDeclaredReduction_returnsEmptyWithoutThrowing() { + assertThat(resolver.perWindow(List.of(), MetricType.DAILY_COMMITS_COUNT)).isEmpty(); + } + + @Test + void resolve_noRowsForATypeWithNoDeclaredReduction_returnsEmptyWithoutThrowing() { + assertThat(resolver.resolve(List.of(), MetricType.DAILY_COMMITS_COUNT)).isEmpty(); + } + + @Test + void validateAggregatePeriodCoverage_missingEntryForAnAggregatePeriodType_throws() { + assertThatThrownBy(() -> AggregateWindowResolver.validateAggregatePeriodCoverage(Map.of())) + .isInstanceOf(IllegalStateException.class) + .hasMessageContaining("PR_LEAD_TIME_HOURS_MEDIAN"); + } + + @Test + void validateAggregatePeriodCoverage_realReductions_doesNotThrow() { + assertThatCode(AggregateWindowResolver::new).doesNotThrowAnyException(); + } + private static MetricSnapshot window(LocalDate from, LocalDate to, double value) { MetricSnapshot s = new MetricSnapshot(); s.setDate(from); diff --git a/dev-analytics/src/test/java/com/juliashtal/devanalytics/security/CheckHelperTest.java b/dev-analytics/src/test/java/com/juliashtal/devanalytics/security/CheckHelperTest.java new file mode 100644 index 0000000..4abe760 --- /dev/null +++ b/dev-analytics/src/test/java/com/juliashtal/devanalytics/security/CheckHelperTest.java @@ -0,0 +1,49 @@ +package com.juliashtal.devanalytics.security; + +import com.juliashtal.devanalytics.security.model.CustomUserDetails; +import com.juliashtal.devanalytics.user.model.Role; +import com.juliashtal.devanalytics.user.model.User; +import com.juliashtal.devanalytics.user.repository.UserRepository; +import org.junit.jupiter.api.AfterEach; +import org.junit.jupiter.api.Test; +import org.junit.jupiter.api.extension.ExtendWith; +import org.mockito.InjectMocks; +import org.mockito.Mock; +import org.mockito.junit.jupiter.MockitoExtension; +import org.springframework.security.authentication.UsernamePasswordAuthenticationToken; +import org.springframework.security.core.context.SecurityContextHolder; + +import static org.assertj.core.api.Assertions.assertThat; +import static org.mockito.Mockito.when; + +/** + * {@code currentUser()} must never trust a request-supplied id — it resolves strictly from the + * security context, which is what makes {@code CheckHelper.currentUser()} the safe alternative + * the project's RBAC rule requires over a body/param {@code userId}. + */ +@ExtendWith(MockitoExtension.class) +class CheckHelperTest { + + @Mock UserRepository userRepository; + @InjectMocks CheckHelper checkHelper; + + @AfterEach + void clearSecurityContext() { + SecurityContextHolder.clearContext(); + } + + @Test + void currentUser_authenticated_resolvesByIdFromTheSecurityContext() { + User principalUser = new User(); + principalUser.setId(1L); + principalUser.setRole(Role.DEVELOPER); + SecurityContextHolder.getContext().setAuthentication( + new UsernamePasswordAuthenticationToken(new CustomUserDetails(principalUser), null)); + + User referenced = new User(); + referenced.setId(1L); + when(userRepository.getReferenceById(1L)).thenReturn(referenced); + + assertThat(checkHelper.currentUser()).isSameAs(referenced); + } +} diff --git a/dev-analytics/src/test/java/com/juliashtal/devanalytics/security/JwtAuthFilterTest.java b/dev-analytics/src/test/java/com/juliashtal/devanalytics/security/JwtAuthFilterTest.java new file mode 100644 index 0000000..908965b --- /dev/null +++ b/dev-analytics/src/test/java/com/juliashtal/devanalytics/security/JwtAuthFilterTest.java @@ -0,0 +1,203 @@ +package com.juliashtal.devanalytics.security; + +import com.juliashtal.devanalytics.security.model.CustomUserDetails; +import com.juliashtal.devanalytics.security.service.CustomUserDetailsService; +import com.juliashtal.devanalytics.security.service.JwtService; +import com.juliashtal.devanalytics.user.model.Role; +import com.juliashtal.devanalytics.user.model.User; +import jakarta.servlet.FilterChain; +import jakarta.servlet.http.HttpServletRequest; +import jakarta.servlet.http.HttpServletResponse; +import org.junit.jupiter.api.AfterEach; +import org.junit.jupiter.api.BeforeEach; +import org.junit.jupiter.api.Test; +import org.junit.jupiter.api.extension.ExtendWith; +import org.mockito.Mock; +import org.mockito.junit.jupiter.MockitoExtension; +import org.mockito.junit.jupiter.MockitoSettings; +import org.mockito.quality.Strictness; +import org.springframework.security.authentication.UsernamePasswordAuthenticationToken; +import org.springframework.security.core.Authentication; +import org.springframework.security.core.context.SecurityContextHolder; +import org.springframework.security.core.userdetails.UserDetails; + +import static org.assertj.core.api.Assertions.assertThat; +import static org.mockito.ArgumentMatchers.anyString; +import static org.mockito.Mockito.never; +import static org.mockito.Mockito.verify; +import static org.mockito.Mockito.when; + +/** + * The token-version-logout invariant lives here: a JWT must carry the same {@code tokenVersion} + * as the current {@link User} row, or logout (which bumps the version) would not actually + * invalidate an outstanding access token. + */ +@ExtendWith(MockitoExtension.class) +@MockitoSettings(strictness = Strictness.LENIENT) +class JwtAuthFilterTest { + + @Mock JwtService jwtService; + @Mock CustomUserDetailsService userDetailsService; + @Mock HttpServletRequest request; + @Mock HttpServletResponse response; + @Mock FilterChain filterChain; + + private JwtAuthFilter filter; + + @BeforeEach + void setUp() { + // Built here, not as a field initializer: @Mock fields are injected by the extension + // after instance construction, so building this eagerly would capture null mocks. + filter = new JwtAuthFilter(jwtService, userDetailsService); + } + + @AfterEach + void clearSecurityContext() { + SecurityContextHolder.clearContext(); + } + + private static CustomUserDetails userDetails(long id, int tokenVersion) { + User user = new User(); + user.setId(id); + user.setUsername("alice"); + user.setRole(Role.DEVELOPER); + user.setTokenVersion(tokenVersion); + return new CustomUserDetails(user); + } + + @Test + void doFilterInternal_noAuthorizationHeader_proceedsUnauthenticated() throws Exception { + when(request.getHeader("Authorization")).thenReturn(null); + + filter.doFilterInternal(request, response, filterChain); + + assertThat(SecurityContextHolder.getContext().getAuthentication()).isNull(); + verify(filterChain).doFilter(request, response); + } + + @Test + void doFilterInternal_headerWithoutBearerPrefix_proceedsUnauthenticated() throws Exception { + when(request.getHeader("Authorization")).thenReturn("Basic dXNlcjpwYXNz"); + + filter.doFilterInternal(request, response, filterChain); + + assertThat(SecurityContextHolder.getContext().getAuthentication()).isNull(); + verify(filterChain).doFilter(request, response); + } + + @Test + void doFilterInternal_malformedToken_extractUsernameThrows_proceedsUnauthenticated() throws Exception { + when(request.getHeader("Authorization")).thenReturn("Bearer garbage"); + when(jwtService.extractUsername("garbage")).thenThrow(new RuntimeException("malformed JWT")); + + filter.doFilterInternal(request, response, filterChain); + + assertThat(SecurityContextHolder.getContext().getAuthentication()).isNull(); + verify(filterChain).doFilter(request, response); + } + + @Test + void doFilterInternal_alreadyAuthenticated_doesNotReauthenticate() throws Exception { + Authentication existing = new UsernamePasswordAuthenticationToken("someone", null); + SecurityContextHolder.getContext().setAuthentication(existing); + when(request.getHeader("Authorization")).thenReturn("Bearer valid.jwt"); + when(jwtService.extractUsername("valid.jwt")).thenReturn("alice"); + + filter.doFilterInternal(request, response, filterChain); + + assertThat(SecurityContextHolder.getContext().getAuthentication()).isSameAs(existing); + verify(userDetailsService, never()).loadUserByUsername(anyString()); + } + + @Test + void doFilterInternal_validTokenAndVersion_setsAuthenticationWithAuthorities() throws Exception { + when(request.getHeader("Authorization")).thenReturn("Bearer valid.jwt"); + when(jwtService.extractUsername("valid.jwt")).thenReturn("alice"); + CustomUserDetails details = userDetails(1L, 0); + when(userDetailsService.loadUserByUsername("alice")).thenReturn(details); + when(jwtService.isTokenValid("valid.jwt", details)).thenReturn(true); + when(jwtService.extractTokenVersion("valid.jwt")).thenReturn(0); + + filter.doFilterInternal(request, response, filterChain); + + Authentication auth = SecurityContextHolder.getContext().getAuthentication(); + assertThat(auth).isNotNull(); + assertThat(auth.getPrincipal()).isSameAs(details); + assertThat(auth.getAuthorities()).extracting(Object::toString).containsExactly("ROLE_DEVELOPER"); + verify(filterChain).doFilter(request, response); + } + + @Test + void doFilterInternal_tokenSignatureInvalid_doesNotAuthenticate() throws Exception { + when(request.getHeader("Authorization")).thenReturn("Bearer bad.jwt"); + when(jwtService.extractUsername("bad.jwt")).thenReturn("alice"); + CustomUserDetails details = userDetails(1L, 0); + when(userDetailsService.loadUserByUsername("alice")).thenReturn(details); + when(jwtService.isTokenValid("bad.jwt", details)).thenReturn(false); + + filter.doFilterInternal(request, response, filterChain); + + assertThat(SecurityContextHolder.getContext().getAuthentication()).isNull(); + } + + @Test + void doFilterInternal_tokenVersionStale_doesNotAuthenticate() throws Exception { + // The token predates a logout (which bumps the user's tokenVersion) — it must not + // still grant access. + when(request.getHeader("Authorization")).thenReturn("Bearer stale.jwt"); + when(jwtService.extractUsername("stale.jwt")).thenReturn("alice"); + CustomUserDetails details = userDetails(1L, 2); + when(userDetailsService.loadUserByUsername("alice")).thenReturn(details); + when(jwtService.isTokenValid("stale.jwt", details)).thenReturn(true); + when(jwtService.extractTokenVersion("stale.jwt")).thenReturn(1); + + filter.doFilterInternal(request, response, filterChain); + + assertThat(SecurityContextHolder.getContext().getAuthentication()).isNull(); + } + + @Test + void doFilterInternal_tokenVersionClaimMissing_doesNotAuthenticate() throws Exception { + when(request.getHeader("Authorization")).thenReturn("Bearer noversion.jwt"); + when(jwtService.extractUsername("noversion.jwt")).thenReturn("alice"); + CustomUserDetails details = userDetails(1L, 0); + when(userDetailsService.loadUserByUsername("alice")).thenReturn(details); + when(jwtService.isTokenValid("noversion.jwt", details)).thenReturn(true); + when(jwtService.extractTokenVersion("noversion.jwt")).thenReturn(null); + + filter.doFilterInternal(request, response, filterChain); + + assertThat(SecurityContextHolder.getContext().getAuthentication()).isNull(); + } + + @Test + void doFilterInternal_userDetailsNotCustomUserDetails_skipsVersionCheck() throws Exception { + when(request.getHeader("Authorization")).thenReturn("Bearer valid.jwt"); + when(jwtService.extractUsername("valid.jwt")).thenReturn("bot-user"); + UserDetails generic = org.springframework.security.core.userdetails.User + .withUsername("bot-user").password("x").authorities("ROLE_DEVELOPER").build(); + when(userDetailsService.loadUserByUsername("bot-user")).thenReturn(generic); + when(jwtService.isTokenValid("valid.jwt", generic)).thenReturn(true); + + filter.doFilterInternal(request, response, filterChain); + + // isTokenVersionValid short-circuits to true for a non-CustomUserDetails principal, + // so extractTokenVersion is never consulted. + assertThat(SecurityContextHolder.getContext().getAuthentication()).isNotNull(); + verify(jwtService, never()).extractTokenVersion(anyString()); + } + + @Test + void shouldNotFilter_authPath_returnsTrue() { + when(request.getServletPath()).thenReturn("/api/auth/login"); + + assertThat(filter.shouldNotFilter(request)).isTrue(); + } + + @Test + void shouldNotFilter_nonAuthPath_returnsFalse() { + when(request.getServletPath()).thenReturn("/api/datasources"); + + assertThat(filter.shouldNotFilter(request)).isFalse(); + } +} diff --git a/docs/adding-a-metric.md b/docs/adding-a-metric.md new file mode 100644 index 0000000..bec4fd0 --- /dev/null +++ b/docs/adding-a-metric.md @@ -0,0 +1,190 @@ +# Adding a metric + +The ordered procedure for adding one `MetricType` to the platform, matching thesis §6.2.3. +Follow the steps in order; each one's coupling is named, together with whether an omission +is caught automatically (at application startup, by a test) or not caught at all. Where it +is not caught, the step depends on the three-layer testing discipline and review, not on a +drift guard. + +Use the `add-metric` skill for the generative parts of this procedure (endpoint boilerplate, +frontend wiring). This document is the authoritative list of couplings; the skill's own +"Actual project layout" section predates the `MetricCalculator`/`MetricCalculatorRegistry` +architecture described below and should not be followed for step 3. + +## 1. Define the metric + +Write `docs/metrics/.md`: definition, formula, edge cases, attribution rule, bot +exclusion (state which case per the project's non-negotiable rule), validation plan. This +text is thesis-canonical — copied into the corresponding chapter verbatim, not reworded +after the fact. + +**Coupling:** none yet — this file is read by nothing at runtime. **Not caught** if it is +skipped; it is a documentation obligation, checked only by review and by the thesis +cross-check in step 9. + +## 2. `MetricType` enum constant + +Add the constant to `metrics/model/MetricType.java`, in the category grouping that matches +its neighbours, with: + +- `inAiContext` — true only if the AI summary should see it (step 5). +- `dailySum` — true only for a DAILY-shape metric that sums rather than averages over a + window; must imply `inAiContext` (see step 7). +- `aggregatePeriod` — true only if the metric is recomputed per ISO week + (`MetricsService.writesAggregatePeriod`, which dispatches the calculator's invocation + cadence). This is **not** the same thing as "stored in AGGREGATE shape" — see step 4. +- `unit` — non-blank; the CSV export column header. + +**Coupling:** the enum constant itself. **Caught by test** +(`MetricTypeTest.everyMetric_declaresADisplayUnit` for a blank unit; +`dailySum_isSubsetOfInAiContext` / `aggregatePeriod_isSubsetOfInAiContext` for the two flag +subset rules). The three exact-count assertions in the same file +(`inAiContext_exactlyTwelveMetrics`, `dailySum_exactlyFiveMetrics`, +`aggregatePeriod_exactlyFiveMetrics`) will fail and must be updated by hand if the new +constant sets the corresponding flag — **caught by test**, but the fix is manual, not +automatic. + +## 3. Calculator + +Implement `MetricCalculator` in `metrics/calc`, annotated `@Component`, returning the new +type (or types, for a multi-metric calculator) from `produces()`. Persist through +`MetricSnapshotWriter.save(...)` only (never write `metric_snapshots` directly — see the +project's non-negotiable rule on saving metrics). + +- DAILY shape: `periodFrom`/`periodTo` left null. +- AGGREGATE shape: `periodFrom`/`periodTo` set to the window the calculator actually + computed over. + +`MetricCalculatorRegistry` auto-discovers every `MetricCalculator` bean and validates at +startup that each `MetricType` is produced by exactly one calculator. + +**Coupling:** the calculator's registration and its type coverage. +**Caught at startup** (`MetricCalculatorRegistry`'s constructor throws +`IllegalStateException` naming the uncovered type, or the type claimed twice) — this fires on +every application boot, not only in a test. **Caught by test** independently: +`MetricCalculatorRegistryTest` proves the validation logic with stubs, and +`AggregateStorageShapeDriftTest` proves it against the real, wired calculators — including, +as of R-NF-14, a classpath scan of `metrics.calc` +(`buildRegistry_includesEveryMetricCalculatorImplementation`) that fails if a new +`MetricCalculator` class exists but was not added to that test's hand-written +`buildRegistry()` list, so a new calculator cannot go silently unexercised by the shape-drift +checks below. + +## 4. Reduction entry — AGGREGATE-shape types only + +If the calculator writes `periodFrom`/`periodTo` (AGGREGATE shape), add an entry to +`AggregateWindowResolver.REDUCTIONS` naming how several stored windows (or several +repositories sharing one window) combine into one figure: `MEDIAN`, `MEAN`, `SUM`, or +`WIDEST_WINDOW`. Storage shape is a property of the calculator, **not** of +`MetricType.aggregatePeriod` — the AGGREGATE/DAILY split is 13/8 of the 21 types, while +`aggregatePeriod` is true for only 5 of the 13 AGGREGATE types (the ones recomputed on the +ISO-week grain). Do not use the flag as a proxy for "needs a reduction." + +**Coupling:** the `REDUCTIONS` map entry, and the checked-in `EXPECTED_PERIOD_STORED` set in +`AggregateStorageShapeDriftTest`. + +- If the new type also has `aggregatePeriod = true`: **caught at startup** + (`AggregateWindowResolver`'s constructor validates every `aggregatePeriod` type has a + `Reduction`, per R-NF-14 block 1a) in addition to the test coverage below. +- For all 13 AGGREGATE types regardless of `aggregatePeriod`: **caught by test** — + `AggregateStorageShapeDriftTest.everyCalculator_periodStoredTypes_matchTheCheckedInSet` and + `.periodStoredTypes_allHaveAReductionDeclared` both fail until `EXPECTED_PERIOD_STORED` and + `REDUCTIONS` are updated together. +- At runtime, if a gap reaches production anyway: `AggregateWindowResolver.perWindow`/ + `.resolve` throw `IllegalStateException` naming the type rather than silently defaulting to + `MEDIAN` (R-NF-14 block 1b) — a loud 500, not a wrong number, but this is a **last resort**, + not a substitute for the two checks above. + +## 5. Context list — AI-context types only + +If `inAiContext = true`, add the constant to **both** +`AiContextBuilderService.CONTEXT_METRIC_TYPES` and +`MetricsAnomalyService.CONTEXT_METRIC_TYPES`. Both are hand-written, fixed-order lists (the +presentation order the model reads is a decision, not derivable from the enum), so neither +follows the flag automatically. + +**Coupling:** two independent list literals. +**Caught by test**: `AiContextBuilderServiceTest.contextMetricTypes_matchesInAiContextFlag_inBothDirections` +and `MetricsAnomalyServiceContextTypesTest.contextMetricTypes_matchesInAiContextFlag_inBothDirections` +each pin their list against `MetricType.inAiContext` in both directions, so forgetting either +list, or adding a type to one but not the other, fails one of these two tests. Neither test +adds the missing entry for you. + +If the metric is central to a thesis research question, also read +`MetricsAiService.generateSummary`'s "New context metric types" note (per the project's +"When touching the AI layer" rule) — the prompt must be told about the type, or the model +never mentions data it was silently given. + +## 6. Read endpoint + +Add a `@GetMapping` to `MetricsController` (personal scope) and, if the metric is +team-relevant, `MetricsTeamController`. Reuse `MetricSnapshotService`'s existing +parameterized query methods; add a new one only if the read shape genuinely differs. +`@PreAuthorize` is class-level on both controllers — do not repeat it per method unless the +route's rule differs from the class default (the project's RBAC non-negotiable rule). + +**Coupling:** the endpoint itself. **Not caught** by any drift guard — there is no test that +enumerates `MetricType.values()` and asserts a matching route exists. This step depends +entirely on the three required test layers (unit, integration, controller/slice) being +written for the new endpoint, per the project's testing rules, and on code review. A missing +endpoint's only indirect signal is the `datasource.controller`/`metrics.controller` package +coverage gate (Block 2) dropping if the new route ships untested — that gate does not detect +a route that was never added at all. + +## 7. Tests + +All three layers, per the project's testing rules: + +- **Unit** — the calculator's branch paths (`@ExtendWith(MockitoExtension.class)`). +- **Integration** — the repository query and, if the type is AGGREGATE-shape, that + `AggregateWindowResolver` resolves it correctly (`AggregateWindowResolverTest` has one test + per `Reduction` kind — extend it if a genuinely new combination rule is needed instead of + reusing an existing one). +- **Controller/slice** — the new endpoint's status codes, `@PreAuthorize` enforcement, DTO + shape (`@WebMvcTest` + `@WithMockUser`). + +Also update, if the new type touches what they check: + +- `MetricTypeTest` — the three exact-count assertions named in step 2. +- `AggregateStorageShapeDriftTest` — `EXPECTED_PERIOD_STORED` and `buildRegistry()` (step 3 + and 4). +- `AiContextBuilderServiceTest` / `MetricsAnomalyServiceContextTypesTest` — no edit needed; + they read the flag directly (step 5), but they will fail if the context list edit is + missing. + +## 8. Frontend + +`frontend/src/api/metrics.ts` (typed fetcher), `frontend/src/types/index.ts` if a new DTO +shape is needed, and a chart/card on `DashboardPage.tsx` (or the team dashboard). Run +`npm run build` and commit the rebuilt `static/` bundle — Maven does not build the frontend. + +**Coupling:** none checked by the backend test suite. **Not caught** by anything in this +repository; a metric can ship with a working API and no UI indefinitely. Confirm in the +browser before calling the metric done, per the project's UI verification rule. + +## 9. Thesis cross-check + +`docs/metrics/.md`'s wording must match the controller Javadoc and any UI label +verbatim (the project's thesis-code consistency rule). Manually verify the total metric +count against the thesis appendix table — this repository does not pin +`MetricType.values().length` to a fixed number (R-NF-14 removed +`MetricTypeTest.totalMetricCount_isTwentyOne`, since a raw count test needs editing on every +addition and adds no invariant beyond what steps 2–7 already enforce more specifically). +**Not caught** by any test; this is the one step with no automated backstop at all — treat it +as a required manual checklist item, not optional. + +--- + +## Coupling summary + +| Coupling | Omission caught | +|---|---| +| `MetricType` constant (unit, flag subset rules) | test | +| `MetricType` constant (exact-count assertions) | test (manual fix required) | +| Calculator registration and type coverage | startup + test | +| Reduction entry, `aggregatePeriod = true` type | startup + test | +| Reduction entry, other AGGREGATE type | test (+ runtime `IllegalStateException` as last resort) | +| Context list (both declarations) | test | +| Read endpoint | not caught | +| Frontend wiring | not caught | +| Total metric count vs. thesis appendix | not caught |