core: fix quoted & repeated tag/color filters in search query (#10269)

* core: fix quoted & repeated tag/color filters in search query
* fix extra quotes being added for multi word tag/color filter
* fix multiple tag/color filters not working

Signed-off-by: 01zulfi <85733202+01zulfi@users.noreply.github.com>

* core: prevent db.lookup.notesWithHighlighting from mutating the passed in selector

The FilteredSelector passed into notesWithHighlighting was being mutated. This wasn't an issue in the apps/web because a fresh selector was passed in for every query. But on apps/mobile, the same selector is reused for every query on the search page.

The bug only became apparent when passing in a query like `tag:one`, then after the results came in, continuing the query: `tag:one tag:two`. This only showed results from tag:one since the selector was mutated by the previous lookup.

The mutation happens in FilteredSelector's where method. So, the bug is fixed by cloning the selector before the where method call in notesWithHighlighting.

Signed-off-by: 01zulfi <85733202+01zulfi@users.noreply.github.com>

---------

Signed-off-by: 01zulfi <85733202+01zulfi@users.noreply.github.com>

packages/core/__tests__/lookup.test.js

@@ -209,4 +209,39 @@ describe("notesWithHighlighting", () => {
       );
       expect(searchWithDiacritics.length).toBe(4);
     }));
+
+  test("should not reuse filters (aka mutate selector) between searches", () =>
+    databaseTest().then(async (db) => {
+      const note1Id = await db.notes.add({ title: "note 1" });
+      const note2Id = await db.notes.add({ title: "note 2" });
+      const note3Id = await db.notes.add({ title: "note 3" });
+      const tag1Id = await db.tags.add({ title: "daily" });
+      const tag2Id = await db.tags.add({ title: "academia" });
+      await db.relations.add(
+        { id: tag1Id, type: "tag" },
+        { id: note1Id, type: "note" }
+      );
+      await db.relations.add(
+        { id: tag2Id, type: "tag" },
+        { id: note2Id, type: "note" }
+      );
+
+      /**
+       * this test ensures that the selector passed to `notesWithHighlighting` is not mutated between searches
+       */
+      const selector = db.notes.all;
+
+      const tag1SearchResults = await db.lookup.notesWithHighlighting(
+        "tag:daily",
+        selector
+      );
+      expect(await tag1SearchResults.ids()).toEqual([note1Id]);
+      expect(await selector.ids()).toEqual([note1Id, note2Id, note3Id]);
+      const bothTagResults = await db.lookup.notesWithHighlighting(
+        "tag:daily tag:academia",
+        selector
+      );
+      expect(await bothTagResults.ids()).toEqual([note1Id, note2Id]);
+      expect(await selector.ids()).toEqual([note1Id, note2Id, note3Id]);
+    }));
 });

packages/core/src/api/lookup.ts

@@ -155,6 +155,13 @@ export default class Lookup {
           : [];
 
       const defaultVault = await this.db.vaults.default();
+
+      /**
+       * `notes` is a FilteredSelector whose `where` method mutates the selector.
+       * Thus the selector passed into `notesWithHighlighting` is mutated, which can cause unexpected results.
+       * To avoid this, we clone the selector before `where`.
+       */
+      notes = notes.clone();
       notes = notes.where((eb) => {
         const exprs = [];
         const tagsFilter = this.db.relations

packages/core/src/database/sql-collection.ts

@@ -388,6 +388,17 @@ export class FilteredSelector<T extends Item> {
     this.filter = filter;
   }
 
+  clone() {
+    const selector = new FilteredSelector<T>(
+      this.type,
+      this.filter,
+      this.batchSize
+    );
+    selector._fields = this._fields.slice();
+    selector._limit = this._limit;
+    return selector;
+  }
+
   fields(fields: AnyColumnWithTable<DatabaseSchema, keyof DatabaseSchema>[]) {
     this._fields = fields;
     return this;

packages/core/src/utils/__tests__/query-transformer.test.ts

@@ -77,3 +77,37 @@ for (const [input, expectedOutput] of TRANSFORM_QUERY_TESTS) {
     expect(transformQuery(input).content?.query).toBe(expectedOutput);
   });
 }
+
+test("should remove syntax quotes from multi-word tag filters", () => {
+  expect(transformQuery('tag:"the diary"').tag).toEqual(["the diary"]);
+});
+
+test("should preserve multiple exact tag filter values", () => {
+  expect(transformQuery('tag:"the diary" title:journal tag:work').tag).toEqual([
+    "the diary",
+    "work"
+  ]);
+});
+
+test("should remove syntax quotes from multi-word color filters", () => {
+  expect(transformQuery('color:"deep blue"').color).toEqual(["deep blue"]);
+});
+
+test("should remove syntax quotes added for punctuation in exact filters", () => {
+  expect(transformQuery("tag:foo-bar").tag).toEqual(["foo-bar"]);
+});
+
+test("should separate repeated tag filters", () => {
+  expect(transformQuery("tag:one tag:two").tag).toEqual(["one", "two"]);
+});
+
+test("should separate repeated color filters", () => {
+  expect(transformQuery("color:red color:blue").color).toEqual(["red", "blue"]);
+});
+
+test("should separate repeated quoted tag filters", () => {
+  expect(transformQuery('tag:"one two" tag:"three four"').tag).toEqual([
+    "one two",
+    "three four"
+  ]);
+});

packages/core/src/utils/query-transformer.ts

@@ -38,6 +38,12 @@ type FieldPhraseNode = {
   value: QueryNode;
 };
 
+type FieldToken = {
+  field?: string;
+  fieldOccurrence?: number;
+  token: string;
+};
+
 type OperatorNode = {
   type: "AND" | "OR" | "NOT";
 };
@@ -77,6 +83,8 @@ const SUPPORTED_FIELDS = {
   in_notebook: (ast) => parseBooleanField("in_notebook", ast)
 } satisfies Record<string, (ast: (QueryNode | FieldPhraseNode)[]) => unknown>;
 
+const ARRAY_FIELDS = ["tag", "color"];
+
 function isFieldSupported(field: string) {
   return field in SUPPORTED_FIELDS;
 }
@@ -101,10 +109,15 @@ function parseArrayField(
       (a): a is FieldPhraseNode =>
         a.type === "field_phrase" && a.field === field
     )
-    .map((a) => generateSQL(a.value));
+    .map((a) => unquoteExactValue(generateSQL(a.value)));
   return values.length > 0 ? values : null;
 }
 
+function unquoteExactValue(value: string) {
+  if (!value.startsWith('"') || !value.endsWith('"')) return value;
+  return value.slice(1, -1).replace(/""/g, '"');
+}
+
 function parseDateField(
   field: string,
   ast: (QueryNode | FieldPhraseNode)[]
@@ -146,13 +159,23 @@ function escapeSQLString(str: string): string {
   return str.replace(/"/g, '""');
 }
 
-function tokenizeWithFields(
-  query: string
-): Array<{ field?: string; token: string }> {
-  const tokens: Array<{ field?: string; token: string }> = [];
+function tokenizeWithFields(query: string): FieldToken[] {
+  const tokens: FieldToken[] = [];
   let buffer = "";
   let isQuoted = false;
   let currentField: string | undefined = undefined;
+  let fieldOccurrence = 0;
+
+  const pushToken = () => {
+    if (buffer.length > 0) {
+      tokens.push({
+        field: currentField,
+        fieldOccurrence: currentField ? fieldOccurrence : undefined,
+        token: buffer
+      });
+      buffer = "";
+    }
+  };
 
   for (let i = 0; i < query.length; ++i) {
     const char = query[i];
@@ -160,15 +183,13 @@ function tokenizeWithFields(
       isQuoted = !isQuoted;
     }
     if (char === " " && !isQuoted) {
-      if (buffer.length > 0) {
-        tokens.push({ field: currentField, token: buffer });
-        buffer = "";
-      }
+      pushToken();
     } else if (char === ":" && !isQuoted) {
       // Check for field
       const maybeField = buffer.trim().toLowerCase();
       if (isFieldSupported(maybeField)) {
         currentField = maybeField;
+        fieldOccurrence++;
         buffer = "";
       } else {
         buffer += char;
@@ -177,24 +198,31 @@ function tokenizeWithFields(
       buffer += char;
     }
   }
-  if (buffer.length > 0) tokens.push({ field: currentField, token: buffer });
+  pushToken();
 
   return tokens;
 }
 
 // Helper: group tokens by field
-function groupTokensByField(tokens: Array<{ field?: string; token: string }>) {
+function groupTokensByField(tokens: FieldToken[]) {
   const groups: Array<{ field?: string; tokens: string[] }> = [];
   let currentField: string | undefined = undefined;
+  let currentOccurrence: number | undefined = undefined;
   let currentTokens: string[] = [];
 
-  for (const { field, token } of tokens) {
-    if (field !== currentField) {
+  for (const { field, fieldOccurrence, token } of tokens) {
+    const isArrayField = ARRAY_FIELDS.includes(field || "");
+    const shouldStartNewGroup =
+      field !== currentField ||
+      (isArrayField && fieldOccurrence !== currentOccurrence);
+
+    if (shouldStartNewGroup) {
       if (currentTokens.length > 0) {
         groups.push({ field: currentField, tokens: currentTokens });
         currentTokens = [];
       }
       currentField = field;
+      currentOccurrence = fieldOccurrence;
     }
     currentTokens.push(token);
   }