diff --git a/ldclient/impl/integrations/files/filedata.py b/ldclient/impl/integrations/files/filedata.py index 0f1ef576..e2662417 100644 --- a/ldclient/impl/integrations/files/filedata.py +++ b/ldclient/impl/integrations/files/filedata.py @@ -4,10 +4,11 @@ keep their own implementation and behavior. A data file is a JSON or YAML document with optional ``flags``, ``flagValues``, and -``segments`` members. ``flags`` and ``segments`` hold full definitions keyed by key. -``flagValues`` maps a flag key to a single value. It expands into a full flag definition -that returns that value for every context. The file name extension decides the format: a -file whose name ends in ``.yaml`` or ``.yml`` is YAML, and any other file is JSON. +``segments`` members. ``flags`` and ``segments`` hold full definitions keyed by key, and a +definition marked as deleted fails the load of its file. ``flagValues`` maps a flag key to a +single value. It expands into a full flag definition that returns that value for every +context. The file name extension decides the format: a file whose name ends in ``.yaml`` or +``.yml`` is YAML, and any other file is JSON. """ import codecs @@ -92,6 +93,19 @@ def __init__(self, path: str, message: str): self.path = path +class DeletedEntryError(FileReadError): + """ + Indicates that a file holds a flag or segment entry marked as deleted. An override is removed + by removing its entry, so such a file fails to load like one that cannot be parsed. + """ + + def __init__(self, path: str, kind_name: str, key: str): + FileDataError.__init__(self, '%s "%s" in %s is marked as deleted; remove the entry to remove its override' % (kind_name, key, path)) + self.path = path + self.kind_name = kind_name + self.key = key + + class DuplicateKeyError(FileDataError): """Indicates that the same key appears in more than one file and the handling is FAIL.""" @@ -170,9 +184,11 @@ def read_file(path: str) -> Document: except OSError as e: raise FileReadError(path, "unable to read file: %s" % e) try: - return parse_document(raw, is_yaml_path(path)) + document = parse_document(raw, is_yaml_path(path)) except Exception as e: raise FileReadError(path, "error parsing file: %s" % e) + _reject_deleted_entries(path, document) + return document def has_content(raw: bytes) -> bool: @@ -336,9 +352,25 @@ def _parse_file(path: str, raw: bytes) -> Optional[Document]: """Parses what was read from a file that exists. Returns None when the parser read nothing.""" is_yaml = is_yaml_path(path) try: - return _parse_content(raw, is_yaml) + document = _parse_content(raw, is_yaml) except Exception as e: raise FileReadError(path, "error parsing file as %s, the format of its name: %s" % ("YAML" if is_yaml else "JSON", e)) + if document is not None: + _reject_deleted_entries(path, document) + return document + + +def _reject_deleted_entries(path: str, document: Document) -> None: + """ + Raises :class:`DeletedEntryError` for the first flag or segment entry of a parsed file that is + marked as deleted. The override layer never holds a deleted item, and an override is removed + by removing its entry, so such an entry is a mistake in the file that fails its load. The file + data sources do not use this module and keep accepting such entries. + """ + for kind_name, items in (("flag", document.flags), ("segment", document.segments)): + for key, item in items.items(): + if item.deleted: + raise DeletedEntryError(path, kind_name, key) def _parse_files(contents: List[_FileRead], duplicate_keys_handling: DuplicateKeysHandling) -> Tuple[MergeResult, bytes]: diff --git a/ldclient/impl/integrations/overrides/file_override_source.py b/ldclient/impl/integrations/overrides/file_override_source.py index 220a73b8..852e4ffb 100644 --- a/ldclient/impl/integrations/overrides/file_override_source.py +++ b/ldclient/impl/integrations/overrides/file_override_source.py @@ -47,10 +47,10 @@ class _FileOverrideSource(OverrideSource): """ Reads overrides from the configured files and supplies each successful load to the sink as a full snapshot. A configured file that does not exist contributes no overrides. A file - that cannot be read or parsed fails that load, the last good snapshot stays in effect, and - the load is retried. A file whose content is in effect and that is found with no entries is - held back until it has stayed so for the retry delay, because it may be in the middle of an - in-place rewrite. + that cannot be read or parsed, or that holds an entry marked as deleted, fails that load, + the last good snapshot stays in effect, and the load is retried. A file whose content is in + effect and that is found with no entries is held back until it has stayed so for the retry + delay, because it may be in the middle of an in-place rewrite. """ def __init__(self, paths: List[str], duplicate_keys_handling: DuplicateKeysHandling, change_detection: ChangeDetection, poll_interval: float): diff --git a/ldclient/impl/overrides/layer.py b/ldclient/impl/overrides/layer.py index e1e38a94..e5fa49ae 100644 --- a/ldclient/impl/overrides/layer.py +++ b/ldclient/impl/overrides/layer.py @@ -17,6 +17,19 @@ def _empty_contents() -> LayerContents: return {FEATURES: {}, SEGMENTS: {}} +class DeletedItemError(ValueError): + """ + Indicates that a snapshot holds a flag or segment marked as deleted. The layer never holds a + deleted item, so it rejects the whole snapshot. An override is removed by leaving it out of + the snapshot. + """ + + def __init__(self, kind_name: str, key: str): + super().__init__('%s "%s" is marked as deleted; an override is removed by leaving it out of the snapshot' % (kind_name, key)) + self.kind_name = kind_name + self.key = key + + class OverrideLayer: """ A thread-safe store of override entries, replaced wholesale on each update from an override @@ -41,8 +54,9 @@ def set_all(self, flags: Mapping[str, Any], segments: Mapping[str, Any]) -> Tupl Values may be model objects or their JSON dictionary form. A dictionary is decoded with the model constructor, which raises ``ValueError`` for an invalid definition. A - definition whose key differs from its key in the mapping raises ``ValueError`` too. - Nothing is replaced in either case. + definition whose key differs from its key in the mapping raises ``ValueError`` too, and a + flag or segment marked as deleted, in either form, raises :class:`DeletedItemError`, + which is a ``ValueError``. Nothing is replaced in any of these cases. :return: the previous and the new contents. The returned dictionaries must not be modified. """ @@ -52,6 +66,8 @@ def set_all(self, flags: Mapping[str, Any], segments: Mapping[str, Any]) -> Tupl } for kind, items in decoded.items(): for key, item in items.items(): + if item.deleted: + raise DeletedItemError("flag" if kind == FEATURES else "segment", key) # Evaluation reads an entry by its mapping key, while the all-flags report and # analytics events use the definition's key. A mismatch would serve one key's # override under another, so the whole snapshot is rejected. diff --git a/ldclient/impl/overrides/sink.py b/ldclient/impl/overrides/sink.py index f2389628..76c71a5a 100644 --- a/ldclient/impl/overrides/sink.py +++ b/ldclient/impl/overrides/sink.py @@ -7,7 +7,11 @@ from typing import Any, Callable, Dict, Optional, Set from ldclient.impl.dependency_tracker import DependencyTracker, KindAndKey -from ldclient.impl.overrides.layer import LayerContents, OverrideLayer +from ldclient.impl.overrides.layer import ( + DeletedItemError, + LayerContents, + OverrideLayer +) from ldclient.impl.util import log from ldclient.interfaces import OverrideSink, ReadOnlyStore from ldclient.versioned_data_kind import FEATURES, SEGMENTS, VersionedDataKind @@ -51,7 +55,13 @@ def set_overrides(self, flags: Dict[str, Any], segments: Dict[str, Any]) -> None if self._closed: log.debug("Ignoring an override snapshot supplied after the override source was closed") return - self._apply(flags, segments) + try: + self._apply(flags, segments) + except DeletedItemError as e: + # The layer rejected the whole snapshot before changing anything, so nothing is + # notified. The error also reaches the source, as for an invalid definition. + log.error("Rejected an override snapshot; the overrides in effect stay: %s", e) + raise def close(self) -> None: """ diff --git a/ldclient/integrations/overrides.py b/ldclient/integrations/overrides.py index 4d70e984..0923c2de 100644 --- a/ldclient/integrations/overrides.py +++ b/ldclient/integrations/overrides.py @@ -72,7 +72,8 @@ class FileOverrideSourceBuilder(OverrideSourceBuilder): overrides, and deleting every file removes them all. A file that exists but cannot be read or parsed makes that whole reload fail. The previously loaded overrides stay in effect, the source logs the failure, retries after a short delay, and recovers on its own once the file - is readable again. + is readable again. A file that contains an entry marked as deleted fails to load, and the + overrides in effect stay; to remove an override, remove its entry or delete the file. Files can be rewritten in place: if the source finds a file empty while it is being rewritten, it keeps the overrides in effect until the new content is written. To remove a diff --git a/ldclient/interfaces.py b/ldclient/interfaces.py index a28095b7..1b39a3a3 100644 --- a/ldclient/interfaces.py +++ b/ldclient/interfaces.py @@ -2149,8 +2149,10 @@ def set_overrides(self, flags: Mapping[str, Any], segments: Mapping[str, Any]) - either a :class:`ldclient.impl.model.FeatureFlag` or :class:`ldclient.impl.model.Segment` model object or its JSON dictionary form, whose own key is the key it has in the mapping. A dictionary that is not a valid definition, or a definition whose key differs from its - key in the mapping, raises ``ValueError``, and the layer is left unchanged. The SDK itself - marks the entries as overrides. + key in the mapping, raises ``ValueError``, and the layer is left unchanged. A snapshot + that contains a flag or segment marked as deleted is rejected with ``ValueError``, and + the overrides in effect stay; to remove an override, leave it out of the snapshot. The + SDK itself marks the entries as overrides. This method is safe to call from any thread. Calls are serialized by the SDK. diff --git a/ldclient/testing/impl/test_overrides.py b/ldclient/testing/impl/test_overrides.py index a4054b42..1681d8e4 100644 --- a/ldclient/testing/impl/test_overrides.py +++ b/ldclient/testing/impl/test_overrides.py @@ -2,6 +2,7 @@ Tests for the override layer, the overlay at the store read boundary, and the sink that applies snapshots and notifies flag change listeners. """ +import logging from typing import Any, Dict, List, Optional import pytest @@ -131,6 +132,49 @@ def test_layer_rejects_a_definition_whose_key_differs_from_its_mapping_key(flags assert layer.all(SEGMENTS) == {} +def snapshot_with_deleted_item(kind, as_model: bool): + """ + A snapshot that changes the entries flag1 and segment1 to version 2 and adds an item of the + given kind, named deleted-key, that is marked as deleted, as a model object or in its JSON + dictionary form. Because the snapshot changes the other entries too, applying any part of it + would show. + """ + data = {'key': 'deleted-key', 'version': 2, 'deleted': True} + deleted = {'deleted-key': kind.decode(data) if as_model else data} + flags = {'flag1': flag('flag1', 2), **(deleted if kind == FEATURES else {})} + segments = {'segment1': segment('segment1', 2), **(deleted if kind == SEGMENTS else {})} + return flags, segments + + +def deleted_item_message(kind) -> str: + return '%s "deleted-key" is marked as deleted; an override is removed by leaving it out of the snapshot' % ('flag' if kind == FEATURES else 'segment') + + +@pytest.mark.parametrize('as_model', [False, True], ids=['dict', 'model']) +@pytest.mark.parametrize('kind', [FEATURES, SEGMENTS], ids=['flag', 'segment']) +def test_layer_rejects_a_snapshot_that_holds_a_deleted_item_and_keeps_its_contents(kind, as_model): + layer = OverrideLayer() + layer.set_all({'flag1': flag('flag1')}, {'segment1': segment('segment1')}) + flags, segments = snapshot_with_deleted_item(kind, as_model) + + with pytest.raises(ValueError) as e: + layer.set_all(flags, segments) + + assert str(e.value) == deleted_item_message(kind) + assert layer.get(FEATURES, 'flag1').version == 1 + assert layer.get(SEGMENTS, 'segment1').version == 1 + assert layer.get(kind, 'deleted-key') is None + + +def test_layer_accepts_definitions_marked_as_not_deleted(): + layer = OverrideLayer() + layer.set_all({'flag1': dict(flag('flag1').to_json_dict(), deleted=False)}, {'segment1': dict(segment('segment1').to_json_dict(), deleted=False)}) + stored_flag = layer.get(FEATURES, 'flag1') + assert stored_flag is not None + assert stored_flag.on is True + assert layer.get(SEGMENTS, 'segment1') is not None + + def test_layer_replacement_semantics(): layer = OverrideLayer() assert layer.is_empty is True @@ -475,3 +519,35 @@ def test_sink_invalid_snapshot_raises_and_changes_nothing(): f.sink.set_overrides({'flag1': {'key': 'flag1', 'version': 'bad'}}, {}) assert f.layer.get(FEATURES, 'flag1').version == 1 assert f.take_notified() == [] + + +@pytest.mark.parametrize('listening', [True, False], ids=['listening', 'not-listening']) +@pytest.mark.parametrize('as_model', [False, True], ids=['dict', 'model']) +@pytest.mark.parametrize('kind', [FEATURES, SEGMENTS], ids=['flag', 'segment']) +def test_sink_rejects_a_snapshot_that_holds_a_deleted_item_logs_it_and_notifies_nothing(kind, as_model, listening, caplog): + dependent = FlagBuilder('dependent').version(1).on(True).variations(True, False).fallthrough_variation(1).rules( + FlagRuleBuilder().variation(0).clauses(make_clause_matching_segment_key('segment1')).build() + ).build() + f = SinkFixture(FakeBaseStore(flags={'dependent': dependent})) + f.sink.set_overrides({'flag1': flag('flag1')}, {'segment1': segment('segment1')}) + f.take_notified() + f.listening = listening + caplog.clear() + flags, segments = snapshot_with_deleted_item(kind, as_model) + + with pytest.raises(ValueError) as e: + f.sink.set_overrides(flags, segments) + + assert str(e.value) == deleted_item_message(kind) + assert f.layer.get(FEATURES, 'flag1').version == 1 + assert f.layer.get(SEGMENTS, 'segment1').version == 1 + assert f.layer.get(kind, 'deleted-key') is None + assert f.take_notified() == [] + errors = [r.getMessage() for r in caplog.records if r.levelno == logging.ERROR] + assert errors == ['Rejected an override snapshot; the overrides in effect stay: ' + deleted_item_message(kind)] + + # The sink still accepts the next snapshot. + f.listening = True + f.sink.set_overrides({'flag1': flag('flag1', 3)}, {}) + assert f.layer.get(FEATURES, 'flag1').version == 3 + assert f.take_notified() == ['dependent', 'flag1'] diff --git a/ldclient/testing/integrations/test_file_data_sources_pinned_behavior.py b/ldclient/testing/integrations/test_file_data_sources_pinned_behavior.py index 0037261a..6213d69c 100644 --- a/ldclient/testing/integrations/test_file_data_sources_pinned_behavior.py +++ b/ldclient/testing/integrations/test_file_data_sources_pinned_behavior.py @@ -3,8 +3,8 @@ initializer and synchronizer. The file-based override source is built on separate code and behaves differently in several of these respects. These tests keep the existing sources observably unchanged: the flagValues expansion and its evaluation reason, the version fallback, -the duplicate key and load failure messages, the FDv2 status and error kinds, and the polling -and watching rules. +the duplicate key and load failure messages, the FDv2 status and error kinds, the acceptance of +entries marked as deleted, and the polling and watching rules. """ import logging import os @@ -291,6 +291,55 @@ def collect(): thread.join(5) +# --------------------------------------------------------------------------- +# Entries marked as deleted. The override source rejects a file that holds one. These sources +# accept it, and rejecting it here is a breaking change left to a later major version. +# --------------------------------------------------------------------------- + +DOCUMENT_WITH_DELETED_ENTRIES = ''' +{ + "flags": { + "deleted-flag": {"key": "deleted-flag", "version": 3, "deleted": true}, + "flag1": {"key": "flag1", "on": false, "offVariation": 0, "variations": ["x"]} + }, + "segments": { + "deleted-segment": {"key": "deleted-segment", "deleted": true} + } +} +''' + + +def test_v1_loads_a_file_with_deleted_entries_and_stores_them_as_deleted(tmp_path, caplog): + path = os.path.join(str(tmp_path), 'data.json') + write_file(path, DOCUMENT_WITH_DELETED_ENTRIES) + store = InMemoryFeatureStore() + source = make_v1_source(path, store) + with caplog.at_level(logging.ERROR): + source.start() + try: + assert store.initialized is True + assert [r.getMessage() for r in caplog.records if r.levelno == logging.ERROR] == [] + assert store.get(FEATURES, 'flag1') is not None + assert store.get(FEATURES, 'deleted-flag') is None + assert store.get(SEGMENTS, 'deleted-segment') is None + assert list(store.all(FEATURES, lambda x: x).keys()) == ['flag1'] + finally: + source.stop() + + +def test_v2_supplies_deleted_entries_as_they_are_in_the_file(tmp_path): + path = os.path.join(str(tmp_path), 'data.json') + write_file(path, DOCUMENT_WITH_DELETED_ENTRIES) + result = make_v2_source(path).fetch(MockSelectorStore(Selector.no_selector())) + assert result.value is not None + changes = v2_changes_by_key(result.value.change_set) + assert set(changes.keys()) == {'deleted-flag', 'flag1', 'deleted-segment'} + assert changes['deleted-flag'].object == {'key': 'deleted-flag', 'version': 3, 'deleted': True} + assert changes['deleted-flag'].version == 3 + assert changes['deleted-segment'].kind == ObjectKind.SEGMENT + assert changes['deleted-segment'].object == {'key': 'deleted-segment', 'version': 1, 'deleted': True} + + # --------------------------------------------------------------------------- # Polling rules: modification time only, a missing file is not a change # --------------------------------------------------------------------------- diff --git a/ldclient/testing/integrations/test_file_override_source.py b/ldclient/testing/integrations/test_file_override_source.py index 4ae4f4f8..b564980c 100644 --- a/ldclient/testing/integrations/test_file_override_source.py +++ b/ldclient/testing/integrations/test_file_override_source.py @@ -15,6 +15,7 @@ from ldclient.client import Config, Context, LDClient from ldclient.config import Config as SDKConfig from ldclient.datasystem import custom +from ldclient.evaluation import EvaluationDetail from ldclient.impl.integrations.files import filedata from ldclient.impl.integrations.overrides import file_override_source from ldclient.impl.integrations.overrides.file_override_source import ( @@ -547,6 +548,87 @@ def test_source_retries_a_failed_load_without_a_change_signal(tmp_path, sources, assert flag_values(sink.require_snapshot()) == {'flag1': False} +# --------------------------------------------------------------------------- +# Entries marked as deleted +# --------------------------------------------------------------------------- + +def document_with_entry_marked(file_format: str, kind_name: str, deleted: str) -> str: + """ + A document with the flag value flag1 set to "changed" and an entry "d" of the given kind + whose deleted property is given as it is written in the file. + """ + member = 'flags' if kind_name == 'flag' else 'segments' + if file_format == 'json': + return '{"flagValues": {"flag1": "changed"}, "%s": {"d": {"key": "d", "deleted": %s}}}' % (member, deleted) + return 'flagValues:\n flag1: changed\n%s:\n d:\n key: d\n deleted: %s\n' % (member, deleted) + + +def deleted_entry_failure(kind_name: str, path: str) -> str: + return 'Unable to load flag data: %s "d" in %s is marked as deleted; remove the entry to remove its override' % (kind_name, path) + + +def error_lines(caplog) -> List[str]: + return [r.getMessage() for r in caplog.records if r.levelno == logging.ERROR] + + +@pytest.mark.parametrize('kind_name', ['flag', 'segment']) +@pytest.mark.parametrize('file_format', ['json', pytest.param('yaml', marks=yaml_required)]) +def test_source_initial_load_fails_for_a_file_with_an_entry_marked_as_deleted(tmp_path, sources, caplog, file_format, kind_name): + # The whole load fails, so the other file's overrides are not applied either. + other = os.path.join(str(tmp_path), 'other.json') + write_file(other, '{"flagValues": {"other": true}}') + path = os.path.join(str(tmp_path), 'overrides.' + file_format) + write_file(path, document_with_entry_marked(file_format, kind_name, 'true')) + with caplog.at_level(logging.ERROR): + _, sink = build_source(sources, [other, path]) + sink.require_no_snapshot() + assert error_lines(caplog) == [deleted_entry_failure(kind_name, path)] + + +@pytest.mark.parametrize('kind_name', ['flag', 'segment']) +def test_source_reload_fails_for_an_entry_marked_as_deleted_keeps_the_overrides_and_retries(tmp_path, sources, caplog, kind_name): + path = os.path.join(str(tmp_path), 'overrides.json') + write_file(path, '{"flagValues": {"flag1": "original"}}') + with caplog.at_level(logging.ERROR): + _, sink = build_source(sources, [path], lambda b: b.poll_interval(FileOverrideSourceBuilder.MINIMUM_POLL_INTERVAL)) + assert flag_values(sink.require_snapshot()) == {'flag1': 'original'} + + # The edit also changes flag1, and none of it is applied: the sink is not called. + deleted = document_with_entry_marked('json', kind_name, 'true ') + fixed = document_with_entry_marked('json', kind_name, 'false') + assert len(deleted) == len(fixed) + write_file(path, deleted) + deadline = time.time() + TEST_TIMEOUT + while deleted_entry_failure(kind_name, path) not in error_lines(caplog): + assert time.time() < deadline, "the load failure was not logged" + time.sleep(0.01) + sink.require_no_snapshot() + + # The fix keeps the file's size and modification time, so the poller sees no change and + # only the retry of the failed load picks it up. An entry marked as not deleted loads. + observed = os.stat(path) + write_file(path, fixed) + os.utime(path, ns=(observed.st_atime_ns, observed.st_mtime_ns)) + flags, segments = sink.require_snapshot() + assert flags['flag1'].variations == ['changed'] + assert 'd' in (flags if kind_name == 'flag' else segments) + + +@pytest.mark.parametrize('file_format', ['json', pytest.param('yaml', marks=yaml_required)]) +def test_source_loads_entries_marked_as_not_deleted(tmp_path, sources, file_format): + path = os.path.join(str(tmp_path), 'overrides.' + file_format) + if file_format == 'json': + write_file(path, '{"flags": {"f": {"key": "f", "on": true, "variations": ["x"], "fallthrough": {"variation": 0}, "deleted": false}}, ' + '"segments": {"s": {"key": "s", "deleted": false}}}') + else: + write_file(path, 'flags:\n f:\n key: f\n "on": true\n variations: [x]\n fallthrough: {variation: 0}\n deleted: false\n' + 'segments:\n s:\n key: s\n deleted: false\n') + _, sink = build_source(sources, [path]) + snapshot = sink.require_snapshot() + assert flag_values(snapshot) == {'f': 'x'} + assert list(snapshot[1].keys()) == ['s'] + + @pytest.mark.parametrize("mode", [ChangeDetection.POLLING, pytest.param(ChangeDetection.WATCHING, marks=watchdog_required)]) def test_source_detects_an_edit_made_during_the_initial_load(tmp_path, sources, monkeypatch, mode): path = os.path.join(str(tmp_path), 'overrides.json') @@ -797,3 +879,18 @@ def test_client_side_all_flags_state_includes_a_full_definition_with_client_side state = client.all_flags_state(user, client_side_only=True) assert state.valid assert state.to_values_map() == {'full-flag': True} + + +def test_client_serves_no_override_from_a_file_with_an_entry_marked_as_deleted(tmp_path): + path = os.path.join(str(tmp_path), 'overrides.json') + write_file(path, '{"flagValues": {"value-flag": true}, "flags": {"deleted-flag": {"key": "deleted-flag", "deleted": true}}}') + source = FileOverrideSourceBuilder([path]) + datasystem = custom().synchronizers(HangingSynchronizer().builder).overrides(source).build() + config = Config(sdk_key='SDK_KEY', datasystem_config=datasystem, event_processor_class=MockEventProcessor) + + with LDClient(config, start_wait=0) as client: + user = Context.create('user-key') + not_ready = {'kind': 'ERROR', 'errorKind': 'CLIENT_NOT_READY'} + assert client.variation('deleted-flag', user, 'default') == 'default' + assert client.variation_detail('deleted-flag', user, 'default') == EvaluationDetail('default', None, not_ready) + assert client.variation_detail('value-flag', user, False) == EvaluationDetail(False, None, not_ready) diff --git a/ldclient/testing/integrations/test_filedata.py b/ldclient/testing/integrations/test_filedata.py index 2b2cbd23..1602a1b6 100644 --- a/ldclient/testing/integrations/test_filedata.py +++ b/ldclient/testing/integrations/test_filedata.py @@ -17,6 +17,7 @@ from ldclient.impl.integrations.files import filedata from ldclient.impl.integrations.files.filedata import ( + DeletedEntryError, Document, DuplicateKeyError, DuplicateKeysHandling, @@ -388,6 +389,35 @@ def test_load_files_reports_parse_error_with_path(tmp_path): assert excinfo.value.path == path +@pytest.mark.parametrize('member, kind_name', [('flags', 'flag'), ('segments', 'segment')]) +def test_load_files_fails_for_an_entry_marked_as_deleted_and_names_the_file_and_the_key(tmp_path, member, kind_name): + good = os.path.join(str(tmp_path), 'good.json') + path = os.path.join(str(tmp_path), 'deleted.json') + write_file(good, '{"flagValues": {"flag1": true}}') + write_file(path, '{"%s": {"entry": {"key": "entry", "deleted": true}}}' % member) + expected = '%s "entry" in %s is marked as deleted; remove the entry to remove its override' % (kind_name, path) + + with pytest.raises(DeletedEntryError) as excinfo: + load_files([good, path], DuplicateKeysHandling.FAIL) + # It is a per-file failure, like a file that cannot be parsed. + assert isinstance(excinfo.value, FileReadError) + assert excinfo.value.path == path + assert str(excinfo.value) == expected + + with pytest.raises(DeletedEntryError) as excinfo: + read_file(path) + assert str(excinfo.value) == expected + + +def test_load_files_accepts_entries_marked_as_not_deleted(tmp_path): + path = os.path.join(str(tmp_path), 'data.json') + write_file(path, '{"flags": {"flag1": {"key": "flag1", "on": true, "deleted": false}}, "segments": {"seg1": {"key": "seg1", "deleted": false}}}') + result = load_files([path], DuplicateKeysHandling.FAIL) + assert list(result.flags.keys()) == ['flag1'] + assert result.flags['flag1'].on is True + assert list(result.segments.keys()) == ['seg1'] + + @pytest.mark.skipif(not have_yaml, reason="pyyaml is not installed") def test_load_files_parses_each_file_in_the_format_of_its_extension(tmp_path): json_path = os.path.join(str(tmp_path), 'first.json') diff --git a/ldclient/testing/test_async_client_overrides.py b/ldclient/testing/test_async_client_overrides.py index 8fe69458..1c552086 100644 --- a/ldclient/testing/test_async_client_overrides.py +++ b/ldclient/testing/test_async_client_overrides.py @@ -317,6 +317,60 @@ async def test_override_source_start_failure_is_logged_and_the_client_runs_witho assert source.close_count == 1 +def deleted_flag(key: str) -> dict: + return {'key': key, 'version': 2, 'deleted': True} + + +def flag_and_dependent() -> Dict[str, dict]: + """LaunchDarkly data: the flag "flag", and the flag "dependent", which has it as a prerequisite.""" + return { + 'flag': FlagBuilder('flag').version(1).on(True).variations('ld-value').fallthrough_variation(0).build().to_json_dict(), + 'dependent': FlagBuilder('dependent').version(1).on(True).variations('dependent-on', 'dependent-off').fallthrough_variation(0).off_variation(1).prerequisite('flag', 0).build().to_json_dict(), + } + + +DELETED_FLAG_REJECTED = 'Rejected an override snapshot; the overrides in effect stay: flag "flag" is marked as deleted' + + +@pytest.mark.asyncio +async def test_override_source_that_supplies_a_deleted_flag_at_start_leaves_no_overrides(caplog): + # The sink rejects the whole snapshot, and the error propagates out of the source's start, + # which fails it as an invalid definition does. The client serves LaunchDarkly data. + source = MockOverrideSource(flags={'flag': deleted_flag('flag')}) + client = await make_initialized_client(flag_and_dependent(), source) + try: + assert await client.variation('flag', user, 'default') == 'ld-value' + assert await client.variation_detail('flag', user, 'default') == EvaluationDetail('ld-value', 0, {'kind': 'FALLTHROUGH'}) + assert await client.variation_detail('dependent', user, 'default') == EvaluationDetail('dependent-on', 0, {'kind': 'FALLTHROUGH'}) + assert records_containing(caplog, logging.ERROR, DELETED_FLAG_REJECTED) != [] + assert records_containing(caplog, logging.ERROR, 'Unable to start the override source; the client runs without overrides') != [] + finally: + await client.close() + + +@pytest.mark.asyncio +async def test_snapshot_with_a_deleted_flag_is_rejected_and_the_overrides_in_effect_stay(caplog): + source = MockOverrideSource(flags={'flag': single_value_flag('flag', 'override-value')}) + client = await make_initialized_client(flag_and_dependent(), source) + try: + changes: List[Any] = [] + client.flag_tracker.add_listener(changes.append) + + with pytest.raises(ValueError): + await asyncio.to_thread(source.set_overrides, {'flag': deleted_flag('flag'), 'other': single_value_flag('other', True)}) + + assert await client.variation('flag', user, 'default') == 'override-value' + assert await client.variation_detail('flag', user, 'default') == EvaluationDetail('override-value', 0, {'kind': 'FALLTHROUGH', 'overrideAffected': True}) + assert await client.variation_detail('dependent', user, 'default') == EvaluationDetail('dependent-on', 0, {'kind': 'FALLTHROUGH', 'overrideAffected': True}) + assert await client.variation('other', user, 'default') == 'default' + assert records_containing(caplog, logging.ERROR, DELETED_FLAG_REJECTED) != [] + # A notification would be delivered on the event loop. + await asyncio.sleep(0.3) + assert changes == [] + finally: + await client.close() + + @pytest.mark.asyncio async def test_snapshot_supplied_after_the_client_is_closed_is_ignored(): source = MockOverrideSource() diff --git a/ldclient/testing/test_ldclient_overrides.py b/ldclient/testing/test_ldclient_overrides.py index cd595f29..74cca252 100644 --- a/ldclient/testing/test_ldclient_overrides.py +++ b/ldclient/testing/test_ldclient_overrides.py @@ -295,6 +295,51 @@ def test_override_source_that_supplies_a_snapshot_and_then_fails_to_start_leaves assert client.variation('overridden-flag', user, False) is False +def deleted_flag(key: str) -> dict: + return {'key': key, 'version': 2, 'deleted': True} + + +def flag_and_dependent() -> Dict[str, dict]: + """LaunchDarkly data: the flag "flag", and the flag "dependent", which has it as a prerequisite.""" + return { + 'flag': FlagBuilder('flag').version(1).on(True).variations('ld-value').fallthrough_variation(0).build().to_json_dict(), + 'dependent': FlagBuilder('dependent').version(1).on(True).variations('dependent-on', 'dependent-off').fallthrough_variation(0).off_variation(1).prerequisite('flag', 0).build().to_json_dict(), + } + + +DELETED_FLAG_REJECTED = 'Rejected an override snapshot; the overrides in effect stay: flag "flag" is marked as deleted' + + +def test_override_source_that_supplies_a_deleted_flag_at_start_leaves_no_overrides(caplog): + # The sink rejects the whole snapshot, and the error propagates out of the source's start, + # which fails it as an invalid definition does. The client serves LaunchDarkly data. + source = MockOverrideSource(flags={'flag': deleted_flag('flag')}) + with make_initialized_client(flag_and_dependent(), source) as client: + assert client.variation('flag', user, 'default') == 'ld-value' + assert client.variation_detail('flag', user, 'default') == EvaluationDetail('ld-value', 0, {'kind': 'FALLTHROUGH'}) + assert client.variation_detail('dependent', user, 'default') == EvaluationDetail('dependent-on', 0, {'kind': 'FALLTHROUGH'}) + assert errors_containing(caplog, DELETED_FLAG_REJECTED) != [] + assert errors_containing(caplog, 'Unable to start the override source; the client runs without overrides') != [] + + +def test_snapshot_with_a_deleted_flag_is_rejected_and_the_overrides_in_effect_stay(caplog): + source = MockOverrideSource(flags={'flag': single_value_flag('flag', 'override-value')}) + with make_initialized_client(flag_and_dependent(), source) as client: + changes: Queue = Queue() + client.flag_tracker.add_listener(lambda change: changes.put(change)) + + with pytest.raises(ValueError): + source.set_overrides({'flag': deleted_flag('flag'), 'other': single_value_flag('other', True)}) + + assert client.variation('flag', user, 'default') == 'override-value' + assert client.variation_detail('flag', user, 'default') == EvaluationDetail('override-value', 0, {'kind': 'FALLTHROUGH', 'overrideAffected': True}) + assert client.variation_detail('dependent', user, 'default') == EvaluationDetail('dependent-on', 0, {'kind': 'FALLTHROUGH', 'overrideAffected': True}) + assert client.variation('other', user, 'default') == 'default' + assert errors_containing(caplog, DELETED_FLAG_REJECTED) != [] + with pytest.raises(Empty): + changes.get(timeout=0.3) + + def test_snapshot_supplied_after_the_client_is_closed_is_ignored(): source = MockOverrideSource() client = make_uninitialized_client(source)