fix: improve live session to live quiz migration script #4368
Conversation
…ly migrate cache for running quizzes, migrate meta information in redis
|
Warning Rate limit exceeded@rschlaefli has exceeded the limit for the number of commits or files that can be reviewed per hour. Please wait 7 minutes and 1 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. 📝 Walkthrough📝 Walkthrough📝 WalkthroughWalkthroughThe pull request introduces significant updates to the migration script for transitioning live sessions to a new live quiz format. It adds new imports and several functions to facilitate data transformation, cache management, and database updates. Key functions include Changes
Possibly related PRs
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 using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Caution
Inline review comments failed to post. This is likely due to GitHub's limits when posting large numbers of comments.
Actionable comments posted: 3
🧹 Outside diff range and nitpick comments (1)
packages/graphql/src/scripts/2024-11-07_migrate_live_session_to_live_quiz.ts (1)
611-611: Remove unnecessary console.log statementThe
console.logstatement at line 611 outputs the name of the new live quiz. Unless required for debugging, consider removing it or controlling it with a logging flag for cleaner console output.Apply this diff to remove the statement:
- console.log(newLiveQuizData.data.name)
🛑 Comments failed to post (3)
packages/graphql/src/scripts/2024-11-07_migrate_live_session_to_live_quiz.ts (3)
704-706:
⚠️ Potential issueHandle the case when
newLiveQuizis undefined to prevent runtime errorsIf
applyDBUpdatesForQuizreturnsundefined(e.g., if the live session has already been migrated), accessingnewLiveQuiz.statuswill cause a runtime error. Add a check to ensurenewLiveQuizis defined before accessing its properties.Apply this diff to fix the issue:
const newLiveQuiz = await applyDBUpdatesForQuiz(prisma, liveSession) + if (!newLiveQuiz) { + continue + } if (newLiveQuiz.status === PublicationStatus.PUBLISHED) { await applyCacheUpdatesForQuiz(redisExec, newLiveQuiz) }📝 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 newLiveQuiz = await applyDBUpdatesForQuiz(prisma, liveSession) if (!newLiveQuiz) { continue } if (newLiveQuiz.status === PublicationStatus.PUBLISHED) {
94-97:
⚠️ Potential issueAdd validation for
questionData.idformat to prevent runtime errorsThe current code assumes that
questionData.idfollows the format'elementId-version'(e.g.,'12345-v1'). IfquestionData.iddoesn't conform to this format, splitting and parsing might throw errors. To enhance robustness, add validation to ensure the ID is correctly formatted before parsing.Apply this diff to add validation:
+ const idParts = questionData.id.split('-') + if (idParts.length < 2) { + throw new Error(`Invalid questionData.id format: ${questionData.id}`) + } + const elementId = idParts[0] + const versionPart = idParts[1] + const elementVersion = Number(versionPart.startsWith('v') ? versionPart.slice(1) : versionPart)📝 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 idParts = questionData.id.split('-') if (idParts.length < 2) { throw new Error(`Invalid questionData.id format: ${questionData.id}`) } const elementId = idParts[0] const versionPart = idParts[1] const elementVersion = Number(versionPart.startsWith('v') ? versionPart.slice(1) : versionPart) const elementType = computeElementType({ questionData })
373-408:
⚠️ Potential issueFix the improper use of async functions inside
forEachUsing
forEachwith an async callback does not wait for the promises to resolve, which may lead to incomplete Redis pipeline operations beforepipeline.exec()is called. ReplaceforEachwith afor...ofloop to ensure asynchronous operations complete as intended.Apply the following changes to fix the issue:
- activeBlock.elements.forEach(async (instance) => { + for (const instance of activeBlock.elements) {Committable suggestion skipped: line range outside the PR's diff.
There was a problem hiding this comment.
Caution
Inline review comments failed to post. This is likely due to GitHub's limits when posting large numbers of comments.
Actionable comments posted: 2
🧹 Outside diff range and nitpick comments (3)
packages/graphql/src/scripts/2024-11-07_migrate_live_session_to_live_quiz.ts (3)
Line range hint
42-82: Consider adding type safety for questionData parameter.The function uses
anytype forquestionDatawhich could lead to runtime errors. Consider creating an interface to define the expected shape of the question data.interface QuestionData { type: string; id: string; name: string; content: string; explanation?: string; pointsMultiplier?: number; options?: Record<string, unknown>; } function computeElementType({ questionData }: { questionData: QuestionData }): ElementType { // ... rest of the function }
Line range hint
74-126: Add validation for required questionData fields.The function checks for required fields but logs and throws a generic error. Consider adding specific error messages for each missing field and validating the data structure upfront.
- if (!questionData.id || !questionData.name || !questionData.content) { - console.log('QUESTION DATA') - console.log(questionData) - throw new Error('Missing required question data properties') - } + const requiredFields = ['id', 'name', 'content'] as const; + const missingFields = requiredFields.filter(field => !questionData[field]); + if (missingFields.length > 0) { + throw new Error(`Missing required question data properties: ${missingFields.join(', ')}`); + }
Line range hint
401-626: Enhance data integrity with explicit transaction handling.The function performs multiple database operations without explicit transaction handling, which could lead to partial migrations if errors occur.
Consider wrapping the database operations in an explicit transaction:
async function applyDBUpdatesForQuiz( prisma: PrismaClient, liveSession: LiveSession & { activeBlock?: (SessionBlock & { instances: QuestionInstance[] }) | null blocks: (SessionBlock & { instances: QuestionInstance[] })[] } ) { + return await prisma.$transaction(async (tx) => { // check if the considered live session has already been migrated - const existingLiveQuiz = await prisma.liveQuiz.findFirst({ + const existingLiveQuiz = await tx.liveQuiz.findFirst({ where: { originalId: liveSession.id }, }); if (existingLiveQuiz) { console.log(`DB: Live session ${liveSession.id} has already been migrated`); return existingLiveQuiz; } // ... rest of the function ... - let newLiveQuiz = await prisma.liveQuiz.create(newLiveQuizData); + let newLiveQuiz = await tx.liveQuiz.create(newLiveQuizData); if (liveSession.activeBlock) { - newLiveQuiz = await prisma.liveQuiz.update({ + newLiveQuiz = await tx.liveQuiz.update({ // ... update data ... }); } return newLiveQuiz; + }, { + timeout: 10000, // Set an appropriate timeout + maxWait: 5000, // Maximum time to wait for transaction + }); }
🛑 Comments failed to post (2)
packages/graphql/src/scripts/2024-11-07_migrate_live_session_to_live_quiz.ts (2)
314-399:
⚠️ Potential issueAdd error handling and optimize Redis operations.
The function has several potential issues:
- No error handling for Redis operations
- Potential race condition between cache check and update
- Pipeline execution inside forEach loop is inefficient
Consider applying these improvements:
async function applyCacheUpdatesForQuiz( redisExec: Redis, newLiveQuiz: LiveQuiz & { activeBlock?: ElementBlock & { elements: Element[] } } ) { + const key = `lq:${newLiveQuiz.id}:meta`; + + // Use multi instead of pipeline for atomic operations + const multi = redisExec.multi(); + + try { - if (await redisExec.exists(`lq:${newLiveQuiz.id}:meta`)) { - console.log(`Cache: Live quiz ${newLiveQuiz.id} already migrated`) - return - } + // Use SETNX for atomic check-and-set + multi.hsetnx(key, 'namespace', newLiveQuiz.namespace); + const results = await multi.exec(); + + if (!results?.[0]?.[1]) { + console.log(`Cache: Live quiz ${newLiveQuiz.id} already migrated`); + return; + } - const pipeline = redisExec.pipeline(); + const multi = redisExec.multi(); // ... rest of the cache updates ... - activeBlock.elements.forEach(async (instance) => { + for (const instance of activeBlock.elements) { // ... instance updates ... - }) + } - await pipeline.exec() + await multi.exec(); + } catch (error) { + console.error(`Failed to update cache for quiz ${newLiveQuiz.id}:`, error); + throw error; + } }Committable suggestion skipped: line range outside the PR's diff.
628-701: 🛠️ Refactor suggestion
Improve robustness and performance of the migration process.
The main function could benefit from several improvements:
- Batch processing for better performance
- Proper cleanup on errors
- Configuration management
Consider these improvements:
async function run() { + let redisExec: Redis | null = null; + let prisma: PrismaClient | null = null; + + try { - const redisExec = new Redis({ + redisExec = new Redis({ family: 4, - host: process.env.REDIS_HOST ?? 'localhost', - password: process.env.REDIS_PASS ?? '', - port: Number(process.env.REDIS_PORT) ?? 6379, - tls: process.env.REDIS_TLS ? {} : undefined, + host: process.env.REDIS_HOST || 'localhost', + password: process.env.REDIS_PASS || '', + port: parseInt(process.env.REDIS_PORT || '6379', 10), + tls: process.env.REDIS_TLS === 'true' ? {} : undefined, }); - const prisma = new PrismaClient(); + prisma = new PrismaClient(); const liveSessions = await prisma.liveSession.findMany({ // ... query options ... }); + // Process in batches for better performance + const BATCH_SIZE = 10; + for (let i = 0; i < liveSessions.length; i += BATCH_SIZE) { + const batch = liveSessions.slice(i, i + BATCH_SIZE); + await Promise.all( + batch.map(async (liveSession) => { + try { + const newLiveQuiz = await applyDBUpdatesForQuiz(prisma!, liveSession); + if (newLiveQuiz.status === PublicationStatus.PUBLISHED) { + await applyCacheUpdatesForQuiz(redisExec!, newLiveQuiz); + } + console.log( + `Migrated live session ${liveSession.id} to live quiz ${newLiveQuiz.id}` + ); + } catch (error) { + console.error( + `Failed to migrate session ${liveSession.id}:`, + error + ); + } + }) + ); + } + } catch (error) { + console.error('Migration failed:', error); + throw error; + } finally { + await prisma?.$disconnect(); + await redisExec?.quit(); + } }📝 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./** * Migrates existing live sessions to live quizzes in the database and updates corresponding Redis cache entries * * This function: * 1. Connects to Redis using environment variables or default values * 2. Creates a Prisma client instance * 3. Fetches all live sessions with their associated blocks and question instances * 4. For each live session: * - Creates a new live quiz entry in the database * - Updates Redis cache if the quiz is published * 5. Disconnects from Prisma and Redis after completion * * @requires REDIS_HOST - Redis host (defaults to 'localhost') * @requires REDIS_PASS - Redis password (defaults to '') * @requires REDIS_PORT - Redis port (defaults to 6379) * @requires REDIS_TLS - Redis TLS configuration (optional) * * @throws Will throw an error if database operations fail * @throws Will throw an error if Redis operations fail * * @async */ async function run() { let redisExec: Redis | null = null; let prisma: PrismaClient | null = null; try { redisExec = new Redis({ family: 4, host: process.env.REDIS_HOST || 'localhost', password: process.env.REDIS_PASS || '', port: parseInt(process.env.REDIS_PORT || '6379', 10), tls: process.env.REDIS_TLS === 'true' ? {} : undefined, }); prisma = new PrismaClient(); const liveSessions = await prisma.liveSession.findMany({ include: { blocks: { orderBy: { order: 'asc', }, include: { instances: { orderBy: { order: 'asc', }, }, }, }, activeBlock: { include: { instances: { orderBy: { order: 'asc', }, }, }, }, }, }); // Process in batches for better performance const BATCH_SIZE = 10; for (let i = 0; i < liveSessions.length; i += BATCH_SIZE) { const batch = liveSessions.slice(i, i + BATCH_SIZE); await Promise.all( batch.map(async (liveSession) => { try { const newLiveQuiz = await applyDBUpdatesForQuiz(prisma!, liveSession); if (newLiveQuiz.status === PublicationStatus.PUBLISHED) { await applyCacheUpdatesForQuiz(redisExec!, newLiveQuiz); } console.log( `Migrated live session ${liveSession.id} to live quiz ${newLiveQuiz.id}` ); } catch (error) { console.error( `Failed to migrate session ${liveSession.id}:`, error ); } }) ); } } catch (error) { console.error('Migration failed:', error); throw error; } finally { await prisma?.$disconnect(); await redisExec?.quit(); } }
klicker-uzh
|
||||||||||||||||||||||||||||
| Project |
klicker-uzh
|
| Branch Review |
LiveQuizScriptImprovements
|
| Run status |
|
| Run duration | 11m 09s |
| Commit |
|
| Committer | Roland Schläfli |
| View all properties for this run ↗︎ | |
| Test results | |
|---|---|
Failures
|
0
|
Flaky
|
0
|
Pending
|
0
|
Skipped
|
0
|
Passing
|
140
|
| View all changes introduced in this branch ↗︎ | |
|
Summary by CodeRabbit
New Features
Bug Fixes
Documentation