fix(web):Add validation on updateTags request to ensure that an empty…

… tagName doesn't attach all tags to a bookmark #421 (#428) * [Bug Report] Importing bookmarks adds all tags to all bookmarks #421 fixed an issue that caused all existing tags to be assigned to a new bookmark * Add validation on the input of update tags to ensure that its not empty --------- Co-authored-by: MohamedBassem <[email protected]>
hoarder-app · Sep 26, 2024 · 12d3371 · 12d3371
1 parent e1feece
commit 12d3371
Show file tree

Hide file tree

Showing 2 changed files with 39 additions and 10 deletions.
diff --git a/packages/trpc/routers/bookmarks.test.ts b/packages/trpc/routers/bookmarks.test.ts
@@ -169,7 +169,22 @@ describe("Bookmark Routes", () => {
       "tag2",
       "tag3",
       "tag4",
+      "tag5",
     ]);
+
+    await expect(() =>
+      api.updateTags({ bookmarkId: bookmark.id, attach: [{}], detach: [] }),
+    ).rejects.toThrow(/You must provide either a tagId or a tagName/);
+    await expect(() =>
+      api.updateTags({ bookmarkId: bookmark.id, attach: [], detach: [{}] }),
+    ).rejects.toThrow(/You must provide either a tagId or a tagName/);
+    await expect(() =>
+      api.updateTags({
+        bookmarkId: bookmark.id,
+        attach: [{ tagName: "" }],
+        detach: [{}],
+      }),
+    ).rejects.toThrow(/You must provide either a tagId or a tagName/);
   });
 
   test<CustomTestContext>("update bookmark text", async ({ apiCallers }) => {

diff --git a/packages/trpc/routers/bookmarks.ts b/packages/trpc/routers/bookmarks.ts
@@ -678,18 +678,28 @@ export const bookmarksAppRouter = router({
       z.object({
         bookmarkId: z.string(),
         attach: z.array(
-          z.object({
-            // At least one of the two must be set
-            tagId: z.string().optional(), // If the tag already exists and we know its id we should pass it
-            tagName: z.string().optional(),
-          }),
+          z
+            .object({
+              // At least one of the two must be set
+              tagId: z.string().optional(), // If the tag already exists and we know its id we should pass it
+              tagName: z.string().optional(),
+            })
+            .refine((val) => !!val.tagId || !!val.tagName, {
+              message: "You must provide either a tagId or a tagName",
+              path: ["tagId", "tagName"],
+            }),
         ),
         detach: z.array(
-          z.object({
-            // At least one of the two must be set
-            tagId: z.string().optional(),
-            tagName: z.string().optional(), // Also allow removing by tagName, to make CLI usage easier
-          }),
+          z
+            .object({
+              // At least one of the two must be set
+              tagId: z.string().optional(),
+              tagName: z.string().optional(), // Also allow removing by tagName, to make CLI usage easier
+            })
+            .refine((val) => !!val.tagId || !!val.tagName, {
+              message: "You must provide either a tagId or a tagName",
+              path: ["tagId", "tagName"],
+            }),
         ),
       }),
     )
@@ -767,6 +777,9 @@ export const bookmarksAppRouter = router({
             .returning();
         }
 
+        // If there is nothing to add, the "or" statement will become useless and
+        // the query below will simply select all the existing tags for this user and assign them to the bookmark
+        invariant(toAddTagNames.length > 0 || toAddTagIds.length > 0);
         const allIds = (
           await tx.query.bookmarkTags.findMany({
             where: and(
@@ -797,6 +810,7 @@ export const bookmarksAppRouter = router({
             })),
           )
           .onConflictDoNothing();
+
         await triggerSearchReindex(input.bookmarkId);
         return {
           bookmarkId: input.bookmarkId,