From 43839e9372b902f2a256c1018bf173ec0fb3afc7 Mon Sep 17 00:00:00 2001 From: Iain Sproat <68657+iainsproat@users.noreply.github.com> Date: Fri, 16 May 2025 16:12:55 +0100 Subject: [PATCH 1/5] fix(server/workspaces): gracefully handle duplicate workspace join requests --- .../integration/workspaceJoinRequests.spec.ts | 79 +++++++++++++++++++ 1 file changed, 79 insertions(+) diff --git a/packages/server/modules/workspaces/tests/integration/workspaceJoinRequests.spec.ts b/packages/server/modules/workspaces/tests/integration/workspaceJoinRequests.spec.ts index ada605204..f08a6b006 100644 --- a/packages/server/modules/workspaces/tests/integration/workspaceJoinRequests.spec.ts +++ b/packages/server/modules/workspaces/tests/integration/workspaceJoinRequests.spec.ts @@ -233,6 +233,85 @@ const { FF_WORKSPACES_MODULE_ENABLED } = getFeatureFlags() ) expect(sendWorkspaceJoinRequestReceivedEmailCalls[0].requester).to.equal(user) }) + it('gracefully handles a duplicate request', async () => { + // insert into "workspace_join_requests" ("status", "userId", "workspaceId") values ($1, $2, $3) returning * - duplicate key value violates unique constraint "workspace_join_requests_pkey" + const createWorkspaceJoinRequest = createWorkspaceJoinRequestFactory({ db }) + + const sendWorkspaceJoinRequestReceivedEmailCalls: Parameters[number][] = + [] + const sendWorkspaceJoinRequestReceivedEmail = async ( + args: Parameters[number] + ) => sendWorkspaceJoinRequestReceivedEmailCalls.push(args) + + const user: BasicTestUser = { + id: '', + name: 'John Speckle', + email: `${createRandomString()}@example.org`, + role: Roles.Server.Admin, + verified: true + } + + await createTestUser(user) + + const workspace: BasicTestWorkspace = { + id: '', + slug: '', + ownerId: '', + name: cryptoRandomString({ length: 6 }), + description: cryptoRandomString({ length: 12 }), + discoverabilityEnabled: true + } + await createTestWorkspace(workspace, user, { domain: 'example.org' }) + const domain = { + id: cryptoRandomString({ length: 10 }), + workspaceId: workspace.id, + domain: 'example.org', + verified: true, + createdAt: new Date(), + createdByUserId: user.id, + updatedAt: new Date() + } + + const requestToJoinWorkspace = await requestToJoinWorkspaceFactory({ + createWorkspaceJoinRequest, + sendWorkspaceJoinRequestReceivedEmail: + sendWorkspaceJoinRequestReceivedEmail as unknown as SendWorkspaceJoinRequestReceivedEmail, + getUserById: async () => user as unknown as UserWithOptionalRole, + getWorkspaceWithDomains: async () => + ({ + ...workspace, + domains: [domain] + } as unknown as WorkspaceWithDomains), + getUserEmails: async () => + [{ email: user.email, verified: true }] as unknown as UserEmail[] + }) + + expect( + requestToJoinWorkspace({ workspaceId: workspace.id, userId: user.id }) + ).to.equal(true) + + expect( + (await db(WorkspaceJoinRequests.name) + .where({ + workspaceId: workspace.id, + userId: user.id + }) + .select('status') + .first())!.status + ).to.equal('pending') + + expect(sendWorkspaceJoinRequestReceivedEmailCalls).to.have.length(1) + expect(sendWorkspaceJoinRequestReceivedEmailCalls[0].workspace.id).to.equal( + workspace.id + ) + expect(sendWorkspaceJoinRequestReceivedEmailCalls[0].requester).to.equal(user) + + // attempt to join again + const err = await expectToThrow(() => + requestToJoinWorkspace({ workspaceId: workspace.id, userId: user.id }) + ) + expect(err.message).to.equal(WorkspaceNotDiscoverableError.defaultMessage) + }) }) describe('approveWorkspaceJoinRequestFactory, returns a function that ', () => { From a20684c2bd4809f668bb004c97a627d07fc485c5 Mon Sep 17 00:00:00 2001 From: Iain Sproat <68657+iainsproat@users.noreply.github.com> Date: Fri, 16 May 2025 16:32:00 +0100 Subject: [PATCH 2/5] missing await --- .../workspaces/tests/integration/workspaceJoinRequests.spec.ts | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/packages/server/modules/workspaces/tests/integration/workspaceJoinRequests.spec.ts b/packages/server/modules/workspaces/tests/integration/workspaceJoinRequests.spec.ts index f08a6b006..00e565fba 100644 --- a/packages/server/modules/workspaces/tests/integration/workspaceJoinRequests.spec.ts +++ b/packages/server/modules/workspaces/tests/integration/workspaceJoinRequests.spec.ts @@ -287,7 +287,7 @@ const { FF_WORKSPACES_MODULE_ENABLED } = getFeatureFlags() }) expect( - requestToJoinWorkspace({ workspaceId: workspace.id, userId: user.id }) + await requestToJoinWorkspace({ workspaceId: workspace.id, userId: user.id }) ).to.equal(true) expect( From 765adeecd6b9e6cb5391a3321a9971bb7b056ac9 Mon Sep 17 00:00:00 2001 From: Iain Sproat <68657+iainsproat@users.noreply.github.com> Date: Fri, 16 May 2025 17:44:17 +0100 Subject: [PATCH 3/5] fix(server/workspaces): handle duplicate join request --- .../modules/workspaces/errors/workspace.ts | 6 ++++ .../services/workspaceJoinRequests.ts | 28 ++++++++++++++----- .../integration/workspaceJoinRequests.spec.ts | 5 +++- 3 files changed, 31 insertions(+), 8 deletions(-) diff --git a/packages/server/modules/workspaces/errors/workspace.ts b/packages/server/modules/workspaces/errors/workspace.ts index 90439873d..27c2485ce 100644 --- a/packages/server/modules/workspaces/errors/workspace.ts +++ b/packages/server/modules/workspaces/errors/workspace.ts @@ -84,6 +84,12 @@ export class WorkspaceNotJoinableError extends BaseError { static statusCode = 400 } +export class DuplicateWorkspaceJoinRequestError extends BaseError { + static defaultMessage = 'Duplicate workspace join request' + static code = 'DUPLICATE_WORKSPACE_JOIN_REQUEST' + static statusCode = 400 +} + export class WorkspaceUnverifiedDomainError extends BaseError { static defaultMessage = 'Cannot add unverified domain to workspace' static code = 'WORKSPACE_UNVERIFIED_DOMAIN_ERROR' diff --git a/packages/server/modules/workspaces/services/workspaceJoinRequests.ts b/packages/server/modules/workspaces/services/workspaceJoinRequests.ts index 1be5235b3..dbe050141 100644 --- a/packages/server/modules/workspaces/services/workspaceJoinRequests.ts +++ b/packages/server/modules/workspaces/services/workspaceJoinRequests.ts @@ -1,4 +1,5 @@ import { + DuplicateWorkspaceJoinRequestError, WorkspaceNotDiscoverableError, WorkspaceNotFoundError, WorkspaceNotJoinableError, @@ -19,7 +20,7 @@ import { SendWorkspaceJoinRequestReceivedEmail, UpdateWorkspaceJoinRequestStatus } from '@/modules/workspaces/domain/operations' -import { Roles } from '@speckle/shared' +import { ensureError, Roles } from '@speckle/shared' import { FindEmailsByUserId } from '@/modules/core/domain/userEmails/operations' import { userEmailsCompliantWithWorkspaceDomains } from '@/modules/workspaces/domain/logic' import { EventBus } from '@/modules/shared/services/eventBus' @@ -84,13 +85,26 @@ export const requestToJoinWorkspaceFactory = throw new WorkspaceProtectedError() } - await createWorkspaceJoinRequest({ - workspaceJoinRequest: { - userId, - workspaceId, - status: 'pending' + try { + await createWorkspaceJoinRequest({ + workspaceJoinRequest: { + userId, + workspaceId, + status: 'pending' + } + }) + } catch (e) { + if (e instanceof Error && e.message.includes('duplicate key')) { + throw new DuplicateWorkspaceJoinRequestError( + 'A workspace join request already exists', + { cause: e } + ) } - }) + throw ensureError( + e, + 'Unknown error when attempting to create a new workspace join request' + ) + } await sendWorkspaceJoinRequestReceivedEmail({ workspace, diff --git a/packages/server/modules/workspaces/tests/integration/workspaceJoinRequests.spec.ts b/packages/server/modules/workspaces/tests/integration/workspaceJoinRequests.spec.ts index 00e565fba..218a05cc9 100644 --- a/packages/server/modules/workspaces/tests/integration/workspaceJoinRequests.spec.ts +++ b/packages/server/modules/workspaces/tests/integration/workspaceJoinRequests.spec.ts @@ -5,6 +5,7 @@ import { } from '@/modules/core/helpers/testHelpers' import { getFeatureFlags } from '@/modules/shared/helpers/envHelper' import { + DuplicateWorkspaceJoinRequestError, WorkspaceNotDiscoverableError, WorkspaceNotFoundError } from '@/modules/workspaces/errors/workspace' @@ -44,6 +45,7 @@ import { updateWorkspaceJoinRequestStatusFactory } from '@/modules/workspaces/repositories/workspaceJoinRequests' import { UserEmail } from '@/modules/core/domain/userEmails/types' +import { get } from 'lodash' const { FF_WORKSPACES_MODULE_ENABLED } = getFeatureFlags() @@ -310,7 +312,8 @@ const { FF_WORKSPACES_MODULE_ENABLED } = getFeatureFlags() const err = await expectToThrow(() => requestToJoinWorkspace({ workspaceId: workspace.id, userId: user.id }) ) - expect(err.message).to.equal(WorkspaceNotDiscoverableError.defaultMessage) + + expect(get(err, 'code')).to.equal(DuplicateWorkspaceJoinRequestError.code) }) }) From 29c7e538cbc8f6c2122d5f0814475aa6bfc8b7e5 Mon Sep 17 00:00:00 2001 From: Iain Sproat <68657+iainsproat@users.noreply.github.com> Date: Mon, 19 May 2025 10:08:26 +0100 Subject: [PATCH 4/5] make it idempotent, not an error --- .../modules/workspaces/errors/workspace.ts | 6 ----- .../services/workspaceJoinRequests.ts | 7 ++---- .../integration/workspaceJoinRequests.spec.ts | 23 ++++++++++++------- 3 files changed, 17 insertions(+), 19 deletions(-) diff --git a/packages/server/modules/workspaces/errors/workspace.ts b/packages/server/modules/workspaces/errors/workspace.ts index 27c2485ce..90439873d 100644 --- a/packages/server/modules/workspaces/errors/workspace.ts +++ b/packages/server/modules/workspaces/errors/workspace.ts @@ -84,12 +84,6 @@ export class WorkspaceNotJoinableError extends BaseError { static statusCode = 400 } -export class DuplicateWorkspaceJoinRequestError extends BaseError { - static defaultMessage = 'Duplicate workspace join request' - static code = 'DUPLICATE_WORKSPACE_JOIN_REQUEST' - static statusCode = 400 -} - export class WorkspaceUnverifiedDomainError extends BaseError { static defaultMessage = 'Cannot add unverified domain to workspace' static code = 'WORKSPACE_UNVERIFIED_DOMAIN_ERROR' diff --git a/packages/server/modules/workspaces/services/workspaceJoinRequests.ts b/packages/server/modules/workspaces/services/workspaceJoinRequests.ts index dbe050141..d69ba35f2 100644 --- a/packages/server/modules/workspaces/services/workspaceJoinRequests.ts +++ b/packages/server/modules/workspaces/services/workspaceJoinRequests.ts @@ -1,5 +1,4 @@ import { - DuplicateWorkspaceJoinRequestError, WorkspaceNotDiscoverableError, WorkspaceNotFoundError, WorkspaceNotJoinableError, @@ -95,10 +94,8 @@ export const requestToJoinWorkspaceFactory = }) } catch (e) { if (e instanceof Error && e.message.includes('duplicate key')) { - throw new DuplicateWorkspaceJoinRequestError( - 'A workspace join request already exists', - { cause: e } - ) + // This is a duplicate request, so confirm its existence without resending the email + return true } throw ensureError( e, diff --git a/packages/server/modules/workspaces/tests/integration/workspaceJoinRequests.spec.ts b/packages/server/modules/workspaces/tests/integration/workspaceJoinRequests.spec.ts index 218a05cc9..0a936b5b2 100644 --- a/packages/server/modules/workspaces/tests/integration/workspaceJoinRequests.spec.ts +++ b/packages/server/modules/workspaces/tests/integration/workspaceJoinRequests.spec.ts @@ -5,7 +5,6 @@ import { } from '@/modules/core/helpers/testHelpers' import { getFeatureFlags } from '@/modules/shared/helpers/envHelper' import { - DuplicateWorkspaceJoinRequestError, WorkspaceNotDiscoverableError, WorkspaceNotFoundError } from '@/modules/workspaces/errors/workspace' @@ -45,7 +44,6 @@ import { updateWorkspaceJoinRequestStatusFactory } from '@/modules/workspaces/repositories/workspaceJoinRequests' import { UserEmail } from '@/modules/core/domain/userEmails/types' -import { get } from 'lodash' const { FF_WORKSPACES_MODULE_ENABLED } = getFeatureFlags() @@ -235,8 +233,7 @@ const { FF_WORKSPACES_MODULE_ENABLED } = getFeatureFlags() ) expect(sendWorkspaceJoinRequestReceivedEmailCalls[0].requester).to.equal(user) }) - it('gracefully handles a duplicate request', async () => { - // insert into "workspace_join_requests" ("status", "userId", "workspaceId") values ($1, $2, $3) returning * - duplicate key value violates unique constraint "workspace_join_requests_pkey" + it('duplicate request is idempotent', async () => { const createWorkspaceJoinRequest = createWorkspaceJoinRequestFactory({ db }) const sendWorkspaceJoinRequestReceivedEmailCalls: Parameters[number][] = @@ -309,11 +306,21 @@ const { FF_WORKSPACES_MODULE_ENABLED } = getFeatureFlags() expect(sendWorkspaceJoinRequestReceivedEmailCalls[0].requester).to.equal(user) // attempt to join again - const err = await expectToThrow(() => - requestToJoinWorkspace({ workspaceId: workspace.id, userId: user.id }) - ) + expect( + await requestToJoinWorkspace({ workspaceId: workspace.id, userId: user.id }) + ).to.equal(true) - expect(get(err, 'code')).to.equal(DuplicateWorkspaceJoinRequestError.code) + expect( + (await db(WorkspaceJoinRequests.name) + .where({ + workspaceId: workspace.id, + userId: user.id + }) + .select('status') + .first())!.status + ).to.equal('pending') + + expect(sendWorkspaceJoinRequestReceivedEmailCalls).to.have.length(1) }) }) From ae221df0b0fd21ea1beaa5fb67cc7f8e1248df2c Mon Sep 17 00:00:00 2001 From: Iain Sproat <68657+iainsproat@users.noreply.github.com> Date: Mon, 19 May 2025 11:00:36 +0100 Subject: [PATCH 5/5] Incorporate PR comment --- .../repositories/workspaceJoinRequests.ts | 6 +++- .../services/workspaceJoinRequests.ts | 28 ++++++++----------- 2 files changed, 16 insertions(+), 18 deletions(-) diff --git a/packages/server/modules/workspaces/repositories/workspaceJoinRequests.ts b/packages/server/modules/workspaces/repositories/workspaceJoinRequests.ts index dac1d1393..c0c1fcf8d 100644 --- a/packages/server/modules/workspaces/repositories/workspaceJoinRequests.ts +++ b/packages/server/modules/workspaces/repositories/workspaceJoinRequests.ts @@ -23,7 +23,11 @@ const tables = { export const createWorkspaceJoinRequestFactory = ({ db }: { db: Knex }): CreateWorkspaceJoinRequest => async ({ workspaceJoinRequest }) => { - const res = await tables.workspaceJoinRequests(db).insert(workspaceJoinRequest, '*') + const res = await tables + .workspaceJoinRequests(db) + .insert(workspaceJoinRequest, '*') + .onConflict() + .ignore() return res[0] } diff --git a/packages/server/modules/workspaces/services/workspaceJoinRequests.ts b/packages/server/modules/workspaces/services/workspaceJoinRequests.ts index d69ba35f2..ef6c97b22 100644 --- a/packages/server/modules/workspaces/services/workspaceJoinRequests.ts +++ b/packages/server/modules/workspaces/services/workspaceJoinRequests.ts @@ -19,7 +19,7 @@ import { SendWorkspaceJoinRequestReceivedEmail, UpdateWorkspaceJoinRequestStatus } from '@/modules/workspaces/domain/operations' -import { ensureError, Roles } from '@speckle/shared' +import { Roles } from '@speckle/shared' import { FindEmailsByUserId } from '@/modules/core/domain/userEmails/operations' import { userEmailsCompliantWithWorkspaceDomains } from '@/modules/workspaces/domain/logic' import { EventBus } from '@/modules/shared/services/eventBus' @@ -84,23 +84,17 @@ export const requestToJoinWorkspaceFactory = throw new WorkspaceProtectedError() } - try { - await createWorkspaceJoinRequest({ - workspaceJoinRequest: { - userId, - workspaceId, - status: 'pending' - } - }) - } catch (e) { - if (e instanceof Error && e.message.includes('duplicate key')) { - // This is a duplicate request, so confirm its existence without resending the email - return true + const joinRequest = await createWorkspaceJoinRequest({ + workspaceJoinRequest: { + userId, + workspaceId, + status: 'pending' } - throw ensureError( - e, - 'Unknown error when attempting to create a new workspace join request' - ) + }) + + if (!joinRequest || joinRequest.status !== 'pending') { + // The request was already created, so don't send the email again + return true } await sendWorkspaceJoinRequestReceivedEmail({