Skip to content

fix!: request Redis | RedisCluster in the Dishka provider and read per slot on a cluster - #33

Merged
AlexeyShalaev merged 1 commit into
masterfrom
fix/dishka-redis-union-key
Sep 14, 2026
Merged

AlexeyShalaev merged 1 commit into
masterfrom
fix/dishka-redis-union-key

Conversation

@AlexeyShalaev

Copy link
Copy Markdown
Member

Summary

AsyncRedisIdempotencyProvider asked for redis.asyncio.Redis; redis_client_kit.providers.AsyncRedisProvider provides AsyncRedisClient = Redis | RedisCluster. Dishka resolves by exact key, so make_async_container(AsyncRedisProvider(), AsyncRedisIdempotencyProvider(), ...) failed at construction with GraphMissingFactoryError, and the two org libraries that both talk to Redis could not be wired from the shipped pieces.

The provider now requests Redis | RedisCluster. types.UnionType compares structurally, and DependencyKey(Redis | RedisCluster) built here equals the one built from redis_client_kit.AsyncRedisClient, so no shared alias and no runtime dependency on redis-client-kit is needed. The test group gains redis-client-kit[providers,settings] so the wiring from the issue is tested against the real AsyncRedisProvider, not a look-alike.

Accepting the union had to be honest, so I went through every command the repository issues. get, save, replace, delete are single-key; save_many is a pipeline(transaction=False) the async RedisCluster accepts; delete_many and the expired-key cleanup send multi-key DEL, which redis-py's cluster client splits per slot itself. The one gap was get_many: a single MGET cannot span hash slots and RedisCluster._determine_slot raises RedisClusterException("MGET - all keys must map to the same key slot") — and keys of one operation do land in different slots. On a RedisCluster the repository now calls mget_nonatomic (one MGET per slot, values in input order), so the documented cluster compatibility is true for every method.

Breaking change

A provider of your own annotated -> Redis no longer matches: the container fails at construction with the same GraphMissingFactoryError, now naming Redis | RedisCluster. Change the annotation to -> Redis | RedisCluster. The docs example and docs/agents.md (rule 25) say so.

Rejected

  • A client_type= constructor argument with Redis kept as the default: no break, but the shipped pieces still would not compose by default, which is the complaint.
  • A structural protocol over the commands used: Dishka would still need exactly that key provided, so it does not meet redis-client-kit's union without an adapter.
  • Stating that the repository needs a non-cluster Redis and shipping a checked narrowing: cluster is supportable once get_many reads per slot, so refusing it in the type would be the lie in the other direction.

Nothing to mirror on the sync side: idempotency-kit is async only and redis-client-kit ships no sync provider. No change on the redis-client-kit side.

Verification

  • make check: ruff check, ruff format --check, mypy — all clean.
  • uv run pytest -m unit: 171 passed (169 before, two new).
  • uv run pytest -m integration (Docker): 7 passed.
  • With idempotency_kit/dishka/aio/redis.py and .../repository.py swapped for their origin/master versions, the two new tests fail — GraphMissingFactoryError: Cannot find factory for (Redis, component='') for the wiring test, IdempotencyStorageError: Redis storage failure during mget for the cluster test, because the old code calls mget — and pass with them restored.
  • The reporter's script (AsyncRedisProvider(check_health_on_startup=False) + the three shipped providers, redis-client-kit 0.2.0, no server) fails on master with Cannot find factory for (Redis, component='') and prints built: RedisAsyncIdempotencyRepository client: Redis on this branch.

Type of change

  • Bug fix
  • Breaking change
  • Documentation update

Checklist

  • Tests added or updated
  • make check passes locally (ruff + mypy)
  • CHANGELOG.md updated under [Unreleased] — release-please writes it from the commit
  • Documentation updated (if the public API changed), docs/agents.md included

Related issues

Closes #32

…r slot on a cluster

AsyncRedisIdempotencyProvider asked for redis.asyncio.Redis while
redis_client_kit.providers.AsyncRedisProvider provides the union alias
AsyncRedisClient = Redis | RedisCluster. Dishka resolves by exact key, so a
container holding both never built. The provider now requests the union,
which is structurally the same key, so no shared alias or dependency is
needed; the test group gains redis-client-kit so the wiring from the issue
is tested against the real provider.

The repository takes the union too, and get_many uses mget_nonatomic on a
RedisCluster: a single MGET cannot span hash slots there, so the documented
cluster compatibility was false for that one method.

BREAKING CHANGE: AsyncRedisIdempotencyProvider requests
redis.asyncio.Redis | RedisCluster, the key redis_client_kit.AsyncRedisClient
names. A provider of your own annotated `-> Redis` no longer matches and the
container fails at construction with GraphMissingFactoryError; annotate it
`-> Redis | RedisCluster`.

Closes #32
@codecov

codecov Bot commented Sep 14, 2026

Copy link
Copy Markdown

Codecov Report

✅ All modified and coverable lines are covered by tests.

📢 Thoughts on this report? Let us know!

@AlexeyShalaev

Copy link
Copy Markdown
Member Author

Reporter here — checked the load-bearing claim and it holds.

The design rests on DependencyKey(Redis | RedisCluster) built inside this package equalling the one
built from redis_client_kit.AsyncRedisClient, with no shared alias. Against the released
redis-client-kit 0.2.0 and dishka 1.9.1:

>>> local_union = Redis | RedisCluster
>>> kit_alias = redis_client_kit.AsyncRedisClient
>>> local_union is kit_alias
False
>>> DependencyKey(local_union, None) == DependencyKey(kit_alias, None)
True
>>> hash(DependencyKey(local_union, None)) == hash(DependencyKey(kit_alias, None))
True

So no runtime dependency on redis-client-kit is needed, as the summary says.

On the breaking change: it does not reach us. Nothing in our tree provides -> Redis — our local
provider requested the union already and passed the client on with cast(Any, redis) precisely
because of this mismatch, with a comment pointing at it. That cast and the provider around it are
deleted by this PR, which is the outcome we wanted from the report.

The get_many finding is the part we had not noticed: we would have shipped a cluster config that
raised on multi-slot keys and called it supported. Going through every command before widening the
type was the right order.

@AlexeyShalaev
AlexeyShalaev merged commit d31f171 into master Sep 14, 2026
7 checks passed
@AlexeyShalaev
AlexeyShalaev deleted the fix/dishka-redis-union-key branch September 14, 2026 12:44
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

The dishka provider asks for Redis, redis-client-kit provides Redis | RedisCluster: the container never builds

1 participant