fix: refactor user Redis keys to prevent WRONGTYPE error in admin panel#816
fix: refactor user Redis keys to prevent WRONGTYPE error in admin panel#816
Conversation
The list_all_users method was scanning user:* which also matched
user:{username}:visited_rooms keys (Redis Sets). Calling hgetall()
on Sets causes WRONGTYPE errors.
Refactored key structure to users:{type}:{username} format:
- users:data:{username} - user data hash
- users:admin:{username} - admin status
- users:visited:{username} - visited rooms set
This allows efficient scanning of users:data:* without conflicts.
🤖 Generated with [Claude Code](https://claude.com/claude-code)
Co-Authored-By: Claude Opus 4.5 <noreply@anthropic.com>
WalkthroughThe changes refactor Redis key management by introducing a Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 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[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
📜 Review details
Configuration used: defaults
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (9)
src/zndraw/app/redis_keys.py(2 hunks)src/zndraw/services/admin_service.py(5 hunks)src/zndraw/services/client_service.py(4 hunks)src/zndraw/services/user_service.py(1 hunks)tests/services/test_client_service.py(9 hunks)tests/test_admin_service.py(2 hunks)tests/test_auth_integration.py(2 hunks)tests/test_room_management.py(2 hunks)tests/test_user_service.py(8 hunks)
🧰 Additional context used
📓 Path-based instructions (2)
**/*.py
📄 CodeRabbit inference engine (AGENTS.md)
**/*.py: If sensible, implement collections.abc interfaces for classes, such as MutableMapping or MutableSequence
Use numpy style docstrings
Docstrings must be concise and to the point
Use type hints wherever possible. Importtyping as tif necessary, but uselist[int|float] | Noneinstead oft.Optional[t.List[int|float]]
Imports should always be at the top of the file
Files:
src/zndraw/services/user_service.pytests/test_room_management.pytests/test_auth_integration.pysrc/zndraw/services/client_service.pytests/test_admin_service.pysrc/zndraw/app/redis_keys.pysrc/zndraw/services/admin_service.pytests/test_user_service.pytests/services/test_client_service.py
**/test_*.py
📄 CodeRabbit inference engine (AGENTS.md)
**/test_*.py: Usepytest.mark.parametrizeto avoid code duplication in tests
Tests should be very specific and test only one thing
Avoid complex test setups
Each test must be a function, not a method of a class
Files:
tests/test_room_management.pytests/test_auth_integration.pytests/test_admin_service.pytests/test_user_service.pytests/services/test_client_service.py
🧬 Code graph analysis (8)
src/zndraw/services/user_service.py (1)
src/zndraw/app/redis_keys.py (3)
UserKeys(484-530)data_pattern(513-515)username_from_data_key(523-525)
tests/test_room_management.py (1)
src/zndraw/app/redis_keys.py (2)
UserKeys(484-530)admin_key(504-506)
tests/test_auth_integration.py (1)
src/zndraw/app/redis_keys.py (3)
UserKeys(484-530)hash_key(500-502)hash_key(592-594)
src/zndraw/services/client_service.py (1)
src/zndraw/app/redis_keys.py (4)
UserKeys(484-530)hash_key(500-502)hash_key(592-594)visited_rooms(508-510)
tests/test_admin_service.py (2)
src/zndraw/app/redis_keys.py (2)
UserKeys(484-530)admin_key(504-506)src/zndraw/services/admin_service.py (4)
revoke_admin(93-103)is_admin(105-128)AdminService(15-146)grant_admin(81-91)
src/zndraw/services/admin_service.py (1)
src/zndraw/app/redis_keys.py (4)
UserKeys(484-530)admin_key(504-506)admin_pattern(518-520)username_from_admin_key(528-530)
tests/test_user_service.py (1)
src/zndraw/app/redis_keys.py (4)
UserKeys(484-530)hash_key(500-502)hash_key(592-594)visited_rooms(508-510)
tests/services/test_client_service.py (1)
src/zndraw/app/redis_keys.py (3)
UserKeys(484-530)hash_key(500-502)hash_key(592-594)
🪛 Ruff (0.14.8)
tests/test_admin_service.py
268-268: Possible hardcoded password assigned to argument: "admin_password"
(S106)
tests/test_user_service.py
139-139: Possible hardcoded password assigned to: "password"
(S105)
162-162: Possible hardcoded password assigned to: "password"
(S105)
501-501: Possible hardcoded password assigned to: "same_password"
(S105)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (3)
- GitHub Check: pytest (3.11, ubuntu-latest)
- GitHub Check: pytest (3.13, ubuntu-latest)
- GitHub Check: pytest (3.12, ubuntu-latest)
🔇 Additional comments (36)
src/zndraw/app/redis_keys.py (2)
8-8: LGTM!The
ClassVarimport is correctly placed and necessary for the newUserKeysclass implementation.
483-530: LGTM! Well-designed key abstraction.The
UserKeysclass provides a clean, centralized approach to Redis key management:
ClassVarcorrectly prevents dataclass field treatmentfrozen=Trueensures immutability- Consistent key structure with clear documentation
- Type hints and docstrings throughout
src/zndraw/services/user_service.py (1)
411-415: LGTM! Correctly resolves the WRONGTYPE error.The refactoring to use
UserKeys.data_pattern()ensures thatlist_all_usersonly scans user data hashes (users:data:*) and won't accidentally match other key types likeusers:visited:*sets, which was causing the WRONGTYPE error mentioned in the PR description.src/zndraw/services/admin_service.py (5)
10-10: LGTM!Import of
UserKeysis correctly placed at the top of the file.
89-91: LGTM!Correctly uses
UserKeysabstraction for admin key management.
101-103: LGTM!Correctly uses
UserKeysabstraction for admin key management.
126-128: LGTM!Correctly uses
UserKeysabstraction for admin key lookups.
139-145: LGTM! Correctly handles admin key scanning.The refactoring properly uses
UserKeys.admin_pattern()to scan only admin keys andusername_from_admin_key()to extract usernames. The bytes-to-string decoding is correctly handled.src/zndraw/services/client_service.py (4)
11-11: LGTM!Import of
UserKeysis correctly placed at the top of the file.
41-43: LGTM!Correctly uses
UserKeysabstraction for user data hash access.
97-103: LGTM! Correctly tracks visited rooms.The refactoring properly uses
keys.visited_rooms()to maintain the set of visited rooms separately from user data hashes, which is key to preventing the WRONGTYPE error.
118-120: LGTM!Correctly uses
UserKeysabstraction for visited rooms set access.tests/test_room_management.py (1)
788-806: LGTM! Test correctly uses the new key abstraction.The test properly instantiates
UserKeysand usesadmin_key()to grant admin privileges, consistent with the refactored key management.tests/test_auth_integration.py (1)
195-213: LGTM! Test correctly uses the new key abstraction.The test properly instantiates
UserKeysand useshash_key()for Redis operations, consistent with the refactored key management.tests/services/test_client_service.py (13)
11-11: LGTM!Import of
UserKeysis correctly placed at the top of the file.
18-21: LGTM!Test correctly uses
UserKeysabstraction for assertions.
27-31: LGTM!Test correctly uses
UserKeysabstraction for assertions.
37-41: LGTM!Test correctly uses
UserKeysabstraction for assertions.
51-53: LGTM!Test correctly uses
UserKeysabstraction for assertions.
59-62: LGTM!Test correctly uses
UserKeysabstraction for assertions.
78-80: LGTM!Test correctly uses
UserKeysabstraction for assertions.
238-243: LGTM!Test correctly uses
UserKeysabstraction for assertions.
270-277: LGTM!Test correctly uses
UserKeysabstraction for assertions.
288-293: LGTM!Test correctly uses
UserKeysabstraction for assertions.
352-357: LGTM!Test correctly uses
UserKeysabstraction and validates the new key format.
387-393: LGTM!Test correctly uses
UserKeysabstraction for assertions.
433-439: LGTM!Test correctly uses
UserKeysabstraction for assertions with special characters.tests/test_user_service.py (7)
52-62: LGTM!Test correctly uses
UserKeysabstraction for assertions.
136-150: LGTM!Test correctly uses
UserKeysabstraction for password-related assertions.
158-179: LGTM!Test correctly uses
UserKeysabstraction for registration with username change.
223-246: LGTM!Test correctly uses
UserKeysabstraction to verify timestamp preservation.
362-381: LGTM!Test correctly uses
UserKeysabstraction for timestamp assertions.
467-493: LGTM! Excellent regression test for the WRONGTYPE error fix.This test validates the core issue addressed by this PR. By creating a visited_rooms set alongside user data, it ensures that
list_all_users()correctly scans onlyusers:data:*keys and doesn't attempthgetall()on set-type keys. The comment clearly explains the bug scenario.
497-522: LGTM!Test correctly uses
UserKeysabstraction for password hash assertions.tests/test_admin_service.py (2)
110-111: LGTM! Correct usage of UserKeys API.The test properly verifies the admin key format after granting and checks deletion after revoking. The validation correctly handles both string and bytes representations of "1".
Also applies to: 118-118
272-272: LGTM! Proper validation of new key format and old format absence.The test correctly validates that the new
users:admin:{username}format is used and the legacyadmin:client:{user_name}format is not present. This is a good regression test to ensure the migration is complete.Note: The duplicate import at line 263-264 was already flagged in the earlier comment.
Also applies to: 276-278
| from zndraw.app.redis_keys import UserKeys | ||
|
|
There was a problem hiding this comment.
Move import to top of file.
According to coding guidelines, imports should always be at the top of the file, not inside functions. The same import appears again at line 263, creating duplication.
Apply this diff to move the import to the file header:
"""Tests for AdminService."""
import pytest
from znsocket import MemoryStorage
from zndraw.services import AdminService
+from zndraw.app.redis_keys import UserKeysThen remove the local imports at lines 108-109 and 263-264.
As per coding guidelines, imports should always be at the top of the file.
Committable suggestion skipped: line range outside the PR's diff.
🤖 Prompt for AI Agents
In tests/test_admin_service.py around lines 108-109 and 263-264, the import
"from zndraw.app.redis_keys import UserKeys" is inside the body and duplicated;
move that import to the top-of-file imports section (file header) and remove the
two local imports at lines 108-109 and 263-264 so the module is imported only
once at the top per coding guidelines.
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #816 +/- ##
==========================================
+ Coverage 78.26% 78.29% +0.03%
==========================================
Files 153 153
Lines 18396 18461 +65
==========================================
+ Hits 14397 14454 +57
- Misses 3999 4007 +8 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
2 participants
The list_all_users method was scanning user:* which also matched user:{username}:visited_rooms keys (Redis Sets). Calling hgetall() on Sets causes WRONGTYPE errors.
Refactored key structure to users:{type}:{username} format:
This allows efficient scanning of users:data:* without conflicts.
🤖 Generated with Claude Code
Summary by CodeRabbit
✏️ Tip: You can customize this high-level summary in your review settings.