fix: resolve issue #1396 - IIT Professor List and Stats Cache Invalidation on Modifications#1397
fix: resolve issue #1396 - IIT Professor List and Stats Cache Invalidation on Modifications#1397sonusharma6-dsa wants to merge 2 commits into
Conversation
github-actions
Bot
added
gssoc
level:intermediate
|
Hi @sonusharma6-dsa, thanks for contributing to InternHack! 🎉 I have automatically:
Our workflows will now analyze your changes to classify:
Tip Ensure your PR description references the issue it resolves (e.g. Happy coding! 🚀 |
github-actions
Bot
added
gssoc:approved
|
Warning Review limit reached
More reviews will be available in 17 minutes and 2 seconds. Learn how PR review limits work. Your organization has run out of usage credits. Purchase more in the billing tab. ⌛ How to resolve this issue?After more reviews become available, 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 include higher PR review limits than trial, open-source, and free plans. In all cases, reviews become available again over time. During sustained high-volume PR review activity, CodeRabbit may temporarily slow when the next review becomes available. Please see our Fair Usage Limits Policy for further information. ℹ️ Review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (2)
📝 WalkthroughWalkthroughThis PR extends the cache system with flexible parameter support and implements cache invalidation for professor CRUD operations. Cache middleware gains optional custom TTL and key prefix parameters, while ProfessorService adds create/update/delete methods that clear the professor cache after modifications. Minor cleanup includes deduplicating an import and updating test type annotations. ChangesCache System Improvements
Code Quality Fixes
Estimated code review effort🎯 2 (Simple) | ⏱️ ~12 minutes Possibly related issues
Suggested labels
Poem
🚥 Pre-merge checks | ✅ 3 | ❌ 2❌ Failed checks (2 inconclusive)
✅ Passed checks (3 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
github-actions
Bot
added
quality:exceptional
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
server/src/module/ats/__tests__/ats.service.test.ts (1)
337-339: ⚡ Quick winPrefer more targeted type annotations over blanket
anycasts.The current pattern
(async (args: any) => ...) as anyfixes compilation errors but removes all type checking for these mock implementations. Consider typing just theargsparameter structure instead of casting the entire function:- vi.mocked(prisma.atsScore.create).mockImplementation( - (async (args: any) => - ({ ...MOCK_ATS_ROW, overallScore: (args.data as any).overallScore })) as any, - ); + vi.mocked(prisma.atsScore.create).mockImplementation( + async (args: { data: any }) => + ({ ...MOCK_ATS_ROW, overallScore: args.data.overallScore }) as any, + );This preserves the fix for compilation errors while maintaining better type safety. Apply the same pattern to the other two instances at lines 358-360 and 385-387.
Also applies to: 358-360, 385-387
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@server/src/module/ats/__tests__/ats.service.test.ts` around lines 337 - 339, Replace the blanket any casts on the mock async callbacks with a focused param type: instead of `(async (args: any) => ({ ...MOCK_ATS_ROW, overallScore: (args.data as any).overallScore })) as any`, declare the function parameter with a narrow shape (e.g. `async (args: { data: { overallScore: number } }) => ...`) and remove the `as any` cast and the inner `(args.data as any)` cast; apply the same change to the two other mock callbacks mentioned so that each mock returns `{ ...MOCK_ATS_ROW, overallScore: args.data.overallScore }` while preserving type safety.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@server/src/module/professor/professor.service.ts`:
- Around line 110-125: The create/update/delete methods call
prisma.iitProfessor.* and then await cacheDelPattern("professors:"), which risks
throwing after the DB commit; change the cache invalidation to be non-fatal:
call cacheDelPattern asynchronously and handle its errors without bubbling them
(e.g., fire-and-forget or wrap in try/catch and only log failures) so that
failures to delete cache never roll back or surface after the DB write. Update
the create, update, and delete functions (references: create, update, delete
methods and cacheDelPattern) to ensure the DB operation result is returned even
if cache invalidation fails.
---
Nitpick comments:
In `@server/src/module/ats/__tests__/ats.service.test.ts`:
- Around line 337-339: Replace the blanket any casts on the mock async callbacks
with a focused param type: instead of `(async (args: any) => ({ ...MOCK_ATS_ROW,
overallScore: (args.data as any).overallScore })) as any`, declare the function
parameter with a narrow shape (e.g. `async (args: { data: { overallScore: number
} }) => ...`) and remove the `as any` cast and the inner `(args.data as any)`
cast; apply the same change to the two other mock callbacks mentioned so that
each mock returns `{ ...MOCK_ATS_ROW, overallScore: args.data.overallScore }`
while preserving type safety.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 5ec4e7ac-b3b8-4d39-aa9f-e33186a7b63c
📒 Files selected for processing (4)
server/src/middleware/cache.middleware.tsserver/src/module/admin/admin.controller.tsserver/src/module/ats/__tests__/ats.service.test.tsserver/src/module/professor/professor.service.ts
💤 Files with no reviewable changes (1)
- server/src/module/admin/admin.controller.ts
| async create(data: any) { | ||
| const record = await prisma.iitProfessor.create({ data }); | ||
| await cacheDelPattern("professors:"); | ||
| return record; | ||
| } | ||
|
|
||
| async update(id: number, data: any) { | ||
| const record = await prisma.iitProfessor.update({ where: { id }, data }); | ||
| await cacheDelPattern("professors:"); | ||
| return record; | ||
| } | ||
|
|
||
| async delete(id: number) { | ||
| const record = await prisma.iitProfessor.delete({ where: { id } }); | ||
| await cacheDelPattern("professors:"); | ||
| return record; |
There was a problem hiding this comment.
Do not surface mutation failure after the DB write has already committed.
On Line 111/117/123 the write completes before Line 112/118/124 invalidates cache. If cacheDelPattern fails, the method throws even though data is already persisted, which can cause retry-driven duplicate creates and integration inconsistency.
💡 Suggested fix
export class ProfessorService {
+ private async invalidateProfessorCacheSafely() {
+ try {
+ await cacheDelPattern("professors:");
+ } catch (error) {
+ // Keep write-path successful; log and recover asynchronously if needed.
+ console.error("Professor cache invalidation failed", error);
+ }
+ }
+
async create(data: any) {
const record = await prisma.iitProfessor.create({ data });
- await cacheDelPattern("professors:");
+ await this.invalidateProfessorCacheSafely();
return record;
}
async update(id: number, data: any) {
const record = await prisma.iitProfessor.update({ where: { id }, data });
- await cacheDelPattern("professors:");
+ await this.invalidateProfessorCacheSafely();
return record;
}
async delete(id: number) {
const record = await prisma.iitProfessor.delete({ where: { id } });
- await cacheDelPattern("professors:");
+ await this.invalidateProfessorCacheSafely();
return record;
}
}📝 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.
| async create(data: any) { | |
| const record = await prisma.iitProfessor.create({ data }); | |
| await cacheDelPattern("professors:"); | |
| return record; | |
| } | |
| async update(id: number, data: any) { | |
| const record = await prisma.iitProfessor.update({ where: { id }, data }); | |
| await cacheDelPattern("professors:"); | |
| return record; | |
| } | |
| async delete(id: number) { | |
| const record = await prisma.iitProfessor.delete({ where: { id } }); | |
| await cacheDelPattern("professors:"); | |
| return record; | |
| private async invalidateProfessorCacheSafely() { | |
| try { | |
| await cacheDelPattern("professors:"); | |
| } catch (error) { | |
| // Keep write-path successful; log and recover asynchronously if needed. | |
| console.error("Professor cache invalidation failed", error); | |
| } | |
| } | |
| async create(data: any) { | |
| const record = await prisma.iitProfessor.create({ data }); | |
| await this.invalidateProfessorCacheSafely(); | |
| return record; | |
| } | |
| async update(id: number, data: any) { | |
| const record = await prisma.iitProfessor.update({ where: { id }, data }); | |
| await this.invalidateProfessorCacheSafely(); | |
| return record; | |
| } | |
| async delete(id: number) { | |
| const record = await prisma.iitProfessor.delete({ where: { id } }); | |
| await this.invalidateProfessorCacheSafely(); | |
| return record; | |
| } |
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@server/src/module/professor/professor.service.ts` around lines 110 - 125, The
create/update/delete methods call prisma.iitProfessor.* and then await
cacheDelPattern("professors:"), which risks throwing after the DB commit; change
the cache invalidation to be non-fatal: call cacheDelPattern asynchronously and
handle its errors without bubbling them (e.g., fire-and-forget or wrap in
try/catch and only log failures) so that failures to delete cache never roll
back or surface after the DB write. Update the create, update, and delete
functions (references: create, update, delete methods and cacheDelPattern) to
ensure the DB operation result is returned even if cache invalidation fails.
sonusharma6-dsa
force-pushed
the
fix-issue-1396
branch
from
59b1929 to
c2c5a19
Compare
github-actions
Bot
added
level:beginner
…Stats Cache Invalidation on Modifications
sonusharma6-dsa
force-pushed
the
fix-issue-1396
branch
from
c2c5a19 to
f3a882c
Compare
github-actions
Bot
added
quality:exceptional
|
Closing this PR. The cache invalidation approach is correct in principle, but the three new methods (create, update, delete) use untyped data: any parameters and there are no corresponding routes wired up to call them — adding dead service methods without routes creates confusion. Please open a fresh PR that includes the route wiring and proper TypeScript types for the data parameter. Thank you for the effort! |
2 participants
Closes #1396
Summary by CodeRabbit
New Features
Tests
Chores