fix: Read all items when a kind holds a single key - #444
jsonbailey wants to merge 2 commits into
Conversation
A recursive Consul get returns the bare value string when only one key matches the prefix, instead of a list of key/value pairs. Reading all items of that kind then raised NoMethodError and failed the whole read, so every flag fell back to its default. Read with get_all, which always returns key/value pairs and gives back an empty list when no key matches.
19df746 to
6643e7f
Compare
|
Checked The pin is
Two notes:
Out of scope for this PR, but worth recording: |
Symptom
When a data kind holds exactly one key in Consul, reading all items of that kind raises:
The error propagates out of the store, so the whole all-items read fails rather than one item.
all_flags_statedegrades to{"$flagsState":{},"$valid":false}and every flag falls back to itsdefault.
Root cause
ConsulFeatureStoreCore#get_all_internalread the collection with a recursiveDiplomat::Kv.get.Kv.getcallsreturn_value(return_nil_values, transformation)and leavesreturn_hashat itsdefault
false(diplomat-2.6.6/lib/diplomat/kv.rb:28, thefound == :returnbranch). Inreturn_value(diplomat-2.6.6/lib/diplomat/rest_client.rb:189-203):So a recursive get returns the bare decoded value String when exactly one key matches the prefix.
.eachon a String then raises. Verified against a live Consul (dev agent, diplomat 2.6.6):Kv.get(prefix, {recurse: true}, :return)Kv.get_all(prefix, {}, :return)""(String)[][{key:, value:}][{key:, value:}][{key:, value:}]The fix
Read with
Kv.get_all, which passesreturn_hash = trueand so always returns key/value pairs,and which returns
[]for a 404 when givennot_found = :return(
diplomat-2.6.6/lib/diplomat/kv.rb:106-132).get_allsets:recurseitself. It returns fullkeys, so the existing prefix-stripping is unchanged. The
results == ""guard for the empty casebecomes dead and is removed, because
get_allgives[]instead of"".Why this matters for #443
The
.eachline is not changed by #443, so the bug is pre-existing. But it defeats #443's own fixat n=1: a store whose only
featuresrow is a keyless tombstone — a single-flag project, or everyflag deleted — still raises instead of reading the tombstone and filtering it. #443's new spec does
not catch this because it seeds a live item plus a tombstone, which is two keys.
Test evidence
Two specs added to the shared
persistent_feature_storeexamples inspec/feature_store_spec_base.rb:can read all items when a kind holds a single itemcan read all items when the single item is a tombstone with no keyBefore (on
jb/sdk-2995/tombstone-store-keys, 2 specs x 4 permutations):After, the Consul suite is green:
Full suite against live Redis, Consul and DynamoDB (
LD_SKIP_DATABASE_TESTS=0):bundle exec rubocop: 187 files inspected, no offenses detected.Note on spec placement
The specs go in
persistent_feature_storerather thanany_feature_store, so they also coverRedis and DynamoDB. Both already pass there (16 examples), which confirms the defect is specific to
the Consul client.
They deliberately read through a second store instance.
CachingStoreWrapper#initwarms theall-items cache (
store_wrapper.rb:60-74) and caching is on by default at a 15s TTL, so asingle-instance
allafterinitis served from cache and never callsget_all_internal— itpasses even on the broken code. An
any_feature_storeplacement would therefore not have caughtthis, and would not work for the in-memory store, which has no shared backing for a second
instance and no
write_raw_item.Note
Overview
Fixes Consul feature store failures when a data kind has exactly one key under its prefix.
all/get_all_internalused recursiveDiplomat::Kv.get, which returns a bare value String for a single match instead of key/value pairs, so.eachraised and flag reads degraded to invalid$flagsState.get_all_internalnow usesDiplomat::Kv.get_all, which always returns an enumerable list of{key:, value:}(and[]when empty), so prefix stripping and tombstone handling stay the same without the oldresults == ""guard.Two
persistent_feature_storeexamples assert single-item and single keyless-tombstoneallreads via a second store instance (bypassing warm cache), covering the n=1 case that multi-key tombstone tests missed.Reviewed by Cursor Bugbot for commit 6643e7f. Bugbot is set up for automated code reviews on this repo. Configure here.