fix: filters being duplicated by preprocessors (#4016)

Co-authored-by: chrmod <[email protected]>
ghostery · Jun 10, 2024 · ca06138 · ca06138
1 parent 1342b46
commit ca06138
Show file tree

Hide file tree

Showing 4 changed files with 83 additions and 6 deletions.
diff --git a/packages/adblocker/src/engine/engine.ts b/packages/adblocker/src/engine/engine.ts
@@ -641,9 +641,6 @@ export default class FilterEngine extends EventEmitter<
               .concat(networkFilters.map((filter) => filter.getId())),
           );
 
-          Array.prototype.push.apply(removedCosmeticFilters, cosmeticFilters);
-          Array.prototype.push.apply(removedNetworkFilters, networkFilters);
-
           removedPreprocessors.push(
             new Preprocessor({
               condition,
@@ -663,9 +660,6 @@ export default class FilterEngine extends EventEmitter<
               .concat(networkFilters.map((filter) => filter.getId())),
           );
 
-          Array.prototype.push.apply(newCosmeticFilters, cosmeticFilters);
-          Array.prototype.push.apply(newNetworkFilters, networkFilters);
-
           newPreprocessors.push(
             new Preprocessor({
               condition,

diff --git a/packages/adblocker/src/lists.ts b/packages/adblocker/src/lists.ts
@@ -442,6 +442,11 @@ export function generateDiff(
   return {
     added: Array.from(added),
     removed: Array.from(removed),
+    // For the filters under `preprocessors` property, it doesn't mean those are "filters".
+    // Those are "a list of filters affected by preprocessors" not the "filters" itself.
+    // Therefore, they shouldn't be treated as filters.
+    // Instead, we put "filters" in `added` and `removed` properties.
+    // This provides backward-compatibility and simplicity.
     preprocessors,
   };
 }

diff --git a/packages/adblocker/test/lists.test.ts b/packages/adblocker/test/lists.test.ts
@@ -244,6 +244,44 @@ bar.baz
         },
       },
     });
+
+    expect(
+      generateDiff(
+        `||foo.com^`,
+        `!#if true
+||foo.com^
+!#endif`,
+        config,
+      ),
+    ).to.eql({
+      added: [],
+      removed: [],
+      preprocessors: {
+        true: {
+          added: ['||foo.com^'],
+          removed: [],
+        },
+      },
+    });
+
+    expect(
+      generateDiff(
+        `!#if true
+||foo.com^
+!#endif`,
+        `||foo.com^`,
+        config,
+      ),
+    ).to.eql({
+      added: [],
+      removed: [],
+      preprocessors: {
+        true: {
+          added: [],
+          removed: ['||foo.com^'],
+        },
+      },
+    });
   });
 });
 

diff --git a/packages/adblocker/test/preprocessor.test.ts b/packages/adblocker/test/preprocessor.test.ts
@@ -239,3 +239,43 @@ describe('preprocessors', () => {
     );
   });
 });
+
+describe('updating', () => {
+  it('correctly distinguishes filters and filters with conditions', () => {
+    const engine = FilterEngine.parse(`||foo.com^`, {
+      loadPreprocessors: true,
+    });
+
+    engine.updateFromDiff({
+      preprocessors: {
+        true: {
+          added: ['||foo.com^'],
+        },
+      },
+    });
+
+    expect(engine.getFilters().networkFilters.length).to.be.eql(1);
+
+    engine.updateFromDiff({
+      preprocessors: {
+        true: {
+          removed: ['||foo.com^'],
+        },
+      },
+    });
+
+    expect(engine.getFilters().networkFilters.length).to.be.eql(1);
+
+    const youtubeFilter = 'youtube.com##+js(test)';
+    engine.updateFromDiff({
+      added: [youtubeFilter],
+      preprocessors: {
+        true: {
+          added: [youtubeFilter],
+        },
+      },
+    });
+
+    expect(engine.cosmetics.getFilters()).to.have.length(1);
+  });
+});