-
Notifications
You must be signed in to change notification settings - Fork 12k
feat: add database transaction to createManagedOrganization flow to ensure atomicity #27979
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: main
Are you sure you want to change the base?
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -9,10 +9,13 @@ import { GetManagedOrganizationsInput_2024_08_13 } from "@/modules/organizations | |
| import { UpdateOrganizationInput } from "@/modules/organizations/organizations/inputs/update-managed-organization.input"; | ||
| import { ManagedOrganizationsRepository } from "@/modules/organizations/organizations/managed-organizations.repository"; | ||
| import { ManagedOrganizationsOutputService } from "@/modules/organizations/organizations/services/managed-organizations-output.service"; | ||
| import { PrismaWriteService } from "@/modules/prisma/prisma-write.service"; | ||
| import { ProfilesRepository } from "@/modules/profiles/profiles.repository"; | ||
| import { ConflictException, ForbiddenException, Injectable, NotFoundException } from "@nestjs/common"; | ||
| import { ForbiddenException, Injectable, NotFoundException } from "@nestjs/common"; | ||
| import { MembershipRole } from "@calcom/prisma/enums"; | ||
|
|
||
| import { slugify } from "@calcom/platform-libraries"; | ||
| import { Prisma } from "@calcom/prisma/client"; | ||
|
|
||
| @Injectable() | ||
| export class ManagedOrganizationsService { | ||
|
|
@@ -23,7 +26,8 @@ export class ManagedOrganizationsService { | |
| private readonly organizationsMembershipService: OrganizationsMembershipService, | ||
| private readonly apiKeysService: ApiKeysService, | ||
| private readonly managedOrganizationsOutputService: ManagedOrganizationsOutputService, | ||
| private readonly profilesRepository: ProfilesRepository | ||
| private readonly profilesRepository: ProfilesRepository, | ||
| private readonly dbWrite: PrismaWriteService | ||
| ) {} | ||
|
|
||
| async createManagedOrganization( | ||
|
|
@@ -46,60 +50,83 @@ export class ManagedOrganizationsService { | |
| organizationData.slug = effectiveSlug; | ||
| } | ||
|
|
||
| const existingManagedOrganization = | ||
| return this.dbWrite.prisma.$transaction(async (prisma) => { | ||
| try { | ||
| // existingManagedOrganization is getting checked inside transaction to prevent race conditions | ||
| const existingManagedOrganization = | ||
| await this.managedOrganizationsRepository.getManagedOrganizationBySlug( | ||
| managerOrganizationId, | ||
| effectiveSlug | ||
| ); | ||
|
|
||
| if (existingManagedOrganization) { | ||
| throw new ConflictException( | ||
| `Organization with slug '${organizationData.slug}' already exists. Please, either provide a different slug or change name so that the automatically generated slug is different.` | ||
| ); | ||
| } | ||
|
|
||
| const organization = await this.managedOrganizationsRepository.createManagedOrganization( | ||
| managerOrganizationId, | ||
| { | ||
| ...organizationData, | ||
| isOrganization: true, | ||
| isPlatform: true, | ||
| metadata: organizationData.metadata, | ||
| if (existingManagedOrganization) { | ||
|
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. This does not send API response like previous, it just throws error. Open to change if reviewer thinks it can be better. Same goes to the over all catch block's error throw. |
||
| throw new Error( | ||
| `Organization with slug '${organizationData.slug}' already exists. Please, either provide a different slug or change name so that the automatically generated slug is different.` | ||
| ); | ||
| } | ||
| ); | ||
|
|
||
| await this.organizationsMembershipService.createOrgMembership(organization.id, { | ||
| userId: authUser.id, | ||
| accepted: true, | ||
| role: "OWNER", | ||
| }); | ||
| const organization = await this.managedOrganizationsRepository.createManagedOrganization( | ||
| managerOrganizationId, | ||
| { | ||
| ...organizationData, | ||
| isOrganization: true, | ||
| isPlatform: true, | ||
| metadata: organizationData.metadata, | ||
| }, | ||
| prisma | ||
| ); | ||
|
|
||
| const defaultProfileUsername = `${organization.name}-${authUser.id}`; | ||
| await this.profilesRepository.createProfile( | ||
| organization.id, | ||
| authUser.id, | ||
| authUser.username || defaultProfileUsername | ||
| ); | ||
|
|
||
| await this.managedOrganizationsBillingService.createManagedOrganizationBilling( | ||
| managerOrganizationId, | ||
| organization.id | ||
| ); | ||
|
|
||
| const apiKey = await this.apiKeysService.createApiKey(authUser.id, { | ||
| apiKeyDaysValid, | ||
| apiKeyNeverExpires, | ||
| note: `Managed organization API key. ManagerOrgId: ${managerOrganizationId}. ManagedOrgId: ${organization.id}`, | ||
| teamId: organization.id, | ||
| }); | ||
|
|
||
| const outputOrganization = | ||
| this.managedOrganizationsOutputService.getOutputManagedOrganization(organization); | ||
| await this.organizationsMembershipService.createOrgMembership( | ||
| organization.id, | ||
| { | ||
| userId: authUser.id, | ||
| accepted: true, | ||
| role: MembershipRole.OWNER, | ||
| }, | ||
| prisma | ||
| ); | ||
|
|
||
| return { | ||
| ...outputOrganization, | ||
| apiKey, | ||
| }; | ||
|
|
||
| const defaultProfileUsername = `${organization.name}-${authUser.id}`; | ||
|
|
||
| await this.profilesRepository.createProfile( | ||
| organization.id, | ||
| authUser.id, | ||
| authUser.username || defaultProfileUsername, | ||
| prisma | ||
| ); | ||
|
|
||
| await this.managedOrganizationsBillingService.createManagedOrganizationBilling( | ||
| managerOrganizationId, | ||
| organization.id, | ||
| prisma | ||
| ); | ||
|
|
||
|
|
||
| const apiKey = await this.apiKeysService.createApiKey(authUser.id, { | ||
| apiKeyDaysValid, | ||
| apiKeyNeverExpires, | ||
| note: `Managed organization API key. ManagerOrgId: ${managerOrganizationId}. ManagedOrgId: ${organization.id}`, | ||
| teamId: organization.id | ||
| }, prisma); | ||
|
|
||
| const outputOrganization = | ||
| this.managedOrganizationsOutputService.getOutputManagedOrganization(organization); | ||
|
|
||
| return { | ||
| ...outputOrganization, | ||
| apiKey, | ||
| }; | ||
| } catch (error) { | ||
|
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. as I mentioned in the above comment https://github.com/calcom/cal.com/pull/27979/changes#r2813240667 |
||
| throw new Error(`Managed organization creation failed. ${error instanceof Error ? error.message : String(error)}`) | ||
| } | ||
| }, { | ||
| // @see: https://www.prisma.io/docs/orm/prisma-client/queries/transactions | ||
|
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. this is not required, but is mentioned in the prisma docs. Therefore, added it with docs link |
||
| isolationLevel: Prisma.TransactionIsolationLevel.Serializable, // optional, default defined by database configuration | ||
| maxWait: 5000, // default: 2000 | ||
| timeout: 10000, // default: 5000 | ||
| }); | ||
| } | ||
|
|
||
| private async isManagerOrganizationPlatform(managerOrganizationId: number) { | ||
|
|
||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -7,10 +7,12 @@ export async function checkPermissions(args: { | |
| userId: number; | ||
| teamId?: number; | ||
| role: Prisma.MembershipWhereInput["role"]; | ||
| prismaTransactionClient?: Prisma.TransactionClient | ||
| }) { | ||
| const { teamId, userId, role } = args; | ||
| const { teamId, userId, role, prismaTransactionClient } = args; | ||
| if (!teamId) return; | ||
| const team = await prisma.team.findFirst({ | ||
| const client = prismaTransactionClient ?? prisma | ||
| const team = await client.team.findFirst({ | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. P2: Inefficient database query fetching full record for existence check Prompt for AI agents |
||
| where: { | ||
| id: teamId, | ||
| members: { | ||
|
|
||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This function require both the read and write db service. So added both of them falling back to the default.Open to suggestions here. A single client can also do the work but not appropriate usage I think as we use read,write services where they are appropriate.