Show host and db in Redis check __repr__#662
Conversation
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## main #662 +/- ##
=========================================
Coverage 100.00% 100.00%
=========================================
Files 13 13
Lines 749 765 +16
=========================================
+ Hits 749 765 +16
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:
|
There was a problem hiding this comment.
Pull request overview
This PR addresses issue #659 by implementing a custom __repr__ method for the Redis health check to display the host and database number, allowing users to distinguish between multiple Redis health check instances in the output.
Changes:
- Added a
__repr__method to the Redis health check class that extracts host and db from the Redis client's connection pool and formats them in a readable string representation
|
|
||
| def __repr__(self): | ||
| # include client host name and logical database number to identify them | ||
| # Create a new client for this health check request |
There was a problem hiding this comment.
The comment says "Create a new client for this health check request" but this is misleading - we're in the __repr__ method which is for string representation, not for running a health check. This will confuse future maintainers about the purpose of this code.
| # Create a new client for this health check request | |
| # Create a client instance to derive connection details for representation |
|
@copilot address the unresolved review comments. |
|
@codingjoe I've opened a new pull request, #663, to work on those changes. Once the pull request is ready, I'll request review from you. |
Multiple Redis health check instances are indistinguishable in health
check output. This adds `host`, `db`, and cluster node information to
`Redis.__repr__` so each instance is identifiable in JSON, text, and
feed outputs.
## Changes
- **`Redis.__repr__`**: Extracts `host` and `db` from
`connection_pool.connection_kwargs` for standard Redis clients using
EAFP; for `RedisCluster` clients, extracts startup node names
(`host:port`) from `client.startup_nodes`; falls back to default
dataclass repr for clients that don't expose these (e.g. Sentinel)
- **Tests**: Eight new non-mocked tests covering standard client,
`from_url`, deprecated `client` param, Sentinel fallback, RedisCluster
with startup node hosts, and explicit security tests ensuring passwords
and usernames are never exposed in the repr output
## Security
Credentials are never included in the repr output. The implementation
only extracts `host` and `db` keys explicitly from `connection_kwargs`,
and uses `node.name` (`host:port`) for cluster nodes. Security
regression tests enforce this for all client types:
- Standard client with `password=` kwarg
- Credentials embedded in a Redis URL (`redis://user:pass@host/db`)
- `RedisCluster` with `password=` and `username=` kwargs
## Example
```python
from redis.asyncio import Redis as RedisClient, RedisCluster
from redis.asyncio.cluster import ClusterNode
from health_check.contrib.redis import Redis
check = Redis(client_factory=lambda: RedisClient.from_url("redis://localhost:6379/1"))
repr(check) # "Redis(client=RedisClient(host=localhost, db=1))"
# RedisCluster shows startup node host:port names
cluster_check = Redis(client_factory=lambda: RedisCluster(
startup_nodes=[ClusterNode("node1", 7000), ClusterNode("node2", 7001)]
))
repr(cluster_check) # "Redis(client=RedisCluster(hosts=['node1:7000', 'node2:7001']))"
# Sentinel falls back gracefully — host/db not accessible via connection_pool
sentinel_check = Redis(client_factory=lambda: Sentinel([("localhost", 26379)]).master_for("mymaster"))
repr(sentinel_check) # "Redis()"
# Credentials are never included
secret_check = Redis(client_factory=lambda: RedisClient.from_url("redis://admin:secret@host/0"))
repr(secret_check) # "Redis(client=RedisClient(host=host, db=0))" — no credentials
```
<!-- START COPILOT CODING AGENT TIPS -->
---
💬 We'd love your input! Share your thoughts on Copilot coding agent in
our [2 minute survey](https://gh.io/copilot-coding-agent-survey).
---------
Co-authored-by: copilot-swe-agent[bot] <198982749+Copilot@users.noreply.github.com>
Co-authored-by: codingjoe <1772890+codingjoe@users.noreply.github.com>
Co-authored-by: pre-commit-ci[bot] <66853113+pre-commit-ci[bot]@users.noreply.github.com>
|
pre-commit.ci run |
| def __repr__(self): | ||
| # include client host name and logical database number to identify them | ||
| if self.client_factory is not None: | ||
| client = self.client_factory() | ||
| else: | ||
| # Use the deprecated client parameter (user manages lifecycle) | ||
| client = self.client | ||
|
|
||
| try: | ||
| conn_kwargs = client.connection_pool.connection_kwargs | ||
| host = conn_kwargs["host"] | ||
| db = conn_kwargs["db"] | ||
| return f"Redis(client=RedisClient(host={host}, db={db}))" | ||
| except (AttributeError, KeyError): | ||
| pass | ||
|
|
||
| try: | ||
| hosts = [node.name for node in client.startup_nodes] | ||
| return f"Redis(client=RedisCluster(hosts={hosts!r}))" | ||
| except AttributeError: | ||
| return super().__repr__() |
There was a problem hiding this comment.
The repr method creates a Redis client from client_factory (line 59) but never closes it, causing a resource leak. The run() method properly closes all factory-created clients (line 127), establishing the pattern that factory-created clients must be closed.
Since repr is synchronous and clients require async cleanup (aclose()), this approach is fundamentally incompatible with proper resource management. The repr method should not create clients at all.
Consider storing connection information (host, db) during initialization or caching it on first access in an instance variable. For the deprecated client parameter, you can safely access connection details since the client lifecycle is managed externally.
Fix #659