[notifications] adopt strict zod types and extend test coverage (#36049)

* [notifications] adopt strict zod types and extend test coverage

* [notifications] validate expires date via schema

GitOrigin-RevId: 25dee0578ec443447e4c38a50a7e6c8b44831fe7

services/notifications/app.ts

@@ -12,10 +12,7 @@ import express, {
 import { mongoClient } from './app/js/mongodb.js'
 import NotificationsController from './app/js/NotificationsController.ts'
 import HealthCheckController from './app/js/HealthCheckController.ts'
-import {
-  InvalidParamsError,
-  InvalidRequestError,
-} from '@overleaf/validation-tools'
+import { handleValidationError } from '@overleaf/validation-tools'
 
 const app = express()
 
@@ -49,6 +46,8 @@ app.get('/health_check', HealthCheckController.check)
 
 app.get('*', (req, res) => res.sendStatus(404))
 
+app.use(handleValidationError)
+
 const handleApiError: ErrorRequestHandler = (
   err: Error,
   req: Request,
@@ -56,17 +55,9 @@ const handleApiError: ErrorRequestHandler = (
   next: NextFunction
 ) => {
   req.logger.addFields({ err })
-  if (err instanceof InvalidParamsError) {
-    req.logger.setLevel('warn')
-    res.sendStatus(404)
-  } else if (err instanceof InvalidRequestError) {
-    req.logger.setLevel('warn')
-    res.sendStatus(400)
-  } else {
   req.logger.setLevel('error')
   res.sendStatus(500)
 }
-}
 
 app.use(handleApiError)
 

services/notifications/app/js/HealthCheckController.ts

@@ -24,9 +24,11 @@ async function makeNotification(notificationKey: string, userId: string) {
     method: 'POST',
     json: {
       key: notificationKey,
-      messageOpts: '',
+      // must be an object: addNotification's schema models messageOpts as
+      // an arbitrary key/value map (see NotificationsController.ts), not a
+      // bare string
+      messageOpts: {},
       templateKey: 'f4g5',
-      user_id: userId,
     },
     signal: AbortSignal.timeout(5000),
   }
@@ -38,7 +40,7 @@ const getUserNotificationsResponseSchema = z
   .object({
     _id: zz.objectId(),
     key: z.string(),
-    messageOpts: z.string().optional(),
+    messageOpts: z.record(z.string(), z.unknown()).optional(),
     templateKey: z.string().optional(),
     user_id: zz.objectId(),
   })

services/notifications/app/js/Notifications.js

@@ -35,6 +35,8 @@ async function addNotification(userId, notification, callback) {
   if (notification.expires != null) {
     try {
       doc.expires = new Date(notification.expires)
+      // Rollout-temporary fallback (validation of fallback schema); delete
+      // when this route's REQ_VALIDATION_MODE instrumentation is removed.
       // _testValue assignment will throw if `expires` is not a valid date
       // eslint-disable-next-line no-unused-vars
       const _testValue = doc.expires.toISOString()

services/notifications/app/js/NotificationsController.ts

@@ -6,13 +6,22 @@ import { parseReq, z, zz } from '@overleaf/validation-tools'
 import type { Request, Response } from 'express'
 
 const getUserNotificationsSchema = z.object({
+  params: z.strictObject({
+    user_id: zz.objectId(),
+  }),
+})
+// Rollout-temporary fallback (pre-refinement schema from main); delete
+// when this route's REQ_VALIDATION_MODE instrumentation is removed.
+const getUserNotificationsFallbackSchema = z.object({
   params: z.object({
     user_id: zz.objectId(),
   }),
 })
 
 async function getUserNotifications(req: Request, res: Response) {
-  const { params } = parseReq(req, getUserNotificationsSchema)
+  const { params } = parseReq(req, getUserNotificationsSchema, {
+    fallbackSchema: getUserNotificationsFallbackSchema,
+  })
   logger.debug({ userId: params.user_id }, 'getting user unread notifications')
   metrics.inc('getUserNotifications')
   const notifications = await Notifications.getUserNotifications(params.user_id)
@@ -20,6 +29,28 @@ async function getUserNotifications(req: Request, res: Response) {
 }
 
 const addNotificationSchema = z.object({
+  params: z.strictObject({
+    user_id: zz.objectId(),
+  }),
+  body: z.strictObject({
+    key: z.string(),
+    templateKey: z.string(),
+    // Arbitrary per-template render params: different notification types
+    // populate completely different keys here (see web's
+    // NotificationsBuilder.mjs, the only real production caller), so this is
+    // a genuinely open map rather than an approximated shape.
+    messageOpts: z.record(z.string(), z.unknown()).optional(),
+    // web's NotificationsHandler.createNotification always sends this (it
+    // defaults to `true` client-side before serializing), but
+    // Notifications.js already tolerates a missing value as falsy (skip
+    // overwriting an existing notification), so keep it optional here too.
+    forceCreate: z.boolean().optional(),
+    expires: z.iso.datetime().optional(),
+  }),
+})
+// Rollout-temporary fallback (pre-refinement schema from main); delete
+// when this route's REQ_VALIDATION_MODE instrumentation is removed.
+const addNotificationFallbackSchema = z.object({
   params: z.object({
     user_id: zz.objectId(),
   }),
@@ -27,7 +58,9 @@ const addNotificationSchema = z.object({
 })
 
 async function addNotification(req: Request, res: Response) {
-  const { params, body } = parseReq(req, addNotificationSchema)
+  const { params, body } = parseReq(req, addNotificationSchema, {
+    fallbackSchema: addNotificationFallbackSchema,
+  })
   logger.debug(
     { userId: params.user_id, notification: body },
     'adding notification'
@@ -42,6 +75,14 @@ async function addNotification(req: Request, res: Response) {
 }
 
 const removeNotificationIdSchema = z.object({
+  params: z.strictObject({
+    user_id: zz.objectId(),
+    notification_id: zz.objectId(),
+  }),
+})
+// Rollout-temporary fallback (pre-refinement schema from main); delete
+// when this route's REQ_VALIDATION_MODE instrumentation is removed.
+const removeNotificationIdFallbackSchema = z.object({
   params: z.object({
     user_id: zz.objectId(),
     notification_id: zz.objectId(),
@@ -49,11 +90,13 @@ const removeNotificationIdSchema = z.object({
 })
 
 async function removeNotificationId(req: Request, res: Response) {
-  const { params } = parseReq(req, removeNotificationIdSchema)
+  const { params } = parseReq(req, removeNotificationIdSchema, {
+    fallbackSchema: removeNotificationIdFallbackSchema,
+  })
   logger.debug(
     {
-      userId: req.params.user_id,
-      notificationId: req.params.notification_id,
+      userId: params.user_id,
+      notificationId: params.notification_id,
     },
     'mark id notification as read'
   )
@@ -66,6 +109,16 @@ async function removeNotificationId(req: Request, res: Response) {
 }
 
 const removeNotificationKeySchema = z.object({
+  params: z.strictObject({
+    user_id: zz.objectId(),
+  }),
+  body: z.strictObject({
+    key: z.string(),
+  }),
+})
+// Rollout-temporary fallback (pre-refinement schema from main); delete
+// when this route's REQ_VALIDATION_MODE instrumentation is removed.
+const removeNotificationKeyFallbackSchema = z.object({
   params: z.object({
     user_id: zz.objectId(),
   }),
@@ -75,9 +128,11 @@ const removeNotificationKeySchema = z.object({
 })
 
 async function removeNotificationKey(req: Request, res: Response) {
-  const { params, body } = parseReq(req, removeNotificationKeySchema)
+  const { params, body } = parseReq(req, removeNotificationKeySchema, {
+    fallbackSchema: removeNotificationKeyFallbackSchema,
+  })
   logger.debug(
-    { userId: req.params.user_id, notificationKey: body.key },
+    { userId: params.user_id, notificationKey: body.key },
     'mark key notification as read'
   )
   metrics.inc('removeNotificationKey')
@@ -87,13 +142,22 @@ async function removeNotificationKey(req: Request, res: Response) {
 }
 
 const removeNotificationByKeyOnlySchema = z.object({
+  params: z.strictObject({
+    key: z.string(),
+  }),
+})
+// Rollout-temporary fallback (pre-refinement schema from main); delete
+// when this route's REQ_VALIDATION_MODE instrumentation is removed.
+const removeNotificationByKeyOnlyFallbackSchema = z.object({
   params: z.object({
     key: z.string(),
   }),
 })
 
 async function removeNotificationByKeyOnly(req: Request, res: Response) {
-  const { params } = parseReq(req, removeNotificationByKeyOnlySchema)
+  const { params } = parseReq(req, removeNotificationByKeyOnlySchema, {
+    fallbackSchema: removeNotificationByKeyOnlyFallbackSchema,
+  })
   const notificationKey = params.key
   logger.debug({ notificationKey }, 'mark notification as read by key only')
   metrics.inc('removeNotificationKey')
@@ -102,13 +166,22 @@ async function removeNotificationByKeyOnly(req: Request, res: Response) {
 }
 
 const countNotificationsByKeyOnlySchema = z.object({
+  params: z.strictObject({
+    key: z.string(),
+  }),
+})
+// Rollout-temporary fallback (pre-refinement schema from main); delete
+// when this route's REQ_VALIDATION_MODE instrumentation is removed.
+const countNotificationsByKeyOnlyFallbackSchema = z.object({
   params: z.object({
     key: z.string(),
   }),
 })
 
 async function countNotificationsByKeyOnly(req: Request, res: Response) {
-  const { params } = parseReq(req, countNotificationsByKeyOnlySchema)
+  const { params } = parseReq(req, countNotificationsByKeyOnlySchema, {
+    fallbackSchema: countNotificationsByKeyOnlyFallbackSchema,
+  })
   const notificationKey = params.key
   try {
     const count =
@@ -121,6 +194,13 @@ async function countNotificationsByKeyOnly(req: Request, res: Response) {
 }
 
 const deleteUnreadNotificationsByKeyOnlyBulkSchema = z.object({
+  params: z.strictObject({
+    key: z.string(),
+  }),
+})
+// Rollout-temporary fallback (pre-refinement schema from main); delete
+// when this route's REQ_VALIDATION_MODE instrumentation is removed.
+const deleteUnreadNotificationsByKeyOnlyBulkFallbackSchema = z.object({
   params: z.object({
     key: z.string(),
   }),
@@ -130,7 +210,11 @@ async function deleteUnreadNotificationsByKeyOnlyBulk(
   req: Request,
   res: Response
 ) {
-  const { params } = parseReq(req, deleteUnreadNotificationsByKeyOnlyBulkSchema)
+  const { params } = parseReq(
+    req,
+    deleteUnreadNotificationsByKeyOnlyBulkSchema,
+    { fallbackSchema: deleteUnreadNotificationsByKeyOnlyBulkFallbackSchema }
+  )
   const notificationKey = params.key
   try {
     const count =

services/notifications/test/acceptance/js/NotificationsApi.test.ts

@@ -0,0 +1,295 @@
+import { beforeAll, beforeEach, describe, it, expect } from 'vitest'
+import app from '../../../app.ts'
+import { db, ObjectId } from '../../../app/js/mongodb.js'
+import { expectValidationError } from '@overleaf/validation-tools/testUtils.js'
+import './MongoHelper.ts'
+
+// Use a dedicated port: HealthCheck.test.ts owns the default port (3042) and
+// the vitest workers may run both files in parallel processes.
+const HOST = '127.0.0.1'
+const PORT = 23042
+const BASE_URL = `http://${HOST}:${PORT}`
+
+let runAppPromise: Promise<void> | null = null
+
+async function ensureRunning() {
+  if (!runAppPromise) {
+    runAppPromise = new Promise(resolve => {
+      app.listen(PORT, HOST, () => resolve())
+    })
+  }
+  await runAppPromise
+}
+
+function notificationPayload(key: string) {
+  return {
+    key,
+    messageOpts: { projectName: 'A Project' },
+    templateKey: 'notification_project_invite',
+  }
+}
+
+async function addNotification(userId: string, payload: object) {
+  return await fetch(`${BASE_URL}/user/${userId}`, {
+    method: 'POST',
+    headers: { 'Content-Type': 'application/json' },
+    body: JSON.stringify(payload),
+  })
+}
+
+async function getNotifications(userId: string) {
+  const response = await fetch(`${BASE_URL}/user/${userId}`)
+  expect(response.status).toBe(200)
+  return (await response.json()) as Array<{
+    _id: string
+    user_id: string
+    key: string
+    templateKey?: string
+    messageOpts?: { projectName: string }
+  }>
+}
+
+describe('Notifications API', () => {
+  let userId: string
+  let key: string
+
+  beforeAll(async () => {
+    await ensureRunning()
+  })
+
+  beforeEach(() => {
+    userId = new ObjectId().toString()
+    key = `test-key-${new ObjectId().toString()}`
+  })
+
+  describe('POST /user/:user_id', () => {
+    it('should add a notification', async () => {
+      const response = await addNotification(userId, notificationPayload(key))
+      expect(response.status).toBe(200)
+
+      const notifications = await getNotifications(userId)
+      expect(notifications).toHaveLength(1)
+      expect(notifications[0].user_id).toBe(userId)
+      expect(notifications[0].key).toBe(key)
+      expect(notifications[0].templateKey).toBe('notification_project_invite')
+      expect(notifications[0].messageOpts).toEqual({
+        projectName: 'A Project',
+      })
+    })
+
+    it('should not add a duplicate notification for the same key', async () => {
+      const first = await addNotification(userId, notificationPayload(key))
+      expect(first.status).toBe(200)
+      const second = await addNotification(userId, {
+        ...notificationPayload(key),
+        messageOpts: { projectName: 'Another Project' },
+      })
+      expect(second.status).toBe(200)
+
+      const notifications = await getNotifications(userId)
+      expect(notifications).toHaveLength(1)
+      expect(notifications[0].messageOpts).toEqual({
+        projectName: 'A Project',
+      })
+    })
+
+    it('should overwrite an existing notification with forceCreate', async () => {
+      const first = await addNotification(userId, notificationPayload(key))
+      expect(first.status).toBe(200)
+      const second = await addNotification(userId, {
+        ...notificationPayload(key),
+        messageOpts: { projectName: 'Another Project' },
+        forceCreate: true,
+      })
+      expect(second.status).toBe(200)
+
+      const notifications = await getNotifications(userId)
+      expect(notifications).toHaveLength(1)
+      expect(notifications[0].messageOpts).toEqual({
+        projectName: 'Another Project',
+      })
+    })
+
+    it('should return a 404 for a malformed user id', async () => {
+      const response = await addNotification(
+        'not-an-object-id',
+        notificationPayload(key)
+      )
+      const body = await response.json()
+      expectValidationError({ response, body }, 404, 'user_id')
+    })
+
+    it('should return a 400 for an unknown field in the notification body', async () => {
+      const response = await addNotification(userId, {
+        ...notificationPayload(key),
+        notAField: true,
+      })
+      const body = await response.json()
+      expectValidationError({ response, body }, 400, 'notAField')
+    })
+
+    it('should return a 400 for an invalid expires field', async () => {
+      const response = await addNotification(userId, {
+        ...notificationPayload(key),
+        expires: 'not-a-date',
+      })
+      const body = await response.json()
+      expectValidationError({ response, body }, 400, 'expires')
+    })
+  })
+
+  describe('GET /user/:user_id', () => {
+    it('should return the unread notifications of the user', async () => {
+      await addNotification(userId, notificationPayload(key))
+      const notifications = await getNotifications(userId)
+      expect(notifications).toHaveLength(1)
+      expect(notifications[0].key).toBe(key)
+    })
+
+    it('should return an empty list for a user without notifications', async () => {
+      const notifications = await getNotifications(userId)
+      expect(notifications).toEqual([])
+    })
+
+    it('should return a 404 for a malformed user id', async () => {
+      const response = await fetch(`${BASE_URL}/user/not-an-object-id`)
+      const body = await response.json()
+      expectValidationError({ response, body }, 404, 'user_id')
+    })
+  })
+
+  describe('DELETE /user/:user_id/notification/:notification_id', () => {
+    it('should mark the notification as read', async () => {
+      await addNotification(userId, notificationPayload(key))
+      const [notification] = await getNotifications(userId)
+
+      const response = await fetch(
+        `${BASE_URL}/user/${userId}/notification/${notification._id}`,
+        { method: 'DELETE' }
+      )
+      expect(response.status).toBe(200)
+      expect(await getNotifications(userId)).toEqual([])
+    })
+
+    it('should return a 404 for a malformed notification id', async () => {
+      const response = await fetch(
+        `${BASE_URL}/user/${userId}/notification/not-an-object-id`,
+        { method: 'DELETE' }
+      )
+      const body = await response.json()
+      expectValidationError({ response, body }, 404, 'notification_id')
+    })
+  })
+
+  describe('DELETE /user/:user_id', () => {
+    it('should mark the notification with the given key as read', async () => {
+      await addNotification(userId, notificationPayload(key))
+
+      const response = await fetch(`${BASE_URL}/user/${userId}`, {
+        method: 'DELETE',
+        headers: { 'Content-Type': 'application/json' },
+        body: JSON.stringify({ key }),
+      })
+      expect(response.status).toBe(200)
+      expect(await getNotifications(userId)).toEqual([])
+    })
+
+    it('should return a 400 when the key is missing from the body', async () => {
+      const response = await fetch(`${BASE_URL}/user/${userId}`, {
+        method: 'DELETE',
+        headers: { 'Content-Type': 'application/json' },
+        body: JSON.stringify({}),
+      })
+      const body = await response.json()
+      expectValidationError({ response, body }, 400, 'key')
+    })
+  })
+
+  describe('DELETE /key/:key', () => {
+    it('should mark the notifications with the given key as read for all users', async () => {
+      const otherUserId = new ObjectId().toString()
+      await addNotification(userId, notificationPayload(key))
+      await addNotification(otherUserId, notificationPayload(key))
+
+      const response = await fetch(`${BASE_URL}/key/${key}`, {
+        method: 'DELETE',
+      })
+      expect(response.status).toBe(200)
+
+      // removeNotificationByKeyOnly only updates a single document
+      const unread = [
+        ...(await getNotifications(userId)),
+        ...(await getNotifications(otherUserId)),
+      ]
+      expect(unread).toHaveLength(1)
+    })
+
+    it('should return a 404 without a key', async () => {
+      // Note: this never reaches our zod schema or handleValidationError --
+      // `:key` requires a non-empty segment, so `/key/` doesn't match the
+      // route at all and Express's own default (HTML, non-JSON) 404 handler
+      // responds instead. A body assertion doesn't apply here.
+      const response = await fetch(`${BASE_URL}/key/`, { method: 'DELETE' })
+      expect(response.status).toBe(404)
+    })
+  })
+
+  describe('GET /key/:key/count', () => {
+    it('should count the unread notifications with the given key', async () => {
+      const otherUserId = new ObjectId().toString()
+      await addNotification(userId, notificationPayload(key))
+      await addNotification(otherUserId, notificationPayload(key))
+
+      const response = await fetch(`${BASE_URL}/key/${key}/count`)
+      expect(response.status).toBe(200)
+      expect(await response.json()).toEqual({ count: 2 })
+    })
+
+    it('should return a zero count for an unknown key', async () => {
+      const response = await fetch(`${BASE_URL}/key/${key}/count`)
+      expect(response.status).toBe(200)
+      expect(await response.json()).toEqual({ count: 0 })
+    })
+  })
+
+  describe('DELETE /key/:key/bulk', () => {
+    it('should delete all unread notifications with the given key', async () => {
+      const otherUserId = new ObjectId().toString()
+      await addNotification(userId, notificationPayload(key))
+      await addNotification(otherUserId, notificationPayload(key))
+
+      const response = await fetch(`${BASE_URL}/key/${key}/bulk`, {
+        method: 'DELETE',
+      })
+      expect(response.status).toBe(200)
+      expect(await response.json()).toEqual({ count: 2 })
+
+      expect(await getNotifications(userId)).toEqual([])
+      expect(await getNotifications(otherUserId)).toEqual([])
+      expect(await db.notifications.countDocuments({ key })).toBe(0)
+    })
+
+    it('should return a zero count when there is nothing to delete', async () => {
+      const response = await fetch(`${BASE_URL}/key/${key}/bulk`, {
+        method: 'DELETE',
+      })
+      expect(response.status).toBe(200)
+      expect(await response.json()).toEqual({ count: 0 })
+    })
+  })
+
+  describe('GET /status', () => {
+    it('should respond with a 200', async () => {
+      const response = await fetch(`${BASE_URL}/status`)
+      expect(response.status).toBe(200)
+      expect(await response.text()).toBe('notifications is up')
+    })
+  })
+
+  describe('unknown routes', () => {
+    it('should respond with a 404', async () => {
+      const response = await fetch(`${BASE_URL}/not-a-route`)
+      expect(response.status).toBe(404)
+    })
+  })
+})

services/notifications/test/unit/js/NotificationsController.test.ts

@@ -104,18 +104,22 @@ describe('Notifications Controller', () => {
         modifiedCount: 1,
         upsertedCount: 0,
       })
+      // addNotification's body schema is a strictObject that does not
+      // include user_id (it's only ever read from params) -- drop it here
+      // to match, same as every real caller (web's NotificationsHandler).
+      const { user_id: _user_id, ...notification } = stubbedNotification[0]
       const req = {
         params: {
           user_id: userId,
         },
-        body: stubbedNotification[0],
+        body: notification,
       }
       await new Promise<void>(resolve => {
         controller.addNotification(req, {
           sendStatus: code => {
             expect(Notifications.addNotification).toHaveBeenCalledWith(
               userId,
-              stubbedNotification[0]
+              notification
             )
             expect(code).toBe(200)
             resolve()

services/notifications/vitest.config.acceptance.cjs

@@ -19,6 +19,12 @@ module.exports = defineConfig({
   test: {
     include: ['test/acceptance/js/**/*.test.{js,ts}'],
     isolate: false,
+    // Acceptance files share a single mongo instance and each runs the
+    // migration bootstrap in its own `beforeAll` (see MongoHelper.ts).
+    // Running files in parallel workers races two bootstrap runs against
+    // the same "create the migrations collection" migration. Force
+    // sequential file execution so only one bootstrap runs at a time.
+    fileParallelism: false,
     testNamePattern: process.env.TEST_NAME_PATTERN || undefined,
     ...reporterOptions,
   },