-
-
Notifications
You must be signed in to change notification settings - Fork 10.2k
Feature/openapi rate limit function #5267
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Changes from 2 commits
1b3e122
8b9d09b
1bae71f
a4c84f8
826ea74
9e2c4e8
ace5076
a622f56
4939d70
a9da81d
22ebb4f
edb9d8b
6011aeb
8463974
0fe67f5
5390f62
9126bb3
ddfd8b3
46df20c
e07de19
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -16,9 +16,13 @@ | |
| */ | ||
| package com.ctrip.framework.apollo.openapi.filter; | ||
|
|
||
| import com.ctrip.framework.apollo.openapi.entity.ConsumerToken; | ||
| import com.ctrip.framework.apollo.openapi.util.ConsumerAuditUtil; | ||
| import com.ctrip.framework.apollo.openapi.util.ConsumerAuthUtil; | ||
|
|
||
| import com.ctrip.framework.apollo.portal.component.config.PortalConfig; | ||
| import com.google.common.cache.Cache; | ||
| import com.google.common.cache.CacheBuilder; | ||
| import com.google.common.util.concurrent.RateLimiter; | ||
| import java.io.IOException; | ||
|
|
||
| import javax.servlet.Filter; | ||
|
|
@@ -29,18 +33,29 @@ | |
| import javax.servlet.ServletResponse; | ||
| import javax.servlet.http.HttpServletRequest; | ||
| import javax.servlet.http.HttpServletResponse; | ||
| import org.apache.commons.lang3.tuple.ImmutablePair; | ||
| import org.slf4j.Logger; | ||
| import org.slf4j.LoggerFactory; | ||
| import org.springframework.http.HttpHeaders; | ||
|
|
||
| /** | ||
| * @author Jason Song([email protected]) | ||
| */ | ||
| public class ConsumerAuthenticationFilter implements Filter { | ||
|
|
||
| private static final Logger logger = LoggerFactory.getLogger(ConsumerAuthenticationFilter.class); | ||
|
|
||
| private final ConsumerAuthUtil consumerAuthUtil; | ||
| private final ConsumerAuditUtil consumerAuditUtil; | ||
| private final PortalConfig portalConfig; | ||
youngzil marked this conversation as resolved.
Outdated
Show resolved
Hide resolved
|
||
|
|
||
| public ConsumerAuthenticationFilter(ConsumerAuthUtil consumerAuthUtil, ConsumerAuditUtil consumerAuditUtil) { | ||
| private static final Cache<String, ImmutablePair<Long, RateLimiter>> LIMITER = CacheBuilder.newBuilder().build(); | ||
| private static final int WARMUP_MILLIS = 1000; // ms | ||
youngzil marked this conversation as resolved.
Outdated
Show resolved
Hide resolved
|
||
|
|
||
| public ConsumerAuthenticationFilter(ConsumerAuthUtil consumerAuthUtil, ConsumerAuditUtil consumerAuditUtil, PortalConfig portalConfig) { | ||
| this.consumerAuthUtil = consumerAuthUtil; | ||
| this.consumerAuditUtil = consumerAuditUtil; | ||
| this.portalConfig = portalConfig; | ||
| } | ||
|
|
||
| @Override | ||
|
|
@@ -55,14 +70,28 @@ public void doFilter(ServletRequest req, ServletResponse resp, FilterChain chain | |
| HttpServletResponse response = (HttpServletResponse) resp; | ||
|
|
||
| String token = request.getHeader(HttpHeaders.AUTHORIZATION); | ||
| ConsumerToken consumerToken = consumerAuthUtil.getConsumerToken(token); | ||
|
|
||
| Long consumerId = consumerAuthUtil.getConsumerId(token); | ||
|
|
||
| if (consumerId == null) { | ||
| if (null == consumerToken || consumerToken.getConsumerId() <= 0) { | ||
youngzil marked this conversation as resolved.
Outdated
Show resolved
Hide resolved
|
||
| response.sendError(HttpServletResponse.SC_UNAUTHORIZED, "Unauthorized"); | ||
| return; | ||
| } | ||
|
|
||
| Integer limitCount = consumerToken.getLimitCount(); | ||
| if (portalConfig.isOpenApiLimitEnabled() && limitCount > 0) { | ||
youngzil marked this conversation as resolved.
Outdated
Show resolved
Hide resolved
|
||
| try { | ||
| ImmutablePair<Long, RateLimiter> rateLimiterPair = getOrCreateRateLimiterPair(token, limitCount); | ||
| long warmupToMillis = rateLimiterPair.getLeft() + WARMUP_MILLIS; | ||
nobodyiam marked this conversation as resolved.
Show resolved
Hide resolved
|
||
| if (System.currentTimeMillis() > warmupToMillis && !rateLimiterPair.getRight().tryAcquire()) { | ||
| response.sendError(HttpServletResponse.SC_FORBIDDEN, "Too many call requests, the flow is limited"); | ||
|
||
| return; | ||
| } | ||
|
Comment on lines
+91
to
+92
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Potential race condition in warmup check The warmup check compares two timestamps without synchronization, which could lead to inconsistent behavior in high-concurrency scenarios. Consider using atomic operations or moving the warmup logic into the RateLimiter itself. - long warmupToMillis = rateLimiterPair.getLeft() + WARMUP_MILLIS;
- if (System.currentTimeMillis() > warmupToMillis && !rateLimiterPair.getRight().tryAcquire()) {
+ RateLimiter limiter = rateLimiterPair.getRight();
+ if (!limiter.tryAcquire()) {
response.sendError(HttpServletResponse.SC_FORBIDDEN, "Too many call requests, the flow is limited");
return;
}
|
||
| } catch (Exception e) { | ||
| logger.error("ConsumerAuthenticationFilter ratelimit error", e); | ||
| } | ||
| } | ||
youngzil marked this conversation as resolved.
Show resolved
Hide resolved
|
||
|
|
||
| long consumerId = consumerToken.getConsumerId(); | ||
| consumerAuthUtil.storeConsumerId(request, consumerId); | ||
| consumerAuditUtil.audit(request, consumerId); | ||
|
|
||
|
|
@@ -73,4 +102,14 @@ public void doFilter(ServletRequest req, ServletResponse resp, FilterChain chain | |
| public void destroy() { | ||
| //nothing | ||
| } | ||
|
|
||
| private ImmutablePair<Long, RateLimiter> getOrCreateRateLimiterPair(String key, Integer limitCount) { | ||
| ImmutablePair<Long, RateLimiter> rateLimiterPair = LIMITER.getIfPresent(key); | ||
| if (rateLimiterPair == null) { | ||
| rateLimiterPair = ImmutablePair.of(System.currentTimeMillis(), RateLimiter.create(limitCount)); | ||
| LIMITER.put(key, rateLimiterPair); | ||
| } | ||
| return rateLimiterPair; | ||
| } | ||
youngzil marked this conversation as resolved.
Show resolved
Hide resolved
|
||
|
|
||
| } | ||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -138,12 +138,15 @@ public ConsumerToken getConsumerTokenByAppId(String appId) { | |
| return consumerTokenRepository.findByConsumerId(consumer.getId()); | ||
| } | ||
|
|
||
| public Long getConsumerIdByToken(String token) { | ||
| public ConsumerToken getConsumerTokenByToken(String token) { | ||
| if (Strings.isNullOrEmpty(token)) { | ||
| return null; | ||
| } | ||
| ConsumerToken consumerToken = consumerTokenRepository.findTopByTokenAndExpiresAfter(token, | ||
| new Date()); | ||
| return consumerTokenRepository.findTopByTokenAndExpiresAfter(token, new Date()); | ||
| } | ||
|
|
||
| public Long getConsumerIdByToken(String token) { | ||
| ConsumerToken consumerToken = getConsumerTokenByToken(token); | ||
| return consumerToken == null ? null : consumerToken.getConsumerId(); | ||
| } | ||
|
|
||
|
|
@@ -311,7 +314,9 @@ public void createConsumerAudits(Iterable<ConsumerAudit> consumerAudits) { | |
| @Transactional | ||
| public ConsumerToken createConsumerToken(ConsumerToken entity) { | ||
| entity.setId(0); //for protection | ||
|
|
||
| if (entity.getLimitCount() <= 0) { | ||
| entity.setLimitCount(portalConfig.openApiLimitCount()); | ||
| } | ||
youngzil marked this conversation as resolved.
Outdated
Show resolved
Hide resolved
|
||
| return consumerTokenRepository.save(entity); | ||
| } | ||
|
|
||
|
|
@@ -322,6 +327,7 @@ private ConsumerToken generateConsumerToken(Consumer consumer, Date expires) { | |
|
|
||
| ConsumerToken consumerToken = new ConsumerToken(); | ||
| consumerToken.setConsumerId(consumerId); | ||
| consumerToken.setLimitCount(portalConfig.openApiLimitCount()); | ||
|
||
| consumerToken.setExpires(expires); | ||
| consumerToken.setDataChangeCreatedBy(createdBy); | ||
| consumerToken.setDataChangeCreatedTime(createdTime); | ||
|
|
||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -242,6 +242,14 @@ public String consumerTokenSalt() { | |
| return getValue("consumer.token.salt", "apollo-portal"); | ||
| } | ||
|
|
||
| public int openApiLimitCount() { | ||
| return getIntProperty("open.api.limit.count", 20); | ||
| } | ||
|
|
||
| public boolean isOpenApiLimitEnabled() { | ||
|
||
| return getBooleanProperty("open.api.limit.enabled", false); | ||
| } | ||
|
|
||
| public boolean isEmailEnabled() { | ||
| return getBooleanProperty("email.enabled", false); | ||
| } | ||
|
|
||
| Original file line number | Diff line number | Diff line change | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|
|
|
@@ -16,9 +16,16 @@ | |||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| */ | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| package com.ctrip.framework.apollo.openapi.filter; | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| import com.ctrip.framework.apollo.openapi.entity.ConsumerToken; | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| import com.ctrip.framework.apollo.openapi.util.ConsumerAuditUtil; | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| import com.ctrip.framework.apollo.openapi.util.ConsumerAuthUtil; | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| import com.ctrip.framework.apollo.portal.component.config.PortalConfig; | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| import java.io.IOException; | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| import java.util.concurrent.ExecutorService; | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| import java.util.concurrent.Executors; | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| import java.util.concurrent.TimeUnit; | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| import javax.servlet.ServletException; | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| import org.junit.Before; | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| import org.junit.Test; | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| import org.junit.runner.RunWith; | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
|
|
@@ -33,6 +40,9 @@ | |||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| import static org.mockito.ArgumentMatchers.anyLong; | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| import static org.mockito.ArgumentMatchers.anyString; | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| import static org.mockito.ArgumentMatchers.eq; | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| import static org.mockito.Mockito.atLeast; | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| import static org.mockito.Mockito.atLeastOnce; | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| import static org.mockito.Mockito.atMost; | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| import static org.mockito.Mockito.never; | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| import static org.mockito.Mockito.times; | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| import static org.mockito.Mockito.verify; | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
|
|
@@ -48,6 +58,9 @@ public class ConsumerAuthenticationFilterTest { | |||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| private ConsumerAuthUtil consumerAuthUtil; | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| @Mock | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| private ConsumerAuditUtil consumerAuditUtil; | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| @Mock | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| private PortalConfig portalConfig; | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| @Mock | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| private HttpServletRequest request; | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| @Mock | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
|
|
@@ -57,7 +70,7 @@ public class ConsumerAuthenticationFilterTest { | |||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| @Before | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| public void setUp() throws Exception { | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| authenticationFilter = new ConsumerAuthenticationFilter(consumerAuthUtil, consumerAuditUtil); | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| authenticationFilter = new ConsumerAuthenticationFilter(consumerAuthUtil, consumerAuditUtil, portalConfig); | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| } | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| @Test | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
|
|
@@ -89,4 +102,101 @@ public void testAuthFailed() throws Exception { | |||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| verify(consumerAuditUtil, never()).audit(eq(request), anyLong()); | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| verify(filterChain, never()).doFilter(request, response); | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| } | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| @Test | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| public void testRateLimitSuccessfully() throws Exception { | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| String someToken = "someToken"; | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| Long someConsumerId = 1L; | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| int qps = 5; | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| int durationInSeconds = 10; | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
youngzil marked this conversation as resolved.
Outdated
Show resolved
Hide resolved
|
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| setupRateLimitMocks(someToken, someConsumerId, qps); | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| Runnable task = () -> { | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| try { | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| authenticationFilter.doFilter(request, response, filterChain); | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| } catch (IOException e) { | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| throw new RuntimeException(e); | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| } catch (ServletException e) { | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| throw new RuntimeException(e); | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| } | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| }; | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| executeWithQps(qps, task, durationInSeconds); | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| int total = qps * durationInSeconds; | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| verify(consumerAuthUtil, times(total)).storeConsumerId(request, someConsumerId); | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| verify(consumerAuditUtil, times(total)).audit(request, someConsumerId); | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| verify(filterChain, times(total)).doFilter(request, response); | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| } | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
|
Comment on lines
+107
to
+138
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 🛠️ Refactor suggestion Improve test determinism by refining timing control in rate limit tests The |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| @Test | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| public void testRateLimitPartFailure() throws Exception { | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| String someToken = "someToken"; | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| Long someConsumerId = 1L; | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| int qps = 5; | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| int durationInSeconds = 10; | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| setupRateLimitMocks(someToken, someConsumerId, qps); | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| Runnable task = () -> { | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| try { | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| authenticationFilter.doFilter(request, response, filterChain); | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| } catch (IOException e) { | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| throw new RuntimeException(e); | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| } catch (ServletException e) { | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| throw new RuntimeException(e); | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| } | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| }; | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| executeWithQps(qps + 1, task, durationInSeconds); | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| int leastTimes = qps * durationInSeconds; | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| int mostTimes = (qps + 1) * durationInSeconds; | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| verify(response, atLeastOnce()).sendError(eq(HttpServletResponse.SC_FORBIDDEN), anyString()); | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| verify(consumerAuthUtil, atLeast(leastTimes)).storeConsumerId(request, someConsumerId); | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| verify(consumerAuthUtil, atMost(mostTimes)).storeConsumerId(request, someConsumerId); | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| verify(consumerAuditUtil, atLeast(leastTimes)).audit(request, someConsumerId); | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| verify(consumerAuditUtil, atMost(mostTimes)).audit(request, someConsumerId); | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| verify(filterChain, atLeast(leastTimes)).doFilter(request, response); | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| verify(filterChain, atMost(mostTimes)).doFilter(request, response); | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| } | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
|
Comment on lines
+137
to
+175
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 🛠️ Refactor suggestion Enhance reliability of rate limit failure test Similar to
Comment on lines
+110
to
+175
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 🛠️ Refactor suggestion Test structure looks good, but consider test reliability improvements. The rate limit tests are well-structured with clear assertions, but they might benefit from more deterministic timing control. Consider using a more deterministic approach: - executeWithQps(qps, task, durationInSeconds);
+ RateLimiter rateLimiter = RateLimiter.create(qps);
+ CountDownLatch latch = new CountDownLatch(qps * durationInSeconds);
+ for (int i = 0; i < qps * durationInSeconds; i++) {
+ rateLimiter.acquire();
+ task.run();
+ latch.countDown();
+ }
+ latch.await(durationInSeconds + 1, TimeUnit.SECONDS);
|
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| private void setupRateLimitMocks(String someToken, Long someConsumerId, int qps) { | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| ConsumerToken someConsumerToken = new ConsumerToken(); | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| someConsumerToken.setConsumerId(someConsumerId); | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| someConsumerToken.setLimitCount(qps); | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| when(request.getHeader(HttpHeaders.AUTHORIZATION)).thenReturn(someToken); | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| when(consumerAuthUtil.getConsumerId(someToken)).thenReturn(someConsumerId); | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| when(consumerAuthUtil.getConsumerToken(someToken)).thenReturn(someConsumerToken); | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| when(portalConfig.isOpenApiLimitEnabled()).thenReturn(true); | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| } | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| public static void executeWithQps(int qps, Runnable task, int durationInSeconds) { | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| ExecutorService executor = Executors.newFixedThreadPool(qps); | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| long totalTasks = qps * durationInSeconds; | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| for (int i = 0; i < totalTasks; i++) { | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| executor.submit(task); | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| try { | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| TimeUnit.MILLISECONDS.sleep(1000 / qps); // Control QPS | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| } catch (InterruptedException e) { | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| Thread.currentThread().interrupt(); | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| break; | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| } | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| } | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| executor.shutdown(); | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| } | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
|
Comment on lines
+189
to
+204
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Enhance executeWithQps reliability and resource management. The current implementation has several potential issues:
Consider this improved implementation: public static void executeWithQps(int qps, Runnable task, int durationInSeconds) {
ExecutorService executor = Executors.newFixedThreadPool(qps);
long totalTasks = qps * durationInSeconds;
+ try {
for (int i = 0; i < totalTasks; i++) {
- executor.submit(task);
+ Future<?> future = executor.submit(task);
try {
+ future.get(1000 / qps, TimeUnit.MILLISECONDS); // Ensure task completes
TimeUnit.MILLISECONDS.sleep(1000 / qps); // Control QPS
} catch (InterruptedException e) {
Thread.currentThread().interrupt();
break;
+ } catch (TimeoutException | ExecutionException e) {
+ throw new RuntimeException("Task execution failed", e);
}
}
+ } finally {
executor.shutdown();
+ try {
+ if (!executor.awaitTermination(5, TimeUnit.SECONDS)) {
+ executor.shutdownNow();
+ }
+ } catch (InterruptedException e) {
+ executor.shutdownNow();
+ Thread.currentThread().interrupt();
+ }
+ }
}📝 Committable suggestion
Suggested change
|
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| } | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| Original file line number | Diff line number | Diff line change | ||||
|---|---|---|---|---|---|---|
|
|
@@ -161,6 +161,7 @@ CREATE TABLE `ConsumerToken` ( | |||||
| `Id` int(11) unsigned NOT NULL AUTO_INCREMENT COMMENT '自增Id', | ||||||
| `ConsumerId` int(11) unsigned DEFAULT NULL COMMENT 'ConsumerId', | ||||||
| `Token` varchar(128) NOT NULL DEFAULT '' COMMENT 'token', | ||||||
| `LimitCount` int NOT NULL DEFAULT 20 COMMENT '限流值', | ||||||
|
||||||
| `LimitCount` int NOT NULL DEFAULT 20 COMMENT '限流值', | |
| `LimitCount` int NOT NULL DEFAULT 20 CHECK (`LimitCount` BETWEEN 1 AND 1000) COMMENT 'Rate limit value (requests per minute). Default: 20 req/min. Range: 1-1000.', |
Uh oh!
There was an error while loading. Please reload this page.