Skip to content

feat: add caching layer for embed authentication flow#849

Open
Natwar589 wants to merge 1 commit intotestingfrom
login_data_cached_for_embed
Open

feat: add caching layer for embed authentication flow#849
Natwar589 wants to merge 1 commit intotestingfrom
login_data_cached_for_embed

Conversation

@Natwar589
Copy link
Copy Markdown
Collaborator

  • Cache folder, organization, and user data in Redis with 24-hour TTL
  • Implement cache-first lookup strategy for folder, org, and user queries in embed login and middleware
  • Invalidate folder cache on embed update to maintain data consistency
  • Reduce database queries by leveraging cached data when available

Copy link
Copy Markdown
Contributor

@windsurf-bot windsurf-bot bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Other comments (3)
  • src/controllers/embed.controller.js (43-43) The cache key `embed_folder_${folder_id}` is duplicated in the code. Consider defining it as a constant in the constants file to avoid inconsistencies if the key format needs to change in the future.
  • src/middlewares/gtwyEmbedMiddleware.js (28-28) The cache TTL is hardcoded as 86400 seconds (24 hours) in multiple places. Consider extracting this value to a constant or configuration variable to improve maintainability.
  • src/controllers/embed.controller.js (58-58) The TTL value for the folder cache is hardcoded as 86400 seconds. Consider using a constant or configuration value for this TTL to make it easier to maintain and adjust in the future.

💡 To request another review, post a new comment with "/windsurf-review".

Comment on lines +56 to +59
if (folderFromDb && !folder) {
folder = folderFromDb;
await storeInCache(cacheKeyFolder, folder, 86400);
}
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

There's no error handling for the case where both cachedFolder and folderFromDb are null. This could lead to a null reference error when accessing folder properties later in the code (e.g., folder?.config).

Comment on lines +22 to +29
const cachedOrg = await findInCache(cacheKeyOrg);
if (cachedOrg) {
orgTokenFromDb = JSON.parse(cachedOrg);
} else {
orgTokenFromDb = await getOrganizationById(decodedToken?.org_id);
if (orgTokenFromDb) {
await storeInCache(cacheKeyOrg, orgTokenFromDb, 86400);
}
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The cache operations (findInCache and storeInCache) don't have error handling. Consider adding try/catch blocks around cache operations to gracefully handle potential Redis connection issues or other cache-related errors.

if (checkToken.user_id) checkToken.user_id = encryptString(checkToken.user_id);
const { proxyResponse, name, email } = await createOrGetUser(checkToken, decodedToken, orgTokenFromDb);

const cacheKeyUser = `embed_user_${decodedToken.user_id}_${decodedToken.org_id}`;
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The cache key for the user uses decodedToken.user_id, but earlier in the code checkToken.user_id is encrypted. If these are meant to represent the same user ID, this inconsistency could lead to cache misses or duplicate cache entries.

Copy link
Copy Markdown
Collaborator

@Husainbw786 Husainbw786 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

📝 Code Review: Caching Layer for Embed Authentication

Overall good approach to reduce DB queries. Here are some suggestions to improve robustness and consistency:


🔴 Issues to Address

1. Cache key collision risk (embed.controller.js:43)
The cache key embed_folder_${folder_id} doesn't include any namespace or version prefix. If the folder schema changes or you need to invalidate all folder caches, you'll need to find/delete keys manually.

Suggestion: Use a versioned prefix like v1:embed:folder:${folder_id}


2. Missing error handling on JSON.parse (multiple locations)
If cached data is corrupted or malformed, JSON.parse(cachedFolder) will throw and crash the request.

Suggestion: Wrap in try-catch:

try {
  folder = JSON.parse(cachedFolder);
} catch (e) {
  await deleteInCache(cacheKeyFolder); // Clear corrupted cache
  folder = null;
}

3. Race condition in cache-aside pattern (embed.controller.js:45-56)
If two requests hit simultaneously when cache is empty, both will query DB and write to cache. Not critical but wastes resources.

Suggestion: Consider using a cache lock or accept the trade-off (usually fine for read-heavy workloads).


4. Inconsistent cache invalidation (gtwyEmbedMiddleware.js)
You cache org and user data but there's no invalidation when org or user data changes elsewhere in the codebase. The 24-hour TTL means stale data for up to a day.

Suggestion:

  • Document which operations should invalidate embed_org_* and embed_user_* keys
  • Add invalidation calls to org/user update endpoints

5. Magic number for TTL (86400)
The TTL value is hardcoded in multiple places.

Suggestion: Extract to a constant:

// constants.js
export const EMBED_CACHE_TTL = 86400; // 24 hours

🟡 Minor Suggestions

6. Redundant condition (embed.controller.js:54)

if (folderFromDb && !folder) {

If folder was set from cache, folderFromDb will be null (from Promise.resolve(null)), so the && !folder check is redundant.

7. Consider storeInCache returning the stored value
Would allow cleaner one-liners:

folder = cachedFolder ? JSON.parse(cachedFolder) : await storeInCache(key, await FolderModel.findOne(...), TTL);

✅ What's Good

  • Cache-first strategy will significantly reduce DB load
  • Proper cache invalidation on embed update
  • Parallel Promise.all preserved for non-dependent operations
  • Clean separation of cache key per entity type

Verdict: Approve with suggestions. The caching logic is sound, but add error handling for JSON.parse and consider documenting the invalidation strategy for org/user caches.

Copy link
Copy Markdown
Collaborator

@Husainbw786 Husainbw786 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

✅ Updated Code Review: Caching Layer for Embed Authentication

Great job addressing the previous feedback! The latest version shows solid improvements:


What's Fixed

1. Centralized cache configuration (constant.js)
Excellent! You've added embed_cache with:

  • TTL extracted to a single constant (86400)
  • Key generator functions for folder, org, and user

This makes future changes trivial and prevents key format inconsistencies.

2. JSON.parse error handling (embed.controller.js:48-52, gtwyEmbedMiddleware.js:25-29, 46-50)
All JSON.parse calls now have try-catch with silent fallback to null. Corrupted cache won't crash requests. 👍


🟡 Minor Observations

1. Corrupted cache not deleted
When JSON.parse fails, you set the value to null but don't delete the corrupted cache entry. This means every subsequent request will:

  1. Find the corrupted entry
  2. Fail to parse it
  3. Fall back to DB

Until the 24-hour TTL expires. Consider:

catch {
  await deleteInCache(cacheKeyFolder); // Clear bad entry
  folder = null;
}

2. Org/user cache invalidation still undocumented
Folder cache is properly invalidated in updateEmbed. But embed:org_* and embed:user_* caches don't have corresponding invalidation in org/user update flows. This might cause stale data for up to 24 hours if org or user details change.

Consider adding a comment/TODO or invalidation calls to those update endpoints.


Verdict: Approve

The implementation is solid. The caching pattern is consistent, error handling is in place, and the constants are properly centralized. The minor points above are suggestions for polish, not blockers.

Ship it! 🚀

@Natwar589 Natwar589 force-pushed the login_data_cached_for_embed branch from 9f68384 to 530ac12 Compare March 25, 2026 09:06
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants