AG-36118: fixed duplicate IDs of converted dynamic rules

Merge in ADGUARD-FILTERS/tsurlfilter from fix/AG-36118 to release/v3.1

Squashed commit of the following:

commit bc1743e
Merge: 3bff0a6 2d34890
Author: Dmitriy Seregin <[email protected]>
Date:   Fri Sep 20 16:57:05 2024 +0300

    Merge branch 'release/v3.1' into fix/AG-36118

commit 3bff0a6
Author: Dmitriy Seregin <[email protected]>
Date:   Fri Sep 20 16:41:45 2024 +0300

    added some comments

commit 7a923a4
Author: Dmitriy Seregin <[email protected]>
Date:   Fri Sep 20 14:43:09 2024 +0300

    AG-36118: fixed duplicate IDs of converted dynamic rules

packages/tsurlfilter/CHANGELOG.md

@@ -8,6 +8,15 @@ and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0
 <!-- TODO: manually add compare links for version changes -->
 <!-- e.g. [1.0.77]: https://github.com/AdguardTeam/tsurlfilter/compare/tsurlfilter-v1.0.76...tsurlfilter-v1.0.77 -->
 
+## [3.1.0-alpha.4] - 2024-09-20
+
+### Fixed
+
+- In some cases dynamic rules can have not unique IDs [AdguardBrowserExtension#2953].
+
+[3.1.0-alpha.4]: https://github.com/AdguardTeam/tsurlfilter/releases/tag/tsurlfilter-v3.1.0-alpha.4
+[AdguardBrowserExtension#2953]: https://github.com/AdguardTeam/AdguardBrowserExtension/issues/2953
+
 ## [3.0.3] - 2024-09-19
 
 ### Changed

packages/tsurlfilter/package.json

@@ -1,6 +1,6 @@
 {
   "name": "@adguard/tsurlfilter",
-  "version": "3.1.0-alpha.3",
+  "version": "3.1.0-alpha.4",
   "description": "This is a TypeScript library that implements AdGuard's content blocking rules",
   "main": "dist/es/index.js",
   "module": "dist/es/index.js",

packages/tsurlfilter/src/rules/declarative-converter/rules-converter.ts

@@ -158,9 +158,9 @@ export class DeclarativeRulesConverter {
         };
 
         for (const [filterId, groupedRules] of filters) {
-            const lastUsedId = converted.declarativeRules.length > 0
-                ? converted.declarativeRules[converted.declarativeRules.length - 1].id + 1
-                : DeclarativeRulesConverter.START_DECLARATIVE_RULE_ID;
+            const highestUsedId = converted.declarativeRules.reduce((currentMax, rule) => {
+                return Math.max(currentMax, rule.id);
+            }, DeclarativeRulesConverter.START_DECLARATIVE_RULE_ID);
 
             const {
                 sourceMapValues,
@@ -170,7 +170,7 @@ export class DeclarativeRulesConverter {
             } = await this.convertRules(
                 filterId,
                 groupedRules,
-                lastUsedId,
+                highestUsedId,
                 options,
             );
 
@@ -185,6 +185,10 @@ export class DeclarativeRulesConverter {
             options?.maxNumberOfRegexpRules,
         );
 
+        if (!this.checkRulesHaveUniqueIds(converted.declarativeRules)) {
+            throw new Error('Declarative rules have non-unique identifiers.');
+        }
+
         return converted;
     }
 
@@ -235,6 +239,21 @@ export class DeclarativeRulesConverter {
         return converted;
     }
 
+    /**
+     * Checks that declarative rules have unique identifiers.
+     *
+     * @param rules List of declarative rules.
+     *
+     * @returns True if all rules have unique identifiers, otherwise false.
+     */
+    private static checkRulesHaveUniqueIds(rules: DeclarativeRule[]): boolean {
+        const ids = rules.map(({ id }) => id);
+
+        const uniqueIds = new Set(ids);
+
+        return uniqueIds.size === rules.length;
+    }
+
     /**
      * Check that declarative rules matches the specified constraints and
      * cuts rules if needed as from list also from source map.

packages/tsurlfilter/test/rules/declarative-converter/declarative-converter.test.ts

@@ -757,5 +757,47 @@ describe('DeclarativeConverter', () => {
             expect(errors).toHaveLength(1);
             expect(declarativeRules).toHaveLength(0);
         });
+
+        // In old logic we used last rule id from previous filter to calculate
+        // the id of the first rule in the next filter (last used plus one).
+        // But in some cases it could lead to the same id for different rules,
+        // for example when we have grouping rules in the first filter and
+        // last converted rule will have not the highest id.
+        it('while convert dynamic rules each rule should have unique id', async () => {
+            const filter1 = createFilter([
+                'example.com', // id = 5
+                '||test.com$removeparam=p1', // id = 27
+                '||test.com$removeparam=p2', // id = 70
+                'example.org', // id = 113
+            ], 1);
+            // Length of first rule should be equal to = last rule id from filter1 - second last rule id + bytes needed
+            // to serialize the rule node (which brings some extra offset, depends on the rule).
+            const filter2 = createFilter([
+                'm7sk4ubcalu89gp1q1elsulnhslt2vykalsdjfcvkldfncchfcvcc.zsxnbcfidxyhcwhc',
+                'bad.rule',
+            ], 2);
+
+            const { ruleSet } = await converter.convertDynamicRuleSets([
+                filter1, filter2,
+            ], []);
+            const declarativeRules = await ruleSet.getDeclarativeRules();
+
+            // 5 is because of 2 removeparam rules are converted into 1 declarative rule.
+            expect(declarativeRules).toHaveLength(5);
+
+            // Function to bring more human readable error message.
+            const checkForUniqueRule = (rules: typeof declarativeRules) => {
+                rules.forEach((rule, index) => {
+                    for (let i = 0; i < rules.length; i += 1) {
+                        if (rule.id === rules[i].id && i !== index) {
+                            // eslint-disable-next-line max-len
+                            throw new Error(`Rules have the same id: ${JSON.stringify(rule, null, 2)}, ${JSON.stringify(rules[i], null, 2)}`);
+                        }
+                    }
+                });
+            };
+
+            expect(() => checkForUniqueRule(declarativeRules)).not.toThrow();
+        });
     });
 });