From faa29ba26dbea89e53ece3f8096dfb2615f2796f Mon Sep 17 00:00:00 2001 From: Kevin Meinhardt Date: Tue, 26 Nov 2024 19:10:42 +0100 Subject: [PATCH] Apply suggestions from code review Co-authored-by: William Durand --- src/olympia/blocklist/cron.py | 3 +++ src/olympia/blocklist/mlbf.py | 11 +++++++---- src/olympia/blocklist/tests/test_cron.py | 8 ++++---- 3 files changed, 14 insertions(+), 8 deletions(-) diff --git a/src/olympia/blocklist/cron.py b/src/olympia/blocklist/cron.py index f21afb268eb3..550d44e6d517 100644 --- a/src/olympia/blocklist/cron.py +++ b/src/olympia/blocklist/cron.py @@ -126,6 +126,9 @@ def _upload_mlbf_to_remote_settings(*, force_base=False): ) if create_stash: + # We generate unified stashes, which means they can contain data + # for both soft and hard blocks. We need the base filters of each + # block type to determine what goes in a stash. mlbf.generate_and_write_stash( previous_mlbf=previous_filter, blocked_base_filter=base_filters[BlockType.BLOCKED], diff --git a/src/olympia/blocklist/mlbf.py b/src/olympia/blocklist/mlbf.py index 95415ca96608..46a97614ff3b 100644 --- a/src/olympia/blocklist/mlbf.py +++ b/src/olympia/blocklist/mlbf.py @@ -231,7 +231,8 @@ def generate_and_write_filter(self, block_type: BlockType): Generate and write the bloom filter for a given block type. Included items will be items in the specified block type list. Excluded items will be items in all other data types. - We use the language of inlcude and exclude to distinguish this concept + + We use the language of include and exclude to distinguish this concept from blocked and unblocked which are more specific to the block type. """ stats = {} @@ -294,11 +295,13 @@ def generate_and_write_stash( Generate and write the stash file representing changes between the previous and current bloom filters. See: https://bugzilla.mozilla.org/show_bug.cgi?id=soft-blocking + + Since we might be generating both a filter and a stash at the exact same time, + we need to compute a stash that doesn't include the data already in the newly + created filter. Items that are removed from one block type and added to another are - excluded from the unblocked list to prevent double counting. This - can lead to invalid states in old FX clients that do not support soft - blocking. + excluded from the unblocked list to prevent double counting. If a block type needs a new filter, we do not include any items for that block type in the stash to prevent double counting items. diff --git a/src/olympia/blocklist/tests/test_cron.py b/src/olympia/blocklist/tests/test_cron.py index ef00c300cb85..17118f38baf3 100644 --- a/src/olympia/blocklist/tests/test_cron.py +++ b/src/olympia/blocklist/tests/test_cron.py @@ -534,8 +534,8 @@ def test_upload_stash_unless_enough_changes(self): assert new_mlbf.storage.exists(new_mlbf.stash_path) with new_mlbf.storage.open(new_mlbf.stash_path, 'r') as f: data = json.load(f) - # We expect an empty stash for hard blocks because we are - # uploading a new filter. + # We expect an empty list for hard blocks because we are + # uploading a new hard filter. assert len(data['blocked']) == 0 # We can still see there are 2 hard blocked versions in the new filter. assert len(new_mlbf.data.blocked_items) == 2 @@ -551,7 +551,7 @@ def _test_upload_stash_and_filter( with override_switch('enable-soft-blocking', active=enable_soft_blocking): upload_mlbf_to_remote_settings() - # generation time is set to current time so we can load the mlbf + # Generation time is set to current time so we can load the MLBF. mlbf = MLBF.load_from_storage(self.current_time) if expected_stash is None: @@ -584,7 +584,7 @@ def test_upload_blocked_stash_and_softblock_filter(self): # Expected stash does not change expected_stash = { 'blocked': 1, - # Expect no soft blocks becuase there are enough for a filter + # Expect no soft blocks because there are enough for a filter. 'softblocked': 0, # There are no unblocked versions 'unblocked': 0,