-
Notifications
You must be signed in to change notification settings - Fork 0
more robust rc solution #69
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
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 | ||||
|---|---|---|---|---|---|---|
| @@ -1,4 +1,4 @@ | ||||||
| import { and, eq, ilike, or } from "drizzle-orm"; | ||||||
| import { eq, ilike, or } from "drizzle-orm"; | ||||||
| import { db } from "@/db"; | ||||||
| import { User, UserInsert, user } from "@common/db/schema/user"; | ||||||
|
|
||||||
|
|
@@ -36,36 +36,82 @@ export async function readUserById(id: string): Promise<User | undefined> { | |||||
| return selectedUser; | ||||||
| } | ||||||
|
|
||||||
| /** | ||||||
| * Creates a new user in the database with robust race condition handling. | ||||||
| * | ||||||
| * This function handles several scenarios to prevent duplicate key violations: | ||||||
| * 1. User already exists with the same authId - returns existing user | ||||||
| * 2. User exists with same email but different authId - updates authId and returns user | ||||||
| * 3. No existing user - creates new user with conflict handling | ||||||
| * 4. Race condition where another transaction created the user - catches error and returns existing user | ||||||
| * | ||||||
| * The function uses database transactions and proper error handling to ensure | ||||||
| * that concurrent user creation requests don't result in duplicate key violations. | ||||||
| * | ||||||
| * @param userInsert - The user data to insert | ||||||
| * @returns The created or existing user | ||||||
| * @throws Error if user creation fails for reasons other than race conditions | ||||||
| */ | ||||||
| export async function createUser(userInsert: UserInsert): Promise<User> { | ||||||
| // this is done in transaction to avoid race condition when creating user, for conflicts on authId | ||||||
| // Use database-level upsert to eliminate race conditions entirely | ||||||
| return await db.transaction(async (trx) => { | ||||||
| // First try to find the user by authId | ||||||
| const [existingUser] = await trx | ||||||
| // First try to find the user by authId (primary identifier) | ||||||
| const [existingUserByAuthId] = await trx | ||||||
| .select() | ||||||
| .from(user) | ||||||
| .where( | ||||||
| and( | ||||||
| eq(user.authId, userInsert.authId), | ||||||
| eq(user.email, userInsert.email), | ||||||
| ), | ||||||
| ) | ||||||
| .where(eq(user.authId, userInsert.authId)) | ||||||
| .execute(); | ||||||
|
|
||||||
| if (existingUser) { | ||||||
| return existingUser; | ||||||
| if (existingUserByAuthId) { | ||||||
| return existingUserByAuthId; | ||||||
| } | ||||||
|
|
||||||
| // If no existing user found, create a new one | ||||||
| // Also check for existing user by email to handle edge cases | ||||||
| const [existingUserByEmail] = await trx | ||||||
| .select() | ||||||
| .from(user) | ||||||
| .where(eq(user.email, userInsert.email)) | ||||||
| .execute(); | ||||||
|
|
||||||
| if (existingUserByEmail) { | ||||||
| // If user exists by email but not by authId, update the authId | ||||||
| const [updatedUser] = await trx | ||||||
| .update(user) | ||||||
| .set({ | ||||||
| authId: userInsert.authId, | ||||||
| firstName: userInsert.firstName, | ||||||
| lastName: userInsert.lastName, | ||||||
| picture: userInsert.picture, | ||||||
| privacyPolicyAcceptedAt: userInsert.privacyPolicyAcceptedAt, | ||||||
| termsOfUseAcceptedAt: userInsert.termsOfUseAcceptedAt, | ||||||
| updatedAt: new Date(), | ||||||
| }) | ||||||
| .where(eq(user.id, existingUserByEmail.id)) | ||||||
| .returning() | ||||||
| .execute(); | ||||||
|
|
||||||
| return updatedUser; | ||||||
| } | ||||||
|
|
||||||
| // Create new user with database-level upsert for additional safety | ||||||
| // Use INSERT ... ON CONFLICT to handle any remaining race conditions at the database level | ||||||
| const [insertedUser] = await trx | ||||||
| .insert(user) | ||||||
| .values(userInsert) | ||||||
| .returning() | ||||||
| .onConflictDoUpdate({ | ||||||
| target: [user.email], | ||||||
| target: [user.authId], | ||||||
|
||||||
| target: [user.authId], | |
| target: [user.authId, user.email], |
Copilot
AI
Aug 7, 2025
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.
The comment mentions handling 'remaining race conditions' but the earlier logic already handles user existence by both authId and email. This upsert clause may be redundant given the explicit checks above, making the code unnecessarily complex.
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.
The function updates an existing user's authId when found by email (lines 78-93), but this could lead to security issues. If two different authentication providers return the same email address, this code would reassign the user account to the new authId, potentially giving unauthorized access to another user's account.