From 61fcd980abb4eebe88e70422cc675bc9f3168584 Mon Sep 17 00:00:00 2001 From: podlLev Date: Fri, 24 Jul 2026 10:16:38 +0300 Subject: [PATCH 1/3] fix(rate-limit): exclude static assets from rate limiting filter --- .../ratelimit/RateLimitingFilter.java | 17 +++++ src/main/resources/application.properties | 6 ++ .../ratelimit/RateLimitingFilterTest.java | 65 +++++++++++++++++++ 3 files changed, 88 insertions(+) diff --git a/src/main/java/com/weatherviewer/ratelimit/RateLimitingFilter.java b/src/main/java/com/weatherviewer/ratelimit/RateLimitingFilter.java index db417cc..4f6f190 100644 --- a/src/main/java/com/weatherviewer/ratelimit/RateLimitingFilter.java +++ b/src/main/java/com/weatherviewer/ratelimit/RateLimitingFilter.java @@ -21,6 +21,7 @@ import java.io.IOException; import java.util.Comparator; import java.util.List; +import java.util.Set; /** * Servlet filter that enforces per-client request rate limits, backed by @@ -43,6 +44,9 @@ public class RateLimitingFilter extends OncePerRequestFilter { private static final String RATE_LIMIT_REMAINING_HEADER = "X-RateLimit-Remaining"; private static final String API_PATH_PREFIX = "/api"; + private static final Set STATIC_PREFIXES = Set.of("/css/", "/js/", "/images/", "/static/"); + private static final Set STATIC_EXTENSIONS = Set.of(".svg", ".png", ".ico", ".css", ".js"); + private final RedisFixedWindowRateLimiter rateLimiter; private final RateLimitProperties properties; private List trustedProxyMatchers; @@ -61,6 +65,19 @@ void initTrustedProxies() { .toList(); } + @Override + protected boolean shouldNotFilter(HttpServletRequest request) { + String path = request.getRequestURI(); + if (path == null) { + return false; + } + + boolean isStaticPrefix = STATIC_PREFIXES.stream().anyMatch(path::startsWith); + boolean isStaticExt = STATIC_EXTENSIONS.stream().anyMatch(path::endsWith); + + return isStaticPrefix || isStaticExt; + } + @Override protected void doFilterInternal(@NonNull HttpServletRequest request, @NonNull HttpServletResponse response, diff --git a/src/main/resources/application.properties b/src/main/resources/application.properties index 54482fe..4cc8610 100644 --- a/src/main/resources/application.properties +++ b/src/main/resources/application.properties @@ -52,7 +52,13 @@ scalar.enabled=true scalar.theme=purple scalar.path=/scalar +# --- Locations --- +location.max-per-user=${LOCATION_MAX_PER_USER:100} +location.dashboard.page-size=${LOCATION_DASHBOARD_PAGE_SIZE:4} + # --- Weather API (OpenWeatherMap) --- +weather.dashboard.fetch-pool-size=${WEATHER_DASHBOARD_FETCH_POOL_SIZE:20} +weather.dashboard.fetch-timeout-ms=${WEATHER_DASHBOARD_FETCH_TIMEOUT_MS:4000} weather.base.api.url=https://api.openweathermap.org weather.api.url.suffix=/data/2.5/weather forecast.api.url.suffix=/data/2.5/forecast diff --git a/src/test/java/com/weatherviewer/ratelimit/RateLimitingFilterTest.java b/src/test/java/com/weatherviewer/ratelimit/RateLimitingFilterTest.java index 8bb49a9..7e78901 100644 --- a/src/test/java/com/weatherviewer/ratelimit/RateLimitingFilterTest.java +++ b/src/test/java/com/weatherviewer/ratelimit/RateLimitingFilterTest.java @@ -3,6 +3,7 @@ import com.weatherviewer.model.enums.UnitSystem; import com.weatherviewer.security.SecUser; import jakarta.servlet.FilterChain; +import jakarta.servlet.http.HttpServletRequest; import org.junit.jupiter.api.AfterEach; import org.junit.jupiter.api.BeforeEach; import org.junit.jupiter.api.Test; @@ -18,6 +19,8 @@ import java.util.UUID; import static org.assertj.core.api.Assertions.assertThat; +import static org.junit.jupiter.api.Assertions.assertFalse; +import static org.junit.jupiter.api.Assertions.assertTrue; import static org.mockito.Mockito.*; @ExtendWith(MockitoExtension.class) @@ -545,4 +548,66 @@ void doFilter_withNullPathPrefixInRules_shouldIgnoreItAndFallbackToDefaultRule() eq(properties.getDefaultWindowSeconds())); } + @Test + void shouldNotFilter_nullPath_returnsFalse() throws Exception { + HttpServletRequest request = mock(HttpServletRequest.class); + when(request.getRequestURI()).thenReturn(null); + + when(rateLimiter.tryConsume(anyString(), anyInt(), anyInt())) + .thenReturn(new RedisFixedWindowRateLimiter.RateLimitResult(true, 10, 0)); + + MockHttpServletResponse response = new MockHttpServletResponse(); + FilterChain filterChain = mock(FilterChain.class); + + filter.doFilter(request, response, filterChain); + + verify(rateLimiter).tryConsume(anyString(), anyInt(), anyInt()); + verify(filterChain).doFilter(request, response); + } + + @Test + void shouldNotFilter_noMatches_coversFalseBranch() throws Exception { + MockHttpServletRequest request = new MockHttpServletRequest("GET", "/api/weather"); + MockHttpServletResponse response = new MockHttpServletResponse(); + FilterChain filterChain = mock(FilterChain.class); + + when(rateLimiter.tryConsume(anyString(), anyInt(), anyInt())) + .thenReturn(new RedisFixedWindowRateLimiter.RateLimitResult(true, 10, 0)); + + filter.doFilter(request, response, filterChain); + + verify(rateLimiter).tryConsume(anyString(), anyInt(), anyInt()); + verify(filterChain).doFilter(request, response); + } + + @Test + void shouldNotFilter_matchingExtensionWithNonStaticPrefix_coversSecondBranch() throws Exception { + MockHttpServletRequest request = new MockHttpServletRequest("GET", "/api/logo.png"); + MockHttpServletResponse response = new MockHttpServletResponse(); + FilterChain filterChain = mock(FilterChain.class); + + filter.doFilter(request, response, filterChain); + + verifyNoInteractions(rateLimiter); + verify(filterChain).doFilter(request, response); + } + + @Test + void shouldNotFilter_prefixMatch_returnsTrue() { + MockHttpServletRequest request = new MockHttpServletRequest("GET", "/images/icon"); + assertTrue(filter.shouldNotFilter(request)); + } + + @Test + void shouldNotFilter_extensionMatchOnly_returnsTrue() { + MockHttpServletRequest request = new MockHttpServletRequest("GET", "/api/icon.svg"); + assertTrue(filter.shouldNotFilter(request)); + } + + @Test + void shouldNotFilter_noMatch_returnsFalse() { + MockHttpServletRequest request = new MockHttpServletRequest("GET", "/api/weather"); + assertFalse(filter.shouldNotFilter(request)); + } + } From c14f3aedd60564ba9b1ceb5f985e0f73a408a880 Mon Sep 17 00:00:00 2001 From: podlLev Date: Fri, 24 Jul 2026 10:17:22 +0300 Subject: [PATCH 2/3] refactor(home): handle per-location weather fetch failures gracefully --- .../com/weatherviewer/config/AppConfig.java | 21 +++ .../controller/HomeController.java | 117 ++++++++++----- src/main/resources/templates/home.html | 22 +++ .../controller/HomeControllerTest.java | 133 +++++++++++++----- 4 files changed, 226 insertions(+), 67 deletions(-) diff --git a/src/main/java/com/weatherviewer/config/AppConfig.java b/src/main/java/com/weatherviewer/config/AppConfig.java index 262fc7c..33df369 100644 --- a/src/main/java/com/weatherviewer/config/AppConfig.java +++ b/src/main/java/com/weatherviewer/config/AppConfig.java @@ -4,6 +4,7 @@ import org.springframework.context.annotation.Bean; import org.springframework.context.annotation.Configuration; import org.springframework.http.client.SimpleClientHttpRequestFactory; +import org.springframework.scheduling.concurrent.CustomizableThreadFactory; import org.springframework.security.web.DefaultRedirectStrategy; import org.springframework.security.web.RedirectStrategy; import org.springframework.web.client.RestClient; @@ -11,6 +12,8 @@ import org.springframework.web.servlet.i18n.SessionLocaleResolver; import java.util.Locale; +import java.util.concurrent.ExecutorService; +import java.util.concurrent.Executors; /** * General-purpose application beans that don't belong to a more specific @@ -51,4 +54,22 @@ public LocaleResolver localeResolver() { return slr; } + /** + * Dedicated, bounded thread pool used by + * {@link com.weatherviewer.controller.HomeController} to fetch weather + * for a user's saved locations concurrently. + *

+ * Deliberately separate from the JVM-wide common {@code ForkJoinPool} + * (what a bare {@code parallelStream()} would use): that pool is shared + * with unrelated parallel streams elsewhere in the JVM and has no + * request-scoped bound, so a user with many saved locations could + * starve it for everyone. Sized via {@code weather.dashboard.fetch-pool-size} + * (default 20) and shut down automatically on context close. + */ + @Bean(destroyMethod = "shutdown") + public ExecutorService weatherFetchExecutor( + @Value("${weather.dashboard.fetch-pool-size:20}") int poolSize) { + return Executors.newFixedThreadPool(poolSize, new CustomizableThreadFactory("weather-fetch-")); + } + } diff --git a/src/main/java/com/weatherviewer/controller/HomeController.java b/src/main/java/com/weatherviewer/controller/HomeController.java index 04c271c..23d935c 100644 --- a/src/main/java/com/weatherviewer/controller/HomeController.java +++ b/src/main/java/com/weatherviewer/controller/HomeController.java @@ -8,16 +8,20 @@ import com.weatherviewer.service.helper.UnitConverter; import lombok.RequiredArgsConstructor; import lombok.extern.slf4j.Slf4j; +import org.springframework.beans.factory.annotation.Value; +import org.springframework.data.domain.Page; +import org.springframework.data.domain.PageRequest; import org.springframework.security.core.annotation.AuthenticationPrincipal; import org.springframework.stereotype.Controller; import org.springframework.ui.Model; import org.springframework.web.bind.annotation.*; import org.springframework.web.servlet.mvc.support.RedirectAttributes; -import java.util.LinkedHashMap; -import java.util.List; -import java.util.Map; -import java.util.UUID; +import java.util.*; +import java.util.concurrent.CompletableFuture; +import java.util.concurrent.CompletionException; +import java.util.concurrent.ExecutorService; +import java.util.concurrent.TimeUnit; import java.util.function.Function; import java.util.stream.Collectors; @@ -34,79 +38,128 @@ public class HomeController { private final WeatherApiService weatherApiService; private final LocationService locationService; private final UnitConverter unitConverter; + private final ExecutorService weatherFetchExecutor; + + /** Per-location timeout for the dashboard's concurrent weather fetches. */ + @Value("${weather.dashboard.fetch-timeout-ms:4000}") + private long weatherFetchTimeoutMs; + + /** Locations shown per dashboard page. Not user-adjustable, to keep the weather fan-out per request bounded. */ + @Value("${location.dashboard.page-size:12}") + private int dashboardPageSize; /** - * Renders the dashboard. Locations are fetched pre-sorted by - * {@code sort} ({@code nameAsc}, {@code nameDesc}, {@code favoriteFirst}, - * {@code favoritesOnly}, or the default {@code date}), then current - * weather is fetched for all of them in parallel to keep page load fast - * when a user has many saved locations. + * Renders one page of the dashboard. Locations are fetched pre-sorted + * and pre-paged by {@code sort} ({@code nameAsc}, {@code nameDesc}, + * {@code favoriteFirst}, {@code favoritesOnly}, or the default + * {@code date}) and {@code page} (0-based; page size is fixed via + * {@code location.dashboard.page-size}), then current weather is + * fetched for just that page's locations concurrently on a bounded + * pool ({@link com.weatherviewer.config.AppConfig#weatherFetchExecutor}) + * to keep page load fast without competing with the JVM's shared + * common {@code ForkJoinPool}. + *

+ * Each fetch is capped at {@code weather.dashboard.fetch-timeout-ms} + * and failures are isolated per location: a single slow or failing + * provider call surfaces that one location as unavailable instead of + * failing the whole dashboard. */ @GetMapping("/") public String home(Model model, @AuthenticationPrincipal SecUser user, - @RequestParam(required = false, defaultValue = "date") String sort) { - log.info("Home page requested by user '{}', sort={}", user.getUsername(), sort); - - List userLocations = switch (sort.toLowerCase()) { - case "nameasc" -> locationService.getByUserIdSortedByNameAsc(user.getId()); - case "namedesc" -> locationService.getByUserIdSortedByNameDesc(user.getId()); - case "favoritefirst" -> locationService.getByUserIdSortedByFavorite(user.getId()); - case "favoritesonly" -> locationService.getFavoritesByUserId(user.getId()); - default -> locationService.getByUserIdSortedByDate(user.getId()); - }; - - if (!userLocations.isEmpty()) { - Map locationWeatherMap = userLocations.parallelStream() + @RequestParam(required = false, defaultValue = "date") String sort, + @RequestParam(required = false, defaultValue = "0") int page) { + int safePage = Math.max(page, 0); + log.info("Home page requested by user '{}', sort={}, page={}", user.getUsername(), sort, safePage); + + Page locationPage = locationService.getByUserIdSorted( + user.getId(), sort, PageRequest.of(safePage, dashboardPageSize)); + List pageLocations = locationPage.getContent(); + + if (!pageLocations.isEmpty()) { + Map> pendingWeather = pageLocations.stream() .collect(Collectors.toMap( Function.identity(), - location -> unitConverter.toDisplayUnits( - weatherApiService.getWeatherByLocation(location), user.getUnits()), + location -> CompletableFuture + .supplyAsync(() -> weatherApiService.getWeatherByLocation(location), + weatherFetchExecutor) + .orTimeout(weatherFetchTimeoutMs, TimeUnit.MILLISECONDS), (existing, replacement) -> existing, LinkedHashMap::new )); + + Map locationWeatherMap = new LinkedHashMap<>(); + List unavailableLocationNames = new ArrayList<>(); + + for (Map.Entry> entry : pendingWeather.entrySet()) { + LocationDto location = entry.getKey(); + try { + WeatherDto weather = entry.getValue().join(); + locationWeatherMap.put(location, unitConverter.toDisplayUnits(weather, user.getUnits())); + } catch (CompletionException ex) { + log.warn("Weather fetch failed for location '{}' (user '{}'): {}", + location.getName(), user.getUsername(), ex.getCause() != null + ? ex.getCause().getMessage() : ex.getMessage()); + unavailableLocationNames.add(location.getName()); + } + } + model.addAttribute("locationWeatherMap", locationWeatherMap); - log.info("Weather data prepared for {} locations", locationWeatherMap.size()); + if (!unavailableLocationNames.isEmpty()) { + model.addAttribute("errorMessages", unavailableLocationNames.stream() + .map(name -> "Weather for \"" + name + "\" is temporarily unavailable") + .toList()); + } + log.info("Weather data prepared for {} of {} locations on page {}", + locationWeatherMap.size(), pageLocations.size(), safePage); } model.addAttribute("login", user.getFullName()); model.addAttribute("sort", sort); + model.addAttribute("currentPage", locationPage.getNumber()); + model.addAttribute("totalPages", locationPage.getTotalPages()); + model.addAttribute("totalLocations", locationPage.getTotalElements()); + model.addAttribute("hasPreviousPage", locationPage.hasPrevious()); + model.addAttribute("hasNextPage", locationPage.hasNext()); model.addAttribute("temperatureSymbol", unitConverter.temperatureSymbol(user.getUnits())); model.addAttribute("windSpeedUnit", unitConverter.windSpeedUnit(user.getUnits())); return "home"; } - /** Deletes a saved location (ownership-checked) and redirects back to the dashboard preserving the current sort. */ + /** Deletes a saved location (ownership-checked) and redirects back to the dashboard preserving the current sort/page. */ @DeleteMapping("/locations/{id}") public String deleteLocation(@PathVariable UUID id, @AuthenticationPrincipal SecUser user, RedirectAttributes redirectAttributes, - @RequestParam(required = false, defaultValue = "date") String sort) { + @RequestParam(required = false, defaultValue = "date") String sort, + @RequestParam(required = false, defaultValue = "0") int page) { log.info("User '{}' is deleting location with id={}", user.getUsername(), id); locationService.deleteByIdAndUserId(id, user.getId()); redirectAttributes.addFlashAttribute("successMessage", "Location deleted successfully"); - return "redirect:/?sort=" + sort; + return "redirect:/?sort=" + sort + "&page=" + page; } @PostMapping("/locations/{id}/favorite") public String addToFavorite(@PathVariable UUID id, @AuthenticationPrincipal SecUser user, RedirectAttributes redirectAttributes, - @RequestParam(required = false, defaultValue = "date") String sort) { + @RequestParam(required = false, defaultValue = "date") String sort, + @RequestParam(required = false, defaultValue = "0") int page) { log.info("User {} is adding location {} to favorites", user.getUsername(), id); locationService.addToFavorite(id, user.getId()); redirectAttributes.addFlashAttribute("successMessage", "Location added to favorites"); - return "redirect:/?sort=" + sort; + return "redirect:/?sort=" + sort + "&page=" + page; } @DeleteMapping("/locations/{id}/favorite") public String removeFromFavorite(@PathVariable UUID id, @AuthenticationPrincipal SecUser user, RedirectAttributes redirectAttributes, - @RequestParam(required = false, defaultValue = "date") String sort) { + @RequestParam(required = false, defaultValue = "date") String sort, + @RequestParam(required = false, defaultValue = "0") int page) { log.info("User {} is removing location {} from favorites", user.getUsername(), id); locationService.removeFromFavorite(id, user.getId()); redirectAttributes.addFlashAttribute("successMessage", "Location removed from favorites"); - return "redirect:/?sort=" + sort; + return "redirect:/?sort=" + sort + "&page=" + page; } } diff --git a/src/main/resources/templates/home.html b/src/main/resources/templates/home.html index bb19669..63695ad 100644 --- a/src/main/resources/templates/home.html +++ b/src/main/resources/templates/home.html @@ -45,6 +45,7 @@

Location

+ @@ -52,6 +53,7 @@

Location

+ @@ -72,16 +74,19 @@

Location

+
+
+
@@ -198,6 +203,23 @@

Location Required for Weather Information

Please add a location to view weather. You can do it by using the search bar above.

+ +
diff --git a/src/test/java/com/weatherviewer/controller/HomeControllerTest.java b/src/test/java/com/weatherviewer/controller/HomeControllerTest.java index b8d95f1..ec5458a 100644 --- a/src/test/java/com/weatherviewer/controller/HomeControllerTest.java +++ b/src/test/java/com/weatherviewer/controller/HomeControllerTest.java @@ -12,6 +12,10 @@ import org.springframework.beans.factory.annotation.Autowired; import org.springframework.boot.test.autoconfigure.web.servlet.AutoConfigureMockMvc; import org.springframework.boot.test.context.SpringBootTest; +import org.springframework.data.domain.Page; +import org.springframework.data.domain.PageImpl; +import org.springframework.data.domain.PageRequest; +import org.springframework.data.domain.Pageable; import org.springframework.test.context.bean.override.mockito.MockitoBean; import org.springframework.test.web.servlet.MockMvc; @@ -20,6 +24,7 @@ import java.util.Set; import java.util.UUID; +import static org.mockito.ArgumentMatchers.*; import static org.mockito.Mockito.verify; import static org.mockito.Mockito.when; import static org.springframework.security.test.web.servlet.request.SecurityMockMvcRequestPostProcessors.csrf; @@ -74,15 +79,20 @@ private WeatherDto weatherDto() { .setDate(new Date()); } + private Page pageOf(List locations, Pageable pageable) { + return new PageImpl<>(locations, pageable, locations.size()); + } + @Test void home_returns200AndHomeView() throws Exception { SecUser secUser = secUser(); - when(locationService.getByUserIdSortedByDate(secUser.getId())).thenReturn(List.of()); + when(locationService.getByUserIdSorted(eq(secUser.getId()), anyString(), any(Pageable.class))) + .thenReturn(pageOf(List.of(), PageRequest.of(0, 12))); mockMvc.perform(get("/").with(user(secUser))) .andExpect(status().isOk()) .andExpect(view().name("home")) - .andExpect(model().attributeExists("login", "sort")); + .andExpect(model().attributeExists("login", "sort", "currentPage", "totalPages")); } @Test @@ -91,7 +101,8 @@ void home_withLocations_addsWeatherMap() throws Exception { LocationDto location = locationDto(); WeatherDto weather = weatherDto(); - when(locationService.getByUserIdSortedByDate(secUser.getId())).thenReturn(List.of(location)); + when(locationService.getByUserIdSorted(eq(secUser.getId()), anyString(), any(Pageable.class))) + .thenReturn(pageOf(List.of(location), PageRequest.of(0, 12))); when(weatherApiService.getWeatherByLocation(location)).thenReturn(weather); mockMvc.perform(get("/").with(user(secUser))) @@ -100,62 +111,78 @@ void home_withLocations_addsWeatherMap() throws Exception { } @Test - void home_sortNameAsc_callsCorrectService() throws Exception { + void home_sortNameAsc_passesSortToService() throws Exception { SecUser secUser = secUser(); - when(locationService.getByUserIdSortedByNameAsc(secUser.getId())).thenReturn(List.of()); + when(locationService.getByUserIdSorted(eq(secUser.getId()), eq("nameAsc"), any(Pageable.class))) + .thenReturn(pageOf(List.of(), PageRequest.of(0, 12))); mockMvc.perform(get("/").with(user(secUser)).param("sort", "nameAsc")) .andExpect(status().isOk()); - verify(locationService).getByUserIdSortedByNameAsc(secUser.getId()); + verify(locationService).getByUserIdSorted(eq(secUser.getId()), eq("nameAsc"), any(Pageable.class)); } @Test - void home_sortNameDesc_callsCorrectService() throws Exception { + void home_pageParam_isForwardedToService() throws Exception { SecUser secUser = secUser(); - when(locationService.getByUserIdSortedByNameDesc(secUser.getId())).thenReturn(List.of()); + when(locationService.getByUserIdSorted(eq(secUser.getId()), anyString(), eq(PageRequest.of(2, 12)))) + .thenReturn(pageOf(List.of(), PageRequest.of(2, 12))); - mockMvc.perform(get("/").with(user(secUser)).param("sort", "nameDesc")) - .andExpect(status().isOk()); + mockMvc.perform(get("/").with(user(secUser)).param("page", "2")) + .andExpect(status().isOk()) + .andExpect(model().attribute("currentPage", 2)); - verify(locationService).getByUserIdSortedByNameDesc(secUser.getId()); + verify(locationService).getByUserIdSorted(eq(secUser.getId()), anyString(), eq(PageRequest.of(2, 12))); } @Test - void home_sortFavoriteFirst_callsCorrectService() throws Exception { + void home_negativePageParam_isClampedToZero() throws Exception { SecUser secUser = secUser(); - when(locationService.getByUserIdSortedByFavorite(secUser.getId())).thenReturn(List.of()); + when(locationService.getByUserIdSorted(eq(secUser.getId()), anyString(), eq(PageRequest.of(0, 12)))) + .thenReturn(pageOf(List.of(), PageRequest.of(0, 12))); - mockMvc.perform(get("/").with(user(secUser)).param("sort", "favoriteFirst")) + mockMvc.perform(get("/").with(user(secUser)).param("page", "-5")) .andExpect(status().isOk()); - verify(locationService).getByUserIdSortedByFavorite(secUser.getId()); + verify(locationService).getByUserIdSorted(eq(secUser.getId()), anyString(), eq(PageRequest.of(0, 12))); } @Test - void home_sortFavoritesOnly_callsCorrectService() throws Exception { + void home_paginationAttributes_reflectPageResult() throws Exception { SecUser secUser = secUser(); - when(locationService.getFavoritesByUserId(secUser.getId())).thenReturn(List.of()); + LocationDto location = locationDto(); + WeatherDto weather = weatherDto(); + Page page = new PageImpl<>(List.of(location), PageRequest.of(0, 12), 25); - mockMvc.perform(get("/").with(user(secUser)).param("sort", "favoritesOnly")) - .andExpect(status().isOk()); + when(locationService.getByUserIdSorted(eq(secUser.getId()), anyString(), any(Pageable.class))) + .thenReturn(page); + when(weatherApiService.getWeatherByLocation(location)).thenReturn(weather); - verify(locationService).getFavoritesByUserId(secUser.getId()); + mockMvc.perform(get("/").with(user(secUser))) + .andExpect(status().isOk()) + .andExpect(model().attribute("totalPages", page.getTotalPages())) + .andExpect(model().attribute("totalLocations", page.getTotalElements())) + .andExpect(model().attribute("hasNextPage", true)) + .andExpect(model().attribute("hasPreviousPage", false)); } @Test - void home_duplicateLocations_mergesFavoringExisting() throws Exception { + void home_oneLocationFailsToFetch_othersStillRenderAndFailureIsFlagged() throws Exception { SecUser secUser = secUser(); - LocationDto location = locationDto(); + LocationDto okLocation = locationDto().setName("Kyiv"); + LocationDto failingLocation = locationDto().setName("Atlantis"); WeatherDto weather = weatherDto(); - when(locationService.getByUserIdSortedByDate(secUser.getId())) - .thenReturn(List.of(location, location)); - when(weatherApiService.getWeatherByLocation(location)).thenReturn(weather); + when(locationService.getByUserIdSorted(eq(secUser.getId()), anyString(), any(Pageable.class))) + .thenReturn(pageOf(List.of(okLocation, failingLocation), PageRequest.of(0, 12))); + when(weatherApiService.getWeatherByLocation(okLocation)).thenReturn(weather); + when(weatherApiService.getWeatherByLocation(failingLocation)) + .thenThrow(new RuntimeException("provider unavailable")); mockMvc.perform(get("/").with(user(secUser))) .andExpect(status().isOk()) - .andExpect(model().attributeExists("locationWeatherMap")); + .andExpect(model().attributeExists("locationWeatherMap")) + .andExpect(model().attributeExists("errorMessages")); } @Test @@ -165,45 +192,81 @@ void home_unauthenticated_redirectsToSignIn() throws Exception { } @Test - void deleteLocation_redirectsToHome() throws Exception { + void deleteLocation_redirectsToHomePreservingSortAndPage() throws Exception { SecUser secUser = secUser(); UUID locationId = UUID.randomUUID(); mockMvc.perform(delete("/locations/{id}", locationId) .with(user(secUser)) - .with(csrf())) + .with(csrf()) + .param("sort", "nameAsc") + .param("page", "1")) .andExpect(status().is3xxRedirection()) - .andExpect(redirectedUrlPattern("/?sort=*")); + .andExpect(redirectedUrl("/?sort=nameAsc&page=1")); verify(locationService).deleteByIdAndUserId(locationId, secUser.getId()); } @Test - void addToFavorite_redirectsToHome() throws Exception { + void addToFavorite_redirectsToHomePreservingSortAndPage() throws Exception { SecUser secUser = secUser(); UUID locationId = UUID.randomUUID(); mockMvc.perform(post("/locations/{id}/favorite", locationId) .with(user(secUser)) - .with(csrf())) + .with(csrf()) + .param("sort", "favoriteFirst") + .param("page", "3")) .andExpect(status().is3xxRedirection()) - .andExpect(redirectedUrlPattern("/?sort=*")); + .andExpect(redirectedUrl("/?sort=favoriteFirst&page=3")); verify(locationService).addToFavorite(locationId, secUser.getId()); } @Test - void removeFromFavorite_redirectsToHome() throws Exception { + void removeFromFavorite_redirectsToHomePreservingSortAndPage() throws Exception { SecUser secUser = secUser(); UUID locationId = UUID.randomUUID(); mockMvc.perform(delete("/locations/{id}/favorite", locationId) .with(user(secUser)) - .with(csrf())) + .with(csrf()) + .param("sort", "date") + .param("page", "0")) .andExpect(status().is3xxRedirection()) - .andExpect(redirectedUrlPattern("/?sort=*")); + .andExpect(redirectedUrl("/?sort=date&page=0")); verify(locationService).removeFromFavorite(locationId, secUser.getId()); } + @Test + void home_withDuplicateLocations_executesMapMergeFunction() throws Exception { + SecUser secUser = secUser(); + LocationDto location = locationDto(); + WeatherDto weather = weatherDto(); + + when(locationService.getByUserIdSorted(eq(secUser.getId()), anyString(), any(Pageable.class))) + .thenReturn(pageOf(List.of(location, location), PageRequest.of(0, 12))); + when(weatherApiService.getWeatherByLocation(location)).thenReturn(weather); + + mockMvc.perform(get("/").with(user(secUser))) + .andExpect(status().isOk()); + } + + @Test + void home_completionExceptionWithoutCause_usesDirectExceptionMessage() throws Exception { + SecUser secUser = secUser(); + LocationDto location = locationDto(); + + when(locationService.getByUserIdSorted(eq(secUser.getId()), anyString(), any(Pageable.class))) + .thenReturn(pageOf(List.of(location), PageRequest.of(0, 12))); + + when(weatherApiService.getWeatherByLocation(location)) + .thenThrow(new java.util.concurrent.CompletionException("Direct completion error", null)); + + mockMvc.perform(get("/").with(user(secUser))) + .andExpect(status().isOk()) + .andExpect(model().attributeExists("errorMessages")); + } + } From 50e211d4c23f1e0f7153256230208d4482e705a5 Mon Sep 17 00:00:00 2001 From: podlLev Date: Fri, 24 Jul 2026 10:17:41 +0300 Subject: [PATCH 3/3] feat(location): introduce location limit constraint validation and tests --- .../com/weatherviewer/dto/AddLocationDto.java | 2 + .../repository/LocationRepository.java | 16 ++++ .../service/LocationService.java | 21 ++++ .../service/impl/LocationServiceImpl.java | 27 ++++++ .../validation/annotation/LocationLimit.java | 31 ++++++ .../validator/LocationLimitValidator.java | 51 ++++++++++ .../weatherviewer/dto/AddLocationDtoTest.java | 3 +- .../service/impl/LocationServiceImplTest.java | 96 +++++++++++++++++++ .../validator/LocationLimitValidatorTest.java | 82 ++++++++++++++++ 9 files changed, 328 insertions(+), 1 deletion(-) create mode 100644 src/main/java/com/weatherviewer/validation/annotation/LocationLimit.java create mode 100644 src/main/java/com/weatherviewer/validation/validator/LocationLimitValidator.java create mode 100644 src/test/java/com/weatherviewer/validation/validator/LocationLimitValidatorTest.java diff --git a/src/main/java/com/weatherviewer/dto/AddLocationDto.java b/src/main/java/com/weatherviewer/dto/AddLocationDto.java index 439e209..665afb5 100644 --- a/src/main/java/com/weatherviewer/dto/AddLocationDto.java +++ b/src/main/java/com/weatherviewer/dto/AddLocationDto.java @@ -1,6 +1,7 @@ package com.weatherviewer.dto; import com.weatherviewer.validation.annotation.Latitude; +import com.weatherviewer.validation.annotation.LocationLimit; import com.weatherviewer.validation.annotation.Longitude; import com.weatherviewer.validation.annotation.UniqueLocation; import io.swagger.v3.oas.annotations.media.Schema; @@ -19,6 +20,7 @@ @ToString @Accessors(chain = true) @UniqueLocation +@LocationLimit @Schema(description = "Payload for creating a saved location") public class AddLocationDto { diff --git a/src/main/java/com/weatherviewer/repository/LocationRepository.java b/src/main/java/com/weatherviewer/repository/LocationRepository.java index 6b6e2dd..137468a 100644 --- a/src/main/java/com/weatherviewer/repository/LocationRepository.java +++ b/src/main/java/com/weatherviewer/repository/LocationRepository.java @@ -1,6 +1,8 @@ package com.weatherviewer.repository; import com.weatherviewer.model.Location; +import org.springframework.data.domain.Page; +import org.springframework.data.domain.Pageable; import org.springframework.data.jpa.repository.JpaRepository; import org.springframework.stereotype.Repository; @@ -21,6 +23,20 @@ public interface LocationRepository extends JpaRepository { /** Returns all locations saved by the given user, in no particular order. */ List findByUserId(UUID userId); + /** + * Returns one page of a user's locations, sorted per the given + * {@link org.springframework.data.domain.Pageable}'s + * {@link org.springframework.data.domain.Sort}. Backs the paginated + * home dashboard. + */ + Page findByUserId(UUID userId, Pageable pageable); + + /** Returns one page of a user's favorited locations, sorted per the given Pageable's Sort. */ + Page findByUserIdAndFavoriteTrue(UUID userId, Pageable pageable); + + /** Counts how many locations the given user has saved, used to enforce the per-user cap. */ + long countByUserId(UUID userId); + /** Returns a user's locations, most recently added first. */ List findByUserIdOrderByCreatedAtDesc(UUID userId); diff --git a/src/main/java/com/weatherviewer/service/LocationService.java b/src/main/java/com/weatherviewer/service/LocationService.java index 65f5257..fa1f220 100644 --- a/src/main/java/com/weatherviewer/service/LocationService.java +++ b/src/main/java/com/weatherviewer/service/LocationService.java @@ -48,6 +48,27 @@ public interface LocationService { /** Returns all locations saved by the given user, unsorted. */ List getByUserId(UUID userId); + /** + * Counts how many locations the given user currently has saved. + * Backs {@link com.weatherviewer.validation.annotation.LocationLimit}, + * which rejects new locations once a user hits the configured cap + * ({@code location.max-per-user}) — without this, a single user could + * save an unbounded number of locations and force the dashboard to + * fan out an unbounded number of weather calls on every page load. + */ + long countByUserId(UUID userId); + + /** + * Returns one page of a user's saved locations in the given dashboard + * sort order ({@code date}, {@code nameAsc}, {@code nameDesc}, + * {@code favoriteFirst}, or {@code favoritesOnly} — unrecognized/blank + * values fall back to {@code date}). Backs the paginated home + * dashboard, so a user with many saved locations only loads (and + * fetches weather for) one page's worth per request instead of + * everything at once. + */ + Page getByUserIdSorted(UUID userId, String sort, Pageable pageable); + /** Looks up a user's saved location by its exact coordinates. */ LocationDto getByCoordinatesAndUserId(Double latitude, Double longitude, UUID userId); diff --git a/src/main/java/com/weatherviewer/service/impl/LocationServiceImpl.java b/src/main/java/com/weatherviewer/service/impl/LocationServiceImpl.java index 2bac840..5d01a03 100644 --- a/src/main/java/com/weatherviewer/service/impl/LocationServiceImpl.java +++ b/src/main/java/com/weatherviewer/service/impl/LocationServiceImpl.java @@ -9,7 +9,9 @@ import com.weatherviewer.service.LocationService; import lombok.RequiredArgsConstructor; import org.springframework.data.domain.Page; +import org.springframework.data.domain.PageRequest; import org.springframework.data.domain.Pageable; +import org.springframework.data.domain.Sort; import org.springframework.security.access.AccessDeniedException; import org.springframework.stereotype.Service; import org.springframework.transaction.annotation.Transactional; @@ -68,6 +70,31 @@ public List getByUserId(UUID userId) { return locationMapper.toDtoList(locations); } + @Override + public long countByUserId(UUID userId) { + return locationRepository.countByUserId(userId); + } + + @Override + public Page getByUserIdSorted(UUID userId, String sort, Pageable pageable) { + String normalizedSort = sort == null ? "date" : sort.toLowerCase(); + + if ("favoritesonly".equals(normalizedSort)) { + Pageable request = PageRequest.of(pageable.getPageNumber(), pageable.getPageSize(), + Sort.by(Sort.Direction.DESC, "createdAt")); + return locationRepository.findByUserIdAndFavoriteTrue(userId, request).map(locationMapper::toDto); + } + + Sort sortOrder = switch (normalizedSort) { + case "nameasc" -> Sort.by(Sort.Direction.ASC, "name"); + case "namedesc" -> Sort.by(Sort.Direction.DESC, "name"); + case "favoritefirst" -> Sort.by(Sort.Order.desc("favorite"), Sort.Order.desc("createdAt")); + default -> Sort.by(Sort.Direction.DESC, "createdAt"); + }; + Pageable request = PageRequest.of(pageable.getPageNumber(), pageable.getPageSize(), sortOrder); + return locationRepository.findByUserId(userId, request).map(locationMapper::toDto); + } + @Override public LocationDto getByCoordinatesAndUserId(Double latitude, Double longitude, UUID userId) { Location location = locationRepository.findByLatitudeAndLongitudeAndUserId(latitude, longitude, userId); diff --git a/src/main/java/com/weatherviewer/validation/annotation/LocationLimit.java b/src/main/java/com/weatherviewer/validation/annotation/LocationLimit.java new file mode 100644 index 0000000..90221f5 --- /dev/null +++ b/src/main/java/com/weatherviewer/validation/annotation/LocationLimit.java @@ -0,0 +1,31 @@ +package com.weatherviewer.validation.annotation; + +import com.weatherviewer.validation.validator.LocationLimitValidator; +import jakarta.validation.Constraint; +import jakarta.validation.Payload; + +import java.lang.annotation.*; + +/** + * Class-level constraint on {@link com.weatherviewer.dto.AddLocationDto} + * rejecting a new location once the owning user has reached the + * configured per-user cap ({@code location.max-per-user}, default 100). + *

+ * Without this, a single account (or a script hitting + * {@code /api/v1/locations/my} directly) could save an unbounded number + * of locations, and every dashboard load fans out one weather call per + * saved location. See {@link LocationLimitValidator}. + */ +@Documented +@Constraint(validatedBy = LocationLimitValidator.class) +@Target(ElementType.TYPE) +@Retention(RetentionPolicy.RUNTIME) +public @interface LocationLimit { + + String message() default "Maximum number of saved locations reached"; + + Class[] groups() default {}; + + Class[] payload() default {}; + +} diff --git a/src/main/java/com/weatherviewer/validation/validator/LocationLimitValidator.java b/src/main/java/com/weatherviewer/validation/validator/LocationLimitValidator.java new file mode 100644 index 0000000..d563157 --- /dev/null +++ b/src/main/java/com/weatherviewer/validation/validator/LocationLimitValidator.java @@ -0,0 +1,51 @@ +package com.weatherviewer.validation.validator; + +import com.weatherviewer.dto.AddLocationDto; +import com.weatherviewer.service.LocationService; +import com.weatherviewer.validation.annotation.LocationLimit; +import jakarta.validation.ConstraintValidator; +import jakarta.validation.ConstraintValidatorContext; +import lombok.RequiredArgsConstructor; +import org.springframework.beans.factory.annotation.Value; +import org.springframework.stereotype.Component; + +/** + * Backs {@link LocationLimit}. Counts how many locations the owning user + * already has saved and rejects the new one if that count is already at + * or above {@code location.max-per-user}. + *

+ * Skips the check (returns valid) when {@code userId} is missing, since + * that case is already reported by the DTO's own required-field + * validation and shouldn't also surface a misleading "limit reached" + * message. + */ +@Component +@RequiredArgsConstructor +public class LocationLimitValidator implements ConstraintValidator { + + private final LocationService locationService; + + @Value("${location.max-per-user:100}") + private int maxLocationsPerUser; + + @Override + public boolean isValid(AddLocationDto addLocationDto, ConstraintValidatorContext context) { + if (addLocationDto == null || addLocationDto.getUserId() == null) { + return true; + } + + long currentCount = locationService.countByUserId(addLocationDto.getUserId()); + + if (currentCount >= maxLocationsPerUser) { + context.disableDefaultConstraintViolation(); + context.buildConstraintViolationWithTemplate( + "You've reached the maximum of " + maxLocationsPerUser + " saved locations") + .addPropertyNode("location") + .addConstraintViolation(); + return false; + } + + return true; + } + +} diff --git a/src/test/java/com/weatherviewer/dto/AddLocationDtoTest.java b/src/test/java/com/weatherviewer/dto/AddLocationDtoTest.java index e1d9ba0..78022e4 100644 --- a/src/test/java/com/weatherviewer/dto/AddLocationDtoTest.java +++ b/src/test/java/com/weatherviewer/dto/AddLocationDtoTest.java @@ -1,5 +1,6 @@ package com.weatherviewer.dto; +import com.weatherviewer.validation.validator.LocationLimitValidator; import com.weatherviewer.validation.validator.UniqueLocationValidator; import jakarta.validation.*; import org.junit.jupiter.api.BeforeEach; @@ -15,7 +16,7 @@ class AddLocationDtoTest { @BeforeEach void setUp() { - validator = skipValidator(UniqueLocationValidator.class); + validator = skipValidator(UniqueLocationValidator.class, LocationLimitValidator.class); } private AddLocationDto validDto() { diff --git a/src/test/java/com/weatherviewer/service/impl/LocationServiceImplTest.java b/src/test/java/com/weatherviewer/service/impl/LocationServiceImplTest.java index d8dcad5..25ef368 100644 --- a/src/test/java/com/weatherviewer/service/impl/LocationServiceImplTest.java +++ b/src/test/java/com/weatherviewer/service/impl/LocationServiceImplTest.java @@ -158,6 +158,102 @@ void getByUserId_returnsMappedList() { assertThat(service.getByUserId(userId)).isEqualTo(dtos); } + @Test + void countByUserId_returnsRepositoryCount() { + UUID userId = UUID.randomUUID(); + when(locationRepository.countByUserId(userId)).thenReturn(3L); + + assertThat(service.countByUserId(userId)).isEqualTo(3L); + } + + @Test + void getByUserIdSorted_defaultSort_queriesByCreatedAtDesc() { + UUID userId = UUID.randomUUID(); + Location location = new Location(); + LocationDto dto = new LocationDto(); + Pageable requested = PageRequest.of(0, 12); + org.springframework.data.domain.Page repoPage = new PageImpl<>(List.of(location), requested, 1); + + when(locationRepository.findByUserId(org.mockito.ArgumentMatchers.eq(userId), any(Pageable.class))) + .thenReturn(repoPage); + when(locationMapper.toDto(location)).thenReturn(dto); + + org.springframework.data.domain.Page result = service.getByUserIdSorted(userId, "date", requested); + + assertThat(result.getContent()).containsExactly(dto); + } + + @Test + void getByUserIdSorted_nullSort_fallsBackToDate() { + UUID userId = UUID.randomUUID(); + Pageable requested = PageRequest.of(0, 12); + + when(locationRepository.findByUserId(org.mockito.ArgumentMatchers.eq(userId), any(Pageable.class))) + .thenReturn(new PageImpl<>(List.of(), requested, 0)); + + org.springframework.data.domain.Page result = service.getByUserIdSorted(userId, null, requested); + + assertThat(result.getTotalElements()).isZero(); + } + + @Test + void getByUserIdSorted_nameAsc_queriesByUserId() { + UUID userId = UUID.randomUUID(); + Pageable requested = PageRequest.of(0, 12); + + when(locationRepository.findByUserId(org.mockito.ArgumentMatchers.eq(userId), any(Pageable.class))) + .thenReturn(new PageImpl<>(List.of(), requested, 0)); + + service.getByUserIdSorted(userId, "nameAsc", requested); + + verify(locationRepository).findByUserId(org.mockito.ArgumentMatchers.eq(userId), any(Pageable.class)); + } + + @Test + void getByUserIdSorted_nameDesc_queriesByUserId() { + UUID userId = UUID.randomUUID(); + Pageable requested = PageRequest.of(0, 12); + + when(locationRepository.findByUserId(org.mockito.ArgumentMatchers.eq(userId), any(Pageable.class))) + .thenReturn(new PageImpl<>(List.of(), requested, 0)); + + service.getByUserIdSorted(userId, "nameDesc", requested); + + verify(locationRepository).findByUserId(org.mockito.ArgumentMatchers.eq(userId), any(Pageable.class)); + } + + @Test + void getByUserIdSorted_favoriteFirst_queriesByUserId() { + UUID userId = UUID.randomUUID(); + Pageable requested = PageRequest.of(0, 12); + + when(locationRepository.findByUserId(org.mockito.ArgumentMatchers.eq(userId), any(Pageable.class))) + .thenReturn(new PageImpl<>(List.of(), requested, 0)); + + service.getByUserIdSorted(userId, "favoriteFirst", requested); + + verify(locationRepository).findByUserId(org.mockito.ArgumentMatchers.eq(userId), any(Pageable.class)); + } + + @Test + void getByUserIdSorted_favoritesOnly_queriesFavoriteTrue() { + UUID userId = UUID.randomUUID(); + Location location = new Location(); + LocationDto dto = new LocationDto(); + Pageable requested = PageRequest.of(0, 12); + org.springframework.data.domain.Page repoPage = new PageImpl<>(List.of(location), requested, 1); + + when(locationRepository.findByUserIdAndFavoriteTrue(org.mockito.ArgumentMatchers.eq(userId), any(Pageable.class))) + .thenReturn(repoPage); + when(locationMapper.toDto(location)).thenReturn(dto); + + org.springframework.data.domain.Page result = + service.getByUserIdSorted(userId, "favoritesOnly", requested); + + assertThat(result.getContent()).containsExactly(dto); + verify(locationRepository, never()).findByUserId(any(UUID.class), any(Pageable.class)); + } + @Test void getByCoordinatesAndUserId_returnsMappedDto() { UUID userId = UUID.randomUUID(); diff --git a/src/test/java/com/weatherviewer/validation/validator/LocationLimitValidatorTest.java b/src/test/java/com/weatherviewer/validation/validator/LocationLimitValidatorTest.java new file mode 100644 index 0000000..92c2259 --- /dev/null +++ b/src/test/java/com/weatherviewer/validation/validator/LocationLimitValidatorTest.java @@ -0,0 +1,82 @@ +package com.weatherviewer.validation.validator; + +import com.weatherviewer.dto.AddLocationDto; +import com.weatherviewer.service.LocationService; +import jakarta.validation.ConstraintValidatorContext; +import org.junit.jupiter.api.BeforeEach; +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.test.util.ReflectionTestUtils; + +import java.util.UUID; + +import static org.assertj.core.api.Assertions.assertThat; +import static org.mockito.Mockito.*; + +@ExtendWith(MockitoExtension.class) +class LocationLimitValidatorTest { + + private static final int MAX_LOCATIONS = 100; + + @Mock + private LocationService locationService; + + @InjectMocks + private LocationLimitValidator validator; + + @BeforeEach + void setUp() { + ReflectionTestUtils.setField(validator, "maxLocationsPerUser", MAX_LOCATIONS); + } + + private ConstraintValidatorContext mockContext() { + return mock(ConstraintValidatorContext.class, RETURNS_DEEP_STUBS); + } + + private AddLocationDto validDto() { + return new AddLocationDto() + .setName("Kyiv") + .setLatitude(50.45) + .setLongitude(30.52) + .setUserId(UUID.randomUUID()); + } + + @Test + void null_dto_returnsTrue() { + assertThat(validator.isValid(null, mockContext())).isTrue(); + } + + @Test + void null_userId_returnsTrue() { + AddLocationDto dto = validDto().setUserId(null); + assertThat(validator.isValid(dto, mockContext())).isTrue(); + } + + @Test + void underLimit_returnsTrue() { + AddLocationDto dto = validDto(); + when(locationService.countByUserId(dto.getUserId())).thenReturn((long) (MAX_LOCATIONS - 1)); + + assertThat(validator.isValid(dto, mockContext())).isTrue(); + } + + @Test + void atLimit_returnsFalse() { + AddLocationDto dto = validDto(); + when(locationService.countByUserId(dto.getUserId())).thenReturn((long) MAX_LOCATIONS); + + assertThat(validator.isValid(dto, mockContext())).isFalse(); + } + + @Test + void overLimit_returnsFalse() { + AddLocationDto dto = validDto(); + when(locationService.countByUserId(dto.getUserId())).thenReturn((long) (MAX_LOCATIONS + 1)); + + assertThat(validator.isValid(dto, mockContext())).isFalse(); + } + +}