fix: Read all items when a collection is empty - #524
Merged
Merged
Conversation
A recursive get in python-consul returns None, not an empty list, when no key has the requested prefix. Iterating it raised TypeError out of the Consul store, which failed the whole all-items read, so all_flags_state reported an invalid state and every flag fell back to its default. Add the empty-collection case to the shared persistent store test suite so Redis and DynamoDB are covered too.
jsonbailey
marked this pull request as ready for review
September 23, 2026 15:02
keelerm84
approved these changes
Sep 23, 2026
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.
Symptom
Reading all items of a kind from the Consul store raises:
The error escapes the store, so the whole all-items read fails rather than one
item.
all_flags_state()degrades to{"$flagsState": {}, "$valid": false}andevery flag falls back to its default. A single flag evaluation still works,
because it reads one record by key instead.
This needs no unusual data. It happens on a store the SDK has never written to,
and on any store whose records for a kind were all removed.
A side effect: the store availability monitor sees the raised error, reports the
store unavailable, then reports it available again on the next probe. So the
error also produces a stream of unavailable/available log pairs.
Root cause
_ConsulFeatureStoreCore.get_all_internalinldclient/impl/integrations/consul/consul_feature_store.pyread the collectionwith a recursive
kv.getand iterated the result directly.python-consulreturns(index, None)from a recursive get when no key has therequested prefix. Verified against a live Consul dev agent with the version this
project resolves (
python-consul1.1.0, pinned aspython-consul>=1.0.1):kv.get(prefix, recurse=True)('23344', None)('23993', [{...one record...}])Note the single-key row:
python-consulreturns a one-element list, not a barerecord, so only the zero-key case was broken.
init_internalalready guards itsown recursive get with
set(keys or []);get_all_internaldid not.The fix
Iterate
results or []. This is the same idiominit_internalalready uses forthe same client behavior.
Redis and DynamoDB were checked and are not affected:
redis.hgetallon a missing hash returns{}(and the Redis store already hasa defensive null/empty guard on top of that).
querypaginator yields one page with'Items': []rather than anull, so the loop body simply never runs.
Both were verified empirically against the local containers, not just by reading.
Evidence
Contract tests,
persistent data store/<store>/daemon mode/collection cardinality(daemon mode with the cache off, so every read reaches the store):
Before the fix — Consul:
The contract test service log for that run shows the raised error directly:
Redis and DynamoDB passed all three cardinality cases before the fix as well as
after, which confirms the bug was Consul-only.
After the fix — Consul, Redis, and DynamoDB:
Redis and DynamoDB produce the same three-case pass. The full persistence
sub-suite is also green after the fix: 101 total, 13 skipped, 88 ran, all
tests passed.
The new test
test_all_reads_empty_collectiongoes in the shared suite,ldclient/testing/integrations/persistent_feature_store_test_base.py, so everypersistent store implementation runs it rather than only the one that was broken.
This follows the same approach used for the equivalent fix in the Ruby SDK.
It inits a store with no items of the kind, then reads the collection through a
second store instance. The second instance matters: the shared suite runs each
test with caching both off and on, and
init()populates the all-items cache, soa read on the original instance would be a cache hit and would never reach
get_all_internal.With the source fix reverted, the new test fails on all four Consul
parameterizations (prefix on/off x caching on/off) with the same
TypeError.With the fix, all 12 parameterizations across the three stores pass.
Other checks
make test-all(unit suite including the database integrations): 2014 passed.make lint(mypy, isort, pycodestyle): clean. mypy reports "no issues found in229 source files".
The async side needs no matching change: there is no async Consul store, only
async_redis_feature_store.pyandasync_dynamodb_feature_store.py.No changelog or version changes, per release process.
SDK-3178
Note
Overview
Fixes Consul feature store
all()reads when a kind has no keys:python-consulreturnsNoneinstead of an empty list for a recursivekv.get, which caused aTypeErrorand brokeall_flags_state()for empty or never-written collections.get_all_internalnow iteratesresults or [], matching the existing guard ininit_internal. A shared contract testtest_all_reads_empty_collectionasserts that an initialized empty kind reads as{}via a second store instance so the path hits the backend, not the cache—covering all persistent store implementations.Reviewed by Cursor Bugbot for commit 6b2462e. Bugbot is set up for automated code reviews on this repo. Configure here.