fix: handle RateLimitBlockedException in ExceptionHandlers - #3472
Conversation
The strike-based rate limiting (#3451) introduced RateLimitBlockedException but only handled it in the global filters. When thrown from controller-level rate limiting (e.g. export endpoint), it was unhandled, resulting in HTTP 500 errors reported to Sentry. Add an @ExceptionHandler that returns HTTP 444 with no body, matching the existing behavior in GlobalIpRateLimitFilter and GlobalUserRateLimitFilter.
📝 WalkthroughWalkthroughAdds a global exception handler for Changes
Estimated code review effort🎯 2 (Simple) | ⏱️ ~10 minutes Possibly related PRs
Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
There was a problem hiding this comment.
🧹 Nitpick comments (1)
backend/app/src/main/kotlin/io/tolgee/ExceptionHandlers.kt (1)
260-264: Extract the magic status444to a shared constant and add explanatory comment.The handler correctly mirrors the filter behavior (HTTP 444, no body). However, this non-standard status code is already used in
GlobalIpRateLimitFilter(line 47),GlobalUserRateLimitFilter(line 45), and test expectations—all hardcoded as444. Define a shared constant (e.g.,const val RATE_LIMIT_BLOCKED_STATUS = 444) to keep them synchronized. Additionally, the filters both include explanatory comments about why 444 is used (nginx "No Response" to save bandwidth), but the exception handler lacks this context. Add the same clarifying comment here.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@backend/app/src/main/kotlin/io/tolgee/ExceptionHandlers.kt` around lines 260 - 264, Extract the magic number 444 into a shared constant (e.g., const val RATE_LIMIT_BLOCKED_STATUS = 444) placed in a common accessible location used by GlobalIpRateLimitFilter, GlobalUserRateLimitFilter and tests, then replace the hardcoded 444 in the handleRateLimitBlocked function (and any other occurrences) to use RATE_LIMIT_BLOCKED_STATUS; also add the same explanatory comment used in the filters (explaining nginx "No Response" / saving bandwidth) immediately above the handler so the rationale is documented next to handleRateLimitBlocked and kept consistent across the filters and tests.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@backend/app/src/main/kotlin/io/tolgee/ExceptionHandlers.kt`:
- Around line 260-264: Extract the magic number 444 into a shared constant
(e.g., const val RATE_LIMIT_BLOCKED_STATUS = 444) placed in a common accessible
location used by GlobalIpRateLimitFilter, GlobalUserRateLimitFilter and tests,
then replace the hardcoded 444 in the handleRateLimitBlocked function (and any
other occurrences) to use RATE_LIMIT_BLOCKED_STATUS; also add the same
explanatory comment used in the filters (explaining nginx "No Response" / saving
bandwidth) immediately above the handler so the rationale is documented next to
handleRateLimitBlocked and kept consistent across the filters and tests.
There was a problem hiding this comment.
🧹 Nitpick comments (1)
ee/backend/tests/src/test/kotlin/io/tolgee/ee/selfHostedLimitsAndReporting/CreditLimitTest.kt (1)
110-112:firstOrNull()produces a misleading failure when noerrorMessagechunk is present.If the NDJSON response contains no chunk with an
"errorMessage"key,errorMessageisnulland the assertion on line 112 reports"expected <errorCode> but was <null>"— hiding the real problem (missing field) behind a value mismatch. Usefirst()so the failure immediately says "no such element" at the extraction step, not at the equality check.♻️ Proposed fix
- val errorMessage = - parsed.drop(1).filterIsInstance<Map<*, *>>().mapNotNull { it["errorMessage"] }.firstOrNull() - errorMessage.assert.isEqualTo(errorCode) + val errorMessage = + parsed.drop(1).filterIsInstance<Map<*, *>>().mapNotNull { it["errorMessage"] }.first() + errorMessage.assert.isEqualTo(errorCode)🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@ee/backend/tests/src/test/kotlin/io/tolgee/ee/selfHostedLimitsAndReporting/CreditLimitTest.kt` around lines 110 - 112, The extraction of the errorMessage currently uses firstOrNull(), which masks missing error chunks by returning null and causing a misleading equality failure in errorMessage.assert.isEqualTo(errorCode); change the terminal call to first() on the pipeline parsed.drop(1).filterIsInstance<Map<*, *>>().mapNotNull { it["errorMessage"] } so that a missing "errorMessage" immediately throws "no such element" during extraction and surfaces the real issue.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In
`@ee/backend/tests/src/test/kotlin/io/tolgee/ee/selfHostedLimitsAndReporting/CreditLimitTest.kt`:
- Around line 110-112: The extraction of the errorMessage currently uses
firstOrNull(), which masks missing error chunks by returning null and causing a
misleading equality failure in errorMessage.assert.isEqualTo(errorCode); change
the terminal call to first() on the pipeline
parsed.drop(1).filterIsInstance<Map<*, *>>().mapNotNull { it["errorMessage"] }
so that a missing "errorMessage" immediately throws "no such element" during
extraction and surfaces the real issue.
0404abe to
2d4e4fb
Compare
There was a problem hiding this comment.
🧹 Nitpick comments (1)
ee/backend/tests/src/test/kotlin/io/tolgee/ee/selfHostedLimitsAndReporting/CreditLimitTest.kt (1)
110-116: Consider guarding againstnullbefore asserting.
firstOrNull()makeserrorMessagenullable. If noMapitem carrying"errorMessage"is found, the assertion at line 116 will fail withExpected: null to be equal to: <errorCode>, which conflates "key absent" with "wrong value". A simple non-null check would produce a clearer failure signal:♻️ Proposed improvement
- val errorMessage = - parsed - .drop(1) - .filterIsInstance<Map<*, *>>() - .mapNotNull { it["errorMessage"] } - .firstOrNull() - errorMessage.assert.isEqualTo(errorCode) + val errorMessage = + parsed + .drop(1) + .filterIsInstance<Map<*, *>>() + .mapNotNull { it["errorMessage"] } + .firstOrNull() + checkNotNull(errorMessage) { "No errorMessage found in NDJSON items" } + errorMessage.assert.isEqualTo(errorCode)🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@ee/backend/tests/src/test/kotlin/io/tolgee/ee/selfHostedLimitsAndReporting/CreditLimitTest.kt` around lines 110 - 116, The extracted errorMessage from parsed.drop(1)...firstOrNull() is nullable, so before calling errorMessage.assert.isEqualTo(errorCode) add a null-guard to fail with a clear message if the key is missing; locate the snippet in CreditLimitTest.kt where variable errorMessage is computed and either assertNotNull(errorMessage) (or throw a descriptive assertion failure like "expected errorMessage present but was null") and then compare its value to errorCode to avoid conflating "missing key" with "wrong value".
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In
`@ee/backend/tests/src/test/kotlin/io/tolgee/ee/selfHostedLimitsAndReporting/CreditLimitTest.kt`:
- Around line 110-116: The extracted errorMessage from
parsed.drop(1)...firstOrNull() is nullable, so before calling
errorMessage.assert.isEqualTo(errorCode) add a null-guard to fail with a clear
message if the key is missing; locate the snippet in CreditLimitTest.kt where
variable errorMessage is computed and either assertNotNull(errorMessage) (or
throw a descriptive assertion failure like "expected errorMessage present but
was null") and then compare its value to errorCode to avoid conflating "missing
key" with "wrong value".
Summary
@ExceptionHandlerforRateLimitBlockedExceptioninExceptionHandlers.ktGlobalIpRateLimitFilterandGlobalUserRateLimitFilterProblem
The strike-based rate limiting (#3451, commit 68b5250) introduced
RateLimitBlockedExceptionbut only caught it in the global servlet filters. When thrown from controller-level rate limiting (e.g. the export endpoint'scheckPerUserRateLimit), it bubbled up as an unhandled exception — causing HTTP 500 errors and Sentry noise since Feb 6th.Test plan
RateLimitBlockedExceptionSummary by CodeRabbit
Bug Fixes
Tests