Skip to content

Show host and db in Redis check __repr__ (with cluster support) #663

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
codingjoe merged 6 commits into from
Feb 23, 2026
Merged

Show host and db in Redis check __repr__ (with cluster support) #663

Show file tree
Hide file tree
Changes from 3 commits
Commits
Show all changes
6 commits
Select commit Hold shift + click to select a range
ba3b28b
Initial plan
Copilot Feb 23, 2026
2029ce4
Fix misleading comment in __repr__ and add tests for Redis __repr__
Copilot Feb 23, 2026
ec4b6ce
Add test for RedisCluster client __repr__ fallback
Copilot Feb 23, 2026
56e256b
Implement RedisCluster host display in __repr__
Copilot Feb 23, 2026
d1ff5bc
Add security tests to ensure passwords and usernames are excluded fro…
Copilot Feb 23, 2026
8223212
[pre-commit.ci] auto fixes from pre-commit.com hooks
pre-commit-ci[bot] Feb 23, 2026
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
1 change: 0 additions & 1 deletion health_check/contrib/redis.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -55,7 +55,6 @@ class Redis(HealthCheck):

def __repr__(self):
# include client host name and logical database number to identify them
# Create a new client for this health check request
if self.client_factory is not None:
client = self.client_factory()
else:
Comment on lines 58 to 60
Copy link

Copilot AI Feb 23, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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:

  1. Cache the repr string after the first call to avoid creating multiple clients
  2. Store host/db information during initialization or first run() call instead of creating clients in repr
  3. 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.

Copilot uses AI. Check for mistakes.
Expand Down
61 changes: 61 additions & 0 deletions tests/contrib/test_redis.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -149,6 +149,67 @@ async def test_redis__validation_neither_param(self):
):
RedisHealthCheck()

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()"
Comment on lines +152 to +201
Copy link

Copilot AI Feb 23, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copilot uses AI. Check for mistakes.

def test_redis__repr_cluster_client(self):
"""Verify repr falls back gracefully for RedisCluster clients without connection_pool."""
from redis.asyncio import RedisCluster

check = RedisHealthCheck(
client_factory=lambda: RedisCluster(host="clusterhost", port=7000)
)
# RedisCluster has no connection_pool attribute, so __repr__ should fall back
assert repr(check) == "Redis()"
Copy link
Copy Markdown
Owner

@codingjoe codingjoe Feb 23, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@copilot let's actually implement support clusters and show the hosts.

Copy link
Copy Markdown
Contributor Author

Copilot AI Feb 23, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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()".


@pytest.mark.integration
@pytest.mark.asyncio
async def test_redis__real_connection(self):
Expand Down