Repository: obsidian-tasks-group/obsidian-tasks
Pull request #976 (2022-08-04 17:24:05)

fix: Fix unreleased global filter code on unusual characters
<!--- Provide a general summary of your changes in the Title above -->

# Description

This is a small improvement to the code added in #869. The method `Task.escapeRegExp()` added in that PR created invalid regular expressions with some characters.

This was initially only detectable by manual testing, using unusual characters in the global filter.

Changes in this PR:

- Systematically test the removal of global filters with all the special characters that were being escaped by `Task.escapeRegExp()`
- Fix `Task.escapeRegExp()` to that it does not generated broken regexes for any of the tests

## Motivation and Context

Before this PR, `Task.escapeRegExp()` was not covered by any tests.

I wanted to make sure it was robustly tested and now works, to enable me to release the PR #869, which stops 'Add & edit Task' breaking sub-tags of global filter.

## How has this been tested?

Very thoroughly, via TDD - writing the failing test, then fixing the code.

## Screenshots (if appropriate)

## Types of changes
<!--- What types of changes does your code introduce? Put an `x` in all the boxes that apply: -->
- [x] Bug fix (non-breaking change which fixes an issue)
- [ ] Refactor (non-breaking change which only improves the design or structure of existing code, and making no changes to its external behaviour)
- [ ] New feature (non-breaking change which adds functionality)
- [ ] Breaking change (fix or feature that would cause existing functionality to not work as expected)

## Checklist
<!--- Go over all the following points, and put an `x` in all the boxes that apply. -->
<!--- If you're unsure about any of these, don't hesitate to ask. We're here to help! -->
- [x] My code follows the code style of this project and passes `yarn run lint`.
- [ ] My change requires a change to the documentation.
- [ ] I have updated the documentation accordingly.
- [x] My change has adequate Unit Test coverage.

By creating a Pull Request you agree to our [Code of Conduct](https://github.com/obsidian-tasks-group/obsidian-tasks/blob/main/CODE_OF_CONDUCT.md). For further guidance on contributing please see [contributing guide](https://github.com/obsidian-tasks-group/obsidian-tasks/blob/main/CONTRIBUTING.md)



Patch:
diff --git a/src/Task.ts b/src/Task.ts
index 4365d24b..436090d6 100644
--- a/src/Task.ts
+++ b/src/Task.ts
@@ -867,7 +867,20 @@ export class Task {
      * Taken from https://developer.mozilla.org/en-US/docs/Web/JavaScript/Guide/Regular_Expressions#escaping
      */
     private escapeRegExp(s: string) {
-        return s.replace(/([.*+?^=!:${}()|[]\/\\])/g, '\\$1');
+        // NOTE: = is not escaped, as doing so gives error:
+        //         Invalid regular expression: /(^|\s)hello\=world($|\s)/: Invalid escape
+        // NOTE: ! is not escaped, as doing so gives error:
+        //         Invalid regular expression: /(^|\s)hello\!world($|\s)/: Invalid escape
+        // NOTE: : is not escaped, as doing so gives error:
+        //         Invalid regular expression: /(^|\s)hello\:world($|\s)/: Invalid escape
+        //
+        // Explanation from @AnnaKornfeldSimpson in:
+        // https://github.com/esm7/obsidian-tasks/pull/18#issuecomment-1196115407
+        // From what I can tell, the three missing characters from the original regex - : ! =
+        // are all only considered to have special meanings if they directly follow
+        // a ? (all 3) or a ?< (! and =).
+        // So theoretically if the ? are all escaped, those three characters do not have to be.
+        return s.replace(/([.*+?^${}()|[\]/\\])/g, '\\$1');
     }
 
     /**


Test patch:
diff --git a/tests/Task.test.ts b/tests/Task.test.ts
index 7ea6a26c..dcaa78a8 100644
--- a/tests/Task.test.ts
+++ b/tests/Task.test.ts
@@ -979,6 +979,11 @@ describe('check removal of the global filter', () => {
             markdownTask: '- [ ] task with #t multiple global filters #t',
             expectedDescription: 'task with multiple global filters',
         },
+        {
+            globalFilter: '#t',
+            markdownTask: '- [ ] #t', // confirm behaviour when the description is empty
+            expectedDescription: '',
+        },
     ])(
         'should parse "$markdownTask" and extract "$expectedDescription"',
         ({ globalFilter, markdownTask, expectedDescription }) => {
@@ -1004,3 +1009,128 @@ describe('check removal of the global filter', () => {
         },
     );
 });
+
+describe('check removal of the global filter exhaustively', () => {
+    type GlobalFilterRemoval = {
+        globalFilter: string;
+    };
+
+    function checkDescription(
+        markdownLine: string,
+        expectedDescription: string,
+    ) {
+        const task = constructTaskFromLine(markdownLine);
+
+        // Assert
+        expect(task).not.toBeNull();
+        expect(task!.getDescriptionWithoutGlobalFilter()).toEqual(
+            expectedDescription,
+        );
+    }
+
+    test.each<GlobalFilterRemoval>([
+        {
+            globalFilter: '#t',
+        },
+        // The characters listed below are the ones that are - or were - escaped by
+        // Task.escapeRegExp().
+        // See the developer.mozilla.org reference in that method.
+        // This test validates the escaping of each of those characters.
+        {
+            globalFilter: '.',
+        },
+        {
+            globalFilter: '*',
+        },
+        {
+            globalFilter: '+',
+        },
+        {
+            globalFilter: '?',
+        },
+        {
+            globalFilter: '^',
+        },
+        {
+            // Failed attempt at creating a failing test for when = was not escaped.
+            // When I make Task.escapeRegExp() escape =, I get:
+            // Invalid regular expression: /(^|\s)hello\=world($|\s)/: Invalid escape
+            globalFilter: 'hello=world',
+        },
+        {
+            // Failed attempt at creating a failing test for when ! was not escaped.
+            // When I make Task.escapeRegExp() escape !, I get:
+            // Invalid regular expression: /(^|\s)hello\!world($|\s)/: Invalid escape
+            globalFilter: 'hello!world',
+        },
+        {
+            // Failed attempt at creating a failing test for when : was not escaped.
+            // When I make Task.escapeRegExp() escape :, I get:
+            // Invalid regular expression: /(^|\s)hello\:world($|\s)/: Invalid escape
+            globalFilter: 'hello:world',
+        },
+        {
+            globalFilter: '$',
+        },
+        {
+            globalFilter: '{',
+        },
+        {
+            globalFilter: '}',
+        },
+        {
+            globalFilter: '(',
+        },
+        {
+            globalFilter: ')',
+        },
+        {
+            globalFilter: '|',
+        },
+        {
+            globalFilter: '[',
+        },
+        {
+            globalFilter: ']',
+        },
+        {
+            // Failed attempt at creating a failing test for when / was not escaped
+            globalFilter: '///',
+        },
+        {
+            globalFilter: '\\',
+        },
+    ])(
+        'should parse global filter "$globalFilter" edge cases correctly',
+        ({ globalFilter }) => {
+            // Arrange
+            const originalSettings = getSettings();
+            if (globalFilter != '') {
+                updateSettings({ globalFilter: globalFilter });
+            }
+
+            // Act
+
+            // global filter removed at beginning, middle and end
+            let markdownLine = `- [ ] ${globalFilter} 1 ${globalFilter} 2 ${globalFilter}`;
+            let expectedDescription = '1 2';
+            checkDescription(markdownLine, expectedDescription);
+
+            // global filter not removed if non-empty non-tag characters before or after it
+            markdownLine = `- [ ] ${globalFilter}x 1 x${globalFilter} ${globalFilter}x 2 x${globalFilter}`;
+            expectedDescription = `${globalFilter}x 1 x${globalFilter} ${globalFilter}x 2 x${globalFilter}`;
+            checkDescription(markdownLine, expectedDescription);
+
+            // global filter not removed if non-empty sub-tag characters after it.
+            // Include at least one occurrence of global filter, so we don't pass by luck.
+            markdownLine = `- [ ] ${globalFilter}/x 1 x${globalFilter} ${globalFilter}/x 2 ${globalFilter} ${globalFilter}/x`;
+            expectedDescription = `${globalFilter}/x 1 x${globalFilter} ${globalFilter}/x 2 ${globalFilter}/x`;
+            checkDescription(markdownLine, expectedDescription);
+
+            // Cleanup
+            if (globalFilter != '') {
+                updateSettings(originalSettings);
+            }
+        },
+    );
+});