fix: Allow tombstones without a key property

ldclient/impl/model/feature_flag.py

@@ -104,11 +104,13 @@ def __init__(self, data: dict):
         # be absent even if they are really required in the schema. That's for backward compatibility
         # with test logic that constructed incomplete JSON, and also with the file data source which
         # previously allowed users to get away with leaving out a lot of properties in the JSON.
-        self._key = req_str(data, 'key')
         self._version = req_int(data, 'version')
         self._deleted = opt_bool(data, 'deleted')
         if self._deleted:
+            # Tombstones are not guaranteed to have a key.
+            self._key = opt_str(data, 'key') or ''
             return
+        self._key = req_str(data, 'key')
         self._variations = opt_list(data, 'variations')
         self._on = opt_bool(data, 'on')
         self._off_variation = opt_int(data, 'offVariation')

ldclient/impl/model/segment.py

@@ -73,11 +73,13 @@ def __init__(self, data: dict):
         # be absent even if they are really required in the schema. That's for backward compatibility
         # with test logic that constructed incomplete JSON, and also with the file data source which
         # previously allowed users to get away with leaving out a lot of properties in the JSON.
-        self._key = req_str(data, 'key')
         self._version = req_int(data, 'version')
         self._deleted = opt_bool(data, 'deleted')
         if self._deleted:
+            # Tombstones are not guaranteed to have a key.
+            self._key = opt_str(data, 'key') or ''
             return
+        self._key = req_str(data, 'key')
         self._included = set(opt_str_list(data, 'included'))
         self._excluded = set(opt_str_list(data, 'excluded'))
         self._included_contexts = list(SegmentTarget(item) for item in opt_dict_list(data, 'includedContexts'))

ldclient/testing/impl/test_model_decode.py

@@ -5,6 +5,7 @@
 
 from ldclient.impl.model import *
 from ldclient.testing.builders import *
+from ldclient.versioned_data_kind import FEATURES, SEGMENTS
 
 
 def test_flag_targets_are_stored_as_sets():
@@ -41,3 +42,38 @@ def test_clause_values_preprocessed_with_time_operator(op):
     flag = make_boolean_flag_with_clauses(make_clause(None, "attr", op, 1000, "1970-01-01T00:00:02Z", True))
     assert flag.rules[0].clauses[0]._values == [1000, "1970-01-01T00:00:02Z", True]
     assert list(x.as_time for x in flag.rules[0].clauses[0]._values_preprocessed) == [1000, 2000, None]
+
+
+@pytest.mark.parametrize('kind', [FEATURES, SEGMENTS])
+def test_tombstone_without_key_can_be_decoded(kind):
+    # Other LaunchDarkly SDKs write deleted items to a persistent store with only the version,
+    # so we must be able to read them back.
+    item = kind.decode({"version": 5, "deleted": True})
+    assert item.version == 5
+    assert item.deleted is True
+    assert item.key == ''
+    # The original data must round-trip unchanged, because the store re-serializes it.
+    assert item.to_json_dict() == {"version": 5, "deleted": True}
+
+
+@pytest.mark.parametrize('kind', [FEATURES, SEGMENTS])
+def test_tombstone_with_placeholder_key_can_be_decoded(kind):
+    # The Go SDK and the Relay Proxy write deleted items with a placeholder key.
+    item = kind.decode({"key": "$deleted", "version": 5, "deleted": True})
+    assert item.version == 5
+    assert item.deleted is True
+    assert item.key == '$deleted'
+
+
+@pytest.mark.parametrize('kind', [FEATURES, SEGMENTS])
+def test_tombstone_still_requires_version(kind):
+    with pytest.raises(ValueError):
+        kind.decode({"deleted": True})
+
+
+@pytest.mark.parametrize('kind', [FEATURES, SEGMENTS])
+def test_item_that_is_not_deleted_still_requires_key(kind):
+    with pytest.raises(ValueError):
+        kind.decode({"version": 5})
+    with pytest.raises(ValueError):
+        kind.decode({"version": 5, "deleted": False})

ldclient/testing/test_async_feature_store_helpers.py

@@ -4,7 +4,7 @@
 
 from ldclient.async_feature_store_helpers import AsyncCachingStoreWrapper
 from ldclient.feature_store import CacheConfig
-from ldclient.versioned_data_kind import VersionedDataKind
+from ldclient.versioned_data_kind import FEATURES, SEGMENTS, VersionedDataKind
 
 # These tests exercise the caching-wrapper logic only, using an in-memory mock core, so they run
 # without a Redis instance. They mirror ldclient.testing.test_feature_store_helpers for the sync
@@ -205,6 +205,21 @@ async def test_get_all_removes_deleted_items(self, cached):
         core.force_set(THINGS, item2)
         assert await wrapper.all(THINGS) == {item1["key"]: item1}
 
+    @pytest.mark.asyncio
+    @pytest.mark.parametrize("kind", [FEATURES, SEGMENTS])
+    @pytest.mark.parametrize("cached", [False, True])
+    async def test_get_all_tolerates_tombstone_with_no_key(self, cached, kind):
+        # Other LaunchDarkly SDKs write deleted items to a persistent store with only the
+        # version. The store knows the key, because it is the key the item is stored under.
+        core = MockAsyncCore()
+        wrapper = make_wrapper(core, cached)
+        live_item = {"key": "item1", "version": 1}
+        tombstone = {"version": 2, "deleted": True}
+        core.data[kind] = {"item1": live_item, "item2": tombstone}
+
+        assert await wrapper.all(kind) == {"item1": kind.decode(live_item)}
+        assert await wrapper.get(kind, "item2") is None
+
     @pytest.mark.asyncio
     @pytest.mark.parametrize("cached", [False, True])
     async def test_get_all_changes_None_to_empty_dict(self, cached):

ldclient/testing/test_feature_store_helpers.py

@@ -5,7 +5,7 @@
 
 from ldclient.feature_store import CacheConfig
 from ldclient.feature_store_helpers import CachingStoreWrapper
-from ldclient.versioned_data_kind import VersionedDataKind
+from ldclient.versioned_data_kind import FEATURES, SEGMENTS, VersionedDataKind
 
 THINGS = VersionedDataKind(namespace="things", request_api_path="", stream_api_path="")
 WRONG_THINGS = VersionedDataKind(namespace="wrong", request_api_path="", stream_api_path="")
@@ -189,6 +189,20 @@ def test_get_all_removes_deleted_items(self, cached):
         core.force_set(THINGS, item2)
         assert wrapper.all(THINGS) == {item1["key"]: item1}
 
+    @pytest.mark.parametrize("kind", [FEATURES, SEGMENTS])
+    @pytest.mark.parametrize("cached", [False, True])
+    def test_get_all_tolerates_tombstone_with_no_key(self, cached, kind):
+        # Other LaunchDarkly SDKs write deleted items to a persistent store with only the
+        # version. The store knows the key, because it is the key the item is stored under.
+        core = MockCore()
+        wrapper = make_wrapper(core, cached)
+        live_item = {"key": "item1", "version": 1}
+        tombstone = {"version": 2, "deleted": True}
+        core.data[kind] = {"item1": live_item, "item2": tombstone}
+
+        assert wrapper.all(kind) == {"item1": kind.decode(live_item)}
+        assert wrapper.get(kind, "item2") is None
+
     @pytest.mark.parametrize("cached", [False, True])
     def test_get_all_changes_None_to_empty_dict(self, cached):
         core = MockCore()