fix: handle None rate_limit_timeout in disable_for_rate_limit(). Clos…#3378
fix: handle None rate_limit_timeout in disable_for_rate_limit(). Clos…#3378HarshitPal25 wants to merge 2 commits intointelowlproject:developfrom
Conversation
There was a problem hiding this comment.
Pull request overview
This PR fixes a crash in OrganizationPluginConfiguration.disable_for_rate_limit() that occurs when rate_limit_timeout is None. The method previously attempted to add a datetime and None together, causing a TypeError and leaving plugins permanently disabled without any automated recovery mechanism.
Changes:
- Added guard clause to handle None/empty rate_limit_timeout before attempting datetime arithmetic
- Logs warning when rate_limit_timeout is not configured
- Sets informative disabled_comment explaining manual re-enable is required
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
api_app/models.py
Outdated
| "Disabling the plugin, but no re-enable task will be scheduled." | ||
| ) | ||
| self.disabled_comment = ( | ||
| f"Rate limit hit at {now().strftime('%d %m %Y: %H %M %S')}.\n" |
There was a problem hiding this comment.
The date format used here ('%d %m %Y: %H %M %S') is inconsistent with the format used in disable_manually() and enable_manually() methods in the same class (lines 1130 and 1142), which use '%Y-%m-%d %H:%M:%S'. While this format is consistent with the existing disable_for_rate_limit() code on lines 1095 and 1097, there's a broader inconsistency within the OrganizationPluginConfiguration class that should be addressed for maintainability. Consider standardizing the date format across all methods in this class to '%Y-%m-%d %H:%M:%S' which is the ISO 8601 standard format and more widely used throughout the codebase.
| if not self.rate_limit_timeout: | ||
| logger.warning( | ||
| f"{self} hit a rate limit but rate_limit_timeout isn't configured. " | ||
| "Disabling the plugin, but no re-enable task will be scheduled." | ||
| ) | ||
| self.disabled_comment = ( | ||
| f"Rate limit hit at {now().strftime('%d %m %Y: %H %M %S')}.\n" | ||
| "No re-enable timeout was configured — please re-enable manually." | ||
| ) | ||
| self.save() | ||
| return |
There was a problem hiding this comment.
The fix correctly handles the case when rate_limit_timeout is None, but there's no test coverage for this scenario. The existing test in tests/api_app/test_models.py (test_disable_for_rate_limit) only tests the case where rate_limit_timeout has a value (datetime.timedelta(minutes=1)). Consider adding a test case that verifies the behavior when rate_limit_timeout is None to ensure the fix works as expected and prevents regressions.
When rate_limit_timeout is None, the plugin is now disabled gracefully without scheduling a re-enable task. This prevents errors and improves reliability.
Changes:
• Standardized date format across all rate limit methods to ISO 8601 format ('%Y-%m-%d %H:%M:%S') for consistency with disable_manually() and enable_manually()
• Added test case for disable_for_rate_limit() when rate_limit_timeout is None to ensure proper behavior and prevent regressions
• Follows project standards and passes all linting checks
|
PTAL @mlodic |
Description
OrganizationPluginConfiguration.disable_for_rate_limit()crashes with aTypeErrorwhen
rate_limit_timeoutisNone(not configured). This PR adds a guard to handlethat case gracefully.
Closes #3366
Type of change
Checklist
developRuff) gave 0 errors. If you have correctly installed pre-commit, it does these checks and adjustments on your behalf.