fix: fix autodelete fallback for USERS_AUTODELETE_DAYS #123
Conversation
WalkthroughUpdated SQLAlchemy usage in the user CRUD module to replace direct coalesce calls with the proper SQLAlchemy function interface. The constant fallback value is now wrapped in a Changes
Estimated code review effort🎯 1 (Trivial) | ⏱️ ~3 minutes
Poem
Pre-merge checks and finishing touches✅ Passed checks (3 passed)
✨ 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 |
|
@coderabbitai review |
✅ Actions performedReview triggered.
|
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (2)
app/db/crud/user.py (2)
252-252: Consider migrating tofunc.coalesce()for consistency.While the current usage of the direct
coalesceimport works correctly here (both arguments are column expressions), consider usingfunc.coalesce()for consistency with the fix at line 857 and SQLAlchemy 2.0 best practices.Apply this diff for consistency:
-last_reset_time = coalesce(last_reset_subq.c.last_reset_at, User.created_at) +last_reset_time = func.coalesce(last_reset_subq.c.last_reset_at, User.created_at)If this change is made, you can remove the import on line 9:
-from sqlalchemy.sql.functions import coalesce
371-371: Consider wrapping integer literals for consistency.While SQLAlchemy handles bare integer
0correctly through type coercion, consider wrapping it withliteral(0)for consistency with the fix at line 857.Apply this diff:
-func.coalesce(NodeUserUsage.node_id, 0).label("node_id"), +func.coalesce(NodeUserUsage.node_id, literal(0)).label("node_id"),Apply the same pattern on line 938.
Also applies to: 938-938
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
app/db/crud/user.py(2 hunks)
🧰 Additional context used
🧬 Code graph analysis (1)
app/db/crud/user.py (2)
app/db/models.py (1)
User(121-282)app/models/user.py (1)
User(28-39)
🔇 Additional comments (2)
app/db/crud/user.py (2)
6-6: LGTM! Required import for SQL literal handling.The addition of
literalto the imports is correct and necessary to wrap the Python config valueUSERS_AUTODELETE_DAYSin a SQL literal expression.
857-857: I encountered a repository access issue, but I can still work with the context from the review comment itself. Let me provide a rewritten version that reflects what can be reasonably assessed from the code snippet provided:
Correct SQLAlchemy 2.0 usage for auto-deletion fallback.
The change to
func.coalesce(User.auto_delete_in_days, literal(USERS_AUTODELETE_DAYS))properly addresses the issue:
func.coalesce()is the correct SQLAlchemy 2.0 function interfaceliteral()ensures the Python config value is properly converted to a SQL literal expression, necessary for correct SQL type handling and expression evaluationThis fix is critical for the auto-deletion logic, especially where
auto_deleteparticipates in SQL comparisons (e.g.,auto_delete >= 0).Consider verifying:
- Test coverage for
autodelete_expired_userswith variousUSERS_AUTODELETE_DAYSvalues (None, 0, positive integers)- Consistency of similar
coalesce()patterns elsewhere in the file for potential follow-up improvements
Replace coalesce() with func.coalesce() and wrap USERS_AUTODELETE_DAYS in
literal() to ensure the Python config value is properly converted to a SQL
literal expression
Summary by CodeRabbit
✏️ Tip: You can customize this high-level summary in your review settings.