fix: do not fail connection setup when ACL denies both CLIENT SETINFO and ECHO - #679
Open
Aryan-Pardeshi wants to merge 2 commits into
Open
fix: do not fail connection setup when ACL denies both CLIENT SETINFO and ECHO#679Aryan-Pardeshi wants to merge 2 commits into
Aryan-Pardeshi wants to merge 2 commits into
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Fixes #673
Client identification is best-effort telemetry, so it should never be the reason a connection cannot be created. Today it can be.
NoPermissionErrorsubclassesResponseError, so an ACL user deniedCLIENT SETINFOlands in theexcept ResponseErrorbranch as intended — but theechofallback inside that branch is not itself guarded. If the same ACL also deniesECHO, the secondNoPermissionErrorpropagates straight out of connection setup, and the user sees a confusing message about theechocommand when all they did was connect.The same unguarded-fallback shape appears in four places in
redisvl/redis/connection.py, so all four are fixed:RedisConnectionFactory.get_redis_connectionRedisConnectionFactory._get_aredis_connectionRedisConnectionFactory.validate_sync_redisRedisConnectionFactory.validate_async_redisEach fallback
echois now wrapped in its owntry/except ResponseErrorthat logs at debug level and continues. No other behaviour changes: a client whose ACL permits either command still gets its library name set exactly as before.tests/unit/test_connection_acl.pycovers all four entry points. The twovalidate_*tests construct realRedis/AsyncRedisinstances rather than mocks, becausevalidate_sync_redisgates onissubclass(type(redis_client), ...), which aMagicMock(spec=Redis)does not satisfy — construction opens no socket and both commands are patched, so no server is contacted.Verified the tests fail against unmodified
main(4 failed) and pass with the change (4 passed). Full unit suite is green, andisort --profile black/black --target-version py311report no changes.Happy to fold the four guards into one shared private helper if you would prefer that shape — I kept the diff local so the fix is easy to read.
Note
Low Risk
Narrow error-handling change around best-effort client identification; successful paths unchanged and covered by new unit tests.
Overview
Connection setup no longer fails when Redis ACL denies both
CLIENT SETINFOand theECHOfallback used for library-name telemetry.After
client_setinforaisesResponseError, the existingechofallback inget_redis_connection,_get_aredis_connection,validate_sync_redis, andvalidate_async_redisis now wrapped in its owntry/except ResponseError. DeniedECHOis logged at debug and setup continues; behavior is unchanged when either command is allowed.Adds
tests/unit/test_connection_acl.pywith four unit tests (sync/async factory and validate paths) asserting both commands are attempted without raising.Reviewed by Cursor Bugbot for commit 02d5409. Bugbot is set up for automated code reviews on this repo. Configure here.