diff --git a/services/notifications/app.ts b/services/notifications/app.ts index b23fe24a32..697cbae298 100644 --- a/services/notifications/app.ts +++ b/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,16 +55,8 @@ 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) - } + req.logger.setLevel('error') + res.sendStatus(500) } app.use(handleApiError) diff --git a/services/notifications/app/js/HealthCheckController.ts b/services/notifications/app/js/HealthCheckController.ts index c0fbd9206d..1680af82eb 100644 --- a/services/notifications/app/js/HealthCheckController.ts +++ b/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(), }) diff --git a/services/notifications/app/js/Notifications.js b/services/notifications/app/js/Notifications.js index 1431ed7a4f..5b9e29d780 100644 --- a/services/notifications/app/js/Notifications.js +++ b/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() diff --git a/services/notifications/app/js/NotificationsController.ts b/services/notifications/app/js/NotificationsController.ts index 08e36acbcc..b73a089812 100644 --- a/services/notifications/app/js/NotificationsController.ts +++ b/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 = diff --git a/services/notifications/test/acceptance/js/NotificationsApi.test.ts b/services/notifications/test/acceptance/js/NotificationsApi.test.ts new file mode 100644 index 0000000000..c327118156 --- /dev/null +++ b/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 | 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) + }) + }) +}) diff --git a/services/notifications/test/unit/js/NotificationsController.test.ts b/services/notifications/test/unit/js/NotificationsController.test.ts index af5f1a1e2c..aad544998c 100644 --- a/services/notifications/test/unit/js/NotificationsController.test.ts +++ b/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(resolve => { controller.addNotification(req, { sendStatus: code => { expect(Notifications.addNotification).toHaveBeenCalledWith( userId, - stubbedNotification[0] + notification ) expect(code).toBe(200) resolve() diff --git a/services/notifications/vitest.config.acceptance.cjs b/services/notifications/vitest.config.acceptance.cjs index 23a1e2f6d5..6e3365d5e8 100644 --- a/services/notifications/vitest.config.acceptance.cjs +++ b/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, },