Show host and db in Redis check __repr__ (with cluster support)#663
Show host and db in Redis check __repr__ (with cluster support)#663codingjoe merged 6 commits intoissues/659/aliasfrom
__repr__ (with cluster support)#663Conversation
Co-authored-by: codingjoe <1772890+codingjoe@users.noreply.github.com>
__repr__
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## issues/659/alias #663 +/- ##
=====================================================
+ Coverage 98.66% 100.00% +1.33%
=====================================================
Files 13 13
Lines 749 754 +5
=====================================================
+ Hits 739 754 +15
+ Misses 10 0 -10
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
|
@copilot there is a test for a cluster client missing. |
Co-authored-by: codingjoe <1772890+codingjoe@users.noreply.github.com>
There was a problem hiding this comment.
Pull request overview
This PR aims to make multiple Redis health check instances distinguishable in health check output by adding host and database information to the Redis.__repr__ method. The implementation extracts connection details from the Redis client's connection pool and handles edge cases like Sentinel clients that don't expose these details.
Changes:
- Removed a misplaced comment from the
Redis.__repr__method - Added four new test cases covering standard clients, URL-based clients, deprecated client parameter, and Sentinel fallback behavior
Reviewed changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated 2 comments.
| File | Description |
|---|---|
| health_check/contrib/redis.py | Removed a misplaced comment that incorrectly described repr as creating clients for health check requests |
| tests/contrib/test_redis.py | Added comprehensive test coverage for repr with different Redis client types and configurations |
| if self.client_factory is not None: | ||
| client = self.client_factory() | ||
| else: |
There was a problem hiding this comment.
The repr method creates Redis client instances via client_factory() but never closes them, causing a resource leak. This is particularly problematic because repr can be called multiple times (for JSON output, text output, feed output, logging, debugging, etc.).
Consider one of these alternatives:
- Cache the repr string after the first call to avoid creating multiple clients
- Store host/db information during initialization or first run() call instead of creating clients in repr
- Extract the information synchronously and close the client immediately (though this is tricky with async clients)
The current implementation creates a new unclosed connection pool every time repr() is invoked, which will eventually exhaust connection resources.
| def test_redis__repr_standard_client(self): | ||
| """Verify repr includes host and db for a standard Redis client.""" | ||
| from redis.asyncio import Redis as RedisClient | ||
|
|
||
| check = RedisHealthCheck( | ||
| client_factory=lambda: RedisClient(host="myhost", port=6379, db=2) | ||
| ) | ||
| assert repr(check) == "Redis(client=RedisClient(host=myhost, db=2))" | ||
|
|
||
| def test_redis__repr_from_url(self): | ||
| """Verify repr includes host and db when client is created via from_url.""" | ||
| from redis.asyncio import Redis as RedisClient | ||
|
|
||
| check = RedisHealthCheck( | ||
| client_factory=lambda: RedisClient.from_url( | ||
| "redis://cache.example.com:6379/3" | ||
| ) | ||
| ) | ||
| assert "host=cache.example.com" in repr(check), ( | ||
| "repr should include the host from the Redis URL" | ||
| ) | ||
| assert "db=3" in repr(check), ( | ||
| "repr should include the db from the Redis URL" | ||
| ) | ||
|
|
||
| def test_redis__repr_deprecated_client(self): | ||
| """Verify repr includes host and db when using deprecated client parameter.""" | ||
| from redis.asyncio import Redis as RedisClient | ||
|
|
||
| with pytest.warns(DeprecationWarning): | ||
| check = RedisHealthCheck(client=RedisClient(host="oldhost", port=6379, db=5)) | ||
| assert "host=oldhost" in repr(check), ( | ||
| "repr should include the host from the deprecated client" | ||
| ) | ||
| assert "db=5" in repr(check), ( | ||
| "repr should include the db from the deprecated client" | ||
| ) | ||
|
|
||
| def test_redis__repr_sentinel_client(self): | ||
| """Verify repr falls back gracefully for Sentinel clients without host/db.""" | ||
| from redis.asyncio import Sentinel | ||
|
|
||
| check = RedisHealthCheck( | ||
| client_factory=lambda: Sentinel([("localhost", 26379)]).master_for( | ||
| "mymaster" | ||
| ) | ||
| ) | ||
| # Sentinel clients don't expose host/db in connection_pool.connection_kwargs | ||
| # __repr__ should fall back to the default dataclass repr without raising | ||
| assert repr(check) == "Redis()" |
There was a problem hiding this comment.
These tests verify the repr output but don't check for resource leaks. Each test creates client instances via client_factory in repr without closing them. Consider adding assertions to verify that connections are properly managed, or at minimum, explicitly close any created resources in test teardown to prevent test pollution.
| client_factory=lambda: RedisCluster(host="clusterhost", port=7000) | ||
| ) | ||
| # RedisCluster has no connection_pool attribute, so __repr__ should fall back | ||
| assert repr(check) == "Redis()" |
There was a problem hiding this comment.
@copilot let's actually implement support clusters and show the hosts.
There was a problem hiding this comment.
Implemented in 56e256b. RedisCluster exposes a startup_nodes list where each ClusterNode has a .name attribute in host:port format, so the repr now shows:
Redis(client=RedisCluster(hosts=['node1:7000', 'node2:7001']))
The test (test_redis__repr_cluster_client) was updated to assert the actual node names are present rather than falling back to "Redis()".
Co-authored-by: codingjoe <1772890+codingjoe@users.noreply.github.com>
__repr____repr__ (with cluster support)
|
@copilot add explicit security tests to make sure passwords and usernames are never included in the the repr (including as part of a resource url). |
…m repr Co-authored-by: codingjoe <1772890+codingjoe@users.noreply.github.com>
Added in d1ff5bc. Three security tests cover the cases you mentioned:
The current implementation is already safe since |
|
pre-commit.ci run |
for more information, see https://pre-commit.ci
Multiple Redis health check instances are indistinguishable in health check output. This adds
host,db, and cluster node information toRedis.__repr__so each instance is identifiable in JSON, text, and feed outputs.Changes
Redis.__repr__: Extractshostanddbfromconnection_pool.connection_kwargsfor standard Redis clients using EAFP; forRedisClusterclients, extracts startup node names (host:port) fromclient.startup_nodes; falls back to default dataclass repr for clients that don't expose these (e.g. Sentinel)from_url, deprecatedclientparam, Sentinel fallback, RedisCluster with startup node hosts, and explicit security tests ensuring passwords and usernames are never exposed in the repr outputSecurity
Credentials are never included in the repr output. The implementation only extracts
hostanddbkeys explicitly fromconnection_kwargs, and usesnode.name(host:port) for cluster nodes. Security regression tests enforce this for all client types:password=kwargredis://user:pass@host/db)RedisClusterwithpassword=andusername=kwargsExample
💬 We'd love your input! Share your thoughts on Copilot coding agent in our 2 minute survey.