chore: migrate EmailVerification to pg #9492
Conversation
|
Sasha fell asleep early, and I felt itchy |
| @@ -55,9 +57,9 @@ const signUpVerified = async (email: string) => { | |||
| expect(verifyEmail).toMatchObject({ | |||
| data: { | |||
| verifyEmail: { | |||
| authToken: expect.toBeString(), | |||
There was a problem hiding this comment.
I was getting TS failures here
| @@ -153,7 +155,7 @@ test.skip('autoJoin on multiple teams does not create duplicate `OrganizationUse | |||
| const newEmail = `${faker.internet.userName()}@${domain}`.toLowerCase() | |||
| const {user: newUser} = await signUpVerified(newEmail) | |||
|
|
|||
| expect(newUser.tms).toIncludeSameMembers(teamIds) | |||
| expect(newUser.tms).toEqual(expect.arrayContaining(teamIds)) | |||
| @@ -12,16 +10,14 @@ interface Input { | |||
| } | |||
|
|
|||
| export default class EmailVerification { | |||
| id: string | |||
There was a problem hiding this comment.
Once again, id is now managed by PG
jordanh
force-pushed
the
chore/9491/migrate-emailverification
branch
from
d67de35 to
98755f9
Compare
| .selectFrom('EmailVerification') | ||
| .selectAll() | ||
| .where('token', '=', verificationToken) | ||
| .executeTakeFirst()) as EmailVerification) || null |
There was a problem hiding this comment.
+1 no need to coerce to EmailVerification || null since kysely knows the return type
Signed-off-by: Matt Krick <[email protected]>
|
Warning Rate limit exceeded@mattkrick has exceeded the limit for the number of commits or files that can be reviewed per hour. Please wait 0 minutes and 10 seconds before requesting another review. How to resolve this issue?After the wait time has elapsed, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. Our paid plans have higher rate limits than the trial, open-source and free plans. In all cases, we re-allow further reviews after a brief timeout. Please see our FAQ for further information. WalkthroughThe changes encompass a migration of the Changes
Sequence Diagram(s)sequenceDiagram
participant User
participant Server
participant Database
User->>Server: Sign Up Request
Server->>Database: Check Email Verifications
Database-->>Server: Return Verification Status
Server->>Database: Insert New User
Database-->>Server: Confirm User Insertion
Server-->>User: Send Confirmation Email
Assessment against linked issues
Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on your favorite social media? TipsChatThere are 3 ways to chat with CodeRabbit:
Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (invoked as PR comments)
Additionally, you can add CodeRabbit Configuration File (
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 0
Outside diff range, codebase verification and nitpick comments (2)
packages/server/postgres/migrations/1709351575000_moveEmailVerification.ts (2)
8-66:upfunction approved with a suggestion.The function correctly handles the migration of data from RethinkDB to PostgreSQL.
Consider adding logging to improve traceability and debugging.
+ console.log('Starting migration of EmailVerification data'); + console.log('Creating indexes in RethinkDB'); + console.log('Fetching data in batches from RethinkDB'); + console.log('Inserting data into PostgreSQL'); + console.log('Migration completed successfully');
68-76:downfunction approved with a suggestion.The function correctly handles the rollback of migration.
Consider adding logging to improve traceability and debugging.
+ console.log('Starting rollback of EmailVerification migration'); + console.log('Deleting data from PostgreSQL'); + console.log('Dropping indexes in RethinkDB'); + console.log('Rollback completed successfully');
Signed-off-by: Matt Krick <[email protected]>
There was a problem hiding this comment.
Actionable comments posted: 4
Outside diff range, codebase verification and nitpick comments (1)
packages/server/email/createEmailVerification.ts (1)
Update Required: Function Call Mismatch
The function
createEmailVerificationis called with incorrect parameters inautoJoin.test.ts. The new implementation expectspassword,invitationToken,pseudoId, andredirectTo, but the test file only providespassword.
- File:
packages/server/__tests__/autoJoin.test.ts- Line:
await createEmailVerification({email, password})Please update the test file to match the new function signature.
Analysis chain
Line range hint
15-18:
LGTM! But verify the function usage in the codebase.The code changes are approved.
However, ensure that all function calls to
createEmailVerificationmatch the new implementation.Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Verify all function calls to `createEmailVerification` match the new implementation. # Test: Search for the function usage. Expect: Only occurrences of the new implementation. rg --type ts -A 5 $'createEmailVerification'Length of output: 3179
| const pg = getKysely() | ||
| await pg | ||
| .insertInto('EmailVerification') | ||
| .values({ | ||
| email, | ||
| token: verifiedEmailToken, | ||
| hashedPassword, | ||
| pseudoId, | ||
| invitationToken, | ||
| expiration: new Date(Date.now() + Threshold.EMAIL_VERIFICATION_LIFESPAN) | ||
| }) | ||
| .execute() |
There was a problem hiding this comment.
Ensure proper error handling for database operations.
The insertion operation should handle potential errors to avoid unhandled promise rejections.
- await pg
+ try {
+ await pg
.insertInto('EmailVerification')
.values({
email,
token: verifiedEmailToken,
hashedPassword,
pseudoId,
invitationToken,
expiration: new Date(Date.now() + Threshold.EMAIL_VERIFICATION_LIFESPAN)
})
.execute()
+ } catch (error) {
+ return {error: {message: 'Database error: Unable to save verification data'}}
+ }Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| const pg = getKysely() | |
| await pg | |
| .insertInto('EmailVerification') | |
| .values({ | |
| email, | |
| token: verifiedEmailToken, | |
| hashedPassword, | |
| pseudoId, | |
| invitationToken, | |
| expiration: new Date(Date.now() + Threshold.EMAIL_VERIFICATION_LIFESPAN) | |
| }) | |
| .execute() | |
| const pg = getKysely() | |
| try { | |
| await pg | |
| .insertInto('EmailVerification') | |
| .values({ | |
| email, | |
| token: verifiedEmailToken, | |
| hashedPassword, | |
| pseudoId, | |
| invitationToken, | |
| expiration: new Date(Date.now() + Threshold.EMAIL_VERIFICATION_LIFESPAN) | |
| }) | |
| .execute() | |
| } catch (error) { | |
| return {error: {message: 'Database error: Unable to save verification data'}} | |
| } |
| export async function down() { | ||
| const client = new Client(getPgConfig()) | ||
| await client.connect() | ||
| await client.query(` | ||
| DROP TABLE IF EXISTS "EmailVerification"; | ||
| `) | ||
| await client.end() |
There was a problem hiding this comment.
Ensure proper error handling for database operations.
The table drop operation should handle potential errors to avoid unhandled promise rejections.
- const client = new Client(getPgConfig())
- await client.connect()
- await client.query(`
- DROP TABLE IF EXISTS "EmailVerification";
- `)
- await client.end()
+ const client = new Client(getPgConfig())
+ try {
+ await client.connect()
+ await client.query(`
+ DROP TABLE IF EXISTS "EmailVerification";
+ `)
+ } catch (error) {
+ console.error('Database error:', error)
+ throw error
+ } finally {
+ await client.end()
+ }Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| export async function down() { | |
| const client = new Client(getPgConfig()) | |
| await client.connect() | |
| await client.query(` | |
| DROP TABLE IF EXISTS "EmailVerification"; | |
| `) | |
| await client.end() | |
| export async function down() { | |
| const client = new Client(getPgConfig()) | |
| try { | |
| await client.connect() | |
| await client.query(` | |
| DROP TABLE IF EXISTS "EmailVerification"; | |
| `) | |
| } catch (error) { | |
| console.error('Database error:', error) | |
| throw error | |
| } finally { | |
| await client.end() | |
| } | |
| } |
| export async function up() { | ||
| await connectRethinkDB() | ||
| const pg = new Kysely<any>({ | ||
| dialect: new PostgresDialect({ | ||
| pool: getPg() | ||
| }) | ||
| }) | ||
| await sql` | ||
| CREATE TABLE "EmailVerification" ( | ||
| "id" INT GENERATED BY DEFAULT AS IDENTITY PRIMARY KEY, | ||
| "email" "citext" NOT NULL, | ||
| "expiration" TIMESTAMP WITH TIME ZONE NOT NULL, | ||
| "token" VARCHAR(100) NOT NULL, | ||
| "hashedPassword" VARCHAR(100), | ||
| "invitationToken" VARCHAR(100), | ||
| "pseudoId" VARCHAR(100) | ||
| ); | ||
|
|
||
| CREATE INDEX IF NOT EXISTS "idx_EmailVerification_email" ON "EmailVerification"("email"); | ||
| CREATE INDEX IF NOT EXISTS "idx_EmailVerification_token" ON "EmailVerification"("token"); | ||
| `.execute(pg) |
There was a problem hiding this comment.
Ensure proper error handling for database operations.
The table creation and data migration operations should handle potential errors to avoid unhandled promise rejections.
- await connectRethinkDB()
- const pg = new Kysely<any>({
- dialect: new PostgresDialect({
- pool: getPg()
- })
- })
- await sql`
- CREATE TABLE "EmailVerification" (
- "id" INT GENERATED BY DEFAULT AS IDENTITY PRIMARY KEY,
- "email" "citext" NOT NULL,
- "expiration" TIMESTAMP WITH TIME ZONE NOT NULL,
- "token" VARCHAR(100) NOT NULL,
- "hashedPassword" VARCHAR(100),
- "invitationToken" VARCHAR(100),
- "pseudoId" VARCHAR(100)
- );
- CREATE INDEX IF NOT EXISTS "idx_EmailVerification_email" ON "EmailVerification"("email");
- CREATE INDEX IF NOT EXISTS "idx_EmailVerification_token" ON "EmailVerification"("token");
- `.execute(pg)
- const rData = await r.table('EmailVerification').coerceTo('array').run()
- const insertData = rData.map((row) => {
- const {email, expiration, hashedPassword, token, invitationToken, pseudoId} = row
- return {
- email,
- expiration,
- hashedPassword,
- token,
- invitationToken,
- pseudoId
- }
- })
- await pg.insertInto('EmailVerification').values(insertData).execute()
+ try {
+ await connectRethinkDB()
+ const pg = new Kysely<any>({
+ dialect: new PostgresDialect({
+ pool: getPg()
+ })
+ })
+ await sql`
+ CREATE TABLE "EmailVerification" (
+ "id" INT GENERATED BY DEFAULT AS IDENTITY PRIMARY KEY,
+ "email" "citext" NOT NULL,
+ "expiration" TIMESTAMP WITH TIME ZONE NOT NULL,
+ "token" VARCHAR(100) NOT NULL,
+ "hashedPassword" VARCHAR(100),
+ "invitationToken" VARCHAR(100),
+ "pseudoId" VARCHAR(100)
+ );
+ CREATE INDEX IF NOT EXISTS "idx_EmailVerification_email" ON "EmailVerification"("email");
+ CREATE INDEX IF NOT EXISTS "idx_EmailVerification_token" ON "EmailVerification"("token");
+ `.execute(pg)
+ const rData = await r.table('EmailVerification').coerceTo('array').run()
+ const insertData = rData.map((row) => {
+ const {email, expiration, hashedPassword, token, invitationToken, pseudoId} = row
+ return {
+ email,
+ expiration,
+ hashedPassword,
+ token,
+ invitationToken,
+ pseudoId
+ }
+ })
+ await pg.insertInto('EmailVerification').values(insertData).execute()
+ } catch (error) {
+ console.error('Database error:', error)
+ throw error
+ }Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| export async function up() { | |
| await connectRethinkDB() | |
| const pg = new Kysely<any>({ | |
| dialect: new PostgresDialect({ | |
| pool: getPg() | |
| }) | |
| }) | |
| await sql` | |
| CREATE TABLE "EmailVerification" ( | |
| "id" INT GENERATED BY DEFAULT AS IDENTITY PRIMARY KEY, | |
| "email" "citext" NOT NULL, | |
| "expiration" TIMESTAMP WITH TIME ZONE NOT NULL, | |
| "token" VARCHAR(100) NOT NULL, | |
| "hashedPassword" VARCHAR(100), | |
| "invitationToken" VARCHAR(100), | |
| "pseudoId" VARCHAR(100) | |
| ); | |
| CREATE INDEX IF NOT EXISTS "idx_EmailVerification_email" ON "EmailVerification"("email"); | |
| CREATE INDEX IF NOT EXISTS "idx_EmailVerification_token" ON "EmailVerification"("token"); | |
| `.execute(pg) | |
| export async function up() { | |
| try { | |
| await connectRethinkDB() | |
| const pg = new Kysely<any>({ | |
| dialect: new PostgresDialect({ | |
| pool: getPg() | |
| }) | |
| }) | |
| await sql` | |
| CREATE TABLE "EmailVerification" ( | |
| "id" INT GENERATED BY DEFAULT AS IDENTITY PRIMARY KEY, | |
| "email" "citext" NOT NULL, | |
| "expiration" TIMESTAMP WITH TIME ZONE NOT NULL, | |
| "token" VARCHAR(100) NOT NULL, | |
| "hashedPassword" VARCHAR(100), | |
| "invitationToken" VARCHAR(100), | |
| "pseudoId" VARCHAR(100) | |
| ); | |
| CREATE INDEX IF NOT EXISTS "idx_EmailVerification_email" ON "EmailVerification"("email"); | |
| CREATE INDEX IF NOT EXISTS "idx_EmailVerification_token" ON "EmailVerification"("token"); | |
| `.execute(pg) | |
| const rData = await r.table('EmailVerification').coerceTo('array').run() | |
| const insertData = rData.map((row) => { | |
| const {email, expiration, hashedPassword, token, invitationToken, pseudoId} = row | |
| return { | |
| email, | |
| expiration, | |
| hashedPassword, | |
| token, | |
| invitationToken, | |
| pseudoId | |
| } | |
| }) | |
| await pg.insertInto('EmailVerification').values(insertData).execute() | |
| } catch (error) { | |
| console.error('Database error:', error) | |
| throw error | |
| } | |
| } |
| const pg = getKysely() | ||
| const now = new Date() | ||
| const emailVerification = (await r | ||
| .table('EmailVerification') | ||
| .getAll(verificationToken, {index: 'token'}) | ||
| .nth(0) | ||
| .default(null) | ||
| .run()) as EmailVerification | ||
| const emailVerification = await pg | ||
| .selectFrom('EmailVerification') | ||
| .selectAll() | ||
| .where('token', '=', verificationToken) | ||
| .executeTakeFirst() |
There was a problem hiding this comment.
Ensure proper error handling for database operations.
The database query operation should handle potential errors to avoid unhandled promise rejections.
- const emailVerification = await pg
- .selectFrom('EmailVerification')
- .selectAll()
- .where('token', '=', verificationToken)
- .executeTakeFirst()
+ let emailVerification
+ try {
+ emailVerification = await pg
+ .selectFrom('EmailVerification')
+ .selectAll()
+ .where('token', '=', verificationToken)
+ .executeTakeFirst()
+ } catch (error) {
+ return {error: {message: 'Database error: Unable to verify email'}}
+ }Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| const pg = getKysely() | |
| const now = new Date() | |
| const emailVerification = (await r | |
| .table('EmailVerification') | |
| .getAll(verificationToken, {index: 'token'}) | |
| .nth(0) | |
| .default(null) | |
| .run()) as EmailVerification | |
| const emailVerification = await pg | |
| .selectFrom('EmailVerification') | |
| .selectAll() | |
| .where('token', '=', verificationToken) | |
| .executeTakeFirst() | |
| const pg = getKysely() | |
| const now = new Date() | |
| let emailVerification | |
| try { | |
| emailVerification = await pg | |
| .selectFrom('EmailVerification') | |
| .selectAll() | |
| .where('token', '=', verificationToken) | |
| .executeTakeFirst() | |
| } catch (error) { | |
| return {error: {message: 'Database error: Unable to verify email'}} | |
| } |
Signed-off-by: Matt Krick <[email protected]>
Description
Resolves #9491
Testing scenarios
Migrations
EmailVerificationon pgCan verify email address with token
Final checklist
Summary by CodeRabbit