From 801a766840b5fefee82195331d6ae257e2b0f092 Mon Sep 17 00:00:00 2001 From: Claude Date: Sat, 31 Oct 2026 10:50:45 +0000 Subject: [PATCH 1/3] Remember unknown SDLC projects briefly; rate limit API requests per client - external-projects-missing cache (30s): a project the SDLC system doesn't know is answered from the cache instead of asking again each time, and found soon after it is created there; a sync replaces it with the project. Used directly through the CacheManager, so failures are caught and logged like the cache error handler does - ClientRateLimitFilter on /api/* (FilterRegistrationBean, after the trace filter): requests per client IP across all instances (100/s, 429 + Retry-After); server.forward-headers-strategy=native takes the client IP from X-Forwarded-For only from internal proxy addresses - Tests: unknown answer remembered then found after expiry, sync clears it; per-client 429 with Retry-After and X-Trace-Id, separate per client, non-API paths unlimited; the repeated-lookup test counts calls after the first lookup, as the HTTP client may retry a GET on a stale connection Co-Authored-By: Claude Opus 5.5 Claude-Session: https://claude.ai/code/session_012MLyPXGHeCc1fvTeB8JnT2 --- README.md | 7 ++- .../resourceapi/config/CacheConfig.java | 4 +- .../config/CacheTtlProperties.java | 5 +- .../config/RateLimitProperties.java | 7 ++- .../services/ExternalProjectService.java | 55 ++++++++++++++++++- .../web/ClientRateLimitConfig.java | 23 ++++++++ .../web/ClientRateLimitFilter.java | 44 +++++++++++++++ src/main/resources/application.properties | 7 +++ .../cache/CacheIntegrationTest.java | 52 ++++++++++++++++-- .../ClientRateLimitIntegrationTest.java | 55 +++++++++++++++++++ .../ratelimit/RateLimitPropertiesTest.java | 6 +- .../resources/config/application.properties | 2 + 12 files changed, 247 insertions(+), 20 deletions(-) create mode 100644 src/main/java/com/company/resourceapi/web/ClientRateLimitConfig.java create mode 100644 src/main/java/com/company/resourceapi/web/ClientRateLimitFilter.java create mode 100644 src/test/java/com/company/resourceapi/ratelimit/ClientRateLimitIntegrationTest.java diff --git a/README.md b/README.md index 3b7c054..f7249d9 100644 --- a/README.md +++ b/README.md @@ -79,10 +79,10 @@ The application uses Redis (`spring.data.redis.host`/`port`, default `localhost: | Cache | What | TTL | |---|---|---| | `sdlc-systems` | SDLC systems by id, read on every create, update and SDLC call | `resourceapi.cache.sdlc-systems-ttl` (10m) | -| `external-projects` | `GET /api/v2/projects/{id}/external` answers per SDLC system and external id; "unknown" answers are not cached | `resourceapi.cache.external-projects-ttl` (5m) | +| `external-projects` | `GET /api/v2/projects/{id}/external` answers per SDLC system and external id | `resourceapi.cache.external-projects-ttl` (5m) | +| `external-projects-missing` | "the SDLC system doesn't know this project" answers, kept briefly so a project created there is found soon | `resourceapi.cache.missing-external-projects-ttl` (30s) | -A background sync always asks the SDLC system and refreshes the cached answer (or removes it when the SDLC -system no longer knows the project). SDLC systems are not changed +A background sync always asks the SDLC system and refreshes both caches with its answer. SDLC systems are not changed through this API; one changed directly in the database is picked up when its entry expires. The cache is an optimization: if Redis is unreachable, cache reads and writes are logged and skipped (each waits at most the 500 ms Redis timeouts) and requests are served without it. @@ -104,6 +104,7 @@ available again one period later, which is also what `Retry-After` says): |---|---|---| | Calls to one SDLC system (`resourceapi.rate-limit.sdlc.*`), protecting its API quota; cache hits don't count | 10 per 1s, waiting up to 1s for a permit | `503` + `Retry-After` (our own limit, not the SDLC system's failure); a background sync logs it | | `POST /api/v2/projects/{id}/sync` per project (`resourceapi.rate-limit.sync.*`); requests refused anyway (unknown project, sync already running) don't count, one refused because no background slot is free (`503`) does | 6 per minute | `429` + `Retry-After` | +| All `/api/**` requests per client IP (`resourceapi.rate-limit.client.*`); behind a reverse proxy the IP comes from `X-Forwarded-For`, trusted only from internal proxy addresses (`server.forward-headers-strategy=native`) | 100 per second | `429` + `Retry-After` | A changed rate is applied to the existing limiter when an instance first uses it; unused limiters expire from Redis after an hour. Invalid limits (e.g. `permits=0`) stop the application at startup. If Redis can't be reached, requests are allowed (limiting protects, losing it shouldn't diff --git a/src/main/java/com/company/resourceapi/config/CacheConfig.java b/src/main/java/com/company/resourceapi/config/CacheConfig.java index b70a71d..0131fbd 100644 --- a/src/main/java/com/company/resourceapi/config/CacheConfig.java +++ b/src/main/java/com/company/resourceapi/config/CacheConfig.java @@ -26,6 +26,7 @@ public class CacheConfig { public static final String SDLC_SYSTEMS = "sdlc-systems"; public static final String EXTERNAL_PROJECTS = "external-projects"; + public static final String MISSING_EXTERNAL_PROJECTS = "external-projects-missing"; /** * Starts from Boot's defaults (spring.cache.redis.*: key prefix, null values, ...) and only @@ -45,7 +46,8 @@ RedisCacheManagerBuilderCustomizer redisCacheCustomizer(CacheTtlProperties ttl) var defaults = builder.cacheDefaults().serializeValuesWith(SerializationPair.fromSerializer(serializer)); builder.cacheDefaults(defaults) .withCacheConfiguration(SDLC_SYSTEMS, defaults.entryTtl(ttl.sdlcSystemsTtl())) - .withCacheConfiguration(EXTERNAL_PROJECTS, defaults.entryTtl(ttl.externalProjectsTtl())); + .withCacheConfiguration(EXTERNAL_PROJECTS, defaults.entryTtl(ttl.externalProjectsTtl())) + .withCacheConfiguration(MISSING_EXTERNAL_PROJECTS, defaults.entryTtl(ttl.missingExternalProjectsTtl())); }; } diff --git a/src/main/java/com/company/resourceapi/config/CacheTtlProperties.java b/src/main/java/com/company/resourceapi/config/CacheTtlProperties.java index e1399f2..597d037 100644 --- a/src/main/java/com/company/resourceapi/config/CacheTtlProperties.java +++ b/src/main/java/com/company/resourceapi/config/CacheTtlProperties.java @@ -9,9 +9,12 @@ * * @param sdlcSystemsTtl SDLC systems by id; they change rarely and only outside this API * @param externalProjectsTtl projects as described by their SDLC system ({@code GET .../external}) + * @param missingExternalProjectsTtl "the SDLC system doesn't know this project" answers; short, so a + * project created there is found soon */ @ConfigurationProperties("resourceapi.cache") public record CacheTtlProperties( @DefaultValue("10m") Duration sdlcSystemsTtl, - @DefaultValue("5m") Duration externalProjectsTtl) { + @DefaultValue("5m") Duration externalProjectsTtl, + @DefaultValue("30s") Duration missingExternalProjectsTtl) { } diff --git a/src/main/java/com/company/resourceapi/config/RateLimitProperties.java b/src/main/java/com/company/resourceapi/config/RateLimitProperties.java index 37e3cba..2ed983f 100644 --- a/src/main/java/com/company/resourceapi/config/RateLimitProperties.java +++ b/src/main/java/com/company/resourceapi/config/RateLimitProperties.java @@ -11,12 +11,13 @@ * Distributed rate limits ({@code resourceapi.rate-limit.*}), shared by all instances through Redis. * Validated at startup: a broken limit would otherwise fail, or silently stop limiting, at runtime. * - * @param sdlc calls to one SDLC system (protects its API quota) - * @param sync sync requests for one project + * @param sdlc calls to one SDLC system (protects its API quota) + * @param sync sync requests for one project + * @param client API requests from one client (IP address) */ @Validated @ConfigurationProperties("resourceapi.rate-limit") -public record RateLimitProperties(@NotNull @Valid Limit sdlc, @NotNull @Valid Limit sync) { +public record RateLimitProperties(@NotNull @Valid Limit sdlc, @NotNull @Valid Limit sync, @NotNull @Valid Limit client) { /** * At most {@code permits} per sliding window of {@code period}: each permit becomes available again diff --git a/src/main/java/com/company/resourceapi/services/ExternalProjectService.java b/src/main/java/com/company/resourceapi/services/ExternalProjectService.java index f1d428e..7b064e1 100644 --- a/src/main/java/com/company/resourceapi/services/ExternalProjectService.java +++ b/src/main/java/com/company/resourceapi/services/ExternalProjectService.java @@ -1,6 +1,7 @@ package com.company.resourceapi.services; import static com.company.resourceapi.config.CacheConfig.EXTERNAL_PROJECTS; +import static com.company.resourceapi.config.CacheConfig.MISSING_EXTERNAL_PROJECTS; import com.company.resourceapi.clients.ExternalProject; import com.company.resourceapi.clients.SdlcProjectClient; @@ -10,6 +11,9 @@ import com.company.resourceapi.exceptions.RateLimitExceededException.Scope; import java.util.Optional; import lombok.RequiredArgsConstructor; +import lombok.extern.slf4j.Slf4j; +import org.springframework.cache.Cache; +import org.springframework.cache.CacheManager; import org.springframework.cache.annotation.CacheEvict; import org.springframework.cache.annotation.CachePut; import org.springframework.cache.annotation.Cacheable; @@ -22,16 +26,27 @@ */ @Service @RequiredArgsConstructor +@Slf4j public class ExternalProjectService { private final SdlcProjectClient sdlcProjectClient; private final RateLimiters rateLimiters; private final RateLimitProperties rateLimits; + private final CacheManager cacheManager; - /** Cached; a project unknown to the SDLC system isn't cached, so it is found once it exists. */ + /** + * Cached. A project unknown to the SDLC system is remembered separately and briefly, so repeated + * lookups don't each call the SDLC system, while a project created there is still found soon. + */ @Cacheable(cacheNames = EXTERNAL_PROJECTS, key = "#sdlcSystem.id + ':' + #externalId", unless = "#result == null") public Optional findProject(SdlcSystem sdlcSystem, String externalId) { - return fetch(sdlcSystem, externalId); + var key = key(sdlcSystem, externalId); + if (isKnownMissing(key)) { + return Optional.empty(); + } + var project = fetch(sdlcSystem, externalId); + rememberMissing(key, project.isEmpty()); + return project; } /** @@ -45,7 +60,41 @@ public Optional findProject(SdlcSystem sdlcSystem, String exter evict = @CacheEvict(cacheNames = EXTERNAL_PROJECTS, key = "#sdlcSystem.id + ':' + #externalId", condition = "#result == null or #result.isEmpty()")) public Optional refreshProject(SdlcSystem sdlcSystem, String externalId) { - return fetch(sdlcSystem, externalId); + var project = fetch(sdlcSystem, externalId); + rememberMissing(key(sdlcSystem, externalId), project.isEmpty()); + return project; + } + + // The "missing" cache is used directly, which bypasses the cache error handler: failures are + // handled here the same way, by working without it. + + private boolean isKnownMissing(String key) { + try { + return missingCache().get(key) != null; + } catch (RuntimeException e) { + log.warn("Cache '{}' failed to get entry with key '{}': {}", MISSING_EXTERNAL_PROJECTS, key, e.getMessage()); + return false; + } + } + + private void rememberMissing(String key, boolean missing) { + try { + if (missing) { + missingCache().put(key, Boolean.TRUE); + } else { + missingCache().evict(key); + } + } catch (RuntimeException e) { + log.warn("Cache '{}' failed to update entry with key '{}': {}", MISSING_EXTERNAL_PROJECTS, key, e.getMessage()); + } + } + + private Cache missingCache() { + return cacheManager.getCache(MISSING_EXTERNAL_PROJECTS); + } + + private static String key(SdlcSystem sdlcSystem, String externalId) { + return sdlcSystem.getId() + ":" + externalId; } /** Only real calls count against the SDLC system's rate limit, not cache hits. */ diff --git a/src/main/java/com/company/resourceapi/web/ClientRateLimitConfig.java b/src/main/java/com/company/resourceapi/web/ClientRateLimitConfig.java new file mode 100644 index 0000000..5cb3f82 --- /dev/null +++ b/src/main/java/com/company/resourceapi/web/ClientRateLimitConfig.java @@ -0,0 +1,23 @@ +package com.company.resourceapi.web; + +import com.company.resourceapi.config.RateLimitProperties; +import com.company.resourceapi.services.RateLimiters; +import org.springframework.boot.web.servlet.FilterRegistrationBean; +import org.springframework.context.annotation.Bean; +import org.springframework.context.annotation.Configuration; +import org.springframework.core.Ordered; + +/** Registers {@link ClientRateLimitFilter} for the API only (not Swagger UI or the OpenAPI document). */ +@Configuration +public class ClientRateLimitConfig { + + @Bean + FilterRegistrationBean clientRateLimitFilter(RateLimiters rateLimiters, + RateLimitProperties rateLimits) { + var registration = new FilterRegistrationBean<>(new ClientRateLimitFilter(rateLimiters, rateLimits)); + registration.addUrlPatterns("/api/*"); + // after the server observation (+1) and trace id (+2) filters + registration.setOrder(Ordered.HIGHEST_PRECEDENCE + 3); + return registration; + } +} diff --git a/src/main/java/com/company/resourceapi/web/ClientRateLimitFilter.java b/src/main/java/com/company/resourceapi/web/ClientRateLimitFilter.java new file mode 100644 index 0000000..e0637d8 --- /dev/null +++ b/src/main/java/com/company/resourceapi/web/ClientRateLimitFilter.java @@ -0,0 +1,44 @@ +package com.company.resourceapi.web; + +import static org.springframework.http.HttpHeaders.RETRY_AFTER; + +import com.company.resourceapi.config.RateLimitProperties; +import com.company.resourceapi.services.RateLimiters; +import jakarta.servlet.FilterChain; +import jakarta.servlet.ServletException; +import jakarta.servlet.http.HttpServletRequest; +import jakarta.servlet.http.HttpServletResponse; +import java.io.IOException; +import lombok.RequiredArgsConstructor; +import lombok.extern.slf4j.Slf4j; +import org.springframework.http.HttpStatus; +import org.springframework.web.filter.OncePerRequestFilter; + +/** + * Limits API requests per client IP address, across all instances (429 + Retry-After). Behind a + * reverse proxy the address comes from X-Forwarded-For, which Tomcat only trusts from internal proxy + * addresses ({@code server.forward-headers-strategy=native}), so clients can't pick their own key. + * + *

Registered for {@code /api/*} by {@link ClientRateLimitConfig}, after the trace id filter, so a + * refused request still gets its X-Trace-Id. + */ +@RequiredArgsConstructor +@Slf4j +public class ClientRateLimitFilter extends OncePerRequestFilter { + + private final RateLimiters rateLimiters; + private final RateLimitProperties rateLimits; + + @Override + protected void doFilterInternal(HttpServletRequest request, HttpServletResponse response, + FilterChain filterChain) throws ServletException, IOException { + var limit = rateLimits.client(); + if (!rateLimiters.tryAcquire("client-rate:" + request.getRemoteAddr(), limit)) { + log.info("Too many requests from {}", request.getRemoteAddr()); + response.setStatus(HttpStatus.TOO_MANY_REQUESTS.value()); + response.setHeader(RETRY_AFTER, String.valueOf(limit.retryAfterSeconds())); + return; + } + filterChain.doFilter(request, response); + } +} diff --git a/src/main/resources/application.properties b/src/main/resources/application.properties index 1cd9fae..877aed1 100755 --- a/src/main/resources/application.properties +++ b/src/main/resources/application.properties @@ -25,6 +25,7 @@ spring.cache.redis.key-prefix=resourceapi:: spring.cache.redis.cache-null-values=false resourceapi.cache.sdlc-systems-ttl=10m resourceapi.cache.external-projects-ttl=5m +resourceapi.cache.missing-external-projects-ttl=30s # redisson (locks, rate limiters) uses the same Redis; its object names get this prefix resourceapi.redisson.key-prefix=resourceapi: # a project's sync lock expires after this even if never released (e.g. the instance died) @@ -37,6 +38,12 @@ resourceapi.rate-limit.sdlc.max-wait=1s resourceapi.rate-limit.sync.permits=6 resourceapi.rate-limit.sync.period=1m resourceapi.rate-limit.sync.max-wait=0s +# API requests per client IP (429 + Retry-After); behind a reverse proxy the client IP comes from +# X-Forwarded-For, trusted only from internal proxy addresses (Tomcat's RemoteIpValve) +resourceapi.rate-limit.client.permits=100 +resourceapi.rate-limit.client.period=1s +resourceapi.rate-limit.client.max-wait=0s +server.forward-headers-strategy=native # datasource spring.datasource.url=jdbc:h2:mem:test;MODE=MySQL;DATABASE_TO_LOWER=TRUE spring.sql.init.platform=h2 diff --git a/src/test/java/com/company/resourceapi/cache/CacheIntegrationTest.java b/src/test/java/com/company/resourceapi/cache/CacheIntegrationTest.java index 7fa5fff..3e81b5f 100644 --- a/src/test/java/com/company/resourceapi/cache/CacheIntegrationTest.java +++ b/src/test/java/com/company/resourceapi/cache/CacheIntegrationTest.java @@ -1,6 +1,7 @@ package com.company.resourceapi.cache; import static com.company.resourceapi.config.CacheConfig.EXTERNAL_PROJECTS; +import static com.company.resourceapi.config.CacheConfig.MISSING_EXTERNAL_PROJECTS; import static com.company.resourceapi.config.CacheConfig.SDLC_SYSTEMS; import static com.github.tomakehurst.wiremock.client.WireMock.getRequestedFor; import static com.github.tomakehurst.wiremock.client.WireMock.notFound; @@ -38,7 +39,7 @@ import org.springframework.test.web.servlet.MockMvc; /** Redis cache in front of the SDLC systems, against a real Redis (RedisTestContainerInitializer). */ -@SpringBootTest +@SpringBootTest(properties = "resourceapi.cache.missing-external-projects-ttl=1s") @AutoConfigureMockMvc class CacheIntegrationTest { @@ -92,29 +93,60 @@ void givenRepeatedLookupsThenSdlcSystemIsAskedOnce() throws Exception { sdlcSystemServer.stubFor(WireMock.get(urlEqualTo(SDLC_PROJECT_URL)) .willReturn(okJson("{\"key\": \"CACHED\", \"name\": \"From SDLC\"}"))); - for (int i = 0; i < 3; i++) { + mockMvc.perform(get(externalUrl())).andExpect(status().isOk()); + // counted after the first lookup: the HTTP client may retry a GET on a stale pooled connection + int callsForFirstLookup = sdlcSystemServer.findAll(getRequestedFor(urlEqualTo(SDLC_PROJECT_URL))).size(); + + for (int i = 0; i < 2; i++) { mockMvc.perform(get(externalUrl())) .andExpect(status().isOk()) .andExpect(jsonPath("$.name", equalTo("From SDLC"))); } - sdlcSystemServer.verify(1, getRequestedFor(urlEqualTo(SDLC_PROJECT_URL))); + // the next lookups are served from the cache + sdlcSystemServer.verify(callsForFirstLookup, getRequestedFor(urlEqualTo(SDLC_PROJECT_URL))); // stored as JSON in Redis and read back as the record, not a map assertThat(cacheManager.getCache(EXTERNAL_PROJECTS).get(externalKey(), ExternalProject.class)) .isEqualTo(new ExternalProject("CACHED", "From SDLC", null)); } @Test - void givenProjectUnknownToSdlcSystemThenItIsNotCached() throws Exception { + void givenProjectUnknownToSdlcSystemThenThatIsRememberedBriefly() throws Exception { sdlcSystemServer.stubFor(WireMock.get(urlEqualTo(SDLC_PROJECT_URL)) .willReturn(notFound().withHeader("Content-Type", "application/json").withBody("{}"))); mockMvc.perform(get(externalUrl())).andExpect(status().isNotFound()); mockMvc.perform(get(externalUrl())).andExpect(status().isNotFound()); - // asked again, so the project is found as soon as it exists in the SDLC system - sdlcSystemServer.verify(2, getRequestedFor(urlEqualTo(SDLC_PROJECT_URL))); + // one call: the "unknown" answer is remembered, in its own short-lived cache + sdlcSystemServer.verify(1, getRequestedFor(urlEqualTo(SDLC_PROJECT_URL))); + assertThat(cacheManager.getCache(MISSING_EXTERNAL_PROJECTS).get(externalKey())).isNotNull(); assertThat(cacheManager.getCache(EXTERNAL_PROJECTS).get(externalKey())).isNull(); + + // the project is created in the SDLC system: found once the "unknown" answer expired (1s here) + sdlcSystemServer.stubFor(WireMock.get(urlEqualTo(SDLC_PROJECT_URL)) + .willReturn(okJson("{\"key\": \"CACHED\", \"name\": \"Created later\"}"))); + mockMvc.perform(get(externalUrl())).andExpect(status().isNotFound()); + await(() -> statusOf(externalUrl()) == 200); + assertThat(cacheManager.getCache(MISSING_EXTERNAL_PROJECTS).get(externalKey())).isNull(); + } + + @Test + void givenProjectRememberedAsUnknownWhenSyncedThenItIsFoundRightAway() throws Exception { + sdlcSystemServer.stubFor(WireMock.get(urlEqualTo(SDLC_PROJECT_URL)) + .willReturn(notFound().withHeader("Content-Type", "application/json").withBody("{}"))); + mockMvc.perform(get(externalUrl())).andExpect(status().isNotFound()); + + sdlcSystemServer.stubFor(WireMock.get(urlEqualTo(SDLC_PROJECT_URL)) + .willReturn(okJson("{\"key\": \"CACHED\", \"name\": \"Created later\"}"))); + mockMvc.perform(post(ProjectRestController.ENDPOINT + "/" + projectId + "/sync")) + .andExpect(status().isAccepted()); + await(() -> projectRepository.findById(projectId).orElseThrow().getName().equals("Created later")); + + // the sync replaced the "unknown" answer with the project + assertThat(cacheManager.getCache(MISSING_EXTERNAL_PROJECTS).get(externalKey())).isNull(); + mockMvc.perform(get(externalUrl())).andExpect(status().isOk()); + sdlcSystemServer.verify(2, getRequestedFor(urlEqualTo(SDLC_PROJECT_URL))); } @Test @@ -191,4 +223,12 @@ void givenProjectGoneFromSdlcSystemWhenSyncedThenCachedEntryIsRemoved() throws E mockMvc.perform(get(externalUrl())).andExpect(status().isNotFound()); } + + private int statusOf(String url) { + try { + return mockMvc.perform(get(url)).andReturn().getResponse().getStatus(); + } catch (Exception e) { + throw new IllegalStateException(e); + } + } } diff --git a/src/test/java/com/company/resourceapi/ratelimit/ClientRateLimitIntegrationTest.java b/src/test/java/com/company/resourceapi/ratelimit/ClientRateLimitIntegrationTest.java new file mode 100644 index 0000000..5de44fd --- /dev/null +++ b/src/test/java/com/company/resourceapi/ratelimit/ClientRateLimitIntegrationTest.java @@ -0,0 +1,55 @@ +package com.company.resourceapi.ratelimit; + +import static org.springframework.test.web.servlet.request.MockMvcRequestBuilders.get; +import static org.springframework.test.web.servlet.result.MockMvcResultMatchers.header; +import static org.springframework.test.web.servlet.result.MockMvcResultMatchers.status; + +import com.company.resourceapi.controllers.ProjectRestController; +import org.junit.jupiter.api.Test; +import org.springframework.beans.factory.annotation.Autowired; +import org.springframework.boot.test.context.SpringBootTest; +import org.springframework.boot.webmvc.test.autoconfigure.AutoConfigureMockMvc; +import org.springframework.test.web.servlet.MockMvc; +import org.springframework.test.web.servlet.request.RequestPostProcessor; + +/** Requests per client address, shared by all instances through Redis. */ +@SpringBootTest(properties = { + "resourceapi.rate-limit.client.permits=3", + "resourceapi.rate-limit.client.period=2s"}) +@AutoConfigureMockMvc +class ClientRateLimitIntegrationTest { + + private static final String PROJECT_URL = ProjectRestController.ENDPOINT + "/1"; + + @Autowired + private MockMvc mockMvc; + + @Test + void givenTooManyRequestsFromOneClientThenFurtherOnesAreRefused() throws Exception { + for (int i = 0; i < 3; i++) { + mockMvc.perform(get(PROJECT_URL).with(from("10.0.0.1"))).andExpect(status().isOk()); + } + mockMvc.perform(get(PROJECT_URL).with(from("10.0.0.1"))) + .andExpect(status().isTooManyRequests()) + .andExpect(header().string("Retry-After", "2")) + .andExpect(header().exists("X-Trace-Id")); + + // another client has its own limit + mockMvc.perform(get(PROJECT_URL).with(from("10.0.0.2"))).andExpect(status().isOk()); + } + + @Test + void givenNonApiPathsThenTheyAreNotLimited() throws Exception { + for (int i = 0; i < 5; i++) { + mockMvc.perform(get("/v3/api-docs").with(from("10.0.0.3"))).andExpect(status().isOk()); + } + mockMvc.perform(get(PROJECT_URL).with(from("10.0.0.3"))).andExpect(status().isOk()); + } + + private static RequestPostProcessor from(String address) { + return request -> { + request.setRemoteAddr(address); + return request; + }; + } +} diff --git a/src/test/java/com/company/resourceapi/ratelimit/RateLimitPropertiesTest.java b/src/test/java/com/company/resourceapi/ratelimit/RateLimitPropertiesTest.java index b6a8b81..3540100 100644 --- a/src/test/java/com/company/resourceapi/ratelimit/RateLimitPropertiesTest.java +++ b/src/test/java/com/company/resourceapi/ratelimit/RateLimitPropertiesTest.java @@ -18,9 +18,9 @@ void givenInvalidLimitsThenTheyAreRejected() { var noPeriod = new Limit(5, null, Duration.ZERO); var valid = new Limit(5, Duration.ofSeconds(1), Duration.ZERO); - assertThat(validator.validate(new RateLimitProperties(noPermits, valid))).hasSize(1); - assertThat(validator.validate(new RateLimitProperties(valid, noPeriod))).hasSize(1); - assertThat(validator.validate(new RateLimitProperties(valid, valid))).isEmpty(); + assertThat(validator.validate(new RateLimitProperties(noPermits, valid, valid))).hasSize(1); + assertThat(validator.validate(new RateLimitProperties(valid, noPeriod, valid))).hasSize(1); + assertThat(validator.validate(new RateLimitProperties(valid, valid, valid))).isEmpty(); } @Test diff --git a/src/test/resources/config/application.properties b/src/test/resources/config/application.properties index c2a9ab1..67b2d67 100644 --- a/src/test/resources/config/application.properties +++ b/src/test/resources/config/application.properties @@ -1,2 +1,4 @@ # Each test application context gets its own in-memory database so schema.sql/data.sql run on a clean slate spring.datasource.url=jdbc:h2:mem:test-${random.uuid};MODE=MySQL;DATABASE_TO_LOWER=TRUE +# tests send many requests from one address; ClientRateLimitIntegrationTest sets its own limit +resourceapi.rate-limit.client.permits=100000 From 41981eafb797c9d495080dbf608103a8ec6c0764 Mon Sep 17 00:00:00 2001 From: Claude Date: Sat, 31 Oct 2026 11:05:33 +0000 Subject: [PATCH 2/3] Address review: one external-projects cache, Redis outage pause, shared 429 - "Unknown project" is cached as Spring's null value in the external-projects cache with a per-entry TTL (30s unknown, 5m found), replacing the separate cache and the hand-written lookups; a sync simply @CachePut-replaces it. - RateLimiters stops asking Redis for 10s after a failure, so an outage doesn't add a timeout to every request. - Limiters expire 10 periods after last use (at least a minute) instead of an hour, so per-IP limiters don't pile up. - The per-client filter answers through the shared exception handler; refused callers are logged at DEBUG. - README: server.tomcat.remoteip.internal-proxies for the per-IP limit. - Tests: longer negative-cache TTL, no timing-sensitive 404 assertion, delta-based call counts, unit test for the outage pause. Co-Authored-By: Claude Opus 5.5 Claude-Session: https://claude.ai/code/session_012MLyPXGHeCc1fvTeB8JnT2 --- README.md | 20 ++++-- .../resourceapi/config/CacheConfig.java | 14 ++-- .../services/ExternalProjectService.java | 70 ++----------------- .../resourceapi/services/RateLimiters.java | 37 ++++++++-- .../resourceapi/web/ApiExceptionHandler.java | 10 ++- .../web/ClientRateLimitConfig.java | 8 ++- .../web/ClientRateLimitFilter.java | 18 ++--- src/main/resources/application.properties | 1 - .../cache/CacheIntegrationTest.java | 32 +++++---- .../ratelimit/RateLimitIntegrationTest.java | 9 +++ .../ratelimit/RateLimitersTest.java | 49 +++++++++++++ 11 files changed, 163 insertions(+), 105 deletions(-) create mode 100644 src/test/java/com/company/resourceapi/ratelimit/RateLimitersTest.java diff --git a/README.md b/README.md index f7249d9..3629212 100644 --- a/README.md +++ b/README.md @@ -79,10 +79,9 @@ The application uses Redis (`spring.data.redis.host`/`port`, default `localhost: | Cache | What | TTL | |---|---|---| | `sdlc-systems` | SDLC systems by id, read on every create, update and SDLC call | `resourceapi.cache.sdlc-systems-ttl` (10m) | -| `external-projects` | `GET /api/v2/projects/{id}/external` answers per SDLC system and external id | `resourceapi.cache.external-projects-ttl` (5m) | -| `external-projects-missing` | "the SDLC system doesn't know this project" answers, kept briefly so a project created there is found soon | `resourceapi.cache.missing-external-projects-ttl` (30s) | +| `external-projects` | `GET /api/v2/projects/{id}/external` answers per SDLC system and external id | `resourceapi.cache.external-projects-ttl` (5m); "the SDLC system doesn't know this project" (stored as a cached null) only `resourceapi.cache.missing-external-projects-ttl` (30s), so a project created there is found soon | -A background sync always asks the SDLC system and refreshes both caches with its answer. SDLC systems are not changed +A background sync always asks the SDLC system and replaces the cached answer with its own. SDLC systems are not changed through this API; one changed directly in the database is picked up when its entry expires. The cache is an optimization: if Redis is unreachable, cache reads and writes are logged and skipped (each waits at most the 500 ms Redis timeouts) and requests are served without it. @@ -104,11 +103,20 @@ available again one period later, which is also what `Retry-After` says): |---|---|---| | Calls to one SDLC system (`resourceapi.rate-limit.sdlc.*`), protecting its API quota; cache hits don't count | 10 per 1s, waiting up to 1s for a permit | `503` + `Retry-After` (our own limit, not the SDLC system's failure); a background sync logs it | | `POST /api/v2/projects/{id}/sync` per project (`resourceapi.rate-limit.sync.*`); requests refused anyway (unknown project, sync already running) don't count, one refused because no background slot is free (`503`) does | 6 per minute | `429` + `Retry-After` | -| All `/api/**` requests per client IP (`resourceapi.rate-limit.client.*`); behind a reverse proxy the IP comes from `X-Forwarded-For`, trusted only from internal proxy addresses (`server.forward-headers-strategy=native`) | 100 per second | `429` + `Retry-After` | +| All `/api/**` requests per client IP (`resourceapi.rate-limit.client.*`); behind a reverse proxy the IP comes from `X-Forwarded-For`, trusted only from internal proxy addresses (`server.forward-headers-strategy=native`, see below) | 100 per second | `429` + `Retry-After` | A changed rate is applied to the existing limiter when an instance first uses it; unused limiters expire from -Redis after an hour. Invalid limits (e.g. `permits=0`) stop the application at startup. If Redis can't be reached, requests are allowed (limiting protects, losing it shouldn't -stop the application). +Redis after 10 periods (at least a minute), so per-IP limiters don't pile up. Invalid limits (e.g. +`permits=0`) stop the application at startup. If Redis can't be reached, requests are allowed (limiting +protects, losing it shouldn't stop the application), and Redis is not asked again for 10 seconds, so an +outage doesn't add a timeout to every request. + +The per-IP limit only means something if the application sees the client's address. Tomcat takes it from +`X-Forwarded-For` when the request comes from an address matching `server.tomcat.remoteip.internal-proxies` +(by default 10/8, 192.168/16, 172.16/12, 169.254/16, 127/8 and IPv6 local addresses). If the reverse proxy or +load balancer has another address, set that property to match it; otherwise every request appears to come from +the proxy and all clients share one limit. Never match addresses clients can connect from directly, or they +can choose their own key by sending `X-Forwarded-For`. **Local development**: `./mvnw spring-boot:run` starts Redis from `compose.yaml` automatically (`spring-boot-docker-compose`); with `java -jar`, start it yourself with `docker compose up -d` (it listens on diff --git a/src/main/java/com/company/resourceapi/config/CacheConfig.java b/src/main/java/com/company/resourceapi/config/CacheConfig.java index bd7f328..c89783e 100644 --- a/src/main/java/com/company/resourceapi/config/CacheConfig.java +++ b/src/main/java/com/company/resourceapi/config/CacheConfig.java @@ -10,6 +10,7 @@ import org.springframework.cache.annotation.EnableCaching; import org.springframework.cache.interceptor.CacheErrorHandler; import org.springframework.cache.interceptor.LoggingCacheErrorHandler; +import org.springframework.cache.support.NullValue; import org.springframework.context.annotation.Bean; import org.springframework.context.annotation.Configuration; import org.springframework.context.annotation.Role; @@ -26,12 +27,11 @@ @EnableCaching @EnableConfigurationProperties(CacheTtlProperties.class) // cached values are rebuilt from JSON by type name: reflection a native image must know about -@RegisterReflectionForBinding({SdlcSystem.class, ExternalProject.class}) +@RegisterReflectionForBinding({SdlcSystem.class, ExternalProject.class, NullValue.class}) public class CacheConfig { public static final String SDLC_SYSTEMS = "sdlc-systems"; public static final String EXTERNAL_PROJECTS = "external-projects"; - public static final String MISSING_EXTERNAL_PROJECTS = "external-projects-missing"; /** * Starts from Boot's defaults (spring.cache.redis.*: key prefix, null values, ...) and only @@ -43,16 +43,22 @@ RedisCacheManagerBuilderCustomizer redisCacheCustomizer(CacheTtlProperties ttl) .allowIfSubType("com.company.resourceapi.") .allowIfSubType("java.lang.") .allowIfSubType("java.time.") + // Spring's marker for a cached null ("the SDLC system doesn't know this project") + .allowIfSubType(NullValue.class) .build(); var serializer = GenericJacksonJsonRedisSerializer.builder() .enableDefaultTyping(typeValidator) + .enableSpringCacheNullValueSupport() .build(); return builder -> { var defaults = builder.cacheDefaults().serializeValuesWith(SerializationPair.fromSerializer(serializer)); builder.cacheDefaults(defaults) .withCacheConfiguration(SDLC_SYSTEMS, defaults.entryTtl(ttl.sdlcSystemsTtl())) - .withCacheConfiguration(EXTERNAL_PROJECTS, defaults.entryTtl(ttl.externalProjectsTtl())) - .withCacheConfiguration(MISSING_EXTERNAL_PROJECTS, defaults.entryTtl(ttl.missingExternalProjectsTtl())); + // a found project is kept longer than "unknown", so a project created there is found soon + .withCacheConfiguration(EXTERNAL_PROJECTS, defaults.entryTtl((key, value) -> + value == null || value instanceof NullValue + ? ttl.missingExternalProjectsTtl() + : ttl.externalProjectsTtl())); }; } diff --git a/src/main/java/com/company/resourceapi/services/ExternalProjectService.java b/src/main/java/com/company/resourceapi/services/ExternalProjectService.java index 7b064e1..88d39e2 100644 --- a/src/main/java/com/company/resourceapi/services/ExternalProjectService.java +++ b/src/main/java/com/company/resourceapi/services/ExternalProjectService.java @@ -1,7 +1,6 @@ package com.company.resourceapi.services; import static com.company.resourceapi.config.CacheConfig.EXTERNAL_PROJECTS; -import static com.company.resourceapi.config.CacheConfig.MISSING_EXTERNAL_PROJECTS; import com.company.resourceapi.clients.ExternalProject; import com.company.resourceapi.clients.SdlcProjectClient; @@ -11,13 +10,8 @@ import com.company.resourceapi.exceptions.RateLimitExceededException.Scope; import java.util.Optional; import lombok.RequiredArgsConstructor; -import lombok.extern.slf4j.Slf4j; -import org.springframework.cache.Cache; -import org.springframework.cache.CacheManager; -import org.springframework.cache.annotation.CacheEvict; import org.springframework.cache.annotation.CachePut; import org.springframework.cache.annotation.Cacheable; -import org.springframework.cache.annotation.Caching; import org.springframework.stereotype.Service; /** @@ -26,75 +20,25 @@ */ @Service @RequiredArgsConstructor -@Slf4j public class ExternalProjectService { private final SdlcProjectClient sdlcProjectClient; private final RateLimiters rateLimiters; private final RateLimitProperties rateLimits; - private final CacheManager cacheManager; /** - * Cached. A project unknown to the SDLC system is remembered separately and briefly, so repeated - * lookups don't each call the SDLC system, while a project created there is still found soon. + * Cached, including the answer "the SDLC system doesn't know this project" (cached as null), which + * is kept only briefly so a project created there is found soon (see CacheConfig). */ - @Cacheable(cacheNames = EXTERNAL_PROJECTS, key = "#sdlcSystem.id + ':' + #externalId", unless = "#result == null") + @Cacheable(cacheNames = EXTERNAL_PROJECTS, key = "#sdlcSystem.id + ':' + #externalId") public Optional findProject(SdlcSystem sdlcSystem, String externalId) { - var key = key(sdlcSystem, externalId); - if (isKnownMissing(key)) { - return Optional.empty(); - } - var project = fetch(sdlcSystem, externalId); - rememberMissing(key, project.isEmpty()); - return project; + return fetch(sdlcSystem, externalId); } - /** - * Always asks the SDLC system and updates the cache with the answer: a found project replaces the - * cached one, a project the SDLC system no longer knows removes it. - */ - @Caching( - put = @CachePut(cacheNames = EXTERNAL_PROJECTS, key = "#sdlcSystem.id + ':' + #externalId", - unless = "#result == null"), - // unlike for @CachePut, #result is the Optional itself here (late evictions get the raw return value) - evict = @CacheEvict(cacheNames = EXTERNAL_PROJECTS, key = "#sdlcSystem.id + ':' + #externalId", - condition = "#result == null or #result.isEmpty()")) + /** Always asks the SDLC system and caches its answer, found or unknown, replacing the cached one. */ + @CachePut(cacheNames = EXTERNAL_PROJECTS, key = "#sdlcSystem.id + ':' + #externalId") public Optional refreshProject(SdlcSystem sdlcSystem, String externalId) { - var project = fetch(sdlcSystem, externalId); - rememberMissing(key(sdlcSystem, externalId), project.isEmpty()); - return project; - } - - // The "missing" cache is used directly, which bypasses the cache error handler: failures are - // handled here the same way, by working without it. - - private boolean isKnownMissing(String key) { - try { - return missingCache().get(key) != null; - } catch (RuntimeException e) { - log.warn("Cache '{}' failed to get entry with key '{}': {}", MISSING_EXTERNAL_PROJECTS, key, e.getMessage()); - return false; - } - } - - private void rememberMissing(String key, boolean missing) { - try { - if (missing) { - missingCache().put(key, Boolean.TRUE); - } else { - missingCache().evict(key); - } - } catch (RuntimeException e) { - log.warn("Cache '{}' failed to update entry with key '{}': {}", MISSING_EXTERNAL_PROJECTS, key, e.getMessage()); - } - } - - private Cache missingCache() { - return cacheManager.getCache(MISSING_EXTERNAL_PROJECTS); - } - - private static String key(SdlcSystem sdlcSystem, String externalId) { - return sdlcSystem.getId() + ":" + externalId; + return fetch(sdlcSystem, externalId); } /** Only real calls count against the SDLC system's rate limit, not cache hits. */ diff --git a/src/main/java/com/company/resourceapi/services/RateLimiters.java b/src/main/java/com/company/resourceapi/services/RateLimiters.java index f5bcefa..75d1218 100644 --- a/src/main/java/com/company/resourceapi/services/RateLimiters.java +++ b/src/main/java/com/company/resourceapi/services/RateLimiters.java @@ -18,18 +18,24 @@ *

A limiter is set up with the configured rate the first time this instance uses it (updating a * limiter created with another rate), and again if Redis dropped it after it expired from disuse. * Without Redis (unreachable, or no Redisson client) requests are allowed: limiting is a protection, - * losing it shouldn't stop the application. + * losing it shouldn't stop the application. After a failure Redis is left alone for a short while, + * so an outage doesn't add a Redis timeout to every request. */ @Component @Slf4j public class RateLimiters { - /** Unused limiters are removed from Redis after this. */ - private static final Duration KEEP_ALIVE = Duration.ofHours(1); + /** Unused limiters are removed from Redis after this many periods, but not sooner than MIN_KEEP_ALIVE. */ + private static final int KEEP_ALIVE_PERIODS = 10; + private static final Duration MIN_KEEP_ALIVE = Duration.ofMinutes(1); + /** How long requests are allowed without asking Redis after it failed. */ + static final Duration PAUSE_AFTER_FAILURE = Duration.ofSeconds(10); /** Limiters this instance has set up, most recently used kept (bounded: one per project). */ private static final int MAX_REMEMBERED = 10_000; private final RedissonClient redisson; + /** System.nanoTime() until which Redis isn't asked; 0 while it works. */ + private volatile long pausedUntil; private final Map setUp = Collections.synchronizedMap(new LinkedHashMap<>(16, 0.75f, true) { @Override protected boolean removeEldestEntry(Map.Entry eldest) { @@ -43,7 +49,7 @@ public RateLimiters(ObjectProvider redisson) { /** @return whether a permit was obtained, waiting at most the limit's max wait */ public boolean tryAcquire(String name, RateLimitProperties.Limit limit) { - if (redisson == null) { + if (redisson == null || isPaused()) { return true; } try { @@ -62,17 +68,36 @@ public boolean tryAcquire(String name, RateLimitProperties.Limit limit) { return acquire(limiter, limit); } } catch (RuntimeException e) { - log.warn("Rate limiter {} unavailable, allowing the request: {}", name, e.getMessage()); + pausedUntil = System.nanoTime() + PAUSE_AFTER_FAILURE.toNanos(); + log.warn("Rate limiter {} unavailable, allowing requests without rate limits for {}: {}", + name, PAUSE_AFTER_FAILURE, e.getMessage()); return true; } } + private boolean isPaused() { + long until = pausedUntil; + if (until == 0) { + return false; + } + if (System.nanoTime() - until < 0) { + return true; + } + pausedUntil = 0; + return false; + } + + static Duration keepAlive(RateLimitProperties.Limit limit) { + var periods = limit.period().multipliedBy(KEEP_ALIVE_PERIODS); + return periods.compareTo(MIN_KEEP_ALIVE) > 0 ? periods : MIN_KEEP_ALIVE; + } + private static boolean acquire(RRateLimiter limiter, RateLimitProperties.Limit limit) { return limiter.tryAcquireAsync(1, limit.maxWait()).toCompletableFuture().join(); } private void setUp(String name, RRateLimiter limiter, RateLimitProperties.Limit limit) { - var keepAlive = Duration.ofMillis(Math.max(KEEP_ALIVE.toMillis(), limit.period().toMillis())); + var keepAlive = keepAlive(limit); boolean created = limiter.trySetRateAsync(RateType.OVERALL, limit.permits(), limit.period(), keepAlive) .toCompletableFuture().join(); if (!created) { diff --git a/src/main/java/com/company/resourceapi/web/ApiExceptionHandler.java b/src/main/java/com/company/resourceapi/web/ApiExceptionHandler.java index 50ed5a8..cdb0b5d 100644 --- a/src/main/java/com/company/resourceapi/web/ApiExceptionHandler.java +++ b/src/main/java/com/company/resourceapi/web/ApiExceptionHandler.java @@ -39,8 +39,14 @@ ResponseEntity handleSyncInProgress(SyncInProgressException e) { */ @ExceptionHandler(RateLimitExceededException.class) ResponseEntity handleRateLimitExceeded(RateLimitExceededException e) { - log.info(e.getMessage()); - var status = e.getScope() == RateLimitExceededException.Scope.CALLER ? TOO_MANY_REQUESTS : SERVICE_UNAVAILABLE; + var caller = e.getScope() == RateLimitExceededException.Scope.CALLER; + // a refused caller is expected, possibly many times a second; holding back our own calls is worth seeing + if (caller) { + log.debug(e.getMessage()); + } else { + log.info(e.getMessage()); + } + var status = caller ? TOO_MANY_REQUESTS : SERVICE_UNAVAILABLE; return ResponseEntity.status(status).header(RETRY_AFTER, String.valueOf(e.getRetryAfterSeconds())).build(); } diff --git a/src/main/java/com/company/resourceapi/web/ClientRateLimitConfig.java b/src/main/java/com/company/resourceapi/web/ClientRateLimitConfig.java index 5cb3f82..d552051 100644 --- a/src/main/java/com/company/resourceapi/web/ClientRateLimitConfig.java +++ b/src/main/java/com/company/resourceapi/web/ClientRateLimitConfig.java @@ -2,10 +2,12 @@ import com.company.resourceapi.config.RateLimitProperties; import com.company.resourceapi.services.RateLimiters; +import org.springframework.beans.factory.annotation.Qualifier; import org.springframework.boot.web.servlet.FilterRegistrationBean; import org.springframework.context.annotation.Bean; import org.springframework.context.annotation.Configuration; import org.springframework.core.Ordered; +import org.springframework.web.servlet.HandlerExceptionResolver; /** Registers {@link ClientRateLimitFilter} for the API only (not Swagger UI or the OpenAPI document). */ @Configuration @@ -13,8 +15,10 @@ public class ClientRateLimitConfig { @Bean FilterRegistrationBean clientRateLimitFilter(RateLimiters rateLimiters, - RateLimitProperties rateLimits) { - var registration = new FilterRegistrationBean<>(new ClientRateLimitFilter(rateLimiters, rateLimits)); + RateLimitProperties rateLimits, + @Qualifier("handlerExceptionResolver") HandlerExceptionResolver exceptionResolver) { + var registration = new FilterRegistrationBean<>( + new ClientRateLimitFilter(rateLimiters, rateLimits, exceptionResolver)); registration.addUrlPatterns("/api/*"); // after the server observation (+1) and trace id (+2) filters registration.setOrder(Ordered.HIGHEST_PRECEDENCE + 3); diff --git a/src/main/java/com/company/resourceapi/web/ClientRateLimitFilter.java b/src/main/java/com/company/resourceapi/web/ClientRateLimitFilter.java index e0637d8..570a96a 100644 --- a/src/main/java/com/company/resourceapi/web/ClientRateLimitFilter.java +++ b/src/main/java/com/company/resourceapi/web/ClientRateLimitFilter.java @@ -1,8 +1,8 @@ package com.company.resourceapi.web; -import static org.springframework.http.HttpHeaders.RETRY_AFTER; - import com.company.resourceapi.config.RateLimitProperties; +import com.company.resourceapi.exceptions.RateLimitExceededException; +import com.company.resourceapi.exceptions.RateLimitExceededException.Scope; import com.company.resourceapi.services.RateLimiters; import jakarta.servlet.FilterChain; import jakarta.servlet.ServletException; @@ -10,9 +10,8 @@ import jakarta.servlet.http.HttpServletResponse; import java.io.IOException; import lombok.RequiredArgsConstructor; -import lombok.extern.slf4j.Slf4j; -import org.springframework.http.HttpStatus; import org.springframework.web.filter.OncePerRequestFilter; +import org.springframework.web.servlet.HandlerExceptionResolver; /** * Limits API requests per client IP address, across all instances (429 + Retry-After). Behind a @@ -20,23 +19,24 @@ * addresses ({@code server.forward-headers-strategy=native}), so clients can't pick their own key. * *

Registered for {@code /api/*} by {@link ClientRateLimitConfig}, after the trace id filter, so a - * refused request still gets its X-Trace-Id. + * refused request still gets its X-Trace-Id. The refusal is answered like any other + * {@link RateLimitExceededException}, by the application's exception handling. */ @RequiredArgsConstructor -@Slf4j public class ClientRateLimitFilter extends OncePerRequestFilter { private final RateLimiters rateLimiters; private final RateLimitProperties rateLimits; + private final HandlerExceptionResolver exceptionResolver; @Override protected void doFilterInternal(HttpServletRequest request, HttpServletResponse response, FilterChain filterChain) throws ServletException, IOException { var limit = rateLimits.client(); if (!rateLimiters.tryAcquire("client-rate:" + request.getRemoteAddr(), limit)) { - log.info("Too many requests from {}", request.getRemoteAddr()); - response.setStatus(HttpStatus.TOO_MANY_REQUESTS.value()); - response.setHeader(RETRY_AFTER, String.valueOf(limit.retryAfterSeconds())); + var refusal = new RateLimitExceededException(Scope.CALLER, + "Too many requests from " + request.getRemoteAddr(), limit.retryAfterSeconds()); + exceptionResolver.resolveException(request, response, null, refusal); return; } filterChain.doFilter(request, response); diff --git a/src/main/resources/application.properties b/src/main/resources/application.properties index 877aed1..ade8268 100755 --- a/src/main/resources/application.properties +++ b/src/main/resources/application.properties @@ -22,7 +22,6 @@ spring.data.redis.repositories.enabled=false # explicit: Redisson brings a JCache provider, which Boot would otherwise prefer over Redis spring.cache.type=redis spring.cache.redis.key-prefix=resourceapi:: -spring.cache.redis.cache-null-values=false resourceapi.cache.sdlc-systems-ttl=10m resourceapi.cache.external-projects-ttl=5m resourceapi.cache.missing-external-projects-ttl=30s diff --git a/src/test/java/com/company/resourceapi/cache/CacheIntegrationTest.java b/src/test/java/com/company/resourceapi/cache/CacheIntegrationTest.java index 3e81b5f..d15b903 100644 --- a/src/test/java/com/company/resourceapi/cache/CacheIntegrationTest.java +++ b/src/test/java/com/company/resourceapi/cache/CacheIntegrationTest.java @@ -1,7 +1,6 @@ package com.company.resourceapi.cache; import static com.company.resourceapi.config.CacheConfig.EXTERNAL_PROJECTS; -import static com.company.resourceapi.config.CacheConfig.MISSING_EXTERNAL_PROJECTS; import static com.company.resourceapi.config.CacheConfig.SDLC_SYSTEMS; import static com.github.tomakehurst.wiremock.client.WireMock.getRequestedFor; import static com.github.tomakehurst.wiremock.client.WireMock.notFound; @@ -39,7 +38,7 @@ import org.springframework.test.web.servlet.MockMvc; /** Redis cache in front of the SDLC systems, against a real Redis (RedisTestContainerInitializer). */ -@SpringBootTest(properties = "resourceapi.cache.missing-external-projects-ttl=1s") +@SpringBootTest(properties = "resourceapi.cache.missing-external-projects-ttl=3s") @AutoConfigureMockMvc class CacheIntegrationTest { @@ -116,19 +115,22 @@ void givenProjectUnknownToSdlcSystemThenThatIsRememberedBriefly() throws Excepti .willReturn(notFound().withHeader("Content-Type", "application/json").withBody("{}"))); mockMvc.perform(get(externalUrl())).andExpect(status().isNotFound()); + int callsForFirstLookup = sdlcSystemServer.findAll(getRequestedFor(urlEqualTo(SDLC_PROJECT_URL))).size(); mockMvc.perform(get(externalUrl())).andExpect(status().isNotFound()); - // one call: the "unknown" answer is remembered, in its own short-lived cache - sdlcSystemServer.verify(1, getRequestedFor(urlEqualTo(SDLC_PROJECT_URL))); - assertThat(cacheManager.getCache(MISSING_EXTERNAL_PROJECTS).get(externalKey())).isNotNull(); - assertThat(cacheManager.getCache(EXTERNAL_PROJECTS).get(externalKey())).isNull(); + // the "unknown" answer is remembered (as a cached null), with the short time to live (3s here) + sdlcSystemServer.verify(callsForFirstLookup, getRequestedFor(urlEqualTo(SDLC_PROJECT_URL))); + var cached = cacheManager.getCache(EXTERNAL_PROJECTS).get(externalKey()); + assertThat(cached).isNotNull(); + assertThat(cached.get()).isNull(); + assertThat(redisTemplate.getExpire(redisKey())).isBetween(1L, 3L); - // the project is created in the SDLC system: found once the "unknown" answer expired (1s here) + // the project is created in the SDLC system: found once the "unknown" answer expired sdlcSystemServer.stubFor(WireMock.get(urlEqualTo(SDLC_PROJECT_URL)) .willReturn(okJson("{\"key\": \"CACHED\", \"name\": \"Created later\"}"))); - mockMvc.perform(get(externalUrl())).andExpect(status().isNotFound()); await(() -> statusOf(externalUrl()) == 200); - assertThat(cacheManager.getCache(MISSING_EXTERNAL_PROJECTS).get(externalKey())).isNull(); + // ... and kept with the long time to live (5m) + assertThat(redisTemplate.getExpire(redisKey())).isGreaterThan(60L); } @Test @@ -142,11 +144,13 @@ void givenProjectRememberedAsUnknownWhenSyncedThenItIsFoundRightAway() throws Ex mockMvc.perform(post(ProjectRestController.ENDPOINT + "/" + projectId + "/sync")) .andExpect(status().isAccepted()); await(() -> projectRepository.findById(projectId).orElseThrow().getName().equals("Created later")); + int callsUntilSynced = sdlcSystemServer.findAll(getRequestedFor(urlEqualTo(SDLC_PROJECT_URL))).size(); - // the sync replaced the "unknown" answer with the project - assertThat(cacheManager.getCache(MISSING_EXTERNAL_PROJECTS).get(externalKey())).isNull(); + // the sync replaced the "unknown" answer with the project, well within its 3s + assertThat(cacheManager.getCache(EXTERNAL_PROJECTS).get(externalKey(), ExternalProject.class)) + .isEqualTo(new ExternalProject("CACHED", "Created later", null)); mockMvc.perform(get(externalUrl())).andExpect(status().isOk()); - sdlcSystemServer.verify(2, getRequestedFor(urlEqualTo(SDLC_PROJECT_URL))); + sdlcSystemServer.verify(callsUntilSynced, getRequestedFor(urlEqualTo(SDLC_PROJECT_URL))); } @Test @@ -190,6 +194,10 @@ private String externalKey() { return sdlcSystem.getId() + ":" + EXTERNAL_ID; } + private String redisKey() { + return keyPrefix + EXTERNAL_PROJECTS + "::" + externalKey(); + } + private static void await(BooleanSupplier condition) throws InterruptedException { var deadline = Instant.now().plus(Duration.ofSeconds(5)); while (!condition.getAsBoolean()) { diff --git a/src/test/java/com/company/resourceapi/ratelimit/RateLimitIntegrationTest.java b/src/test/java/com/company/resourceapi/ratelimit/RateLimitIntegrationTest.java index 0c18a57..28884ed 100644 --- a/src/test/java/com/company/resourceapi/ratelimit/RateLimitIntegrationTest.java +++ b/src/test/java/com/company/resourceapi/ratelimit/RateLimitIntegrationTest.java @@ -178,6 +178,15 @@ void givenSyncAlreadyRunningThenRefusedRequestDoesNotUseAPermit() throws Excepti await(() -> !otherInstanceLock.isLocked(), Duration.ofSeconds(5)); } + @Test + void givenLimiterThenItExpiresFromRedisSoonAfterItsLastUse() throws Exception { + mockMvc.perform(get(externalUrl(project("A")))).andExpect(status().isOk()); + + // 10 periods of 2s is less than the 1-minute minimum + long ttl = redisson.getRateLimiter("sdlc-rate:" + sdlcSystem.getId()).remainTimeToLive(); + assertThat(ttl).isBetween(Duration.ofSeconds(50).toMillis(), Duration.ofMinutes(1).toMillis()); + } + @Test void givenLimiterRemovedFromRedisThenItIsSetUpAgain() throws Exception { mockMvc.perform(get(externalUrl(project("A")))).andExpect(status().isOk()); diff --git a/src/test/java/com/company/resourceapi/ratelimit/RateLimitersTest.java b/src/test/java/com/company/resourceapi/ratelimit/RateLimitersTest.java new file mode 100644 index 0000000..6b6f7aa --- /dev/null +++ b/src/test/java/com/company/resourceapi/ratelimit/RateLimitersTest.java @@ -0,0 +1,49 @@ +package com.company.resourceapi.ratelimit; + +import static org.assertj.core.api.Assertions.assertThat; +import static org.mockito.ArgumentMatchers.anyString; +import static org.mockito.Mockito.mock; +import static org.mockito.Mockito.times; +import static org.mockito.Mockito.verify; +import static org.mockito.Mockito.when; + +import com.company.resourceapi.config.RateLimitProperties.Limit; +import com.company.resourceapi.services.RateLimiters; +import java.time.Duration; +import org.junit.jupiter.api.Test; +import org.redisson.api.RedissonClient; +import org.springframework.beans.factory.ObjectProvider; + +/** Behavior without a working Redis; with one, see RateLimitIntegrationTest. */ +class RateLimitersTest { + + private static final Limit LIMIT = new Limit(1, Duration.ofSeconds(1), Duration.ZERO); + + @Test + void givenRedisFailsThenRequestsAreAllowedWithoutAskingItAgainForAWhile() { + var redisson = mock(RedissonClient.class); + when(redisson.getRateLimiter(anyString())).thenThrow(new IllegalStateException("Redis is down")); + var rateLimiters = new RateLimiters(providerOf(redisson)); + + for (int i = 0; i < 5; i++) { + assertThat(rateLimiters.tryAcquire("test", LIMIT)).isTrue(); + } + + // one failed attempt, then Redis is left alone instead of each request waiting for its timeout + verify(redisson, times(1)).getRateLimiter(anyString()); + } + + @Test + void givenNoRedissonClientThenRequestsAreAllowed() { + var rateLimiters = new RateLimiters(providerOf(null)); + + assertThat(rateLimiters.tryAcquire("test", LIMIT)).isTrue(); + } + + @SuppressWarnings("unchecked") + private static ObjectProvider providerOf(RedissonClient redisson) { + ObjectProvider provider = mock(ObjectProvider.class); + when(provider.getIfAvailable()).thenReturn(redisson); + return provider; + } +} From 39f3d560d9de9f312292c16b1ad0682780b7015b Mon Sep 17 00:00:00 2001 From: Claude Date: Sat, 31 Oct 2026 11:06:48 +0000 Subject: [PATCH 3/3] Tighten RateLimiters visibility; note cached-null dependency Co-Authored-By: Claude Opus 5.5 Claude-Session: https://claude.ai/code/session_012MLyPXGHeCc1fvTeB8JnT2 --- src/main/java/com/company/resourceapi/config/CacheConfig.java | 3 ++- .../java/com/company/resourceapi/services/RateLimiters.java | 4 ++-- 2 files changed, 4 insertions(+), 3 deletions(-) diff --git a/src/main/java/com/company/resourceapi/config/CacheConfig.java b/src/main/java/com/company/resourceapi/config/CacheConfig.java index c89783e..8b0533d 100644 --- a/src/main/java/com/company/resourceapi/config/CacheConfig.java +++ b/src/main/java/com/company/resourceapi/config/CacheConfig.java @@ -35,7 +35,8 @@ public class CacheConfig { /** * Starts from Boot's defaults (spring.cache.redis.*: key prefix, null values, ...) and only - * replaces the value serializer and sets each cache's time to live. + * replaces the value serializer and sets each cache's time to live. External projects rely on + * null values being cached (spring.cache.redis.cache-null-values, true by default). */ @Bean RedisCacheManagerBuilderCustomizer redisCacheCustomizer(CacheTtlProperties ttl) { diff --git a/src/main/java/com/company/resourceapi/services/RateLimiters.java b/src/main/java/com/company/resourceapi/services/RateLimiters.java index 75d1218..787c239 100644 --- a/src/main/java/com/company/resourceapi/services/RateLimiters.java +++ b/src/main/java/com/company/resourceapi/services/RateLimiters.java @@ -29,7 +29,7 @@ public class RateLimiters { private static final int KEEP_ALIVE_PERIODS = 10; private static final Duration MIN_KEEP_ALIVE = Duration.ofMinutes(1); /** How long requests are allowed without asking Redis after it failed. */ - static final Duration PAUSE_AFTER_FAILURE = Duration.ofSeconds(10); + private static final Duration PAUSE_AFTER_FAILURE = Duration.ofSeconds(10); /** Limiters this instance has set up, most recently used kept (bounded: one per project). */ private static final int MAX_REMEMBERED = 10_000; @@ -87,7 +87,7 @@ private boolean isPaused() { return false; } - static Duration keepAlive(RateLimitProperties.Limit limit) { + private static Duration keepAlive(RateLimitProperties.Limit limit) { var periods = limit.period().multipliedBy(KEEP_ALIVE_PERIODS); return periods.compareTo(MIN_KEEP_ALIVE) > 0 ? periods : MIN_KEEP_ALIVE; }