Skip to content

Commit

Permalink
fix: Fix unreleased global filter code on unusual characters (#976)
Browse files Browse the repository at this point in the history 
* test: Extra global filter tests - 2 fail, git hooks disabled

TODO Make the test more succinct

* test: Interim commit - 1 fail, git hooks disabled

Work towards making the new tests more succinct

* test: Interim commit - multiple fails, git hooks disabled

Work towards making the new tests more succinct

* fix: Escape * character in global filter

* fix: Escape . character in global filter

* test: Remove unwanted console output in test

* fix: Escape + character in global filter

* fix: Escape ? character in global filter

* fix: Escape ^ character in global filter

* fix: Escape $ character in global filter

* comment: Note some special characters that do not fail

when used on their own as global filter.
So a better test will be needed to show why they need to
be escaped.

* fix: Escape { character in global filter

* fix: Escape } character in global filter

* fix: Escape ( character in global filter

* fix: Escape ) character in global filter

* comment: Add comment-out copy of original regexp

* fix: Escape | character in global filter

* fix: Escape [ character in global filter

* fix: Escape ] character in global filter

This fixes the missing escape in the original implementation

* fix: Escape \ character in global filter

* fix: Escape / character in global filter

NB No failing test for this. I was not able to
find a global filter pattern with '/' in that made the tests fail

* test: Tests indicated I should not escape = character

* test: Tests indicated I should not escape ! character

* test: Tests indicated I should not escape : character

* comment: Remove comments showing original escapeRegExp()

This is a manual revert of 2fe7b65

* comment: Reinstate * which I lost in merge

* comment: Add explanation of non-escaped characters

I copied the text from @AnnaKornfeldSimpson's explanation in:
esm7#18 (comment)

Co-Authored-By: Anna Kornfeld Simpson <AnnaKornfeldSimpson@users.noreply.github.com>

* comment: Add to explain the character selection in test

Co-authored-by: Erez Shermer <erezshermer@gmail.com>
Co-authored-by: Anna Kornfeld Simpson <AnnaKornfeldSimpson@users.noreply.github.com>
  • Loading branch information
@claremacrae @esm7 @AnnaKornfeldSimpson
3 people authored Aug 4, 2022
1 parent e7c0f16 commit 427459b
Show file tree
Hide file tree
Showing 2 changed files with 144 additions and 1 deletion.
15 changes: 14 additions & 1 deletion src/Task.ts
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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');
}

/**
Expand Down
130 changes: 130 additions & 0 deletions tests/Task.test.ts
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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 }) => {
Expand All @@ -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);
}
},
);
});

0 comments on commit 427459b

Please sign in to comment.