Feature/openapi rate limit function #5267
Conversation
dosubot
bot
added
the
size:L
WalkthroughThe changes in this pull request introduce a rate limiting feature to the Apollo Portal's OpenAPI. Key updates include the addition of a Changes
Assessment against linked issues
Possibly related PRs
Suggested labels
Suggested reviewers
Poem
Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on your favorite social media? 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 10
🧹 Outside diff range and nitpick comments (19)
scripts/sql/src/delta/v230-v240/apolloportaldb-v230-v240.sql (3)
22-23: Consider adding validation and indexing for the rate limit column.The schema modification looks good overall, but consider these improvements:
- Add a CHECK constraint to ensure LimitCount is positive
- Consider adding an index if this column will be frequently queried during rate limit checks
ALTER TABLE `ConsumerToken` - ADD COLUMN `LimitCount` int NOT NULL DEFAULT 20 COMMENT '限流值' AFTER `Token`; + ADD COLUMN `LimitCount` int NOT NULL DEFAULT 20 CHECK (`LimitCount` > 0) COMMENT 'Rate limit value' AFTER `Token`, + ADD INDEX idx_consumer_token_limit (`LimitCount`);
22-23: Translate Chinese comment to English for better international collaboration.The column comment '限流值' (rate limit value) should be in English to maintain consistency and improve accessibility for international developers.
16-25: Add rollback script for safe migrations.To ensure safe deployments and rollbacks:
- Create a corresponding down migration script
- Document the migration process
- Consider adding a migration verification step
Would you like me to help create a rollback script and migration verification steps?
apollo-portal/src/main/java/com/ctrip/framework/apollo/portal/spi/configuration/AuthFilterConfiguration.java (1)
Line range hint
1-47: Consider adding documentation for rate limiting configuration.While the implementation looks correct, it would be helpful to add documentation about the rate limiting feature, especially:
- How to configure rate limits through PortalConfig
- Default rate limiting behavior
- Impact on existing consumers
scripts/sql/profiles/mysql-database-not-specified/delta/v230-v240/apolloportaldb-v230-v240.sql (1)
29-30: Schema change looks good, with some suggestions for improvement.The ALTER TABLE statement safely adds the rate limiting capability to ConsumerToken table. However, consider the following suggestions:
- Consider adding an English translation alongside the Chinese comment for better international collaboration
- Document the rationale for the default value of 20 requests, either in the comment or in accompanying documentation
Consider updating the comment to be bilingual:
- ADD COLUMN `LimitCount` int NOT NULL DEFAULT 20 COMMENT '限流值' AFTER `Token`; + ADD COLUMN `LimitCount` int NOT NULL DEFAULT 20 COMMENT 'Rate limit value (限流值)' AFTER `Token`;scripts/sql/profiles/h2-default/delta/v230-v240/apolloportaldb-v230-v240.sql (1)
34-34: Consider documenting the default rate limit value.The ALTER TABLE statement correctly adds the LimitCount column with appropriate constraints. However, a few suggestions:
- The default value of 20 should be documented to explain why this specific number was chosen.
- Consider adding an English translation alongside the Chinese comment for better international collaboration.
Consider this alternative with bilingual comments:
-ALTER TABLE `ConsumerToken` ADD COLUMN `LimitCount` int NOT NULL DEFAULT 20 COMMENT '限流值' AFTER `Token`; +ALTER TABLE `ConsumerToken` ADD COLUMN `LimitCount` int NOT NULL DEFAULT 20 COMMENT 'Rate limit value (限流值)' AFTER `Token`;apollo-portal/src/main/java/com/ctrip/framework/apollo/openapi/entity/ConsumerToken.java (2)
Line range hint
44-73: Add JavaDoc documentation for the rate limiting functionality.The new rate limiting fields and methods should be documented to explain their purpose, units (requests per second/minute/hour), and any constraints.
Add the following documentation:
/** * The maximum number of requests allowed for this consumer token. * A value of null or 0 indicates no rate limiting. */ @Column(name = "`LimitCount`", nullable = false) private Integer limitCount; /** * Gets the rate limit count for this consumer token. * @return the maximum number of requests allowed, or null if no limit */ public Integer getLimitCount() { return limitCount; } /** * Sets the rate limit count for this consumer token. * @param limitCount the maximum number of requests allowed (must be non-negative) * @throws IllegalArgumentException if limitCount is negative */ public void setLimitCount(Integer limitCount) { // ... implementation ... }
44-46: Consider using primitive type for limitCount.Since this is a rate limiting feature and the column is non-nullable, consider using a primitive
intinstead ofIntegerto prevent null checks in the rate limiting logic.@Column(name = "`LimitCount`", nullable = false) - private Integer limitCount; + private int limitCount;apollo-portal/src/main/java/com/ctrip/framework/apollo/portal/component/config/PortalConfig.java (2)
245-247: Consider adding input validation for the rate limit count.While the implementation is correct, consider adding validation to ensure the rate limit is a positive number. This could prevent potential issues with invalid configurations.
public int openApiLimitCount() { - return getIntProperty("open.api.limit.count", 20); + int limit = getIntProperty("open.api.limit.count", 20); + return checkInt(limit, 1, Integer.MAX_VALUE, 20); }
245-251: Add configuration documentation.Please add Javadoc comments for both methods to document:
- The purpose of each configuration
- The meaning and impact of the default values
- The relationship between these settings and rate limiting behavior
+/** + * Gets the rate limit count for OpenAPI requests. + * This value determines how many requests a consumer token can make within a time window. + * + * @return the rate limit count, defaults to 20 requests + */ public int openApiLimitCount() { return getIntProperty("open.api.limit.count", 20); } +/** + * Checks if OpenAPI rate limiting is enabled. + * When disabled, no rate limiting will be applied regardless of the limit count. + * + * @return true if rate limiting is enabled, false otherwise (defaults to false) + */ public boolean isOpenApiLimitEnabled() { return getBooleanProperty("open.api.limit.enabled", false); }apollo-portal/src/main/java/com/ctrip/framework/apollo/openapi/service/ConsumerService.java (3)
317-319: Consider separating zero from negative values.The current implementation treats zero the same as negative values. Consider:
- Using a constant for the minimum allowed limit
- Handling zero separately from negative values
+ private static final int MIN_LIMIT_COUNT = 1; + @Transactional public ConsumerToken createConsumerToken(ConsumerToken entity) { entity.setId(0); //for protection - if (entity.getLimitCount() <= 0) { + if (entity.getLimitCount() < MIN_LIMIT_COUNT) { entity.setLimitCount(portalConfig.openApiLimitCount()); }
330-330: Consider extracting default limit count to a constant.To maintain consistency and make future updates easier, consider extracting the default limit count to a class constant. This would ensure both
createConsumerTokenandgenerateConsumerTokenuse the same logic for default values.+ private static final int DEFAULT_LIMIT_COUNT = -1; // Will be replaced by portalConfig value + private ConsumerToken generateConsumerToken(Consumer consumer, Date expires) { // ... - consumerToken.setLimitCount(portalConfig.openApiLimitCount()); + consumerToken.setLimitCount(DEFAULT_LIMIT_COUNT); // ... }
Line range hint
141-330: Consider adding documentation and tests for rate limiting.The rate limiting implementation looks solid, but could benefit from:
- JavaDoc documentation explaining the rate limiting behavior
- Unit tests covering:
- Default limit count assignment
- Token expiration with rate limits
- Edge cases (0, negative values)
- Integration tests verifying rate limiting behavior end-to-end
Would you like me to help generate:
- JavaDoc documentation for the rate limiting methods?
- Unit test cases covering the rate limiting functionality?
scripts/sql/src/apolloportaldb.sql (1)
158-158: Consider additional rate limiting configurations.While the current implementation with a single
LimitCountcolumn is a good start, consider these enhancements for a more robust rate limiting solution:
- Time window configuration (e.g., requests per minute/hour)
- Burst limit configuration
- Different limits for different endpoints
This could be achieved by either:
- Adding additional columns to this table
- Creating a separate rate limit configuration table
Would you like me to propose the schema for these additional rate limiting configurations?
scripts/sql/profiles/mysql-database-not-specified/apolloportaldb.sql (1)
165-165: Consider adding an index for rate limit lookups.Since the
LimitCountcolumn will be frequently accessed during API request processing for rate limit checks, consider adding an index to optimize these lookups.`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 '限流值', + KEY `IX_LimitCount` (`LimitCount`), PRIMARY KEY (`Id`),scripts/sql/profiles/mysql-default/apolloportaldb.sql (2)
170-170: Consider adding a maximum value constraint.To prevent potential abuse, consider adding a maximum value constraint to the
LimitCountcolumn. This would ensure that even if a token's limit is modified, it cannot exceed a safe threshold.- `LimitCount` int NOT NULL DEFAULT 20 COMMENT '限流值', + `LimitCount` int NOT NULL DEFAULT 20 CHECK (`LimitCount` <= 1000) COMMENT '限流值 (maximum: 1000)',
170-170: Document migration strategy for existing records.Since this is an alteration to an existing table, please ensure you have a migration strategy for existing consumer tokens. The default value will apply to new records, but you might want to review and potentially adjust limits for existing tokens based on their usage patterns.
Consider the following approaches:
- Let existing records use the default value of 20
- Implement a migration script that sets limits based on historical usage patterns
- Temporarily set higher limits for existing tokens and gradually adjust them
apollo-portal/src/main/java/com/ctrip/framework/apollo/openapi/filter/ConsumerAuthenticationFilter.java (1)
106-113: Consider UsingconsumerIdInstead oftokenas Cache KeyUsing the
tokenstring as the cache key may lead to increased memory usage if tokens are long strings or change frequently. It may be more efficient to use theconsumerIdas the cache key.Modify the
getOrCreateRateLimiterPairmethod and related calls to useconsumerIdas the key:- private ImmutablePair<Long, RateLimiter> getOrCreateRateLimiterPair(String key, Integer limitCount) { + private ImmutablePair<Long, RateLimiter> getOrCreateRateLimiterPair(Long consumerId, Integer limitCount) { - ImmutablePair<Long, RateLimiter> rateLimiterPair = LIMITER.getIfPresent(key); + ImmutablePair<Long, RateLimiter> rateLimiterPair = LIMITER.getIfPresent(String.valueOf(consumerId)); - LIMITER.put(key, rateLimiterPair); + LIMITER.put(String.valueOf(consumerId), rateLimiterPair);Adjust the calling code accordingly:
- ImmutablePair<Long, RateLimiter> rateLimiterPair = getOrCreateRateLimiterPair(token, limitCount); + ImmutablePair<Long, RateLimiter> rateLimiterPair = getOrCreateRateLimiterPair(consumerToken.getConsumerId(), limitCount);apollo-portal/src/test/java/com/ctrip/framework/apollo/openapi/filter/ConsumerAuthenticationFilterTest.java (1)
185-200: UseScheduledExecutorServicefor accurate QPS controlThe
executeWithQpsmethod may not achieve accurate QPS due to the limitations ofTimeUnit.MILLISECONDS.sleepin controlling timing precisely. Switching to aScheduledExecutorServicecan provide more accurate scheduling of tasks at fixed rates, enhancing the precision of your tests.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (15)
apollo-portal/src/main/java/com/ctrip/framework/apollo/openapi/entity/ConsumerToken.java(3 hunks)apollo-portal/src/main/java/com/ctrip/framework/apollo/openapi/filter/ConsumerAuthenticationFilter.java(4 hunks)apollo-portal/src/main/java/com/ctrip/framework/apollo/openapi/service/ConsumerService.java(3 hunks)apollo-portal/src/main/java/com/ctrip/framework/apollo/openapi/util/ConsumerAuthUtil.java(2 hunks)apollo-portal/src/main/java/com/ctrip/framework/apollo/portal/component/config/PortalConfig.java(1 hunks)apollo-portal/src/main/java/com/ctrip/framework/apollo/portal/spi/configuration/AuthFilterConfiguration.java(2 hunks)apollo-portal/src/test/java/com/ctrip/framework/apollo/openapi/filter/ConsumerAuthenticationFilterTest.java(5 hunks)scripts/sql/profiles/h2-default/apolloportaldb.sql(1 hunks)scripts/sql/profiles/h2-default/delta/v230-v240/apolloportaldb-v230-v240.sql(1 hunks)scripts/sql/profiles/mysql-database-not-specified/apolloportaldb.sql(1 hunks)scripts/sql/profiles/mysql-database-not-specified/delta/v230-v240/apolloportaldb-v230-v240.sql(1 hunks)scripts/sql/profiles/mysql-default/apolloportaldb.sql(1 hunks)scripts/sql/profiles/mysql-default/delta/v230-v240/apolloportaldb-v230-v240.sql(1 hunks)scripts/sql/src/apolloportaldb.sql(1 hunks)scripts/sql/src/delta/v230-v240/apolloportaldb-v230-v240.sql(1 hunks)
✅ Files skipped from review due to trivial changes (1)
- scripts/sql/profiles/mysql-default/delta/v230-v240/apolloportaldb-v230-v240.sql
🔇 Additional comments (17)
scripts/sql/src/delta/v230-v240/apolloportaldb-v230-v240.sql (1)
22-23: Document the default rate limit value.
The default value of 20 seems arbitrary. Consider:
- Adding a comment explaining why 20 was chosen as the default
- Making this configurable via environment variables or system properties
- Documenting this value in the README or configuration guide
apollo-portal/src/main/java/com/ctrip/framework/apollo/portal/spi/configuration/AuthFilterConfiguration.java (3)
22-22: LGTM: Required import for rate limiting configuration.
The addition of PortalConfig import is necessary to support the new rate limiting functionality.
32-34: LGTM: Method signature updated to support rate limiting.
The addition of PortalConfig parameter is consistent with the rate limiting requirements from issue #5244.
Let's verify that all callers of this bean are updated with the new parameter:
✅ Verification successful
Let me try a different search strategy to find the relevant information.
LGTM: The new parameter is correctly used in all instances
Based on the search results, I can confirm that:
- The
ConsumerAuthenticationFilteris only instantiated in two places:- In the
AuthFilterConfigurationbean definition - In the test class
ConsumerAuthenticationFilterTest
- In the
- Both instances correctly use all three parameters including the new
PortalConfig - There are no other usages or instantiations of this filter in the codebase
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Check for any autowired usages of openApiAuthenticationFilter
# to ensure they're compatible with the new signature
# Search for any @Autowired or @Resource usage of this filter
rg -l "(@Autowired|@Resource).*openApiAuthenticationFilter"
# Search for any manual instantiations of AuthFilterConfiguration
ast-grep --pattern 'new AuthFilterConfiguration($$$)'
Length of output: 5009
Script:
#!/bin/bash
# Search for the bean definition and its usages
rg -l "ConsumerAuthenticationFilter" --type java
# Then let's look at the actual content of files containing this filter
rg "ConsumerAuthenticationFilter" -A 5 -B 5 --type java
Length of output: 12544
38-38: LGTM: Filter initialization updated with rate limiting support.
The ConsumerAuthenticationFilter constructor is correctly updated to include the portalConfig parameter for rate limiting.
Let's verify the constructor signature matches:
✅ Verification successful
Let me try another search pattern to find the constructor definition.
Let me search specifically for the constructor implementation.
Constructor signature matches the filter initialization
The verification confirms that the ConsumerAuthenticationFilter constructor signature exactly matches the initialization in AuthFilterConfiguration. The constructor accepts three parameters: ConsumerAuthUtil, ConsumerAuditUtil, and PortalConfig, which are correctly provided in the same order during filter initialization.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Verify ConsumerAuthenticationFilter constructor signature
# Search for the constructor definition
ast-grep --pattern 'ConsumerAuthenticationFilter($$$) {
$$$
}'
Length of output: 5009
Script:
#!/bin/bash
# Search for ConsumerAuthenticationFilter class definition and constructor
rg -A 10 "class ConsumerAuthenticationFilter"
Length of output: 3175
Script:
#!/bin/bash
# Search for ConsumerAuthenticationFilter constructor implementation
rg -B 2 -A 5 "ConsumerAuthenticationFilter\(.*\)"
Length of output: 3525
apollo-portal/src/main/java/com/ctrip/framework/apollo/openapi/util/ConsumerAuthUtil.java (2)
19-19: LGTM: Import addition is appropriate.
The ConsumerToken import is necessary for the new functionality and follows proper import ordering.
41-44: Verify integration with authentication flow.
The new method introduces a different way to handle consumer tokens. Let's verify:
- The relationship between this method and existing authentication flow
- Potential impact on current consumers
✅ Verification successful
Let me gather more specific information about the authentication flow integration.
This will help us understand:
- How the new getConsumerToken method is used in the authentication filter
- The relationship between ConsumerService and token handling
- The test coverage and expected behavior of the ConsumerAuthUtil
Based on the analysis of the code, I can now provide the final response:
The new getConsumerToken method is properly integrated with the authentication flow
The verification shows that:
- The method is correctly used in ConsumerAuthenticationFilter as part of the token validation flow
- It maintains consistency with existing token handling patterns by:
- Being used for initial authentication before any consumer operations
- Supporting rate limiting through the token's limitCount property
- Integrating with the existing consumer ID storage mechanism
- The implementation is well-tested as shown in ConsumerAuthUtilTest
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Analyze the usage patterns and integration points
# Check for authentication filter usage
echo "Checking authentication filter implementation..."
ast-grep --pattern 'class $_ implements Filter {
$$$
getConsumerToken($$$)
$$$
}'
# Look for existing token handling patterns
echo "Checking existing token handling patterns..."
rg -l "getConsumerId|storeConsumerId|retrieveConsumerId" --type java
# Check for potential security implications
echo "Checking security-related patterns..."
rg -l "SecurityContext|Authentication|@Secured|@PreAuthorize" --type java
Length of output: 6035
Script:
#!/bin/bash
# Check ConsumerAuthenticationFilter implementation
echo "Checking ConsumerAuthenticationFilter implementation..."
cat apollo-portal/src/main/java/com/ctrip/framework/apollo/openapi/filter/ConsumerAuthenticationFilter.java
# Check ConsumerService implementation
echo "Checking ConsumerService implementation..."
cat apollo-portal/src/main/java/com/ctrip/framework/apollo/openapi/service/ConsumerService.java
# Check ConsumerAuthUtil test cases
echo "Checking ConsumerAuthUtil test cases..."
cat apollo-portal/src/test/java/com/ctrip/framework/apollo/openapi/util/ConsumerAuthUtilTest.java
Length of output: 24687
scripts/sql/profiles/mysql-database-not-specified/delta/v230-v240/apolloportaldb-v230-v240.sql (1)
29-30: Verify rate limit configuration.
The default rate limit of 20 seems arbitrary. Let's verify if this aligns with the system's requirements and configuration.
scripts/sql/profiles/h2-default/delta/v230-v240/apolloportaldb-v230-v240.sql (2)
16-16: Verify migration script coverage for other database platforms.
The script is correctly generated for H2 database. However, we should ensure equivalent migration scripts exist for other supported database platforms.
Let's check for corresponding migration scripts:
#!/bin/bash
# Description: Check for corresponding v230-v240 migration scripts for other DB platforms
fd -g "*v230-v240*.sql" scripts/sql/profilesAlso applies to: 21-23
28-30: LGTM! Verify H2Function class availability.
The UNIX_TIMESTAMP function alias is properly defined with idempotency using IF NOT EXISTS.
Let's verify the referenced Java class exists:
✅ Verification successful
H2Function class and unixTimestamp method verified successfully
The H2Function class exists at apollo-common/src/main/java/com/ctrip/framework/apollo/common/jpa/H2Function.java and contains the required unixTimestamp method implementation. The function is properly used across multiple H2 database initialization and migration scripts, confirming its integration with the application code.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Verify H2Function class exists and contains unixTimestamp method
ast-grep --pattern 'class H2Function {
$$$
static $_ unixTimestamp($_) {
$$$
}
$$$
}'
Length of output: 709
Script:
#!/bin/bash
# Search for H2Function class definition using ripgrep
rg -l "class H2Function"
# Search for unixTimestamp method or function using ripgrep
rg "unixTimestamp" -A 5
Length of output: 7147
apollo-portal/src/main/java/com/ctrip/framework/apollo/openapi/entity/ConsumerToken.java (1)
85-85: LGTM: toString() method properly updated.
The toString() implementation correctly includes the new field while maintaining the existing format.
apollo-portal/src/main/java/com/ctrip/framework/apollo/portal/component/config/PortalConfig.java (1)
249-251: Verify the default disabled state aligns with deployment strategy.
The rate limiting feature is disabled by default, which is good for backward compatibility. However, please ensure this aligns with the deployment strategy:
- Will there be documentation guiding administrators to enable this feature?
- Should there be logging when rate limiting is disabled in production?
Let's check if there's any deployment documentation:
apollo-portal/src/main/java/com/ctrip/framework/apollo/openapi/service/ConsumerService.java (2)
148-151: LGTM! Clean refactoring of token retrieval.
The method maintains backward compatibility while leveraging the new token retrieval logic.
141-146: LGTM! Verify token expiration handling.
The method is well-structured with proper null checks and expiration handling.
scripts/sql/src/apolloportaldb.sql (1)
158-158: LGTM! The LimitCount column is well-defined.
The column definition is appropriate for rate limiting:
- Integer type is suitable for count-based limits
- NOT NULL with DEFAULT 20 ensures backward compatibility
- Clear comment documents the purpose
scripts/sql/profiles/h2-default/apolloportaldb.sql (1)
164-164: Verify migration strategy and implementation details.
While the schema change looks good, please ensure:
- Migration strategy:
- Existing tokens will get the default limit of 20
- Consider if existing high-volume clients need different limits
- Implementation considerations:
- Rate limit enforcement should be clearly documented in API documentation
- Clients should be notified about the new rate limiting feature
- Consider adding rate limit headers in API responses (X-RateLimit-*)
Let's check if there are any high-volume consumers that might be affected:
✅ Verification successful
Based on the search results, I can now provide a final response since I have enough context about the rate limiting implementation:
Migration strategy is properly handled by the default value
The rate limiting implementation is well-designed with safe defaults:
-
The schema change adds
LimitCountwith a default value of 20, which means:- Existing tokens will automatically get the default 20 requests/sec limit
- No manual migration needed as MySQL handles the default value
-
The implementation in
ConsumerAuthenticationFiltershows proper rate limit enforcement:- Uses Google Guava's RateLimiter for reliable rate limiting
- Returns 403 Forbidden when limit is exceeded
- Has a 1-second warmup period for gradual ramp-up
The default of 20 req/sec is a reasonable starting point that can be adjusted per token if needed.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Check ConsumerAudit table for high-volume API users
ast-grep --pattern 'CREATE TABLE `ConsumerAudit`'
# Note: In production, you would analyze ConsumerAudit records
# to identify high-volume consumers, but we can't do that in
# the sandbox environment as it only contains schema definitions
Length of output: 51
Script:
#!/bin/bash
# Let's check for related schema definitions and rate limiting implementation
# 1. Look for ConsumerAudit table definition with different quotes
rg "CREATE TABLE.*ConsumerAudit" -A 10
# 2. Search for rate limiting related code and configurations
rg -i "rate.*limit" -A 3
# 3. Look for API documentation mentioning rate limits
fd -e md -e txt | xargs rg -i "rate.*limit"
# 4. Check for any existing token/consumer related tables
rg "CREATE TABLE.*Token|CREATE TABLE.*Consumer" -A 10
Length of output: 73927
scripts/sql/profiles/mysql-database-not-specified/apolloportaldb.sql (1)
165-165: LGTM! Well-structured schema change for rate limiting.
The addition of the LimitCount column to the ConsumerToken table is properly defined with appropriate constraints and a reasonable default value of 20. This aligns well with the rate limiting requirements from issue #5244.
scripts/sql/profiles/mysql-default/apolloportaldb.sql (1)
170-170: Column addition looks good but verify the default limit.
The new LimitCount column is well-structured with appropriate constraints. However, please verify if the default value of 20 requests aligns with your rate limiting requirements.
✅ Verification successful
Default rate limit of 20 is well justified and consistent across codebase
The default value of 20 for LimitCount is appropriate and consistently implemented:
- Used consistently across all database profiles (mysql, h2) and migration scripts
- Matches the rate limiting implementation in
ConsumerAuthenticationFilterwhich uses Guava'sRateLimiter - Test cases validate the rate limiting functionality with configurable QPS values
- The limit is configurable per consumer token, with 20 serving as a reasonable default
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Search for any configuration or documentation that might justify the default value
rg -i "rate.*limit.*default.*20|default.*rate.*limit.*20"
# Search for any existing rate limit implementations
ast-grep --pattern 'limit.*=.*20'
Length of output: 95
Script:
#!/bin/bash
# Search for rate limit related configurations or constants
rg -i "rate.*limit|ratelimit" -A 3 -B 3
# Look for any numeric constants around 20 in configuration files
fd -e yaml -e properties -e conf -e xml | xargs rg ".*20.*" -A 2 -B 2
# Search for rate limit related documentation
fd README.md CONTRIBUTING.md CHANGELOG.md | xargs rg -i "rate.*limit"
# Check for related issue discussions in SQL comments
rg -i "limit.*comment" scripts/sql/
Length of output: 210963
apollo-portal/src/main/java/com/ctrip/framework/apollo/openapi/util/ConsumerAuthUtil.java
Show resolved
Hide resolved
apollo-portal/src/main/java/com/ctrip/framework/apollo/openapi/entity/ConsumerToken.java
Outdated
Show resolved
Hide resolved
apollo-portal/src/main/java/com/ctrip/framework/apollo/openapi/entity/ConsumerToken.java
Outdated
Show resolved
Hide resolved
| @@ -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 '限流值', | |||
There was a problem hiding this comment.
🛠️ Refactor suggestion
Consider documenting rate limit implementation details.
The new LimitCount column is added with a default value of 20, but there are some considerations:
- The comment '限流值' (Rate limit value) should include more details about:
- The time window for the rate limit (e.g., requests per second/minute)
- The rationale behind the default value of 20
- Consider adding a maximum value constraint to prevent unreasonable limits
Apply this diff to enhance the column definition:
- `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.',📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| `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.', |
...al/src/main/java/com/ctrip/framework/apollo/openapi/filter/ConsumerAuthenticationFilter.java
Outdated
Show resolved
Hide resolved
...al/src/main/java/com/ctrip/framework/apollo/openapi/filter/ConsumerAuthenticationFilter.java
Outdated
Show resolved
Hide resolved
...al/src/main/java/com/ctrip/framework/apollo/openapi/filter/ConsumerAuthenticationFilter.java
Outdated
Show resolved
Hide resolved
| @Test | ||
| public void testRateLimitSuccessfully() 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, 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); | ||
|
|
||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Improve test determinism by refining timing control in rate limit tests
The testRateLimitSuccessfully method's reliance on TimeUnit.MILLISECONDS.sleep to control QPS can lead to non-deterministic test results due to thread scheduling and execution overhead. To ensure consistent and reliable test outcomes, consider mocking the rate limiter or using a ScheduledExecutorService for precise timing of task execution.
| @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); | ||
|
|
||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Enhance reliability of rate limit failure test
Similar to testRateLimitSuccessfully, the testRateLimitPartFailure method may produce inconsistent results due to timing inaccuracies. To improve test reliability, consider mocking the rate limiter to simulate rate limit breaches or refactoring the test to reduce dependency on precise timing control.
There was a problem hiding this comment.
Actionable comments posted: 3
🧹 Outside diff range and nitpick comments (7)
CHANGES.md (1)
22-22: Enhance the changelog entry for better documentation.The current entry could be more descriptive to better reflect the significance of this feature. Consider expanding it to include:
- Reference to the addressed issue (Apollo OpenAPI在apollo-portal中缺失限流功能，是否可以补充限流功能 #5244)
- Key implementation details (LimitCount field, default value)
- Impact on existing functionality
Suggested revision:
-* [Feature: Added current limiting function to ConsumerToken](https://github.com/apolloconfig/apollo/pull/5267) +* [Feature: Added rate limiting functionality to ConsumerToken with configurable LimitCount (default: 20), addressing potential API abuse (#5244)](https://github.com/apolloconfig/apollo/pull/5267)apollo-portal/src/test/java/com/ctrip/framework/apollo/SkipAuthorizationConfiguration.java (1)
55-58: Consider enhancing test configuration flexibility.The mock configuration correctly implements the rate limiting behavior. However, consider these improvements:
- Extract the magic number
20as a constant to match the default configuration inPortalConfig- Consider adding methods to customize the mock response for different test scenarios (e.g., different limit counts)
Example refactor:
public class SkipAuthorizationConfiguration { + private static final int DEFAULT_LIMIT_COUNT = 20; // Match PortalConfig default + @Primary @Bean public ConsumerAuthUtil consumerAuthUtil() { final ConsumerAuthUtil mock = mock(ConsumerAuthUtil.class); when(mock.getConsumerId(any())).thenReturn(1L); ConsumerToken someConsumerToken = new ConsumerToken(); someConsumerToken.setConsumerId(1L); - someConsumerToken.setLimitCount(20); + someConsumerToken.setLimitCount(DEFAULT_LIMIT_COUNT); when(mock.getConsumerToken(any())).thenReturn(someConsumerToken); return mock; }apollo-portal/src/main/java/com/ctrip/framework/apollo/openapi/filter/ConsumerAuthenticationFilter.java (1)
52-53: Consider making warmup period configurableThe hardcoded warmup period of 1 second and cache size of 50000 might not be suitable for all deployments. Consider making these configurable through
PortalConfigto allow customization based on deployment needs.- private static final int WARMUP_MILLIS = 1000; // ms - private static final int RATE_LIMITER_CACHE_MAX_SIZE = 50000; + @Value("${apollo.consumer.token.warmup-millis:1000}") + private int warmupMillis; + @Value("${apollo.consumer.token.cache-size:50000}") + private int rateLimiterCacheMaxSize;apollo-portal/src/test/java/com/ctrip/framework/apollo/openapi/filter/ConsumerAuthenticationFilterTest.java (1)
110-137: Consider potential resource management improvements in rate limit tests.While the test logic is comprehensive, there are potential resource management concerns:
- The ExecutorService is not properly terminated if the test fails
- There's no timeout mechanism for the overall test execution
Consider wrapping the test execution in a try-finally block and adding test timeouts:
@Test + @Timeout(value = 30, unit = TimeUnit.SECONDS) public void testRateLimitSuccessfully() throws Exception { + ExecutorService executor = null; try { // ... existing test code ... } finally { + if (executor != null) { + executor.shutdownNow(); + executor.awaitTermination(5, TimeUnit.SECONDS); + } } }Also applies to: 140-173
docs/zh/deployment/distributed-deployment-guide.md (1)
1409-1413: Consider adding an example to illustrate the rate limiting behavior.While the documentation is clear, it would be helpful to add a simple example showing how the rate limiting works when enabled. For instance:
When enabled (open.api.limit.enabled=true), if a ConsumerToken has limitCount=20, it will be limited to 20 requests per second when accessing Apollo OpenAPI interfaces.docs/en/deployment/distributed-deployment-guide.md (2)
1458-1463: Documentation for open.api.limit.count looks good!The documentation clearly explains the new configuration for setting the default rate limit value for ConsumerToken. The description is accurate and aligns with the PR objectives.
Consider adding an example configuration value to make it more concrete:
The default value is 20. When creating a third-party application in the Open Platform Authorization Management, the default current limit value of the created ConsumerToken is this value. +For example: Setting open.api.limit.count=100 would allow each token to make 100 requests per second by default.
1464-1469: Documentation for open.api.limit.enabled looks good!The documentation clearly explains the new configuration for enabling/disabling the rate limiting functionality. The description accurately describes how it works with the limitCount field.
Consider adding a note about runtime behavior:
The default value is `false`. When set to `true`, it means that each ConsumerToken request to the Apollo OpenAPI interface will be limited to a specific QPS, and the current limit value is the value of the `limitCount` field in the `ConsumerToken` table. +Note: Changes to this setting take effect immediately without requiring a restart.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (6)
CHANGES.md(1 hunks)apollo-portal/src/main/java/com/ctrip/framework/apollo/openapi/filter/ConsumerAuthenticationFilter.java(4 hunks)apollo-portal/src/test/java/com/ctrip/framework/apollo/SkipAuthorizationConfiguration.java(2 hunks)apollo-portal/src/test/java/com/ctrip/framework/apollo/openapi/filter/ConsumerAuthenticationFilterTest.java(6 hunks)docs/en/deployment/distributed-deployment-guide.md(1 hunks)docs/zh/deployment/distributed-deployment-guide.md(1 hunks)
🔇 Additional comments (5)
apollo-portal/src/test/java/com/ctrip/framework/apollo/SkipAuthorizationConfiguration.java (1)
20-20: LGTM!
The ConsumerToken import is appropriately added to support the rate limiting implementation.
apollo-portal/src/main/java/com/ctrip/framework/apollo/openapi/filter/ConsumerAuthenticationFilter.java (2)
59-62: LGTM!
The constructor properly initializes the new PortalConfig dependency.
88-101: Verify rate limiting behavior with integration tests
While the implementation looks good, please ensure comprehensive integration tests are added to verify:
- Rate limiting behavior under different configurations
- Warmup period effectiveness
- Behavior under concurrent load
- Error scenarios
apollo-portal/src/test/java/com/ctrip/framework/apollo/openapi/filter/ConsumerAuthenticationFilterTest.java (1)
19-28: LGTM: Dependencies and constructor changes are well structured.
The new dependencies and constructor changes properly support the rate limiting feature. The PortalConfig mock is correctly initialized and injected.
Also applies to: 61-62, 73-73
docs/zh/deployment/distributed-deployment-guide.md (1)
1403-1408: LGTM! Clear documentation for the new rate limit default value configuration.
The documentation clearly explains the purpose, version requirement, and default value for the open.api.limit.count configuration.
| long warmupToMillis = rateLimiterPair.getLeft() + WARMUP_MILLIS; | ||
| if (System.currentTimeMillis() > warmupToMillis && !rateLimiterPair.getRight().tryAcquire()) { | ||
| response.sendError(HttpServletResponse.SC_FORBIDDEN, "Too many call requests, the flow is limited"); | ||
| return; | ||
| } |
There was a problem hiding this comment.
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;
}Committable suggestion skipped: line range outside the PR's diff.
...al/src/main/java/com/ctrip/framework/apollo/openapi/filter/ConsumerAuthenticationFilter.java
Outdated
Show resolved
Hide resolved
apollo-portal/src/main/java/com/ctrip/framework/apollo/openapi/entity/ConsumerToken.java
Outdated
Show resolved
Hide resolved
apollo-portal/src/main/java/com/ctrip/framework/apollo/openapi/entity/ConsumerToken.java
Outdated
Show resolved
Hide resolved
...al/src/main/java/com/ctrip/framework/apollo/openapi/filter/ConsumerAuthenticationFilter.java
Outdated
Show resolved
Hide resolved
...al/src/main/java/com/ctrip/framework/apollo/openapi/filter/ConsumerAuthenticationFilter.java
Outdated
Show resolved
Hide resolved
...al/src/main/java/com/ctrip/framework/apollo/openapi/filter/ConsumerAuthenticationFilter.java
Show resolved
Hide resolved
| ImmutablePair<Long, RateLimiter> rateLimiterPair = getOrCreateRateLimiterPair(token, limitCount); | ||
| long warmupToMillis = rateLimiterPair.getLeft() + WARMUP_MILLIS; | ||
| if (System.currentTimeMillis() > warmupToMillis && !rateLimiterPair.getRight().tryAcquire()) { | ||
| response.sendError(HttpServletResponse.SC_FORBIDDEN, "Too many call requests, the flow is limited"); |
There was a problem hiding this comment.
Is 429 more suitable for this scenario?
| @@ -322,6 +327,7 @@ private ConsumerToken generateConsumerToken(Consumer consumer, Date expires) { | |||
|
|
|||
| ConsumerToken consumerToken = new ConsumerToken(); | |||
| consumerToken.setConsumerId(consumerId); | |||
| consumerToken.setLimitCount(portalConfig.openApiLimitCount()); | |||
There was a problem hiding this comment.
The rate limit should be set on the UI page, with a default value that users can modify.
apollo-portal/src/main/java/com/ctrip/framework/apollo/openapi/service/ConsumerService.java
Outdated
Show resolved
Hide resolved
| return getIntProperty("open.api.limit.count", 20); | ||
| } | ||
|
|
||
| public boolean isOpenApiLimitEnabled() { |
There was a problem hiding this comment.
Since rate limits are set at the token level, adding a global flag seems unnecessary. Instead, we could add an "Enable Rate Limit" option on the consumer token creation page. Users can toggle this option to configure the rate limit if desired; otherwise, leaving it unchecked will set the rate limit to 0.
# Conflicts: # CHANGES.md
dosubot
bot
added
size:XL
There was a problem hiding this comment.
Actionable comments posted: 13
🧹 Outside diff range and nitpick comments (18)
scripts/sql/src/delta/v230-v240/apolloportaldb-v230-v240.sql (1)
22-23: Consider adding IF NOT EXISTS clause for safer deployment.While the script is part of a versioned migration, adding an IF NOT EXISTS clause would make it more robust during deployments and prevent errors if the column already exists.
-ALTER TABLE `ConsumerToken` - ADD COLUMN `RateLimit` int NOT NULL DEFAULT '0' COMMENT '限流值' AFTER `Token`; +ALTER TABLE `ConsumerToken` + ADD COLUMN IF NOT EXISTS `RateLimit` int NOT NULL DEFAULT '0' COMMENT 'Rate limit value (限流值)' AFTER `Token`;Additionally, consider adding an English translation to the comment for better international developer experience.
scripts/sql/profiles/mysql-database-not-specified/delta/v230-v240/apolloportaldb-v230-v240.sql (1)
29-30: Consider using BIGINT for future scalabilityThe
intdata type might be limiting for scenarios requiring very high rate limits. Consider usingBIGINTto accommodate potential future needs, especially for high-throughput APIs.- ADD COLUMN `RateLimit` int NOT NULL DEFAULT '0' COMMENT '限流值' AFTER `Token`; + ADD COLUMN `RateLimit` bigint NOT NULL DEFAULT '0' COMMENT '限流值' AFTER `Token`;scripts/sql/profiles/mysql-default/delta/v230-v240/apolloportaldb-v230-v240.sql (2)
31-32: Specify integer column size and add migration documentationConsider the following improvements:
- Specify the integer size (e.g., INT(11)) for clarity
- Add a comment explaining the column placement after Token
- Document the migration in a separate upgrade guide
ALTER TABLE `ConsumerToken` - ADD COLUMN `RateLimit` int NOT NULL DEFAULT '0' COMMENT '限流值' AFTER `Token`; + ADD COLUMN `RateLimit` int(11) NOT NULL DEFAULT '5000' COMMENT 'Rate limit (requests per hour)' AFTER `Token`;
31-32: Ensure migration safety and reversibilityPlease consider adding:
- A rollback script in case the migration needs to be reversed
- Instructions for handling the migration in a high-availability setup
- Performance impact assessment for large tables
Example rollback script:
ALTER TABLE `ConsumerToken` DROP COLUMN `RateLimit`;apollo-portal/src/main/java/com/ctrip/framework/apollo/portal/entity/vo/consumer/ConsumerCreateRequestVO.java (1)
88-94: Add documentation for rate limit propertiesThe rate limit methods should be documented to clarify their purpose and valid ranges.
Add JavaDoc comments:
+ /** + * Get the rate limit value for the consumer token. + * @return the number of requests allowed per time window + */ public int getRateLimit() { return rateLimit; } + /** + * Set the rate limit for the consumer token. + * @param rateLimit the number of requests allowed per time window (must be greater than 0) + */ public void setRateLimit(int rateLimit) { this.rateLimit = rateLimit; }apollo-portal/src/main/java/com/ctrip/framework/apollo/openapi/entity/ConsumerToken.java (1)
45-47: Consider initializing the rateLimit field with a default value.While the field is marked as
nullable=falsein the database, theIntegertype can still be null in Java. Consider either:
- Initializing with a default value, or
- Adding null checks in the setter method
@PositiveOrZero @Column(name = "`RateLimit`", nullable = false) - private Integer rateLimit; + private Integer rateLimit = 0;apollo-portal/src/test/java/com/ctrip/framework/apollo/portal/controller/ConsumerControllerTest.java (1)
Line range hint
26-41: Add test cases for rate limit validationThe createWithBadRequest test method should include cases for invalid rate limit values. Consider adding test cases for:
- Negative rate limits
- Zero rate limit
- Maximum allowed rate limit
@Test void createWithBadRequest() { ConsumerService consumerService = Mockito.mock(ConsumerService.class); ConsumerController consumerController = new ConsumerController(consumerService); ConsumerCreateRequestVO requestVO = new ConsumerCreateRequestVO(); + // invalid rate limit + requestVO.setRateLimit(-1); + assertThrows(BadRequestException.class, () -> consumerController.create(requestVO, null)); + requestVO.setRateLimit(0); + assertThrows(BadRequestException.class, () -> consumerController.create(requestVO, null)); + requestVO.setRateLimit(Integer.MAX_VALUE); + assertThrows(BadRequestException.class, () -> consumerController.create(requestVO, null)); + requestVO.setRateLimit(100); // valid rate limit + // blank appId assertThrows(BadRequestException.class, () -> consumerController.create(requestVO, null)); requestVO.setAppId("appId1");apollo-portal/src/main/java/com/ctrip/framework/apollo/openapi/filter/ConsumerAuthenticationFilter.java (2)
53-60: LGTM! Consider adding documentation for the constants.The cache configuration and constants look good. The cache has appropriate expiry and size limits. Consider adding Javadoc comments explaining the purpose of
WARMUP_MILLISandRATE_LIMITER_CACHE_MAX_SIZEconstants.+ /** + * Initial warmup period for the rate limiter to stabilize its rate. + */ private static final int WARMUP_MILLIS = 1000; // ms + /** + * Maximum number of rate limiters to cache to prevent memory issues. + */ private static final int RATE_LIMITER_CACHE_MAX_SIZE = 50000;
87-101: Consider extracting rate limiting logic to a separate method.The rate limiting implementation is correct, but the code readability could be improved by extracting the logic into a dedicated method.
- Integer rateLimit = consumerToken.getRateLimit(); - if (null != rateLimit && rateLimit > 0) { - try { - ImmutablePair<Long, RateLimiter> rateLimiterPair = getOrCreateRateLimiterPair(token, rateLimit); - long warmupToMillis = rateLimiterPair.getLeft() + WARMUP_MILLIS; - if (System.currentTimeMillis() > warmupToMillis && !rateLimiterPair.getRight().tryAcquire()) { - response.sendError(TOO_MANY_REQUESTS, "Too Many Requests, the flow is limited"); - return; - } - } catch (Exception e) { - logger.error("ConsumerAuthenticationFilter ratelimit error", e); - response.sendError(HttpServletResponse.SC_INTERNAL_SERVER_ERROR, "Rate limiting failed"); - return; - } - } + if (!checkRateLimit(token, consumerToken.getRateLimit(), response)) { + return; + } + + private boolean checkRateLimit(String token, Integer rateLimit, HttpServletResponse response) throws IOException { + if (null == rateLimit || rateLimit <= 0) { + return true; + } + try { + ImmutablePair<Long, RateLimiter> rateLimiterPair = getOrCreateRateLimiterPair(token, rateLimit); + long warmupToMillis = rateLimiterPair.getLeft() + WARMUP_MILLIS; + if (System.currentTimeMillis() > warmupToMillis && !rateLimiterPair.getRight().tryAcquire()) { + response.sendError(TOO_MANY_REQUESTS, "Too Many Requests, the flow is limited"); + return false; + } + return true; + } catch (Exception e) { + logger.error("ConsumerAuthenticationFilter ratelimit error", e); + response.sendError(HttpServletResponse.SC_INTERNAL_SERVER_ERROR, "Rate limiting failed"); + return false; + } + }apollo-portal/src/main/java/com/ctrip/framework/apollo/portal/controller/ConsumerController.java (2)
84-90: Consider adding an upper bound for rate limitWhile the current validation ensures the rate limit is positive when enabled, consider adding an upper bound to prevent potential abuse or system overload.
if (requestVO.isRateLimitEnabled()) { - if (requestVO.getRateLimit() <= 0) { + int rateLimit = requestVO.getRateLimit(); + if (rateLimit <= 0) { throw BadRequestException.rateLimitIsInvalid(); } + if (rateLimit > MAX_RATE_LIMIT) { + throw new BadRequestException("Rate limit cannot exceed " + MAX_RATE_LIMIT); + } } else { requestVO.setRateLimit(0); }
84-98: Consider implementing rate limit monitoringTo ensure the effectiveness of the rate limiting feature, consider adding monitoring capabilities:
- Log rate limit violations for security auditing
- Add metrics to track rate limit usage patterns
- Implement alerts for frequent rate limit hits
This will help track the feature's usage and identify potential abuse patterns.
apollo-portal/src/test/java/com/ctrip/framework/apollo/openapi/filter/ConsumerAuthenticationFilterTest.java (1)
179-186: Add defensive programming checks in setupRateLimitMocksConsider adding null checks and validation for the input parameters to make the test helper more robust:
private void setupRateLimitMocks(String someToken, Long someConsumerId, int qps) { + if (someToken == null || someConsumerId == null || qps <= 0) { + throw new IllegalArgumentException("Invalid test parameters"); + } ConsumerToken someConsumerToken = new ConsumerToken(); someConsumerToken.setConsumerId(someConsumerId); someConsumerToken.setRateLimit(qps);apollo-portal/src/main/resources/static/scripts/controller/open/OpenManageController.js (1)
Line range hint
44-181: Consider rate limiting implementation details.The frontend implementation should align with the backend rate limiting strategy. Consider the following architectural points:
- Ensure the rate limit UI clearly communicates the time window for the limit (e.g., requests per second/minute/hour)
- Consider adding a tooltip or help text explaining the rate limiting behavior
- The backend should return appropriate HTTP 429 (Too Many Requests) responses when limits are exceeded
- Consider implementing rate limit headers (X-RateLimit-*) for better client integration
Would you like me to provide specific implementation details for any of these suggestions?
apollo-portal/src/test/resources/sql/openapi/NamespaceControllerTest.testCreateAppNamespace.sql (1)
74-75: Document the new RateLimit field in the test setup comments.The test data setup includes a rate limit value of 20, but this new field isn't explained in the file's documentation section. Consider updating the comments to explain the purpose and significance of the rate limit value in this test scenario.
apollo-portal/src/test/resources/sql/openapi/ConsumerServiceIntegrationTest.testFindAppIdsAuthorizedByConsumerId.sql (1)
81-82: Consider enhancing test coverage for rate limiting scenarios.The hard-coded rate limit value of 20 requires documentation and additional test cases to validate the rate limiting functionality effectively.
Consider the following improvements:
- Add a comment explaining the significance of the rate limit value:
INSERT INTO "ConsumerToken" (`Id`, `ConsumerId`, `Token`, `RateLimit`, `Expires`, `DataChange_CreatedBy`, `DataChange_LastModifiedBy`) VALUES +-- Default rate limit of 20 requests per <time_unit> for integration testing (1000, 1000, '3c16bf5b1f44b465179253442460e8c0ad845289', 20, '2098-12-31 10:00:00', 'apollo', 'apollo');
- Add test cases for edge cases:
-- Test token with zero rate limit INSERT INTO "ConsumerToken" (`Id`, `ConsumerId`, `Token`, `RateLimit`, `Expires`, `DataChange_CreatedBy`, `DataChange_LastModifiedBy`) VALUES (1001, 1000, '3c16bf5b1f44b465179253442460e8c0ad845290', 0, '2098-12-31 10:00:00', 'apollo', 'apollo'); -- Test token with high rate limit INSERT INTO "ConsumerToken" (`Id`, `ConsumerId`, `Token`, `RateLimit`, `Expires`, `DataChange_CreatedBy`, `DataChange_LastModifiedBy`) VALUES (1002, 1000, '3c16bf5b1f44b465179253442460e8c0ad845291', 1000, '2098-12-31 10:00:00', 'apollo', 'apollo');apollo-portal/src/test/resources/sql/openapi/ConsumerServiceIntegrationTest.commonData.sql (1)
88-89: Consider using more realistic test dataThe current test data uses:
- A hard-coded token value which could be mistaken for a real token
- A very far future expiry date (year 2098) which might not catch edge cases
Consider using:
- Clearly marked test tokens (e.g., prefixed with "test_")
- More realistic expiry dates to better simulate production scenarios
INSERT INTO "ConsumerToken" (`Id`, `ConsumerId`, `Token`, `RateLimit`, `Expires`, `DataChange_CreatedBy`, `DataChange_LastModifiedBy`) VALUES -(1000, 1000, '3c16bf5b1f44b465179253442460e8c0ad845289', 20, '2098-12-31 10:00:00', 'apollo', 'apollo'); +(1000, 1000, 'test_token_consumer_1000', 20, DATE_ADD(CURRENT_TIMESTAMP, INTERVAL 30 DAY), 'apollo', 'apollo');apollo-portal/src/main/resources/static/i18n/zh-CN.json (1)
657-661: LGTM! Consider enhancing the QPS tooltip.The translations for the rate limiting feature are clear and well-structured. However, the QPS tooltip could be more informative.
Consider updating the tooltip to include the acceptable range and behavior when limit is reached:
- "Open.Manage.Consumer.RateLimitValueTips": "（单位:次/秒，例如: 100 表示每秒最多发布 100 次配置）", + "Open.Manage.Consumer.RateLimitValueTips": "（单位:次/秒，例如: 100 表示每秒最多发布 100 次配置。超出限制时请求将被拒绝，最小值: 1）",apollo-portal/src/main/java/com/ctrip/framework/apollo/openapi/service/ConsumerService.java (1)
141-146: Ensure proper handling of expired tokens ingetConsumerTokenByTokenIn the method
getConsumerTokenByToken, tokens are fetched withexpiresAfterthe current date. Ensure that tokens nearing expiration are considered, and handle edge cases where token expiration might cause unexpected behavior. Additionally, consider logging attempts to use expired tokens for monitoring and security purposes.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (27)
CHANGES.md(1 hunks)apollo-common/src/main/java/com/ctrip/framework/apollo/common/exception/BadRequestException.java(1 hunks)apollo-portal/src/main/java/com/ctrip/framework/apollo/openapi/entity/ConsumerToken.java(4 hunks)apollo-portal/src/main/java/com/ctrip/framework/apollo/openapi/filter/ConsumerAuthenticationFilter.java(4 hunks)apollo-portal/src/main/java/com/ctrip/framework/apollo/openapi/service/ConsumerService.java(3 hunks)apollo-portal/src/main/java/com/ctrip/framework/apollo/portal/controller/ConsumerController.java(3 hunks)apollo-portal/src/main/java/com/ctrip/framework/apollo/portal/entity/vo/consumer/ConsumerCreateRequestVO.java(2 hunks)apollo-portal/src/main/resources/static/i18n/en.json(1 hunks)apollo-portal/src/main/resources/static/i18n/zh-CN.json(1 hunks)apollo-portal/src/main/resources/static/open/add-consumer.html(2 hunks)apollo-portal/src/main/resources/static/scripts/controller/open/OpenManageController.js(2 hunks)apollo-portal/src/test/java/com/ctrip/framework/apollo/SkipAuthorizationConfiguration.java(2 hunks)apollo-portal/src/test/java/com/ctrip/framework/apollo/openapi/filter/ConsumerAuthenticationFilterTest.java(6 hunks)apollo-portal/src/test/java/com/ctrip/framework/apollo/openapi/v1/controller/NamespaceControllerTest.java(3 hunks)apollo-portal/src/test/java/com/ctrip/framework/apollo/portal/controller/ConsumerControllerTest.java(2 hunks)apollo-portal/src/test/resources/sql/openapi/ConsumerServiceIntegrationTest.commonData.sql(1 hunks)apollo-portal/src/test/resources/sql/openapi/ConsumerServiceIntegrationTest.testFindAppIdsAuthorizedByConsumerId.sql(1 hunks)apollo-portal/src/test/resources/sql/openapi/NamespaceControllerTest.testCreateAppNamespace.sql(1 hunks)docs/zh/deployment/distributed-deployment-guide.md(0 hunks)scripts/sql/profiles/h2-default/apolloportaldb.sql(1 hunks)scripts/sql/profiles/h2-default/delta/v230-v240/apolloportaldb-v230-v240.sql(1 hunks)scripts/sql/profiles/mysql-database-not-specified/apolloportaldb.sql(1 hunks)scripts/sql/profiles/mysql-database-not-specified/delta/v230-v240/apolloportaldb-v230-v240.sql(1 hunks)scripts/sql/profiles/mysql-default/apolloportaldb.sql(1 hunks)scripts/sql/profiles/mysql-default/delta/v230-v240/apolloportaldb-v230-v240.sql(1 hunks)scripts/sql/src/apolloportaldb.sql(1 hunks)scripts/sql/src/delta/v230-v240/apolloportaldb-v230-v240.sql(1 hunks)
💤 Files with no reviewable changes (1)
- docs/zh/deployment/distributed-deployment-guide.md
🚧 Files skipped from review as they are similar to previous changes (7)
- CHANGES.md
- apollo-portal/src/test/java/com/ctrip/framework/apollo/SkipAuthorizationConfiguration.java
- scripts/sql/profiles/h2-default/apolloportaldb.sql
- scripts/sql/profiles/h2-default/delta/v230-v240/apolloportaldb-v230-v240.sql
- scripts/sql/profiles/mysql-database-not-specified/apolloportaldb.sql
- scripts/sql/profiles/mysql-default/apolloportaldb.sql
- scripts/sql/src/apolloportaldb.sql
🧰 Additional context used
📓 Learnings (1)
apollo-portal/src/main/java/com/ctrip/framework/apollo/openapi/entity/ConsumerToken.java (1)
Learnt from: nobodyiam
PR: apolloconfig/apollo#5267
File: apollo-portal/src/main/java/com/ctrip/framework/apollo/openapi/entity/ConsumerToken.java:70-72
Timestamp: 2024-11-12T13:51:09.239Z
Learning: In `apollo-portal`, prefer using `@PositiveOrZero` from `javax.validation.constraints` to validate that integer fields are non-negative.
🔇 Additional comments (26)
scripts/sql/src/delta/v230-v240/apolloportaldb-v230-v240.sql (1)
22-23: Verify database backup/restore procedures.
This schema change adds a new column that affects rate limiting functionality. Please ensure that:
- Database backup procedures are updated if they have column-specific configurations
- The deployment process includes proper documentation for DBAs
- Application code is updated to handle both pre-migration (without column) and post-migration (with column) scenarios
✅ Verification successful
Let me check the content of these files to better understand the backup/restore procedures and documentation.
Let me check the existing database migration process and tools to understand how schema changes are handled.
Let me check if there are any application code files that handle the rate limiting functionality to understand how the new column is used.
Based on the code analysis, I can now provide a final response about the database schema change:
Schema change is properly handled through migration scripts and Flyway
The rate limit column addition is properly managed through:
- Migration scripts exist for both MySQL and H2 databases in
scripts/sql/profiles/*/delta/v230-v240/ - Default value of 0 is set for backward compatibility
- Flyway is used for automated database migrations as seen in deployment guide
The change is safe because:
- The column is added with a NOT NULL constraint and DEFAULT '0' value
- No backup-specific configurations exist that need updating
- The application code already handles rate limiting through ConsumerToken entity and related services
- Migration scripts are provided for all supported database types (MySQL, H2)
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Check if there are any backup configuration files that need updating
# Look for backup configuration files
fd -e yaml -e yml -e conf -e xml -e properties -e json . | xargs rg -l 'ConsumerToken|backup|restore'
# Look for documentation files that might need updating
fd -e md -e txt . | xargs rg -l 'ConsumerToken|backup|restore'
Length of output: 446
Script:
#!/bin/bash
# Check documentation content related to ConsumerToken and backup/restore
echo "=== Checking distributed deployment guide ==="
rg -A 5 -B 5 'ConsumerToken|backup|restore' 'docs/en/deployment/distributed-deployment-guide.md'
echo -e "\n=== Checking design docs ==="
rg -A 5 -B 5 'ConsumerToken|backup|restore' 'docs/en/design/apollo-design.md'
echo -e "\n=== Checking user guide ==="
rg -A 5 -B 5 'ConsumerToken|backup|restore' 'docs/en/portal/apollo-user-guide.md'
# Check if there are any existing database migration scripts or tools
echo -e "\n=== Checking existing migration scripts ==="
fd migration scripts/sql
Length of output: 4285
Script:
#!/bin/bash
# Check database migration related files and tools
echo "=== Checking SQL migration structure ==="
fd . scripts/sql -t f
echo -e "\n=== Checking Flyway related files ==="
rg -l 'flyway|Flyway' .
echo -e "\n=== Checking SQL migration scripts content ==="
rg -A 5 -B 5 'ALTER TABLE|CREATE TABLE|ConsumerToken' 'scripts/sql'
Length of output: 253610
Script:
#!/bin/bash
# Search for rate limit related code
echo "=== Checking rate limit implementation ==="
rg -l "RateLimit|rate.?limit" .
echo -e "\n=== Checking ConsumerToken usage ==="
rg -A 5 -B 5 "ConsumerToken" . --type java
Length of output: 72253
scripts/sql/profiles/mysql-database-not-specified/delta/v230-v240/apolloportaldb-v230-v240.sql (2)
16-17: Ensure backward compatibility during deployment
The migration adds a new column with a NOT NULL constraint and default value, which is safe for existing data. However, verify the deployment strategy:
- Ensure the application code is deployed after this schema change
- Consider adding this change to the changelog
#!/bin/bash
# Check if there's a corresponding changelog entry
rg -l "RateLimit.*ConsumerToken" --type mdAlso applies to: 29-30
29-30: Verify the default rate limit behavior
The default value of 0 for RateLimit needs careful consideration:
- Ensure the application code properly interprets 0 (e.g., as "unlimited" or "blocked")
- Consider whether a more restrictive default would be safer
Consider adding an index on the Token column if it's frequently used in WHERE clauses during rate limit checks:
ALTER TABLE `ConsumerToken` ADD INDEX idx_token (Token);✅ Verification successful
Let's search for rate limit handling patterns in the codebase to understand how the value 0 is interpreted. I've broadened the search patterns and included file discovery to locate relevant implementation files.
Rate limit of 0 is properly handled as disabled rate limiting
Based on the code evidence:
- In
ConsumerAuthenticationFilter.java, rate limiting is only applied whenrateLimit != null && rateLimit > 0 - In
ConsumerController.java, a rate limit of 0 is explicitly set when rate limiting is disabled - The system validates that when rate limiting is enabled, the value must be > 0, otherwise throws
BadRequestException.rateLimitIsInvalid()
Therefore, the default value of 0 is intentional and correctly implemented to indicate disabled rate limiting. No changes are needed.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Check how 0 RateLimit is handled in the codebase
ast-grep --pattern 'if ($token.getRateLimit() == 0) {
$$$
}'
Length of output: 192
Script:
#!/bin/bash
# Search for RateLimit related code patterns
rg -A 5 "RateLimit.*ConsumerToken|ConsumerToken.*RateLimit"
# Search for rate limit handling patterns
rg -A 5 "getRateLimit|setRateLimit"
# Look for Java/Kotlin files containing ConsumerToken
fd -e java -e kt ConsumerToken
Length of output: 15074
scripts/sql/profiles/mysql-default/delta/v230-v240/apolloportaldb-v230-v240.sql (2)
28-29: LGTM: Correct database selected
The script correctly targets the ApolloPortalDB database for the consumer token modifications.
31-32:
Consider safety implications of default rate limit value
The default value of 0 for RateLimit could be interpreted as "unlimited requests allowed" which might pose a security risk for existing tokens. Consider:
- Using a positive default value to ensure basic rate limiting
- Adding explicit documentation about the rate limit unit (requests per second/minute/hour)
Let's check if there are existing tokens that would be affected:
apollo-portal/src/main/java/com/ctrip/framework/apollo/portal/entity/vo/consumer/ConsumerCreateRequestVO.java (1)
Line range hint 1-95: Verify the impact of rate limit changes
The changes look good overall, but we should verify the integration with the rate limiting implementation.
Let's check how these fields are used in the codebase:
Consider adding documentation in README or wiki about:
- The rate limiting feature
- Configuration guidelines for setting appropriate rate limits
- Impact on existing consumers
✅ Verification successful
Rate limiting implementation is properly integrated and functional
The verification shows a complete implementation of rate limiting across all required components:
- ConsumerService handles rate limit configuration during token generation
- ConsumerAuthenticationFilter enforces rate limiting with proper error handling
- Database schema includes RateLimit column in ConsumerToken table
- ConsumerController validates rate limit values during consumer creation
The implementation includes proper validation, error handling, and warm-up periods for rate limiters, making it production-ready.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Verify the integration of rate limit fields with other components
# Check for rate limit validation in controllers/services
echo "Checking for rate limit validation..."
rg -l "getRateLimit|setRateLimit|rateLimitE?nabled" --type java
# Check for database migration scripts
echo "Checking for related database changes..."
fd -e sql . | xargs rg -l "RateLimit"
# Check for test coverage
echo "Checking for test coverage..."
fd -e java -g "*Test.java" | xargs rg -l "rateLimit"
Length of output: 1918
Script:
#!/bin/bash
# Let's examine the implementation details more closely
# Check the ConsumerService implementation for rate limit handling
echo "=== ConsumerService Implementation ==="
rg -A 10 "rateLimitE?nabled|rateLimit" apollo-portal/src/main/java/com/ctrip/framework/apollo/openapi/service/ConsumerService.java
# Check the authentication filter for rate limit enforcement
echo "=== Authentication Filter Implementation ==="
rg -A 10 "rateLimitE?nabled|rateLimit" apollo-portal/src/main/java/com/ctrip/framework/apollo/openapi/filter/ConsumerAuthenticationFilter.java
# Check the SQL schema changes
echo "=== Database Schema Changes ==="
cat scripts/sql/src/delta/v230-v240/apolloportaldb-v230-v240.sql
# Check the controller implementation
echo "=== Consumer Controller Implementation ==="
rg -A 10 "rateLimitE?nabled|rateLimit" apollo-portal/src/main/java/com/ctrip/framework/apollo/portal/controller/ConsumerController.java
Length of output: 4036
apollo-portal/src/main/java/com/ctrip/framework/apollo/openapi/entity/ConsumerToken.java (4)
21-21: LGTM!
The import statement for @PositiveOrZero is correctly added and aligns with the project's validation practices.
68-74: LGTM!
The getter and setter methods are correctly implemented. The @PositiveOrZero annotation will handle validation at the framework level.
87-87: LGTM!
The toString() method is correctly updated to include the new rateLimit field.
45-47: Verify integration with ConsumerAuthenticationFilter.
Let's ensure the rate limit field is properly utilized in the authentication filter.
✅ Verification successful
Rate limiting integration is properly implemented in ConsumerAuthenticationFilter
The verification shows that the rateLimit field is correctly integrated:
- ConsumerAuthenticationFilter uses Guava's RateLimiter to enforce rate limits
- Rate limiting logic is implemented in the filter's doFilter method:
- Retrieves rateLimit from ConsumerToken
- Creates/reuses a RateLimiter instance based on the token and limit
- Enforces the limit with proper error handling (429 Too Many Requests)
- Test coverage exists with test cases for both successful and failed rate limiting scenarios
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Check if ConsumerAuthenticationFilter properly uses the rateLimit field
# Search for references to rateLimit in ConsumerAuthenticationFilter
echo "Checking ConsumerAuthenticationFilter implementation:"
rg -A 5 "rateLimit|RateLimit" --type java
# Look for rate limit related tests
echo -e "\nChecking test coverage:"
rg -l "ConsumerToken.*rateLimit|RateLimit.*test" --type java
Length of output: 22391
apollo-portal/src/test/java/com/ctrip/framework/apollo/openapi/v1/controller/NamespaceControllerTest.java (1)
57-61: Consider adding rate limit specific test cases.
While the HTTP request handling improvements look good, given that this PR implements rate limiting functionality, consider adding test cases that verify:
- Rate limit header presence in responses
- Rate limit enforcement behavior
- Different rate limit scenarios (within limit, at limit, exceeding limit)
Let's check if there are any other rate limit related tests:
Consider adding a test method like:
@Test
public void shouldEnforceRateLimits() {
// Test setup with rate limited token
OpenAppNamespaceDTO dto = createValidNamespaceDTO();
// Test within limit
ResponseEntity<OpenAppNamespaceDTO> response = restTemplate.exchange(
url("/openapi/v1/apps/{appId}/appnamespaces"),
HttpMethod.POST,
new HttpEntity<>(dto, HTTP_HEADERS_WITH_TOKEN),
OpenAppNamespaceDTO.class,
dto.getAppId()
);
// Verify rate limit headers
HttpHeaders headers = response.getHeaders();
Assert.assertNotNull(headers.get("X-RateLimit-Limit"));
Assert.assertNotNull(headers.get("X-RateLimit-Remaining"));
// Test exceeding limit
// ... additional test scenarios
}apollo-portal/src/main/java/com/ctrip/framework/apollo/openapi/filter/ConsumerAuthenticationFilter.java (3)
62-65: LGTM! Constructor changes look good.
The addition of PortalConfig parameter is clean and properly initialized.
80-85: LGTM! Token validation is properly implemented.
The change from direct consumer ID to ConsumerToken object with proper null checking improves the robustness of the authentication process.
115-122: LGTM! Thread-safe implementation of rate limiter creation.
The implementation correctly uses Cache.get with a supplier for thread-safe rate limiter creation, as suggested in the past review comments.
apollo-portal/src/main/java/com/ctrip/framework/apollo/portal/controller/ConsumerController.java (2)
Line range hint 138-167: LGTM! Code style improvements
The addition of braces and consistent formatting improves code readability and maintainability.
98-98: Verify rate limit implementation in ConsumerService
The rate limit is correctly passed to the token generation service. Let's verify the implementation in the service layer.
✅ Verification successful
Rate limit implementation is properly handled in ConsumerService
The verification confirms that:
- The
ConsumerTokenentity has a properrateLimitfield marked as@PositiveOrZeroandnullable = false - The
generateConsumerTokenmethod correctly sets the rate limit value from the input parameter - The implementation chain from controller to service to entity is consistent
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Check if ConsumerService properly handles the rate limit parameter
ast-grep --pattern 'generateAndSaveConsumerToken($consumer, $rateLimit, $expires) {
$$$
}'
# Check if ConsumerToken entity has the rate limit field
ast-grep --pattern 'class ConsumerToken {
$$$
private $type rateLimit;
$$$
}'
Length of output: 180
Script:
#!/bin/bash
# Let's try a different approach to find the ConsumerService implementation
rg -l "class ConsumerService" --type java
# Search for ConsumerToken class definition
rg -l "class ConsumerToken" --type java
# Once we find the files, let's examine their content
rg "generateAndSaveConsumerToken|ConsumerToken" --type java -A 5
Length of output: 45373
Script:
#!/bin/bash
# Let's check the ConsumerToken entity class for rate limit field
rg "class ConsumerToken" -A 20 apollo-portal/src/main/java/com/ctrip/framework/apollo/openapi/entity/ConsumerToken.java
# Check the implementation of generateConsumerToken method
rg "generateConsumerToken.*consumer.*rateLimit.*expires" -A 15 apollo-portal/src/main/java/com/ctrip/framework/apollo/openapi/service/ConsumerService.java
Length of output: 1995
apollo-portal/src/test/java/com/ctrip/framework/apollo/openapi/filter/ConsumerAuthenticationFilterTest.java (4)
Line range hint 19-66: LGTM: New imports and fields are appropriate
The additions properly support the new rate limiting functionality with necessary concurrent utilities and mocks.
76-76: LGTM: Constructor updated correctly
The constructor initialization properly includes the new PortalConfig dependency.
Line range hint 84-102: LGTM: Authentication tests properly updated
The tests have been correctly modified to use ConsumerToken instead of direct consumerId, maintaining proper test coverage for the authentication flow.
113-176: Consider potential race conditions in concurrent test execution
While the rate limit tests are well-structured, they might be susceptible to race conditions when running in a CI environment or on different hardware. Consider:
- Adding test retry capabilities for flaky scenarios
- Using a more deterministic approach for time-based assertions
- Adding appropriate test timeouts
apollo-portal/src/main/resources/static/scripts/controller/open/OpenManageController.js (1)
171-181: 🛠️ Refactor suggestion
Enhance rate limit validation and consistency.
Several improvements are needed in the rate limit validation:
- The same typo exists:
rateLimitEenabledshould berateLimitEnabled - Missing upper bound validation for
rateLimit - Validation logic duplicates the toggle function's behavior
Let's verify the error message translation key:
Consider extracting the validation into a separate function and applying these improvements:
+ function validateRateLimit() {
+ if ($scope.consumer.rateLimitEenabled) {
+ if ($scope.consumer.rateLimit < 1) {
+ toastr.warning($translate.instant('Open.Manage.Consumer.RateLimitValue.Min.Error'));
+ return false;
+ }
+ if ($scope.consumer.rateLimit > 10000) { // Add appropriate max value
+ toastr.warning($translate.instant('Open.Manage.Consumer.RateLimitValue.Max.Error'));
+ return false;
+ }
+ return true;
+ }
+ $scope.consumer.rateLimit = 0;
+ return true;
+ }
function createConsumer() {
$scope.submitBtnDisabled = true;
if (!$scope.consumer.appId) {
toastr.warning($translate.instant('Open.Manage.PleaseEnterAppId'));
$scope.submitBtnDisabled = false;
return;
}
- if ($scope.consumer.rateLimitEenabled) {
- if ($scope.consumer.rateLimit < 1) {
- toastr.warning($translate.instant('Open.Manage.Consumer.RateLimitValue.Error'));
- $scope.submitBtnDisabled = false;
- return;
- }
- } else {
- $scope.consumer.rateLimit = 0;
+ if (!validateRateLimit()) {
+ $scope.submitBtnDisabled = false;
+ return;
}apollo-portal/src/main/resources/static/open/add-consumer.html (1)
95-106: 🛠️ Refactor suggestion
Enhance rate limit input validation and accessibility
The rate limit value input needs additional validation and accessibility improvements:
- Missing min/max constraints
- Missing required validation when visible
- Missing error handling
- Missing accessibility attributes
Apply these improvements:
<div class="form-group" ng-show="consumer.rateLimitEenabled">
<label class="col-sm-2 control-label">
{{ 'Open.Manage.Consumer.RateLimitValue' | translate }}
</label>
<div class="col-sm-3">
<input type="number"
ng-model="consumer.rateLimit"
name="rateLimit"
+ min="1"
+ max="1000"
+ required
+ ng-required="consumer.rateLimitEnabled"
+ aria-label="{{ 'Open.Manage.Consumer.RateLimitValue' | translate }}"
+ aria-describedby="rateLimitValueTips"
+ ng-pattern="/^[0-9]+$/"
/>
- <small>{{'Open.Manage.Consumer.RateLimitValueTips' | translate }}</small>
+ <small id="rateLimitValueTips">{{'Open.Manage.Consumer.RateLimitValueTips' | translate }}</small>
+ <div ng-messages="form.rateLimit.$error" role="alert" ng-if="form.rateLimit.$dirty">
+ <div ng-message="required">{{ 'Common.IsRequired' | translate }}</div>
+ <div ng-message="min">{{ 'Common.MinValue' | translate }}</div>
+ <div ng-message="max">{{ 'Common.MaxValue' | translate }}</div>
+ <div ng-message="pattern">{{ 'Common.OnlyNumbers' | translate }}</div>
+ </div>
</div>
</div>Additionally, ensure these changes are coordinated with the backend validation in ConsumerController.
✅ Verification successful
Let me gather more information about the validation and translations.
Based on the findings, I can now provide the final response:
Refine rate limit input validation based on existing implementation
The rate limit validation should align with the backend implementation and existing translations:
<div class="form-group" ng-show="consumer.rateLimitEenabled">
<label class="col-sm-2 control-label">
{{ 'Open.Manage.Consumer.RateLimitValue' | translate }}
</label>
<div class="col-sm-3">
<input type="number"
ng-model="consumer.rateLimit"
name="rateLimit"
+ min="1"
+ required
+ ng-required="consumer.rateLimitEnabled"
+ aria-label="{{ 'Open.Manage.Consumer.RateLimitValue' | translate }}"
+ aria-describedby="rateLimitValueTips"
/>
- <small>{{'Open.Manage.Consumer.RateLimitValueTips' | translate }}</small>
+ <small id="rateLimitValueTips">{{'Open.Manage.Consumer.RateLimitValueTips' | translate }}</small>
+ <div ng-messages="form.rateLimit.$error" role="alert" ng-if="form.rateLimit.$dirty">
+ <div ng-message="required">{{ 'Open.Manage.Consumer.RateLimitValue.Error' | translate }}</div>
+ <div ng-message="min">{{ 'Open.Manage.Consumer.RateLimitValue.Error' | translate }}</div>
+ </div>
</div>
</div>The changes are aligned with:
- Backend validation in
ConsumerControllerwhich requires value > 0 - Existing translation keys for error messages
- Rate limit implementation in
ConsumerAuthenticationFilter
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Verify translation keys and backend validation
# Check if the translation keys exist
rg -l "Open\.Manage\.Consumer\.RateLimitValue(Tips)?" --type json
# Check backend validation
ast-grep --pattern 'class ConsumerController {
$$$
@RequestMapping($$$)
public $$ create$$(ConsumerToken token) {
$$$
}
$$$
}'
Length of output: 319
Script:
#!/bin/bash
# Check the content of translation files for rate limit related keys
rg "Open\.Manage\.Consumer\.RateLimit" apollo-portal/src/main/resources/static/i18n/en.json -A 1
# Check if Common translation keys exist
rg "Common\.(IsRequired|MinValue|MaxValue|OnlyNumbers)" apollo-portal/src/main/resources/static/i18n/en.json
# Look for existing rate limit validation in ConsumerController or related classes
rg -t java "rateLimit|RateLimit" apollo-portal/src/main/java/ -A 2
# Check for existing form validation patterns in other forms
rg "ng-pattern|ng-messages" apollo-portal/src/main/resources/static/ -A 2
Length of output: 11912
apollo-portal/src/test/resources/sql/openapi/NamespaceControllerTest.testCreateAppNamespace.sql (1)
74-75: Verify schema migration for RateLimit column.
The addition of the RateLimit column to the ConsumerToken table requires corresponding schema migration scripts.
✅ Verification successful
Based on the search results, I found that:
- The schema migration script exists in
scripts/sql/profiles/mysql-default/delta/v230-v240/apolloportaldb-v230-v240.sqlwhich adds the RateLimit column:
ALTER TABLE `ConsumerToken` ADD COLUMN `RateLimit` int NOT NULL DEFAULT '0' COMMENT '限流值' AFTER `Token`;- The updated table definition is present in multiple SQL files including
scripts/sql/profiles/mysql-default/apolloportaldb.sqlandscripts/sql/src/apolloportaldb.sqlwhich show the RateLimit column is properly defined:
CREATE TABLE `ConsumerToken` (
...
`RateLimit` int NOT NULL DEFAULT '0' COMMENT '限流值',
...
)Schema migration for RateLimit column is properly handled.
The RateLimit column addition is covered by:
- Migration script to add the column
- Updated table definitions in base SQL files
- Default value and constraints are consistently defined
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Check for schema migration scripts that add the RateLimit column
# Look for migration scripts
echo "Searching for migration scripts..."
fd -e sql . | while read -r file; do
echo "Checking $file for RateLimit column addition..."
rg -i "alter.*table.*consumertoken.*add.*ratelimit|create.*table.*consumertoken.*ratelimit" "$file"
done
# Verify if ConsumerToken table definition includes RateLimit
echo "Checking for ConsumerToken table definition..."
fd -e sql . | while read -r file; do
echo "Checking $file for ConsumerToken table definition..."
rg -A 10 "CREATE.*TABLE.*ConsumerToken" "$file"
done
Length of output: 95087
apollo-portal/src/test/resources/sql/openapi/ConsumerServiceIntegrationTest.testFindAppIdsAuthorizedByConsumerId.sql (1)
81-82: Verify test coverage for rate limiting functionality.
Since this integration test focuses on finding authorized app IDs, ensure there are dedicated test files for rate limiting scenarios.
✅ Verification successful
Rate limiting functionality is well-tested with dedicated test coverage
The codebase has comprehensive rate limit testing:
ConsumerAuthenticationFilterTestcontains dedicated test methods for rate limiting scenarios:testRateLimitSuccessfully()testRateLimitPartFailure()- Includes proper test setup with QPS and duration parameters
- Test data for rate limits is consistently set up across multiple integration test SQL files
- Rate limit implementation is properly distributed across relevant components:
- Authentication filter for enforcement
- Consumer service and controller for management
- Entity classes for data modeling
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Check for rate limit test coverage in the test directory
# Look for test files related to rate limiting
echo "Searching for rate limit test files..."
fd -e java -e kt -e scala "Test.*Rate.*" "apollo-portal/src/test"
# Look for rate limit assertions in test files
echo "Searching for rate limit test assertions..."
rg -l "RateLimit|rateLimit" "apollo-portal/src/test"
Length of output: 805
Script:
#!/bin/bash
# Let's examine the content of the test files to understand the rate limit test coverage
# Check ConsumerAuthenticationFilterTest for rate limit tests
echo "=== ConsumerAuthenticationFilterTest.java ==="
rg -A 10 "RateLimit|rateLimit" "apollo-portal/src/test/java/com/ctrip/framework/apollo/openapi/filter/ConsumerAuthenticationFilterTest.java"
# Check SQL files for rate limit test data setup
echo -e "\n=== SQL Test Files ==="
for file in apollo-portal/src/test/resources/sql/openapi/*.sql; do
echo -e "\n--- $(basename "$file") ---"
rg -A 2 "RateLimit|rateLimit" "$file"
done
# Look for rate limit related code in main source
echo -e "\n=== Rate Limit Implementation ==="
fd -e java -e kt -e scala . "apollo-portal/src/main" -x rg -l "RateLimit|rateLimit" {}
Length of output: 4861
apollo-portal/src/main/resources/static/i18n/en.json (1)
657-661: LGTM! The new localization entries for rate limiting are well structured.
The added translations provide clear and helpful messages for the rate limiting feature, including:
- Toggle for enabling/disabling rate limiting
- Descriptive tooltips explaining the feature
- Clear format guidance for QPS values
- Appropriate error messaging
apollo-portal/src/main/java/com/ctrip/framework/apollo/openapi/service/ConsumerService.java (1)
148-149: Null check when retrieving consumer ID by token
In getConsumerIdByToken, if consumerToken is null, the method returns null. Ensure that all callers of this method handle the possibility of a null return value to prevent potential NullPointerException.
Run the following script to find all usages of getConsumerIdByToken and check for null handling:
✅ Verification successful
Based on the search results, I can generate the final response as I have enough information to verify the concern about null handling.
Null checks are properly handled by all callers of getConsumerIdByToken
The method is called in the following locations, and all of them handle the null case appropriately:
ConsumerService.assignNamespaceRoleToConsumer: Explicitly checks for null and throws BadRequestExceptionConsumerService.assignCreateApplicationRoleToConsumer: Explicitly checks for null and throws BadRequestExceptionConsumerService.assignAppRoleToConsumer: Passes the potentially null value to another method that handles itConsumerAuthUtil.getConsumerId: Returns the null value which is expected behavior for authentication utilities- Test cases explicitly verify null handling behavior
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Find all usages of getConsumerIdByToken and ensure null checks are in place.
# Search for method calls to getConsumerIdByToken and display surrounding lines for context.
rg 'getConsumerIdByToken\(' -A 5 -B 2
Length of output: 9508
.../main/java/com/ctrip/framework/apollo/portal/entity/vo/consumer/ConsumerCreateRequestVO.java
Outdated
Show resolved
Hide resolved
.../main/java/com/ctrip/framework/apollo/portal/entity/vo/consumer/ConsumerCreateRequestVO.java
Outdated
Show resolved
Hide resolved
.../src/test/java/com/ctrip/framework/apollo/openapi/v1/controller/NamespaceControllerTest.java
Outdated
Show resolved
Hide resolved
| Mockito.when(consumerService.generateAndSaveConsumerToken(Mockito.any(), Mockito.any(), Mockito.any())) | ||
| .thenReturn(ConsumerToken); |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Set rateLimit in test ConsumerToken object
The test creates a ConsumerToken object but doesn't set the rateLimit property. This should be set to ensure the test accurately represents the production scenario with rate limiting.
{
ConsumerToken ConsumerToken = new ConsumerToken();
ConsumerToken.setToken(token);
+ ConsumerToken.setRateLimit(expectedRateLimit);
Mockito.when(consumerService.generateAndSaveConsumerToken(Mockito.any(), Mockito.any(), Mockito.any()))
.thenReturn(ConsumerToken);
}Committable suggestion skipped: line range outside the PR's diff.
| @@ -63,7 +63,7 @@ | |||
|
|
|||
| Mockito.verify(consumerService, Mockito.times(1)).createConsumer(Mockito.any()); | |||
| Mockito.verify(consumerService, Mockito.times(1)) | |||
| .generateAndSaveConsumerToken(Mockito.any(), Mockito.any()); | |||
| .generateAndSaveConsumerToken(Mockito.any(), Mockito.any(),Mockito.any()); | |||
There was a problem hiding this comment.
🛠️ Refactor suggestion
Enhance test coverage for rate limit parameter
The test should verify the actual value of the rateLimit parameter instead of using Mockito.any(). Consider adding test cases with specific rate limit values to ensure the rate limiting feature works as expected.
- .generateAndSaveConsumerToken(Mockito.any(), Mockito.any(),Mockito.any());
+ .generateAndSaveConsumerToken(
+ Mockito.eq("appId1"),
+ Mockito.any(),
+ Mockito.eq(expectedRateLimit)
+ );Committable suggestion skipped: line range outside the PR's diff.
...lo-portal/src/main/java/com/ctrip/framework/apollo/portal/controller/ConsumerController.java
Outdated
Show resolved
Hide resolved
apollo-portal/src/main/resources/static/scripts/controller/open/OpenManageController.js
Outdated
Show resolved
Hide resolved
| INSERT INTO "ConsumerToken" (`Id`, `ConsumerId`, `Token`, `RateLimit`, `Expires`, `DataChange_CreatedBy`, `DataChange_LastModifiedBy`) VALUES | ||
| (1000, 1000, '3c16bf5b1f44b465179253442460e8c0ad845289', 20, '2098-12-31 10:00:00', 'apollo', 'apollo'); |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Enhance test data coverage for rate limiting scenarios
The current test data only includes a single consumer token with a rate limit of 20. Consider adding test cases for:
- Tokens with different rate limit values
- Tokens without rate limits (null or 0)
- Tokens with expired dates
INSERT INTO "ConsumerToken" (`Id`, `ConsumerId`, `Token`, `RateLimit`, `Expires`, `DataChange_CreatedBy`, `DataChange_LastModifiedBy`) VALUES
-(1000, 1000, '3c16bf5b1f44b465179253442460e8c0ad845289', 20, '2098-12-31 10:00:00', 'apollo', 'apollo');
+(1000, 1000, '3c16bf5b1f44b465179253442460e8c0ad845289', 20, '2098-12-31 10:00:00', 'apollo', 'apollo'),
+(1001, 1001, '4d27cg6c2g55c576280364553571e9d1be956390', 50, '2098-12-31 10:00:00', 'apollo', 'apollo'),
+(1002, 1002, '5e38dh7d3h66d687391475664682f0e2cf067401', 0, '2098-12-31 10:00:00', 'apollo', 'apollo'),
+(1003, 1003, '6f49ei8e4i77e798402586775793g1f3dg178512', 30, '2023-12-31 10:00:00', 'apollo', 'apollo');📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| INSERT INTO "ConsumerToken" (`Id`, `ConsumerId`, `Token`, `RateLimit`, `Expires`, `DataChange_CreatedBy`, `DataChange_LastModifiedBy`) VALUES | |
| (1000, 1000, '3c16bf5b1f44b465179253442460e8c0ad845289', 20, '2098-12-31 10:00:00', 'apollo', 'apollo'); | |
| INSERT INTO "ConsumerToken" (`Id`, `ConsumerId`, `Token`, `RateLimit`, `Expires`, `DataChange_CreatedBy`, `DataChange_LastModifiedBy`) VALUES | |
| (1000, 1000, '3c16bf5b1f44b465179253442460e8c0ad845289', 20, '2098-12-31 10:00:00', 'apollo', 'apollo'), | |
| (1001, 1001, '4d27cg6c2g55c576280364553571e9d1be956390', 50, '2098-12-31 10:00:00', 'apollo', 'apollo'), | |
| (1002, 1002, '5e38dh7d3h66d687391475664682f0e2cf067401', 0, '2098-12-31 10:00:00', 'apollo', 'apollo'), | |
| (1003, 1003, '6f49ei8e4i77e798402586775793g1f3dg178512', 30, '2023-12-31 10:00:00', 'apollo', 'apollo'); |
apollo-portal/src/main/java/com/ctrip/framework/apollo/openapi/service/ConsumerService.java
Outdated
Show resolved
Hide resolved
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Outside diff range and nitpick comments (2)
apollo-portal/src/main/java/com/ctrip/framework/apollo/portal/entity/vo/consumer/ConsumerCreateRequestVO.java (1)
80-94: Add Javadoc for rate limiting methodsThe new rate limiting methods should be documented to explain their purpose and constraints.
Add these method docs:
/** * @return true if rate limiting is enabled for this consumer */ public boolean isRateLimitEnabled() { return rateLimitEnabled; } /** * @param rateLimitEnabled whether to enable rate limiting for this consumer */ public void setRateLimitEnabled(boolean rateLimitEnabled) { this.rateLimitEnabled = rateLimitEnabled; } /** * @return the maximum number of requests allowed per second when rate limiting is enabled */ public int getRateLimit() { return rateLimit; } /** * @param rateLimit the maximum number of requests allowed per second (must be positive when rate limiting is enabled) */ public void setRateLimit(int rateLimit) { this.rateLimit = rateLimit; }apollo-portal/src/main/java/com/ctrip/framework/apollo/portal/controller/ConsumerController.java (1)
84-91: Enhance rate limit validationWhile the basic validation is good, consider these improvements:
- Add an upper bound check to prevent excessive rate limits
- Handle null rate limit values explicitly
- Use constants instead of magic numbers
+ private static final int DEFAULT_RATE_LIMIT = 0; + private static final int MAX_RATE_LIMIT = 10000; // Adjust based on your requirements + if (requestVO.isRateLimitEnabled()) { - if (requestVO.getRateLimit() <= 0) { + Integer rateLimit = requestVO.getRateLimit(); + if (rateLimit == null || rateLimit <= 0 || rateLimit > MAX_RATE_LIMIT) { throw BadRequestException.rateLimitIsInvalid(); } } else { - requestVO.setRateLimit(0); + requestVO.setRateLimit(DEFAULT_RATE_LIMIT); }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (7)
apollo-common/src/main/java/com/ctrip/framework/apollo/common/exception/BadRequestException.java(1 hunks)apollo-portal/src/main/java/com/ctrip/framework/apollo/openapi/service/ConsumerService.java(6 hunks)apollo-portal/src/main/java/com/ctrip/framework/apollo/portal/controller/ConsumerController.java(3 hunks)apollo-portal/src/main/java/com/ctrip/framework/apollo/portal/entity/vo/consumer/ConsumerCreateRequestVO.java(2 hunks)apollo-portal/src/main/resources/static/open/add-consumer.html(2 hunks)apollo-portal/src/main/resources/static/scripts/controller/open/OpenManageController.js(2 hunks)apollo-portal/src/test/java/com/ctrip/framework/apollo/openapi/v1/controller/NamespaceControllerTest.java(3 hunks)
🚧 Files skipped from review as they are similar to previous changes (4)
- apollo-common/src/main/java/com/ctrip/framework/apollo/common/exception/BadRequestException.java
- apollo-portal/src/main/java/com/ctrip/framework/apollo/openapi/service/ConsumerService.java
- apollo-portal/src/main/resources/static/open/add-consumer.html
- apollo-portal/src/main/resources/static/scripts/controller/open/OpenManageController.js
🔇 Additional comments (3)
apollo-portal/src/main/java/com/ctrip/framework/apollo/portal/entity/vo/consumer/ConsumerCreateRequestVO.java (1)
Line range hint 29-94: Verify integration with ConsumerToken
The rate limiting fields added here need to be properly transferred to the ConsumerToken entity.
✅ Verification successful
Rate limit fields are properly integrated with ConsumerToken
The verification shows that:
- ConsumerToken entity has the
rateLimitfield properly defined with validation - ConsumerService properly handles rate limit through
generateAndSaveConsumerTokenandgenerateConsumerTokenmethods - Database migration script exists:
ALTER TABLE ConsumerToken ADD COLUMN RateLimit int NOT NULL DEFAULT '0' COMMENT '限流值'
The integration is complete and no further action is needed.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Verify that ConsumerToken entity and related components properly handle these new fields
# Check if ConsumerToken has corresponding fields
echo "Checking ConsumerToken entity..."
rg -A 5 "class ConsumerToken"
# Check if ConsumerService properly transfers these fields
echo "Checking ConsumerService implementation..."
rg -A 10 "ConsumerCreateRequestVO.*ConsumerToken"
# Check if database migration scripts exist
echo "Checking for migration scripts..."
fd -e sql . | rg -i "consumer.*token|rate.*limit"
Length of output: 1113
Script:
#!/bin/bash
# Let's check for rate limit related fields in ConsumerToken and any service implementations
# Check ConsumerToken for rate limit fields
echo "Checking ConsumerToken for rate limit fields..."
rg -l "rateLimitEnabled|rateLimit" apollo-portal/src/main/java/com/ctrip/framework/apollo/openapi/entity/ConsumerToken.java
# Check for service implementations that might handle the conversion
echo "Checking service implementations..."
rg -A 10 "ConsumerCreateRequestVO" apollo-portal/src/main/java/com/ctrip/framework/apollo/portal/service/
# Check for any rate limit related database changes
echo "Checking SQL migrations..."
fd . -e sql -x cat {} \; | rg -i "alter.*consumer|create.*consumer"
# Check for any Consumer related service implementations
echo "Checking Consumer service implementations..."
rg -l "class.*Consumer.*Service" apollo-portal/src/main/java/
Length of output: 5228
Script:
#!/bin/bash
# Let's examine the ConsumerService implementation and ConsumerToken entity in detail
# Check ConsumerToken entity for rate limit fields
echo "Checking ConsumerToken entity content..."
cat apollo-portal/src/main/java/com/ctrip/framework/apollo/openapi/entity/ConsumerToken.java
# Check ConsumerService implementation
echo "Checking ConsumerService implementation..."
cat apollo-portal/src/main/java/com/ctrip/framework/apollo/openapi/service/ConsumerService.java
# Look for any rate limit related code in the service
echo "Checking for rate limit related implementations..."
rg -A 5 "rateLimit|rateLimitEnabled" apollo-portal/src/main/java/com/ctrip/framework/apollo/openapi/service/
Length of output: 23578
apollo-portal/src/main/java/com/ctrip/framework/apollo/portal/controller/ConsumerController.java (2)
Line range hint 138-167: LGTM! Improved code readability
The addition of braces and consistent indentation enhances code readability and follows better coding practices.
98-98: Verify rate limit handling in ConsumerService
The rate limit is now passed to generateAndSaveConsumerToken, but we should verify its implementation in ConsumerService.
✅ Verification successful
Let me check the implementation of the service method to verify the rate limit handling.
Rate limit handling is properly implemented in ConsumerService
The implementation in ConsumerService properly validates and handles the rate limit:
- Validates rate limit is non-negative using
Preconditions.checkArgument(rateLimit != null && rateLimit >= 0, "Rate limit must be non-negative") - Stores rate limit in ConsumerToken entity
- Handles rate limit within a transaction (method is called from a @transactional context)
- Properly persists the rate limit value through ConsumerTokenRepository
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Search for the implementation of generateAndSaveConsumerToken
ast-grep --pattern 'generateAndSaveConsumerToken($token, $rateLimit, $expires) {
$$$
}'
# Search for related tests
rg -l "generateAndSaveConsumerToken.*rateLimit.*expires" --type java
Length of output: 254
Script:
#!/bin/bash
# Get the implementation details of ConsumerService
cat apollo-portal/src/main/java/com/ctrip/framework/apollo/openapi/service/ConsumerService.java
Length of output: 17058
.../main/java/com/ctrip/framework/apollo/portal/entity/vo/consumer/ConsumerCreateRequestVO.java
Show resolved
Hide resolved
| restTemplate.exchange( | ||
| url("/openapi/v1/apps/{appId}/appnamespaces"), | ||
| dto, OpenAppNamespaceDTO.class, dto.getAppId() | ||
| HttpMethod.POST, | ||
| new HttpEntity<>(dto, HTTP_HEADERS_WITH_TOKEN), | ||
| OpenAppNamespaceDTO.class, dto.getAppId() |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Add test cases for rate limiting scenarios.
While the HTTP request handling improvements are good, this test class should be extended to cover the new rate limiting functionality. Consider adding test cases for:
- Requests within rate limit
- Requests exceeding rate limit
- Edge cases around limit boundaries
Example test structure:
@Test
public void shouldRejectRequestWhenRateLimitExceeded() {
// Setup token with rate limit
// Send requests up to limit
// Verify next request is rejected
}
@Test
public void shouldAllowRequestsWithinRateLimit() {
// Setup token with rate limit
// Send requests below limit
// Verify all requests succeed
}# Conflicts: # CHANGES.md
...al/src/main/java/com/ctrip/framework/apollo/openapi/filter/ConsumerAuthenticationFilter.java
Outdated
Show resolved
Hide resolved
...al/src/main/java/com/ctrip/framework/apollo/openapi/filter/ConsumerAuthenticationFilter.java
Outdated
Show resolved
Hide resolved
apollo-portal/src/main/java/com/ctrip/framework/apollo/openapi/service/ConsumerService.java
Outdated
Show resolved
Hide resolved
...rc/test/java/com/ctrip/framework/apollo/openapi/filter/ConsumerAuthenticationFilterTest.java
Outdated
Show resolved
Hide resolved
.../src/test/java/com/ctrip/framework/apollo/openapi/v1/controller/NamespaceControllerTest.java
Outdated
Show resolved
Hide resolved
...ortal/src/test/java/com/ctrip/framework/apollo/portal/controller/ConsumerControllerTest.java
Outdated
Show resolved
Hide resolved
dosubot
bot
added
size:L
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Outside diff range and nitpick comments (2)
apollo-portal/src/main/java/com/ctrip/framework/apollo/openapi/filter/ConsumerAuthenticationFilter.java (2)
82-83: Add null check for rateLimitThe current check
null != rateLimit && rateLimit > 0is good, but consider logging when rateLimit is null for debugging purposes.Integer rateLimit = consumerToken.getRateLimit(); +if (rateLimit == null) { + logger.debug("No rate limit configured for token: {}", consumerToken.getToken()); +} if (null != rateLimit && rateLimit > 0) {
87-90: Consider adding request details in rate limit error responseThe error message could be more helpful by including the current rate limit value.
-response.sendError(TOO_MANY_REQUESTS, "Too Many Requests, the flow is limited"); +response.sendError(TOO_MANY_REQUESTS, + String.format("Too Many Requests. Rate limit: %d requests per second", rateLimit));
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (7)
apollo-portal/src/main/java/com/ctrip/framework/apollo/openapi/filter/ConsumerAuthenticationFilter.java(4 hunks)apollo-portal/src/main/java/com/ctrip/framework/apollo/openapi/service/ConsumerService.java(6 hunks)apollo-portal/src/main/java/com/ctrip/framework/apollo/portal/spi/configuration/AuthFilterConfiguration.java(1 hunks)apollo-portal/src/main/resources/static/i18n/en.json(1 hunks)apollo-portal/src/test/java/com/ctrip/framework/apollo/SkipAuthorizationConfiguration.java(2 hunks)apollo-portal/src/test/java/com/ctrip/framework/apollo/openapi/filter/ConsumerAuthenticationFilterTest.java(6 hunks)apollo-portal/src/test/java/com/ctrip/framework/apollo/portal/controller/ConsumerControllerTest.java(2 hunks)
🚧 Files skipped from review as they are similar to previous changes (4)
- apollo-portal/src/main/java/com/ctrip/framework/apollo/portal/spi/configuration/AuthFilterConfiguration.java
- apollo-portal/src/main/resources/static/i18n/en.json
- apollo-portal/src/test/java/com/ctrip/framework/apollo/SkipAuthorizationConfiguration.java
- apollo-portal/src/test/java/com/ctrip/framework/apollo/portal/controller/ConsumerControllerTest.java
🔇 Additional comments (12)
apollo-portal/src/main/java/com/ctrip/framework/apollo/openapi/filter/ConsumerAuthenticationFilter.java (3)
50-56: LGTM: Constants are well-defined and documented
The constants are appropriately chosen:
WARMUP_MILLIS: 1-second warmup addresses the initial rate limiting accuracy issueRATE_LIMITER_CACHE_MAX_SIZE: 20000 entries is reasonable for memory managementTOO_MANY_REQUESTS: Using 429 status code follows HTTP standards for rate limiting
110-117: LGTM: Thread-safe rate limiter creation
The implementation correctly uses Cache.get() with a supplier lambda for thread-safe creation of rate limiters. The error handling is appropriate.
82-96: Verify rate limiting test coverage
Let's verify that the rate limiting implementation is properly tested.
✅ Verification successful
Let me gather more information about the test coverage by examining the actual test implementations.
Let me check the complete test implementation to understand the test scenarios better.
Rate limiting implementation is well tested
The rate limiting implementation has comprehensive test coverage with proper test scenarios:
- Tests successful rate limiting within QPS limits
- Tests rate limiting enforcement when QPS is exceeded
- Verifies correct error responses (429 Too Many Requests)
- Includes concurrent execution tests with controlled QPS
- Validates both successful and failed scenarios with proper assertions
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Search for rate limiting test cases
echo "Searching for rate limiting tests..."
rg -l "testRateLimit" --type java
# Search for specific test scenarios
echo "Searching for specific test scenarios..."
rg "ConsumerAuthenticationFilterTest" -A 10 --type java
Length of output: 1839
Script:
#!/bin/bash
# Search for rate limit test implementations
rg "testRateLimit|rateLimit.*test" -B 2 -A 15 apollo-portal/src/test/java/com/ctrip/framework/apollo/openapi/filter/ConsumerAuthenticationFilterTest.java
Length of output: 1175
Script:
#!/bin/bash
# Get the complete test file content to analyze test coverage
cat apollo-portal/src/test/java/com/ctrip/framework/apollo/openapi/filter/ConsumerAuthenticationFilterTest.java
Length of output: 6990
apollo-portal/src/test/java/com/ctrip/framework/apollo/openapi/filter/ConsumerAuthenticationFilterTest.java (4)
Line range hint 19-57: LGTM! Well-structured imports and constants.
The new imports support concurrent testing needs, and the TOO_MANY_REQUESTS constant follows HTTP standards.
81-83: LGTM! Clean test setup.
The ConsumerToken setup aligns well with the rate limiting changes.
85-85: LGTM! Consistent token-based authentication updates.
The shift from getConsumerId to getConsumerToken aligns with the new token-based rate limiting architecture.
Also applies to: 99-99
177-185: LGTM! Clean and focused mock setup.
The setupRateLimitMocks helper provides a clear and maintainable way to configure the test environment.
apollo-portal/src/main/java/com/ctrip/framework/apollo/openapi/service/ConsumerService.java (5)
141-146: LGTM!
The implementation correctly handles null checks and token expiration validation.
148-150: LGTM!
Clean refactoring that maintains backward compatibility while supporting the new rate limiting feature.
360-360: Verify intended method visibility change
The createConsumerRole method's visibility has been changed from private to package-private. This change wasn't mentioned in the PR objectives and could unintentionally expose internal implementation details.
Let's check if this method is needed by other classes in the same package:
✅ Verification successful
The search results show that createConsumerRole is only used within the ConsumerService class itself and in its corresponding test class ConsumerServiceTest. The test class is mocking this method which requires package-private visibility for mocking in tests. This explains the intentional visibility change from private to package-private.
Package-private visibility is appropriate for testing purposes
The visibility change of createConsumerRole method from private to package-private is justified as it enables mocking in the test class while still maintaining encapsulation within the package. This is a common Java testing pattern.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Search for createConsumerRole usage in the same package
rg "createConsumerRole" --type java
Length of output: 2794
325-328: Clarify rate limit semantics
The default value of 0 for invalid rate limits is ambiguous. It's unclear whether 0 means "no limit" or "no access". This could lead to confusion in rate limit enforcement.
Consider:
- Document the rate limit semantics in code comments
- Use a named constant for the default value
- Consider using Optional to make the absence of a rate limit explicit
+ // Default rate limit of 0 means [specify meaning: unlimited access/no access]
+ private static final Integer DEFAULT_RATE_LIMIT = 0;
+
if (rateLimit == null || rateLimit < 0) {
- rateLimit = 0;
+ rateLimit = DEFAULT_RATE_LIMIT;
}Let's verify how rate limits are enforced:
✅ Verification successful
Let's examine the rate limit enforcement logic to understand how the value is used.
Based on the ConsumerAuthenticationFilter implementation, I can now provide a definitive response about the rate limit semantics:
Rate limit of 0 means "no rate limiting" - this is safe and clear from the code
The rate limit enforcement logic in ConsumerAuthenticationFilter.java shows that rate limiting is only applied when rateLimit != null && rateLimit > 0. This means:
- A value of 0 (or null) bypasses rate limiting completely
- Only positive values enable rate limiting, where the value represents the permitted request rate
Therefore, the current implementation is actually correct and unambiguous, though it could benefit from documentation. Here's the suggested improvement:
+ // Rate limit of 0 means no rate limiting will be applied
+ // Only positive values enable rate limiting with the specified rate
if (rateLimit == null || rateLimit < 0) {
rateLimit = 0;
}🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Search for rate limit enforcement logic
rg -l "rateLimit|RateLimit" --type java
Length of output: 829
Script:
#!/bin/bash
# Check rate limit enforcement in ConsumerAuthenticationFilter
rg -A 10 "rateLimit" apollo-portal/src/main/java/com/ctrip/framework/apollo/openapi/filter/ConsumerAuthenticationFilter.java
# Check rate limit usage in ConsumerToken
rg -A 5 "rateLimit" apollo-portal/src/main/java/com/ctrip/framework/apollo/openapi/entity/ConsumerToken.java
# Check rate limit related tests
rg -A 10 "rateLimit" apollo-portal/src/test/java/com/ctrip/framework/apollo/openapi/filter/ConsumerAuthenticationFilterTest.java
Length of output: 1519
123-126: 🛠️ Refactor suggestion
Add validation for rateLimit parameter
The rateLimit parameter should be validated at the entry point of the service method to ensure data consistency.
Add validation after the existing precondition check:
public ConsumerToken generateAndSaveConsumerToken(Consumer consumer, Integer rateLimit, Date expires) {
Preconditions.checkArgument(consumer != null, "Consumer can not be null");
+ Preconditions.checkArgument(rateLimit == null || rateLimit >= 0, "Rate limit must be non-negative");
ConsumerToken consumerToken = generateConsumerToken(consumer, rateLimit, expires);Likely invalid or redundant comment.
| @Test | ||
| public void testRateLimitSuccessfully() throws Exception { | ||
| String someToken = "someToken"; | ||
| Long someConsumerId = 1L; | ||
| int qps = 5; | ||
| int durationInSeconds = 3; | ||
|
|
||
| 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); | ||
|
|
||
| } | ||
|
|
||
|
|
||
| @Test | ||
| public void testRateLimitPartFailure() throws Exception { | ||
| String someToken = "someToken"; | ||
| Long someConsumerId = 1L; | ||
| int qps = 5; | ||
| int durationInSeconds = 3; | ||
|
|
||
| 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); | ||
| } | ||
| }; | ||
|
|
||
| int realQps = qps + 10; | ||
| executeWithQps(realQps, task, durationInSeconds); | ||
|
|
||
| int leastTimes = qps * durationInSeconds; | ||
| int mostTimes = realQps * durationInSeconds; | ||
|
|
||
| verify(response, atLeastOnce()).sendError(eq(TOO_MANY_REQUESTS), 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); | ||
|
|
||
| } |
There was a problem hiding this comment.
🛠️ 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);Committable suggestion skipped: line range outside the PR's diff.
| 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(); | ||
| } |
There was a problem hiding this comment.
Enhance executeWithQps reliability and resource management.
The current implementation has several potential issues:
- No proper verification of task completion
- Potential thread pool resource leak
- No shutdown timeout handling
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
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| 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(); | |
| } | |
| 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++) { | |
| 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(); | |
| } | |
| } | |
| } |
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Outside diff range and nitpick comments (2)
apollo-portal/src/test/java/com/ctrip/framework/apollo/openapi/filter/ConsumerAuthenticationFilterTest.java (2)
56-56: Consider using HttpStatus enum for TOO_MANY_REQUESTSInstead of using a magic number, consider using
HttpStatus.TOO_MANY_REQUESTS.value()from Spring's HttpStatus enum for better maintainability and clarity.- private static final int TOO_MANY_REQUESTS = 429; + private static final int TOO_MANY_REQUESTS = HttpStatus.TOO_MANY_REQUESTS.value();
141-175: Enhance rate limit failure verificationThe test verifies the HTTP status code but doesn't validate the error message content. Consider adding:
verify(response, atLeastOnce()).sendError( eq(TOO_MANY_REQUESTS), contains("Rate limit exceeded") // Verify error message );
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (1)
apollo-portal/src/test/java/com/ctrip/framework/apollo/openapi/filter/ConsumerAuthenticationFilterTest.java(6 hunks)
🔇 Additional comments (3)
apollo-portal/src/test/java/com/ctrip/framework/apollo/openapi/filter/ConsumerAuthenticationFilterTest.java (3)
81-83: LGTM! Clean transition to token-based authentication
The updates properly reflect the shift from direct consumer ID to token-based authentication while maintaining test coverage.
Also applies to: 85-85, 99-99
110-138: 🛠️ Refactor suggestion
Consider making rate limit test more deterministic
While the test logic is sound, timing-based tests can be flaky. Consider using a mocked clock or rate limiter for more deterministic results.
189-204:
Improve resource management in executeWithQps
The current implementation has several issues:
- No proper verification of task completion
- Potential thread pool resource leak
- No shutdown timeout handling
Additionally, consider adding logging for rate limit violations to aid in debugging:
public static void executeWithQps(int qps, Runnable task, int durationInSeconds) {
ExecutorService executor = Executors.newFixedThreadPool(qps);
long totalTasks = qps * durationInSeconds;
+ int completedTasks = 0;
try {
for (int i = 0; i < totalTasks; i++) {
Future<?> future = executor.submit(task);
try {
future.get(1000 / qps, TimeUnit.MILLISECONDS);
+ completedTasks++;
TimeUnit.MILLISECONDS.sleep(1000 / qps);
} catch (InterruptedException e) {
Thread.currentThread().interrupt();
break;
} catch (TimeoutException e) {
+ log.warn("Task {} exceeded rate limit", i);
}
}
} finally {
executor.shutdown();
if (!executor.awaitTermination(5, TimeUnit.SECONDS)) {
executor.shutdownNow();
}
+ log.info("Completed {}/{} tasks", completedTasks, totalTasks);
}
}
There was a problem hiding this comment.
Copilot reviewed 13 out of 28 changed files in this pull request and generated no suggestions.
Files not reviewed (15)
- apollo-portal/src/main/resources/static/i18n/en.json: Language not supported
- apollo-portal/src/main/resources/static/i18n/zh-CN.json: Language not supported
- apollo-portal/src/main/resources/static/open/add-consumer.html: Language not supported
- apollo-portal/src/test/resources/sql/openapi/ConsumerServiceIntegrationTest.commonData.sql: Language not supported
- apollo-portal/src/test/resources/sql/openapi/ConsumerServiceIntegrationTest.testFindAppIdsAuthorizedByConsumerId.sql: Language not supported
- apollo-portal/src/test/resources/sql/openapi/NamespaceControllerTest.testCreateAppNamespace.sql: Language not supported
- apollo-portal/src/main/java/com/ctrip/framework/apollo/portal/controller/ConsumerController.java: Evaluated as low risk
- apollo-portal/src/main/resources/static/scripts/controller/open/OpenManageController.js: Evaluated as low risk
- docs/zh/deployment/distributed-deployment-guide.md: Evaluated as low risk
- apollo-portal/src/main/java/com/ctrip/framework/apollo/openapi/filter/ConsumerAuthenticationFilter.java: Evaluated as low risk
- apollo-portal/src/test/java/com/ctrip/framework/apollo/portal/controller/ConsumerControllerTest.java: Evaluated as low risk
- apollo-portal/src/main/java/com/ctrip/framework/apollo/openapi/util/ConsumerAuthUtil.java: Evaluated as low risk
- apollo-common/src/main/java/com/ctrip/framework/apollo/common/exception/BadRequestException.java: Evaluated as low risk
- apollo-portal/src/test/java/com/ctrip/framework/apollo/openapi/filter/ConsumerAuthenticationFilterTest.java: Evaluated as low risk
- apollo-portal/src/main/java/com/ctrip/framework/apollo/portal/spi/configuration/AuthFilterConfiguration.java: Evaluated as low risk
...al/src/main/java/com/ctrip/framework/apollo/openapi/filter/ConsumerAuthenticationFilter.java
Show resolved
Hide resolved
dosubot
bot
added
size:XL
There was a problem hiding this comment.
Actionable comments posted: 4
🧹 Outside diff range and nitpick comments (6)
apollo-portal/src/main/java/com/ctrip/framework/apollo/portal/entity/vo/consumer/ConsumerInfo.java (1)
110-116: Add JavaDoc documentation for rate limit getter/setter.The rate limit methods should be documented to clarify their purpose and valid values. Additionally, consider adding validation in the setter.
+ /** + * Gets the rate limit for API requests. + * @return the maximum number of requests allowed per time window + */ public Integer getRateLimit() { return rateLimit; } + /** + * Sets the rate limit for API requests. + * @param rateLimit the maximum number of requests allowed per time window (must be non-negative) + * @throws IllegalArgumentException if rateLimit is negative + */ public void setRateLimit(Integer rateLimit) { + if (rateLimit != null && rateLimit < 0) { + throw new IllegalArgumentException("Rate limit must be non-negative"); + } this.rateLimit = rateLimit; }apollo-portal/src/main/java/com/ctrip/framework/apollo/openapi/filter/ConsumerAuthenticationFilter.java (2)
84-98: Rate limiting implementation looks solid with proper error handlingThe implementation correctly:
- Checks for valid rate limit values
- Handles rate limiting with proper error responses
- Includes error handling that prevents requests from proceeding on failures
However, the warmup period handling needs clarification in the code comments.
Add a comment explaining the warmup period's purpose:
private static final int WARMUP_MILLIS = 1000; // ms +// Warmup period to handle initial rate limiting inaccuracies in Guava's RateLimiter
77-82: Token validation looks good but could use null-safety improvementThe token validation is properly implemented, but we could make it more null-safe.
Consider using Optional to handle the token more elegantly:
-String token = request.getHeader(HttpHeaders.AUTHORIZATION); -ConsumerToken consumerToken = consumerAuthUtil.getConsumerToken(token); - -if (null == consumerToken) { +ConsumerToken consumerToken = Optional.ofNullable(request.getHeader(HttpHeaders.AUTHORIZATION)) + .map(consumerAuthUtil::getConsumerToken) + .orElse(null); +if (consumerToken == null) {apollo-portal/src/main/resources/static/open/manage.html (2)
82-84: Enhance rate limit value display.The current implementation could benefit from improved user experience:
- Add formatting for rate limit values (e.g., "unlimited" for 0 or null)
- Consider adding a visual indicator (icon/color) for enabled/disabled rate limits
- Add a tooltip explaining the rate limit meaning (e.g., "requests per minute")
Example enhancement:
-<td style="width: 10%">{{ consumer.rateLimit }}</td> +<td style="width: 10%"> + <div ng-if="consumer.rateLimit"> + <span title="Maximum requests per minute">{{ consumer.rateLimit }}</span> + <i class="glyphicon glyphicon-dashboard"></i> + </div> + <div ng-if="!consumer.rateLimit"> + <span class="text-muted">Unlimited</span> + </div> +</td>
Line range hint
65-84: UI changes align well with rate limiting objectives.The UI modifications effectively support the rate limiting feature requested in #5244:
- Rate limit values are clearly displayed in the consumer management interface
- The implementation maintains consistency with existing UI patterns
- The changes integrate well with the ConsumerToken modifications
Consider adding a column sort capability for rate limit values to help administrators quickly identify consumers with high/low limits.
apollo-portal/src/main/java/com/ctrip/framework/apollo/openapi/service/ConsumerService.java (1)
337-348: LGTM! Consider extracting default rate limitThe implementation handles rate limit well. Consider extracting the default value to a constant for better maintainability.
+ private static final Integer DEFAULT_RATE_LIMIT = 0; + private ConsumerToken generateConsumerToken(Consumer consumer, Integer rateLimit, Date expires) { long consumerId = consumer.getId(); String createdBy = userInfoHolder.getUser().getUserId(); Date createdTime = new Date(); if (rateLimit == null || rateLimit < 0) { - rateLimit = 0; + rateLimit = DEFAULT_RATE_LIMIT; }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (5)
apollo-portal/src/main/java/com/ctrip/framework/apollo/openapi/filter/ConsumerAuthenticationFilter.java(4 hunks)apollo-portal/src/main/java/com/ctrip/framework/apollo/openapi/service/ConsumerService.java(11 hunks)apollo-portal/src/main/java/com/ctrip/framework/apollo/portal/entity/vo/consumer/ConsumerInfo.java(2 hunks)apollo-portal/src/main/resources/static/open/manage.html(3 hunks)apollo-portal/src/main/resources/static/scripts/controller/open/OpenManageController.js(2 hunks)
🔇 Additional comments (10)
apollo-portal/src/main/java/com/ctrip/framework/apollo/portal/entity/vo/consumer/ConsumerInfo.java (1)
Line range hint 36-116: Verify consistent rate limit validation across related classes.
Let's ensure that rate limit validation is consistent across all related classes.
✅ Verification successful
Based on the search results, I can see that rate limit validation is implemented across multiple classes, but in different ways:
- In
ConsumerToken, the rate limit is marked as non-nullable at the database level with@Column(name = "RateLimit", nullable = false) - In
ConsumerController, there's programmatic validation:if (requestVO.isRateLimitEnabled()) { if (requestVO.getRateLimit() <= 0) { throw BadRequestException.rateLimitIsInvalid(); } }
- In
ConsumerService, there's also validation:if (rateLimit == null || rateLimit < 0) { rateLimit = 0; }
Let me verify if there are any other validation annotations in the codebase that we might have missed:
Rate limit validation is consistently implemented through programmatic checks
The codebase shows a consistent approach to rate limit validation through programmatic checks rather than validation annotations:
- Database-level validation:
ConsumerTokenensures non-null values with@Column(nullable = false) - Service-level validation:
ConsumerServicehandles null/negative values by defaulting to 0 - Controller-level validation:
ConsumerControllerenforces positive values when rate limiting is enabled
The current implementation in ConsumerInfo without validation annotations is consistent with the codebase's approach to rate limit validation.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Check rate limit validation in related classes
# Check ConsumerToken class for rate limit validation
echo "Checking ConsumerToken validation..."
rg -A 5 "rateLimit|RateLimit" --type java
# Check for validation annotations in related classes
echo "Checking validation annotations..."
ast-grep --pattern 'class $_ {
$$$
@NotNull
@Min($_)
private Integer rateLimit;
$$$
}'
Length of output: 30875
Script:
#!/bin/bash
# Check for validation annotations in consumer-related classes
rg -B 2 -A 2 "@(NotNull|Min|Valid)" --type java apollo-portal/src/main/java/com/ctrip/framework/apollo/portal/entity/vo/consumer/
rg -B 2 -A 2 "@(NotNull|Min|Valid)" --type java apollo-portal/src/main/java/com/ctrip/framework/apollo/openapi/entity/
Length of output: 252
apollo-portal/src/main/java/com/ctrip/framework/apollo/openapi/filter/ConsumerAuthenticationFilter.java (3)
56-58: Cache configuration looks good with expiration policy
The cache configuration has been properly updated with:
- 1-hour expiration after access
- Maximum size of 20,000 entries
- This aligns well with the previous discussion about cache management.
112-119: Thread-safe implementation of rate limiter creation
The implementation correctly uses Cache.get with a loader function, making it thread-safe. The exception handling is also appropriate.
84-85: Verify rate limit configuration usage
Let's verify the rate limit configuration implementation across the codebase.
✅ Verification successful
Rate limit implementation is properly verified and working as expected
The rate limit implementation is correctly implemented with the following key aspects:
- ConsumerToken entity properly stores the rate limit as a non-negative integer with
@PositiveOrZerovalidation - ConsumerAuthenticationFilter implements rate limiting using Google's RateLimiter with:
- Per-token rate limiting using a cache with 1-hour expiration
- Proper error handling (429 Too Many Requests)
- 1-second warmup period
- Comprehensive test coverage in ConsumerAuthenticationFilterTest verifies:
- Rate limit success scenarios
- Rate limit failure scenarios
- Concurrent request handling
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Verify rate limit implementation and configuration
# Check ConsumerToken rate limit field usage
echo "Checking ConsumerToken rate limit usage:"
rg -l "getRateLimit|setRateLimit" --type java
# Check rate limit configuration in properties/yaml files
echo -e "\nChecking rate limit configuration:"
fd -e properties -e yaml | xargs rg "rateLimit|rate.limit" -l
# Check rate limit related tests
echo -e "\nChecking rate limit tests:"
fd "Test.java" | xargs rg "rateLimit|rateLimiter" -l
Length of output: 1199
Script:
#!/bin/bash
# Let's analyze the rate limit implementation details
# Check rate limit implementation in ConsumerToken
echo "ConsumerToken implementation:"
cat apollo-portal/src/main/java/com/ctrip/framework/apollo/openapi/entity/ConsumerToken.java
# Check rate limit usage in ConsumerAuthenticationFilter
echo -e "\nConsumerAuthenticationFilter implementation:"
cat apollo-portal/src/main/java/com/ctrip/framework/apollo/openapi/filter/ConsumerAuthenticationFilter.java
# Check rate limit tests
echo -e "\nRate limit tests:"
cat apollo-portal/src/test/java/com/ctrip/framework/apollo/openapi/filter/ConsumerAuthenticationFilterTest.java
# Check consumer service implementation
echo -e "\nConsumer service implementation:"
cat apollo-portal/src/main/java/com/ctrip/framework/apollo/openapi/service/ConsumerService.java
Length of output: 32039
apollo-portal/src/main/resources/static/scripts/controller/open/OpenManageController.js (1)
171-180: 🛠️ Refactor suggestion
Enhance rate limit validation for better error handling.
The current validation can be improved to handle edge cases and provide better user feedback:
- Add upper bound check for rate limit
- Add validation for non-numeric values
- Provide specific error message about valid range
if ($scope.consumer.rateLimitEnabled) {
- if (!$scope.consumer.rateLimit || $scope.consumer.rateLimit < 1) {
+ const rateLimit = parseInt($scope.consumer.rateLimit);
+ if (isNaN(rateLimit) || rateLimit < 1 || rateLimit > 1000) {
toastr.warning($translate.instant('Open.Manage.Consumer.RateLimitValue.Error'));
$scope.submitBtnDisabled = false;
return;
}
+ $scope.consumer.rateLimit = rateLimit; // Ensure numeric type
} else {
$scope.consumer.rateLimit = 0;
}Let's verify if there are any existing rate limit validations in the backend:
apollo-portal/src/main/resources/static/open/manage.html (1)
65-66: Verify translation key for rate limit.
The new column uses the translation key 'Open.Manage.Consumer.RateLimitValue'. Please ensure this key is defined in the translation files.
✅ Verification successful
Translation key for rate limit is properly defined
The translation key 'Open.Manage.Consumer.RateLimitValue' is correctly defined in both English and Chinese translation files with appropriate values and related tooltips.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Search for the translation key in i18n files
rg -l "Open\.Manage\.Consumer\.RateLimitValue" --type json
Length of output: 169
Script:
#!/bin/bash
# Check the content of translation files to verify the key has proper values
rg "Open\.Manage\.Consumer\.RateLimitValue" --type json -C 1
Length of output: 1509
apollo-portal/src/main/java/com/ctrip/framework/apollo/openapi/service/ConsumerService.java (4)
141-150: LGTM! Well-structured token retrieval logic
The refactoring improves code reusability and maintains proper validation.
Line range hint 201-212: LGTM! Clean integration of rate limit into consumer info
Line range hint 416-434: LGTM! Efficient batch processing of rate limits
The implementation efficiently retrieves rate limits in batch along with other consumer information.
Line range hint 442-457: LGTM! Proper cleanup of consumer data
The implementation ensures complete cleanup of consumer data including tokens.
| private Integer rateLimit; | ||
|
|
There was a problem hiding this comment.
🛠️ Refactor suggestion
Add validation constraints for the rateLimit field.
The rateLimit field should have validation constraints to ensure data integrity and prevent potential issues:
+import javax.validation.constraints.Min;
+import javax.validation.constraints.NotNull;
public class ConsumerInfo {
// ... other fields ...
+ @NotNull(message = "Rate limit cannot be null")
+ @Min(value = 0, message = "Rate limit must be non-negative")
private Integer rateLimit;Committable suggestion skipped: line range outside the PR's diff.
| $scope.toggleRateLimitEnabledInput = function() { | ||
| if (!$scope.consumer.rateLimitEnabled) { | ||
| $scope.consumer.rateLimit = 0; | ||
| } | ||
| }; |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Enhance toggle function implementation for better user experience.
The current implementation only handles the disable case. Consider these improvements:
- Initialize a default value when enabling the rate limit
- Add validation for the rate limit value
- Add type safety checks
$scope.toggleRateLimitEnabledInput = function() {
+ // Ensure rateLimit is a number
+ $scope.consumer.rateLimit = parseInt($scope.consumer.rateLimit) || 0;
+
if (!$scope.consumer.rateLimitEnabled) {
$scope.consumer.rateLimit = 0;
+ } else {
+ // Set default value when enabling if current value is 0
+ if ($scope.consumer.rateLimit <= 0) {
+ $scope.consumer.rateLimit = 1;
+ }
}
};📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| $scope.toggleRateLimitEnabledInput = function() { | |
| if (!$scope.consumer.rateLimitEnabled) { | |
| $scope.consumer.rateLimit = 0; | |
| } | |
| }; | |
| $scope.toggleRateLimitEnabledInput = function() { | |
| // Ensure rateLimit is a number | |
| $scope.consumer.rateLimit = parseInt($scope.consumer.rateLimit) || 0; | |
| if (!$scope.consumer.rateLimitEnabled) { | |
| $scope.consumer.rateLimit = 0; | |
| } else { | |
| // Set default value when enabling if current value is 0 | |
| if ($scope.consumer.rateLimit <= 0) { | |
| $scope.consumer.rateLimit = 1; | |
| } | |
| } | |
| }; |
apollo-portal/src/main/java/com/ctrip/framework/apollo/openapi/service/ConsumerService.java
Outdated
Show resolved
Hide resolved
| public ConsumerToken generateAndSaveConsumerToken(Consumer consumer, Integer rateLimit, Date expires) { | ||
| Preconditions.checkArgument(consumer != null, "Consumer can not be null"); | ||
|
|
||
| ConsumerToken consumerToken = generateConsumerToken(consumer, expires); | ||
| ConsumerToken consumerToken = generateConsumerToken(consumer, rateLimit, expires); |
There was a problem hiding this comment.
Add parameter validation for rateLimit
The method accepts rateLimit without validation. While there's validation in generateConsumerToken, it should be done at the entry point.
Add this validation at the beginning of the method:
public ConsumerToken generateAndSaveConsumerToken(Consumer consumer, Integer rateLimit, Date expires) {
Preconditions.checkArgument(consumer != null, "Consumer can not be null");
+ Preconditions.checkArgument(rateLimit == null || rateLimit >= 0, "Rate limit must be non-negative");
+ Preconditions.checkArgument(expires != null, "Expires date can not be null");📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| public ConsumerToken generateAndSaveConsumerToken(Consumer consumer, Integer rateLimit, Date expires) { | |
| Preconditions.checkArgument(consumer != null, "Consumer can not be null"); | |
| ConsumerToken consumerToken = generateConsumerToken(consumer, expires); | |
| ConsumerToken consumerToken = generateConsumerToken(consumer, rateLimit, expires); | |
| public ConsumerToken generateAndSaveConsumerToken(Consumer consumer, Integer rateLimit, Date expires) { | |
| Preconditions.checkArgument(consumer != null, "Consumer can not be null"); | |
| Preconditions.checkArgument(rateLimit == null || rateLimit >= 0, "Rate limit must be non-negative"); | |
| Preconditions.checkArgument(expires != null, "Expires date can not be null"); | |
| ConsumerToken consumerToken = generateConsumerToken(consumer, rateLimit, expires); |
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Outside diff range and nitpick comments (2)
apollo-portal/src/main/java/com/ctrip/framework/apollo/openapi/service/ConsumerService.java (2)
261-270: Add null safety to rate limit retrievalThe current implementation has proper null handling for
consumerToken, but could be made more concise using Java 8 Optional.Consider this more concise implementation:
private List<Integer> getRateLimit(List<Long> consumerIdList) { List<Integer> list = new ArrayList<>(consumerIdList.size()); for (Long consumerId : consumerIdList) { - ConsumerToken consumerToken = consumerTokenRepository.findByConsumerId(consumerId); - Integer rateLimit = consumerToken != null ? consumerToken.getRateLimit() : 0; + Integer rateLimit = Optional.ofNullable(consumerTokenRepository.findByConsumerId(consumerId)) + .map(ConsumerToken::getRateLimit) + .orElse(0); list.add(rateLimit); } return list; }
337-345: Consider extracting rate limit validation logicThe rate limit validation logic could be extracted to a utility method for reuse and better maintainability.
Consider this refactoring:
+ private Integer validateRateLimit(Integer rateLimit) { + return (rateLimit == null || rateLimit < 0) ? 0 : rateLimit; + } private ConsumerToken generateConsumerToken(Consumer consumer, Integer rateLimit, Date expires) { long consumerId = consumer.getId(); String createdBy = userInfoHolder.getUser().getUserId(); Date createdTime = new Date(); - if (rateLimit == null || rateLimit < 0) { - rateLimit = 0; - } + rateLimit = validateRateLimit(rateLimit);
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (1)
apollo-portal/src/main/java/com/ctrip/framework/apollo/openapi/service/ConsumerService.java(11 hunks)
🔇 Additional comments (3)
apollo-portal/src/main/java/com/ctrip/framework/apollo/openapi/service/ConsumerService.java (3)
141-150: LGTM! Well-structured token retrieval logic
The refactoring improves code organization by:
- Separating token retrieval logic
- Adding proper null checks
- Including token expiration validation
123-126:
Add parameter validation for rateLimit
The method should validate the rateLimit parameter at the entry point to ensure data integrity.
Add parameter validation:
public ConsumerToken generateAndSaveConsumerToken(Consumer consumer, Integer rateLimit, Date expires) {
Preconditions.checkArgument(consumer != null, "Consumer can not be null");
+ Preconditions.checkArgument(expires != null, "Expires date can not be null");
+ Preconditions.checkArgument(rateLimit == null || rateLimit >= 0, "Rate limit must be non-negative");Likely invalid or redundant comment.
Line range hint 123-150: Verify rate limit enforcement in ConsumerAuthenticationFilter
The rate limit implementation looks good, but we should verify that it's properly enforced in the authentication filter.
✅ Verification successful
Rate limit is properly enforced in ConsumerAuthenticationFilter
The rate limit implementation is correctly enforced in the ConsumerAuthenticationFilter through:
- A token-based rate limiter cache using Guava's RateLimiter
- Rate limit validation in doFilterInternal:
- Retrieves rate limit from ConsumerToken
- Creates/gets RateLimiter for the token
- Returns HTTP 429 (Too Many Requests) when limit exceeded
- Proper warm-up period handling and error logging
The implementation is also well-tested as evidenced by ConsumerAuthenticationFilterTest which verifies both successful and failed rate limiting scenarios.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Search for rate limit enforcement in ConsumerAuthenticationFilter
ast-grep --pattern 'class ConsumerAuthenticationFilter {
$$$
doFilterInternal($$$) {
$$$
getRateLimit($$$)
$$$
}
$$$
}'
# Search for rate limit usage in tests
rg -l "rateLimit.*ConsumerAuthenticationFilter" -t java
Length of output: 199
Script:
#!/bin/bash
# Let's first find the ConsumerAuthenticationFilter file
fd "ConsumerAuthenticationFilter.java"
# Search for any rate limit related code in authentication filter
rg -A 5 "rateLimit|RateLimit" --type java
# Search for any token validation in filter
ast-grep --pattern 'class ConsumerAuthenticationFilter {
$$$
doFilterInternal($$$) {
$$$
}
}'
Length of output: 30891
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Outside diff range and nitpick comments (2)
apollo-portal/src/test/java/com/ctrip/framework/apollo/openapi/service/ConsumerServiceTest.java (2)
280-280: Clarify the relationship between rate limit and application permissionsThe test sets rateLimit=0 but doesn't explicitly test rate limiting behavior. Consider:
- Documenting why rateLimit is set to 0 in both positive and negative test cases
- Extracting common test setup into helper methods
- Adding test cases that verify the relationship between rate limits and application permissions
Example helper method:
private ConsumerToken createConsumerToken(String token, int rateLimit) { ConsumerToken consumerToken = new ConsumerToken(); consumerToken.setToken(token); consumerToken.setRateLimit(rateLimit); return consumerToken; }
Line range hint
254-280: Enhance test coverage for the rate limiting featureThe current tests only verify the data model changes for rate limiting. Consider adding:
- Integration tests that verify rate limit enforcement in ConsumerAuthenticationFilter
- Test cases for concurrent request handling
- Tests for rate limit configuration changes
- Performance tests to verify impact of rate limiting checks
- Edge cases:
- Rate limit updates while requests are in-flight
- Behavior when rate limit is changed
- System behavior under high load with rate limiting
This will ensure the complete rate limiting feature works as expected across all components.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (1)
apollo-portal/src/test/java/com/ctrip/framework/apollo/openapi/service/ConsumerServiceTest.java(2 hunks)
🔇 Additional comments (1)
apollo-portal/src/test/java/com/ctrip/framework/apollo/openapi/service/ConsumerServiceTest.java (1)
254-254: 🛠️ Refactor suggestion
Add dedicated test coverage for rate limiting functionality
While the rate limit is being set here, this test focuses on application creation permissions rather than rate limiting behavior. Consider adding dedicated test cases that verify:
- Rate limit enforcement
- Different rate limit values (zero, positive, maximum allowed)
- Rate limit validation
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## master #5267 +/- ##
============================================
+ Coverage 51.20% 51.35% +0.14%
- Complexity 2097 2126 +29
============================================
Files 393 393
Lines 12438 12503 +65
Branches 1229 1239 +10
============================================
+ Hits 6369 6421 +52
- Misses 5681 5690 +9
- Partials 388 392 +4 ☔ View full report in Codecov by Sentry. 🚨 Try these New Features:
|
What's the purpose of this PR
Add current limiting function to ConsumerToken
Which issue(s) this PR fixes:
Fixes #5244
Brief changelog
XXXXX
Follow this checklist to help us incorporate your contribution quickly and easily:
mvn clean testto make sure this pull request doesn't break anything.CHANGESlog.Summary by CodeRabbit
Release Notes for Apollo Version 2.4.0
New Features
Bug Fixes
Documentation
Enhancements