Redis implementation in tracking-microservice - #136
Conversation
|
Caution The consumer version of Gemini Code Assist on GitHub has been sunset. All code review activity has officially ceased. |
📝 WalkthroughSummary by CodeRabbit
WalkthroughThe change adds a configurable Redis or memory cache layer. It centralizes cache reads, invalidation, health, metrics, and failure handling. Assessment, content, user-certificate, and certificate services now use cache-backed loaders and tenant-scoped invalidation. ChangesShared caching rollout
Estimated code review effort: 4 (Complex) | ~60 minutes Sequence Diagram(s)sequenceDiagram
participant TrackingService
participant CacheService
participant CacheStore
participant Database
TrackingService->>CacheService: getOrLoad(namespace, key, loader)
CacheService->>CacheStore: read version and cache entry
CacheStore-->>CacheService: cache hit or miss
CacheService->>Database: execute loader on miss or bypass
Database-->>CacheService: tracking result
CacheService->>CacheStore: write cacheable result
CacheService-->>TrackingService: return result
TrackingService->>CacheService: invalidate(namespace)
CacheService->>CacheStore: increment namespace version
🚥 Pre-merge checks | ✅ 3 | ❌ 2❌ Failed checks (1 warning, 1 inconclusive)
✅ Passed checks (3 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
Warning There were issues while running some tools. Please review the errors and either fix the tool's configuration or disable the tool if it's a critical failure. 🔧 ESLint
package.jsonParsing error: ESLint was configured to run on 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 |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 10
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
src/modules/tracking_content/tracking_content.service.ts (1)
443-527: 🔒 Security & Privacy | 🔴 Critical | ⚡ Quick winParameterize the
INclauses instead of interpolating request values.Lines 453-481 build
contentId_text,courseId_text, andunitId_textby concatenating values fromsearchFilterdirectly into the SQL text at lines 505-507. A caller controls these array values, so a string element that contains a quote alters the statement. This is a SQL injection path on a public search endpoint. The relocation into the cached loader keeps the defect.Two further gaps exist in the same block:
- If any array is empty, the query produces
IN ()and Postgres rejects the statement.- If
searchFilter.contentId,courseId,unitId, oruserIdis missing, Line 454 dereferencesundefined.lengthand throws.Use array parameters with
= ANY($n), as thesearchStatusCourseTrackingloader already does at Line 743.🔒 Proposed fix
- loader: async () => { - let contentId_text = ''; - for (let i = 0; i < contentIdArray.length; i++) { - let contentId = contentIdArray[i]; - if (i == 0) { - contentId_text = `${contentId_text}'${contentId}'`; - } else { - contentId_text = `${contentId_text},'${contentId}'`; - } - } - //courseId - let courseId_text = ''; - for (let i = 0; i < courseIdArray.length; i++) { - let courseId = courseIdArray[i]; - if (i == 0) { - courseId_text = `${courseId_text}'${courseId}'`; - } else { - courseId_text = `${courseId_text},'${courseId}'`; - } - } - //unitId - let unitId_text = ''; - for (let i = 0; i < unitIdArray.length; i++) { - let unitId = unitIdArray[i]; - if (i == 0) { - unitId_text = `${unitId_text}'${unitId}'`; - } else { - unitId_text = `${unitId_text},'${unitId}'`; - } - } + loader: async () => { const loaded = []; for (let ii = 0; ii < userIdArray.length; ii++) { - let userId = userIdArray[ii]; + const userId = userIdArray[ii]; const result = await this.dataSource.query( `WITH latest_content AS ( ... "userId" = $1 - AND "courseId" IN (${courseId_text}) - AND "unitId" IN (${unitId_text}) - AND "contentId" IN (${contentId_text}) - AND "tenantId" = $2 + AND "courseId" = ANY($2) + AND "unitId" = ANY($3) + AND "contentId" = ANY($4) + AND "tenantId" = $5 ) ...`, - [userId, tenantId], + [userId, courseIdArray, unitIdArray, contentIdArray, tenantId], );Add array validation before the loader, in the same style as
searchStatusCourseTrackinglines 614-628.🤖 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 `@src/modules/tracking_content/tracking_content.service.ts` around lines 443 - 527, Update the cached loader around output_result to validate contentIdArray, courseIdArray, unitIdArray, and userIdArray before accessing their lengths, returning the established empty-result behavior for missing or empty arrays as in searchStatusCourseTracking. Remove the interpolated contentId_text, courseId_text, and unitId_text SQL fragments, bind the arrays as query parameters, and replace each IN clause with = ANY($n), preserving the existing user and tenant filtering.
🧹 Nitpick comments (5)
src/modules/tracking_content/tracking_content.service.ts (1)
1345-1348: 🚀 Performance & Scalability | 🔵 Trivial | 💤 Low valueScope the
courseinprogressnamespace by tenant.The namespace at Line 1346 has no tenant segment, and the key at Line 1347 hashes only
userIdArray. Every tenant write to content tracking invalidates this single namespace, so one tenant's writes drop cached entries for all tenants. Tenant-scope the namespace to limit invalidation blast radius.courseInProgressreads no tenant header today, so this change also requires a tenant source in this method.🤖 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 `@src/modules/tracking_content/tracking_content.service.ts` around lines 1345 - 1348, Update the courseInProgress cache lookup in the surrounding tracking-content method to obtain the current tenant from the method’s established request/context source, then include that tenant identifier in the courseinprogress namespace (and any required cache key inputs). Preserve user-specific caching while ensuring invalidation for one tenant cannot affect another.src/cache/cache.constants.ts (1)
26-29: 🩺 Stability & Availability | 🔵 Trivial | ⚡ Quick winGuard against non-numeric env values for timing configs.
Number(get(key) ?? default)does not fall back to the default whenget(key)returns an empty string or a non-numeric value. In that caseNumber(...)returnsNaN.NaNthen flows intosetTimeoutcalls incache.service.ts, which treats aNaNdelay as0ms, causing every cache operation to time out immediately and silently degrade caching to "always miss" without any visible error.Add a
Number.isFinitefallback for each numeric field.🛠️ Proposed fix
+function num(value: unknown, fallback: number): number { + const n = Number(value); + return Number.isFinite(n) ? n : fallback; +} + export function loadCacheConfig(get: (key: string) => any): CacheConfig { const disabledNamespacesRaw: string = get('CACHE_DISABLED_NAMESPACES') || ''; return { enabled: String(get('CACHE_ENABLED') ?? 'false').toLowerCase() === 'true', provider: (get('CACHE_PROVIDER') || 'memory') as 'memory' | 'redis', redisUrl: get('REDIS_URL'), keyPrefix: get('CACHE_KEY_PREFIX') || 'tms', disabledNamespaces: disabledNamespacesRaw .split(',') .map((s) => s.trim()) .filter((s) => s.length > 0), - opTimeoutMs: Number(get('CACHE_OP_TIMEOUT_MS') ?? 150), - cbFailures: Number(get('CACHE_CB_FAILURES') ?? 5), - cbCooldownMs: Number(get('CACHE_CB_COOLDOWN_MS') ?? 30000), - metricsIntervalMs: Number(get('CACHE_METRICS_INTERVAL_MS') ?? 60000), + opTimeoutMs: num(get('CACHE_OP_TIMEOUT_MS'), 150), + cbFailures: num(get('CACHE_CB_FAILURES'), 5), + cbCooldownMs: num(get('CACHE_CB_COOLDOWN_MS'), 30000), + metricsIntervalMs: num(get('CACHE_METRICS_INTERVAL_MS'), 60000), }; }🤖 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 `@src/cache/cache.constants.ts` around lines 26 - 29, Update the numeric cache configuration fields opTimeoutMs, cbFailures, cbCooldownMs, and metricsIntervalMs to validate the parsed values with Number.isFinite and use their existing defaults when parsing yields NaN or another non-finite value, including empty or invalid environment values.src/cache/cache.service.spec.ts (1)
35-39: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winAdd a regression test for non-serializable loader results.
Once
writeCacheEntryincache.service.tsis fixed to guardJSON.stringify, add a test where the loader resolves with a circular-reference object (or a value containing aBigInt) and assert thatgetOrLoadstill resolves with that result instead of rejecting.Do you want me to draft this test case?
🤖 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 `@src/cache/cache.service.spec.ts` around lines 35 - 39, Add a regression test in the CacheService test suite that configures the loader used by getOrLoad to resolve with a non-serializable value, such as a circular-reference object or BigInt, and assert that getOrLoad resolves with the original result rather than rejecting. Keep the existing afterEach cleanup and test the behavior through the public getOrLoad path.package.json (1)
33-33: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winRemove the stale cache dependencies.
cache-manager-redis-yetis unused andredis-cache.store.tsimportsredisdirectly. The local cache abstraction is an emptyCacheModule, while@nestjs/cache-managerandcache-manager-memory-storeare still declared in bothpackage.jsonandpackage-lock.json. Drop these unused dependencies and lock entries if memory caching is no longer needed.🤖 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 `@package.json` at line 33, Remove the unused cache dependencies from package.json, including cache-manager-redis-yet, `@nestjs/cache-manager`, and cache-manager-memory-store, and regenerate package-lock.json so their stale lock entries are removed. Preserve the direct redis dependency used by redis-cache.store.ts and verify no remaining code requires the removed packages.src/modules/tracking_assessment/tracking_assessment.service.ts (1)
671-805: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winNumerous prettier and prefer-const violations in the relocated cache-loader code.
Static analysis flags a large number of
prettier/prettierindentation mismatches andprefer-constviolations across Lines 671-805 and 815-865 (for example,contentIdArray,courseIdArray,unitIdArray,userIdArray,temp_result,maxMark,scoreMark,percentage,temp_obj, and others are never reassigned but declared withlet). This code was relocated intoCacheService.getOrLoadloader closures, and the indentation was not adjusted to match, and theletdeclarations were not updated. Run the formatter and switch flaggedletdeclarations toconstto keep the lint pipeline clean.Also applies to: 815-865
🤖 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 `@src/modules/tracking_assessment/tracking_assessment.service.ts` around lines 671 - 805, Format the relocated cache-loader code in the tracking assessment service, including the ranges around the getOrLoad loader and adjacent logic, to satisfy Prettier indentation. In the same code, change every variable that is never reassigned—such as contentIdArray, courseIdArray, unitIdArray, userIdArray, temp_result, maxMark, scoreMark, percentage, and temp_obj—from let to const, while preserving variables that are reassigned.Source: Linters/SAST tools
🤖 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 `@src/cache/cache.module.ts`:
- Around line 13-24: Update the factory in the cache provider configuration to
check config.enabled before entering the Redis branch. When caching is disabled,
return MemoryCacheStore without requiring REDIS_URL or constructing
RedisCacheStore; preserve the existing Redis validation and selection behavior
when config.enabled is true.
In `@src/cache/cache.service.ts`:
- Around line 146-152: Guard serialization in writeCacheEntry by moving
JSON.stringify(result) into a try/catch before invoking withTimeout or
store.set. On serialization failure, call onOpFailure and return without
throwing, preserving the successful getOrLoad result while retaining the
existing async failure handling for cache writes.
In `@src/cache/memory-cache.store.ts`:
- Around line 32-37: Update MemoryCacheStore.incr so the read-modify-write
operation executes synchronously without awaits between reading and writing the
key, ensuring concurrent calls cannot lose increments. Preserve the existing
numeric parsing, default value, stored string format, and returned next value.
In `@src/modules/tracking_assessment/tracking_assessment.service.ts`:
- Around line 336-340: Update the create flow in createAssessmentTracking to
invalidate the per-record assessmentread cache key for the client-supplied
assessmentTrackingId in addition to the existing tenant-level assessment cache
namespace, ensuring newly created records do not serve a cached missing result.
- Around line 671-747: Update searchStatusAssessmentTracking to remove
courseId_text, unitId_text, and contentId_text string construction; use
parameterized array predicates with = ANY(...::text[]) or individually numbered
placeholders, and pass the raw array values through dataSource.query parameters.
Preserve the existing userId and tenantId bindings while ensuring courseId,
unitId, and contentId values are never interpolated into the SQL.
In `@src/modules/tracking_content/tracking_content.service.ts`:
- Around line 651-670: Run Prettier on the new dashboard loader code around the
certificateResults, statusMap, and attemptMap processing in the tracking
service, correcting the reported formatting violations on the query and map
lines while preserving the existing logic.
- Around line 310-314: Add the `contentread:${contentTrackingId}` entity
namespace to the invalidation keys used by `createContentTracking` after it
updates or creates tracking data, alongside the existing content and course
namespaces, so `getContentTrackingDetails` does not serve stale cached details.
- Around line 632-705: The assessment tracking write paths must also invalidate
the course dashboard cache because getOrLoad caches attempt_count under
course:${tenantId}. Update createAssessmentTracking, deleteAssessmentTracking,
and any assessment-tracking update path that changes a course/user’s attempts to
invalidate course:${tenantId} alongside assessment:${tenantId}, reusing the
existing tenant-scoped cache invalidation mechanism.
In `@src/modules/user_certificate/user_certificate.service`..ts:
- Around line 245-262: Restrict the filter iteration in the
UserCourseCertificate query-building flow to an explicit allowlist of valid
UserCourseCertificate column names before interpolating key into SQL or
parameter names. Skip or reject any unapproved key, while preserving the
existing array IN and scalar equality handling for allowed keys.
- Around line 334-338: Update the cache invalidation in the import flow around
the user certificate record lookup and save to use the saved record’s tenantId
(from createUserCertificateDto) rather than request.tenantId. Also invalidate
the corresponding course:${tenantId} namespace, matching enrollUserForCourse, so
both usercert and course caches are cleared for the record’s owning tenant.
---
Outside diff comments:
In `@src/modules/tracking_content/tracking_content.service.ts`:
- Around line 443-527: Update the cached loader around output_result to validate
contentIdArray, courseIdArray, unitIdArray, and userIdArray before accessing
their lengths, returning the established empty-result behavior for missing or
empty arrays as in searchStatusCourseTracking. Remove the interpolated
contentId_text, courseId_text, and unitId_text SQL fragments, bind the arrays as
query parameters, and replace each IN clause with = ANY($n), preserving the
existing user and tenant filtering.
---
Nitpick comments:
In `@package.json`:
- Line 33: Remove the unused cache dependencies from package.json, including
cache-manager-redis-yet, `@nestjs/cache-manager`, and cache-manager-memory-store,
and regenerate package-lock.json so their stale lock entries are removed.
Preserve the direct redis dependency used by redis-cache.store.ts and verify no
remaining code requires the removed packages.
In `@src/cache/cache.constants.ts`:
- Around line 26-29: Update the numeric cache configuration fields opTimeoutMs,
cbFailures, cbCooldownMs, and metricsIntervalMs to validate the parsed values
with Number.isFinite and use their existing defaults when parsing yields NaN or
another non-finite value, including empty or invalid environment values.
In `@src/cache/cache.service.spec.ts`:
- Around line 35-39: Add a regression test in the CacheService test suite that
configures the loader used by getOrLoad to resolve with a non-serializable
value, such as a circular-reference object or BigInt, and assert that getOrLoad
resolves with the original result rather than rejecting. Keep the existing
afterEach cleanup and test the behavior through the public getOrLoad path.
In `@src/modules/tracking_assessment/tracking_assessment.service.ts`:
- Around line 671-805: Format the relocated cache-loader code in the tracking
assessment service, including the ranges around the getOrLoad loader and
adjacent logic, to satisfy Prettier indentation. In the same code, change every
variable that is never reassigned—such as contentIdArray, courseIdArray,
unitIdArray, userIdArray, temp_result, maxMark, scoreMark, percentage, and
temp_obj—from let to const, while preserving variables that are reassigned.
In `@src/modules/tracking_content/tracking_content.service.ts`:
- Around line 1345-1348: Update the courseInProgress cache lookup in the
surrounding tracking-content method to obtain the current tenant from the
method’s established request/context source, then include that tenant identifier
in the courseinprogress namespace (and any required cache key inputs). Preserve
user-specific caching while ensuring invalidation for one tenant cannot affect
another.
🪄 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: Organization UI
Review profile: CHILL
Plan: Pro Plus
Run ID: 36917a8e-06f2-4178-bca0-2323c0232bd2
⛔ Files ignored due to path filters (1)
package-lock.jsonis excluded by!**/package-lock.json
📒 Files selected for processing (19)
caching-strategy.mdpackage.jsonsrc/app.controller.tssrc/app.module.tssrc/cache/cache-key.util.tssrc/cache/cache-store.interface.tssrc/cache/cache.constants.tssrc/cache/cache.module.tssrc/cache/cache.service.spec.tssrc/cache/cache.service.tssrc/cache/memory-cache.store.tssrc/cache/redis-cache.store.tssrc/modules/ai_assessment/ai_assessment.service.tssrc/modules/certificate/certificate.service.tssrc/modules/tracking_assessment/tracking_assessment.controller.tssrc/modules/tracking_assessment/tracking_assessment.service.tssrc/modules/tracking_content/tracking_content.controller.tssrc/modules/tracking_content/tracking_content.service.tssrc/modules/user_certificate/user_certificate.service..ts
💤 Files with no reviewable changes (3)
- src/modules/tracking_assessment/tracking_assessment.controller.ts
- src/modules/tracking_content/tracking_content.controller.ts
- src/modules/ai_assessment/ai_assessment.service.ts
| useFactory: (configService: ConfigService) => { | ||
| const config = loadCacheConfig((k) => configService.get(k)); | ||
| if (config.provider === 'redis') { | ||
| if (!config.redisUrl) { | ||
| throw new Error( | ||
| 'REDIS_URL is required when CACHE_PROVIDER=redis', | ||
| ); | ||
| } | ||
| return new RedisCacheStore(config.redisUrl); | ||
| } | ||
| return new MemoryCacheStore(); | ||
| }, |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟠 Major | ⚡ Quick win
Honor CACHE_ENABLED before requiring Redis configuration.
The factory selects RedisCacheStore and requires REDIS_URL whenever CACHE_PROVIDER=redis, regardless of config.enabled. caching-strategy.md documents CACHE_ENABLED as the master on/off switch, but disabling the cache still fails application bootstrap when REDIS_URL is missing, and still opens an unnecessary Redis connection attempt that CacheService never uses once caching is bypassed at line 52 of cache.service.ts.
Gate the Redis branch on config.enabled so a disabled cache never requires Redis configuration.
🛠️ Proposed fix
useFactory: (configService: ConfigService) => {
const config = loadCacheConfig((k) => configService.get(k));
- if (config.provider === 'redis') {
+ if (config.enabled && config.provider === 'redis') {
if (!config.redisUrl) {
throw new Error(
'REDIS_URL is required when CACHE_PROVIDER=redis',
);
}
return new RedisCacheStore(config.redisUrl);
}
return new MemoryCacheStore();
},📝 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.
| useFactory: (configService: ConfigService) => { | |
| const config = loadCacheConfig((k) => configService.get(k)); | |
| if (config.provider === 'redis') { | |
| if (!config.redisUrl) { | |
| throw new Error( | |
| 'REDIS_URL is required when CACHE_PROVIDER=redis', | |
| ); | |
| } | |
| return new RedisCacheStore(config.redisUrl); | |
| } | |
| return new MemoryCacheStore(); | |
| }, | |
| useFactory: (configService: ConfigService) => { | |
| const config = loadCacheConfig((k) => configService.get(k)); | |
| if (config.enabled && config.provider === 'redis') { | |
| if (!config.redisUrl) { | |
| throw new Error( | |
| 'REDIS_URL is required when CACHE_PROVIDER=redis', | |
| ); | |
| } | |
| return new RedisCacheStore(config.redisUrl); | |
| } | |
| return new MemoryCacheStore(); | |
| }, |
🧰 Tools
🪛 ESLint
[error] 17-19: Replace ⏎··············'REDIS_URL·is·required·when·CACHE_PROVIDER=redis',⏎············ with 'REDIS_URL·is·required·when·CACHE_PROVIDER=redis'
(prettier/prettier)
🤖 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 `@src/cache/cache.module.ts` around lines 13 - 24, Update the factory in the
cache provider configuration to check config.enabled before entering the Redis
branch. When caching is disabled, return MemoryCacheStore without requiring
REDIS_URL or constructing RedisCacheStore; preserve the existing Redis
validation and selection behavior when config.enabled is true.
| private writeCacheEntry(entryKey: string, result: unknown, ttlSeconds: number) { | ||
| this.withTimeout( | ||
| this.store.set(entryKey, JSON.stringify(result), ttlSeconds), | ||
| ) | ||
| .then(() => this.onOpSuccess()) | ||
| .catch(() => this.onOpFailure()); | ||
| } |
There was a problem hiding this comment.
🩺 Stability & Availability | 🔴 Critical | ⚡ Quick win
Guard JSON.stringify in writeCacheEntry — it can crash an otherwise-successful request.
JSON.stringify(result) at line 148 is evaluated synchronously as an argument, before withTimeout/store.set run. If it throws — for a circular reference (plausible for TypeORM entities with bidirectional relations) or a BigInt value — the exception propagates synchronously out of writeCacheEntry, which is called unawaited from getOrLoad (line 90). That rejects the promise getOrLoad returns, even though loader() already produced a valid result.
This breaks the resilience guarantee caching-strategy.md documents: "the request always succeeds; caching degrades, correctness doesn't." The read path already guards the symmetric case (JSON.parse at lines 78-84 is wrapped in try/catch); the write path should get the same treatment.
🛠️ Proposed fix
private writeCacheEntry(entryKey: string, result: unknown, ttlSeconds: number) {
- this.withTimeout(
- this.store.set(entryKey, JSON.stringify(result), ttlSeconds),
- )
- .then(() => this.onOpSuccess())
- .catch(() => this.onOpFailure());
+ let serialized: string;
+ try {
+ serialized = JSON.stringify(result);
+ } catch (err) {
+ this.onOpFailure();
+ this.logger.error(
+ `cache SET serialize failed key=${entryKey}: ${(err as Error).message}`,
+ );
+ return;
+ }
+ this.withTimeout(this.store.set(entryKey, serialized, ttlSeconds))
+ .then(() => this.onOpSuccess())
+ .catch(() => this.onOpFailure());
}🧰 Tools
🪛 ESLint
[error] 146-146: Replace entryKey:·string,·result:·unknown,·ttlSeconds:·number with ⏎····entryKey:·string,⏎····result:·unknown,⏎····ttlSeconds:·number,⏎··
(prettier/prettier)
🤖 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 `@src/cache/cache.service.ts` around lines 146 - 152, Guard serialization in
writeCacheEntry by moving JSON.stringify(result) into a try/catch before
invoking withTimeout or store.set. On serialization failure, call onOpFailure
and return without throwing, preserving the successful getOrLoad result while
retaining the existing async failure handling for cache writes.
| async incr(key: string): Promise<number> { | ||
| const current = await this.get(key); | ||
| const next = (current ? parseInt(current, 10) : 0) + 1; | ||
| await this.set(key, String(next)); | ||
| return next; | ||
| } |
There was a problem hiding this comment.
🗄️ Data Integrity & Integration | 🟠 Major | ⚡ Quick win
Fix non-atomic incr(): concurrent calls can lose increments.
incr() awaits this.get(key) then awaits this.set(key, ...). Each await yields to the event loop. Two concurrent calls to incr() on the same key can both read the same value before either writes, losing one increment.
This directly undermines CacheService.invalidate(), whose correctness depends on each INCR reliably bumping the namespace version by exactly one. A lost increment means the old-version cache entries stay reachable one version longer than intended, serving a value that should have been invalidated.
Make the read-modify-write synchronous so no await separates the read and the write.
🛠️ Proposed fix
async incr(key: string): Promise<number> {
- const current = await this.get(key);
- const next = (current ? parseInt(current, 10) : 0) + 1;
- await this.set(key, String(next));
- return next;
+ const entry = this.entries.get(key);
+ const isValid =
+ entry !== undefined &&
+ (entry.expiresAt === null || entry.expiresAt > Date.now());
+ const current = isValid ? entry.value : null;
+ const next = (current ? parseInt(current, 10) : 0) + 1;
+ this.entries.set(key, { value: String(next), expiresAt: null });
+ return next;
}📝 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 incr(key: string): Promise<number> { | |
| const current = await this.get(key); | |
| const next = (current ? parseInt(current, 10) : 0) + 1; | |
| await this.set(key, String(next)); | |
| return next; | |
| } | |
| async incr(key: string): Promise<number> { | |
| const entry = this.entries.get(key); | |
| const isValid = | |
| entry !== undefined && | |
| (entry.expiresAt === null || entry.expiresAt > Date.now()); | |
| const current = isValid ? entry.value : null; | |
| const next = (current ? parseInt(current, 10) : 0) + 1; | |
| this.entries.set(key, { value: String(next), expiresAt: null }); | |
| return next; | |
| } |
🤖 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 `@src/cache/memory-cache.store.ts` around lines 32 - 37, Update
MemoryCacheStore.incr so the read-modify-write operation executes synchronously
without awaits between reading and writing the key, ensuring concurrent calls
cannot lose increments. Preserve the existing numeric parsing, default value,
stored string format, and returned next value.
| await this.cacheService.invalidate( | ||
| `assessment:${tenantId}`, | ||
| 'createAssessmentTracking', | ||
| ); | ||
|
|
There was a problem hiding this comment.
🗄️ Data Integrity & Integration | 🟠 Major | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Description: Check whether CacheService caches null/negative loader results.
ast-grep outline src/cache/cache.service.ts --items all
rg -n -B3 -A15 'async getOrLoad' src/cache/cache.service.tsRepository: tekdi/tracking-microservice
Length of output: 1509
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "== cache.service outline and relevant implementation =="
sed -n '1,180p' src/cache/cache.service.ts
echo
echo "== tracking_assessment relevant sections =="
sed -n '140,210p' src/modules/tracking_assessment/tracking_assessment.service.ts
sed -n '310,355p' src/modules/tracking_assessment/tracking_assessment.service.ts
sed -n '390,445p' src/modules/tracking_assessment/tracking_assessment.service.ts
echo
echo "== assessmentread cache usages =="
rg -n "assessmentread|assessmentTrackingId|invalidate|readCacheEntry|writeCacheEntry" src/modules/tracking_assessment src/cacheRepository: tekdi/tracking-microservice
Length of output: 20864
Invalidate the per-record cache on create.
CacheService.getOrLoad only writes when isCacheable(result) returns true, and the loader at src/modules/tracking_assessment/tracking_assessment.service.ts:97 returns false for a missing record. Since createAssessmentTracking accepts client-supplied assessmentTrackingId, creating a missing ID does not clear assessmentread:${id}; subsequent reads can return the stale false result until TTL expiration. Invalidate this namespace with the tenant namespace on create.
🛠️ Proposed fix
await this.cacheService.invalidate(
- `assessment:${tenantId}`,
+ [
+ `assessment:${tenantId}`,
+ `assessmentread:${result.assessmentTrackingId}`,
+ ],
'createAssessmentTracking',
);📝 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.
| await this.cacheService.invalidate( | |
| `assessment:${tenantId}`, | |
| 'createAssessmentTracking', | |
| ); | |
| await this.cacheService.invalidate( | |
| [ | |
| `assessment:${tenantId}`, | |
| `assessmentread:${result.assessmentTrackingId}`, | |
| ], | |
| 'createAssessmentTracking', | |
| ); |
🤖 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 `@src/modules/tracking_assessment/tracking_assessment.service.ts` around lines
336 - 340, Update the create flow in createAssessmentTracking to invalidate the
per-record assessmentread cache key for the client-supplied assessmentTrackingId
in addition to the existing tenant-level assessment cache namespace, ensuring
newly created records do not serve a cached missing result.
| if(searchFilter?.courseId && searchFilter?.unitId && searchFilter?.contentId) | ||
| { | ||
| let output_result = []; | ||
| let contentIdArray = searchFilter?.contentId; | ||
| let contentId_text = ''; | ||
| for (let i = 0; i < contentIdArray.length; i++) { | ||
| let contentId = contentIdArray[i]; | ||
| if (i == 0) { | ||
| contentId_text = `${contentId_text}'${contentId}'`; | ||
| } else { | ||
| contentId_text = `${contentId_text},'${contentId}'`; | ||
| } | ||
| } | ||
| //courseId | ||
| let courseIdArray = searchFilter?.courseId; | ||
| let courseId_text = ''; | ||
| for (let i = 0; i < courseIdArray.length; i++) { | ||
| let courseId = courseIdArray[i]; | ||
| if (i == 0) { | ||
| courseId_text = `${courseId_text}'${courseId}'`; | ||
| } else { | ||
| courseId_text = `${courseId_text},'${courseId}'`; | ||
| } | ||
| } | ||
| //unitId | ||
| let unitIdArray = searchFilter?.unitId; | ||
| let unitId_text = ''; | ||
| for (let i = 0; i < unitIdArray.length; i++) { | ||
| let unitId = unitIdArray[i]; | ||
| if (i == 0) { | ||
| unitId_text = `${unitId_text}'${unitId}'`; | ||
| } else { | ||
| unitId_text = `${unitId_text},'${unitId}'`; | ||
| } | ||
| } | ||
| let userIdArray = searchFilter?.userId; | ||
| for (let i = 0; i < userIdArray.length; i++) { | ||
| let userId = userIdArray[i]; | ||
| const result = await this.dataSource.query( | ||
| `WITH latest_assessment AS ( | ||
| SELECT | ||
|
|
||
| const output_result = await this.cacheService.getOrLoad({ | ||
| namespace: `assessment:${tenantId}`, | ||
| key: `status:${hashCacheParts( | ||
| userIdArray, | ||
| contentIdArray, | ||
| courseIdArray, | ||
| unitIdArray, | ||
| )}`, | ||
| ttlSeconds: ASSESSMENT_STATUS_TTL_SECONDS, | ||
| loader: async () => { | ||
| let contentId_text = ''; | ||
| for (let i = 0; i < contentIdArray.length; i++) { | ||
| let contentId = contentIdArray[i]; | ||
| if (i == 0) { | ||
| contentId_text = `${contentId_text}'${contentId}'`; | ||
| } else { | ||
| contentId_text = `${contentId_text},'${contentId}'`; | ||
| } | ||
| } | ||
| //courseId | ||
| let courseId_text = ''; | ||
| for (let i = 0; i < courseIdArray.length; i++) { | ||
| let courseId = courseIdArray[i]; | ||
| if (i == 0) { | ||
| courseId_text = `${courseId_text}'${courseId}'`; | ||
| } else { | ||
| courseId_text = `${courseId_text},'${courseId}'`; | ||
| } | ||
| } | ||
| //unitId | ||
| let unitId_text = ''; | ||
| for (let i = 0; i < unitIdArray.length; i++) { | ||
| let unitId = unitIdArray[i]; | ||
| if (i == 0) { | ||
| unitId_text = `${unitId_text}'${unitId}'`; | ||
| } else { | ||
| unitId_text = `${unitId_text},'${unitId}'`; | ||
| } | ||
| } | ||
| const loaded = []; | ||
| for (let i = 0; i < userIdArray.length; i++) { | ||
| let userId = userIdArray[i]; | ||
| const result = await this.dataSource.query( | ||
| `WITH latest_assessment AS ( | ||
| SELECT | ||
| "assessmentTrackingId", | ||
| "userId", | ||
| "courseId", | ||
| "contentId", | ||
| "attemptId", | ||
| "createdOn", | ||
| "lastAttemptedOn", | ||
| "totalMaxScore", | ||
| "totalScore", | ||
| "updatedOn", | ||
| "timeSpent", | ||
| "unitId", | ||
| "tenantId", | ||
| ROW_NUMBER() OVER (PARTITION BY "userId", "courseId", "unitId", "contentId" ORDER BY CAST("totalScore" AS INTEGER) DESC) as row_num | ||
| FROM | ||
| assessment_tracking | ||
| WHERE | ||
| "evaluatedBy" IS DISTINCT FROM 'AI' AND | ||
| "userId" = $1 | ||
| AND "courseId" IN (${courseId_text}) | ||
| AND "unitId" IN (${unitId_text}) | ||
| AND "contentId" IN (${contentId_text}) | ||
| AND "tenantId" = $2 | ||
| ) | ||
| SELECT |
There was a problem hiding this comment.
🔒 Security & Privacy | 🔴 Critical | ⚡ Quick win
🧩 Analysis chain
🌐 Web query:
node-postgres parameterized array ANY IN clause
💡 Result:
To perform a parameterized query in node-postgres that functions like an SQL IN clause, you should use the PostgreSQL = ANY($1) syntax rather than IN ($1) [1][2][3]. The standard IN operator in SQL expects a comma-separated list of values (e.g., IN (1, 2, 3)), not a single array object. If you pass a JavaScript array as a single parameter to IN ($1), PostgreSQL will treat it as a syntax error or a type mismatch [1][2]. By using = ANY($1), node-postgres correctly maps the JavaScript array to a PostgreSQL array parameter [1][4]. Example usage: const { Client } = require('pg'); const client = new Client; await client.connect; const values = ['larry', 'curly', 'moe']; // Use = ANY($1) instead of IN const query = 'SELECT * FROM stooges WHERE name = ANY($1)'; const res = await client.query(query, [values]); console.log(res.rows); If you need to handle the equivalent of a NOT IN clause, use != ALL($1) instead of NOT IN ($1) [5][6]. For scenarios where you must use the IN operator (e.g., if you are working with older database drivers or specific complex types that require manual query building), you can dynamically generate a string of placeholders [1][3]: const values = ['larry', 'curly', 'moe']; // Generate string like: $1, $2, $3 const placeholders = values.map((_, i) => $${i + 1}).join(','); const query = SELECT * FROM stooges WHERE name IN (${placeholders}); const res = await client.query(query, values); However, using = ANY($1) is generally preferred as it is cleaner, handles arbitrary array lengths naturally, and is fully compatible with parameterized queries [1][2][4].
Citations:
- 1: https://github.com/brianc/node-postgres/wiki/FAQ
- 2: Can no longer pass an array of data to use inside
column IN ($1)parameterized query brianc/node-postgres#1653 - 3: https://stackoverflow.com/questions/53280333/javascript-postgres-db-how-to-use-a-prepared-statement-with-an-array-as-para
- 4: https://stackoverflow.com/questions/10720420/node-postgres-how-to-execute-where-col-in-dynamic-value-list-query
- 5: parameterized query with an IN operator brianc/node-postgres#1452
- 6: [Help] How to use the IN operator with prepared statements? brianc/node-postgres#2853
🏁 Script executed:
#!/bin/bash
set -euo pipefail
file="src/modules/tracking_assessment/tracking_assessment.service.ts"
if [ -f "$file" ]; then
echo "FILE_EXISTS true"
wc -l "$file"
echo "--- outline relevant ---"
ast-grep outline "$file" --match getAssessmentStatuses --view expanded || true
echo "--- lines 640-770 ---"
sed -n '640,770p' "$file" | nl -ba -v640
echo "--- searchFilter validation/type references ---"
rg -n "searchFilter|courseId|unitId|contentId|isUUID|validation" "$file"
else
echo "FILE_MISSING"
git ls-files | rg 'tracking_assessment.service.ts|tracking_assessment'
fiRepository: tekdi/tracking-microservice
Length of output: 415
🏁 Script executed:
#!/bin/bash
set -euo pipefail
file="src/modules/tracking_assessment/tracking_assessment.service.ts"
echo "--- lines 640-780 ---"
awk 'NR>=640 && NR<=780 { printf "%d\t%s\n", NR, $0 }' "$file"
echo "--- searchFilter validation/type references ---"
grep -nE "searchFilter|courseId|unitId|contentId|isUUID|validation|class Search" "$file" || true
echo "--- static probe: IN interpolations and query parameter lists around matching query ---"
python3 - <<'PY'
from pathlib import Path
p=Path("src/modules/tracking_assessment/tracking_assessment.service.ts")
text=p.read_text()
for needle in [
'contentId_text', 'courseId_text', 'unitId_text',
'AND "courseId" IN', 'AND "unitId" IN', 'AND "contentId" IN',
'dataSource.query',
]:
print(f"\n--- {needle} ---")
for i,line in enumerate(text.splitlines(),1):
if needle in line:
print(f"{i}: {line}")
PYRepository: tekdi/tracking-microservice
Length of output: 11844
Remove the string-concatenated IN payloads from searchStatusAssessmentTracking.
searchFilter?.courseId, searchFilter?.unitId, and searchFilter?.contentId are read as raw array elements and interpolated into courseId_text, unitId_text, and contentId_text; these strings are then embedded at AND "courseId" IN (${courseId_text}), AND "unitId" IN (${unitId_text}), and AND "contentId" IN (${contentId_text}). A request value containing a single quote can break out of the string literal and execute arbitrary SQL.
Pass the arrays as query parameters and use = ANY($n::text[]), or generate numbered placeholders such as IN ($3, $4, $5) and pass each array element as a separate bound value.
🧰 Tools
🪛 ESLint
[error] 671-671: Replace (searchFilter?.courseId·&&·searchFilter?.unitId·&&·searchFilter?.contentId) with ·(⏎········searchFilter?.courseId·&&⏎········searchFilter?.unitId·&&⏎········searchFilter?.contentId
(prettier/prettier)
[error] 672-672: Insert )·
(prettier/prettier)
[error] 673-673: Insert ··
(prettier/prettier)
[error] 673-673: 'contentIdArray' is never reassigned. Use 'const' instead.
(prefer-const)
[error] 674-674: Insert ··
(prettier/prettier)
[error] 674-674: 'courseIdArray' is never reassigned. Use 'const' instead.
(prefer-const)
[error] 675-675: Replace ······ with ········
(prettier/prettier)
[error] 675-675: 'unitIdArray' is never reassigned. Use 'const' instead.
(prefer-const)
[error] 676-676: Insert ··
(prettier/prettier)
[error] 676-676: 'userIdArray' is never reassigned. Use 'const' instead.
(prefer-const)
[error] 678-678: Insert ··
(prettier/prettier)
[error] 679-679: Insert ··
(prettier/prettier)
[error] 680-680: Insert ··
(prettier/prettier)
[error] 681-681: Replace ·········· with ············
(prettier/prettier)
[error] 682-682: Insert ··
(prettier/prettier)
[error] 683-683: Insert ··
(prettier/prettier)
[error] 684-684: Insert ··
(prettier/prettier)
[error] 685-685: Insert ··
(prettier/prettier)
[error] 686-686: Insert ··
(prettier/prettier)
[error] 687-687: Insert ··
(prettier/prettier)
[error] 688-688: Insert ··
(prettier/prettier)
[error] 689-689: Replace ·········· with ············
(prettier/prettier)
[error] 690-690: Insert ··
(prettier/prettier)
[error] 690-690: 'contentId' is never reassigned. Use 'const' instead.
(prefer-const)
[error] 691-691: Insert ··
(prettier/prettier)
[error] 692-692: Replace ·············· with ················
(prettier/prettier)
[error] 693-693: Insert ··
(prettier/prettier)
[error] 694-694: Insert ··
(prettier/prettier)
[error] 695-695: Insert ··
(prettier/prettier)
[error] 696-696: Insert ··
(prettier/prettier)
[error] 697-697: Insert ··
(prettier/prettier)
[error] 698-698: Insert ··
(prettier/prettier)
[error] 699-699: Insert ··
(prettier/prettier)
[error] 700-700: Replace ············ with ··············
(prettier/prettier)
[error] 700-700: 'courseId' is never reassigned. Use 'const' instead.
(prefer-const)
[error] 701-701: Insert ··
(prettier/prettier)
[error] 702-702: Insert ··
(prettier/prettier)
[error] 703-703: Insert ··
(prettier/prettier)
[error] 704-704: Insert ··
(prettier/prettier)
[error] 705-705: Replace ············ with ··············
(prettier/prettier)
[error] 706-706: Insert ··
(prettier/prettier)
[error] 707-707: Insert ··
(prettier/prettier)
[error] 708-708: Insert ··
(prettier/prettier)
[error] 709-709: Insert ··
(prettier/prettier)
[error] 710-710: Insert ··
(prettier/prettier)
[error] 710-710: 'unitId' is never reassigned. Use 'const' instead.
(prefer-const)
[error] 711-711: Insert ··
(prettier/prettier)
[error] 712-712: Insert ··
(prettier/prettier)
[error] 713-713: Insert ··
(prettier/prettier)
[error] 714-714: Insert ··
(prettier/prettier)
[error] 715-715: Insert ··
(prettier/prettier)
[error] 716-716: Insert ··
(prettier/prettier)
[error] 717-717: Insert ··
(prettier/prettier)
[error] 718-718: Insert ··
(prettier/prettier)
[error] 719-719: Insert ··
(prettier/prettier)
[error] 719-719: 'userId' is never reassigned. Use 'const' instead.
(prefer-const)
[error] 720-720: Insert ··
(prettier/prettier)
[error] 721-721: Insert ··
(prettier/prettier)
🤖 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 `@src/modules/tracking_assessment/tracking_assessment.service.ts` around lines
671 - 747, Update searchStatusAssessmentTracking to remove courseId_text,
unitId_text, and contentId_text string construction; use parameterized array
predicates with = ANY(...::text[]) or individually numbered placeholders, and
pass the raw array values through dataSource.query parameters. Preserve the
existing userId and tenantId bindings while ensuring courseId, unitId, and
contentId values are never interpolated into the SQL.
Source: Linters/SAST tools
| await this.cacheService.invalidate( | ||
| [`content:${tenantId}`, `course:${tenantId}`, 'courseinprogress'], | ||
| 'createContentTracking', | ||
| ); | ||
|
|
There was a problem hiding this comment.
🗄️ Data Integrity & Integration | 🟠 Major | ⚡ Quick win
Invalidate contentread:${contentTrackingId} after create/update.
createContentTracking can mutate an existing row. Line 260 runs UPDATE content_tracking SET "resumeData" for an existing contentTrackingId, and lines 300-301 insert new detail rows. The invalidation list at Line 310 omits the contentread:${contentTrackingId} namespace that getContentTrackingDetails populates at Line 87. The detail read then serves a stale row for up to CONTENT_READ_TTL_SECONDS (300 seconds).
Add the entity namespace to the invalidation list.
🐛 Proposed fix
await this.cacheService.invalidate(
- [`content:${tenantId}`, `course:${tenantId}`, 'courseinprogress'],
+ [
+ `contentread:${contentTrackingId}`,
+ `content:${tenantId}`,
+ `course:${tenantId}`,
+ 'courseinprogress',
+ ],
'createContentTracking',
);📝 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.
| await this.cacheService.invalidate( | |
| [`content:${tenantId}`, `course:${tenantId}`, 'courseinprogress'], | |
| 'createContentTracking', | |
| ); | |
| await this.cacheService.invalidate( | |
| [ | |
| `contentread:${contentTrackingId}`, | |
| `content:${tenantId}`, | |
| `course:${tenantId}`, | |
| 'courseinprogress', | |
| ], | |
| 'createContentTracking', | |
| ); |
🤖 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 `@src/modules/tracking_content/tracking_content.service.ts` around lines 310 -
314, Add the `contentread:${contentTrackingId}` entity namespace to the
invalidation keys used by `createContentTracking` after it updates or creates
tracking data, alongside the existing content and course namespaces, so
`getContentTrackingDetails` does not serve stale cached details.
| const data = await this.cacheService.getOrLoad({ | ||
| namespace: `course:${tenantId}`, | ||
| key: `dashboard:${hashCacheParts(userIdArray, courseIdArray)}`, | ||
| ttlSeconds: COURSE_STATUS_TTL_SECONDS, | ||
| loader: async () => { | ||
| const certificateQuery = ` | ||
| SELECT "userId", "courseId", status, "issuedOn", "createdOn", "updatedOn" | ||
| FROM user_course_certificate | ||
| WHERE "courseId" = ANY($1) AND "userId" = ANY($2::uuid[]) AND "tenantId" = $3 | ||
| `; | ||
|
|
||
| const attemptQuery = ` | ||
| SELECT "userId", "courseId", COUNT("assessmentTrackingId") as attempt_count | ||
| FROM assessment_tracking | ||
| WHERE "courseId" = ANY($1) AND "userId" = ANY($2::uuid[]) AND "tenantId" = $3 | ||
| GROUP BY "userId", "courseId" | ||
| `; | ||
|
|
||
| const [certificateResults, attemptResults] = await Promise.all([ | ||
| this.dataSource.query(certificateQuery, [courseIdArray, userIdArray, tenantId]), | ||
| this.dataSource.query(attemptQuery, [courseIdArray, userIdArray, tenantId]), | ||
| ]); | ||
|
|
||
| const statusMap = new Map< | ||
| string, | ||
| { status: string; issuedOn: Date | null; createdOn: Date | null; updatedOn: Date | null } | ||
| >(); | ||
| for (const row of certificateResults) { | ||
| statusMap.set(`${row.courseId}_${row.userId}`, { | ||
| status: row.status, | ||
| issuedOn: row.issuedOn, | ||
| createdOn: row.createdOn, | ||
| updatedOn: row.updatedOn, | ||
| }); | ||
| } | ||
|
|
||
| const data = courseIdArray.map((courseId) => { | ||
| const userStatusMap: Record< | ||
| string, | ||
| { | ||
| status: string; | ||
| highestAttempt: number; | ||
| issuedOn: Date | null; | ||
| createdOn: Date | null; | ||
| updatedOn: Date | null; | ||
| const attemptMap = new Map<string, number>(); | ||
| for (const row of attemptResults) { | ||
| attemptMap.set(`${row.courseId}_${row.userId}`, parseInt(row.attempt_count) || 0); | ||
| } | ||
| > = {}; | ||
|
|
||
| for (const userId of userIdArray) { | ||
| const key = `${courseId}_${userId}`; | ||
| const certificate = statusMap.get(key); | ||
| const rawStatus = certificate?.status; | ||
| const status = | ||
| !rawStatus || rawStatus.toLowerCase() === 'enrolled' | ||
| ? 'not_started' | ||
| : rawStatus; | ||
|
|
||
| userStatusMap[userId] = { | ||
| status, | ||
| highestAttempt: attemptMap.get(key) || 0, | ||
| issuedOn: certificate?.issuedOn ?? null, | ||
| createdOn: certificate?.createdOn ?? null, | ||
| updatedOn: certificate?.updatedOn ?? null, | ||
| }; | ||
| } | ||
|
|
||
| return { [courseId]: userStatusMap }; | ||
| return courseIdArray.map((courseId) => { | ||
| const userStatusMap: Record< | ||
| string, | ||
| { | ||
| status: string; | ||
| highestAttempt: number; | ||
| issuedOn: Date | null; | ||
| createdOn: Date | null; | ||
| updatedOn: Date | null; | ||
| } | ||
| > = {}; | ||
|
|
||
| for (const userId of userIdArray) { | ||
| const key = `${courseId}_${userId}`; | ||
| const certificate = statusMap.get(key); | ||
| const rawStatus = certificate?.status; | ||
| const status = | ||
| !rawStatus || rawStatus.toLowerCase() === 'enrolled' | ||
| ? 'not_started' | ||
| : rawStatus; | ||
|
|
||
| userStatusMap[userId] = { | ||
| status, | ||
| highestAttempt: attemptMap.get(key) || 0, | ||
| issuedOn: certificate?.issuedOn ?? null, | ||
| createdOn: certificate?.createdOn ?? null, | ||
| updatedOn: certificate?.updatedOn ?? null, | ||
| }; | ||
| } | ||
|
|
||
| return { [courseId]: userStatusMap }; | ||
| }); | ||
| }, |
There was a problem hiding this comment.
🗄️ Data Integrity & Integration | 🟠 Major | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# List every cacheService.invalidate call and its namespaces across modules.
rg -nP -C4 'cacheService\.invalidate\(' srcRepository: tekdi/tracking-microservice
Length of output: 7587
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "Files:"
git ls-files | rg 'src/modules/(tracking_assessment|tracking_content|certificate|user_certificate)/.*(service|module|controller|dto|*.ts)$|src/modules/.*service\.ts$|src/cache' | sed 's/^/ /' | head -200
echo
echo "tracking_assessment invalidate call sites:"
sed -n '300,360p' src/modules/tracking_assessment/tracking_assessment.service.ts
sed -n '430,470p' src/modules/tracking_assessment/tracking_assessment.service.ts
sed -n '1200,1250p' src/modules/tracking_assessment/tracking_assessment.service.ts
echo
echo "tracking_assessment repository/call graph identifiers:"
rg -n "assessment_tracking|createAssessmentTracking|updateAssessmentTracking|deleteAssessmentTracking|invalidate|assessmentTracking" src/modules/tracking_assessment/tracking_assessment.service.ts src/modules/tracking_assessment | head -200
echo
echo "tracking_content course invalidation context:"
sed -n '280,320p' src/modules/tracking_content/tracking_content.service.ts
sed -n '1280,1310p' src/modules/tracking_content/tracking_content.service.tsRepository: tekdi/tracking-microservice
Length of output: 528
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "Assess whether invalidate calls can propagate across cacheService methods:"
rg -n "class .*Cache|invalidat|invalidate\(|new Map|cache.*set|getOrLoad|cacheService" src -g '*.ts' | head -300Repository: tekdi/tracking-microservice
Length of output: 7833
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "Cache service implementation:"
sed -n '1,190p' src/cache/cache.service.ts
echo
echo "Redis cache store:"
sed -n '1,220p' src/cache/redis-cache.store.ts
echo
echo "Assessment write/delete locations and invalidate context:"
grep -n "async createAssessmentTracking\|async updateAssessmentTracking\|async deleteAssessmentTracking\|cacheService\.invalidate\|assessment_tracking\|repository\." src/modules/tracking_assessmetn/tracking_assessment.service.ts 2>/dev/null || true
grep -n "async createAssessmentTracking\|async updateAssessmentTracking\|async deleteAssessmentTracking\|cacheService\.invalidate\|assessment_tracking\|repository\." src/modules/tracking_assessment/tracking_assessment.service.tsRepository: tekdi/tracking-microservice
Length of output: 8660
Invalidate course:${tenantId} on assessment tracking writes.
createAssessmentTracking() and deleteAssessmentTracking() only bump assessment:${tenantId}, but getOrLoad() under course:${tenantId} caches attempt_count from assessment_tracking; dashboard counts can stay stale after create or delete. Add course:${tenantId} to those invalidations and any update path that changes attempted assessments for a course/user.
🧰 Tools
🪛 ESLint
[error] 651-651: Replace courseIdArray,·userIdArray,·tenantId with ⏎················courseIdArray,⏎················userIdArray,⏎················tenantId,⏎··············
(prettier/prettier)
[error] 652-652: Replace courseIdArray,·userIdArray,·tenantId with ⏎················courseIdArray,⏎················userIdArray,⏎················tenantId,⏎··············
(prettier/prettier)
[error] 657-657: Replace ·status:·string;·issuedOn:·Date·|·null;·createdOn:·Date·|·null;·updatedOn:·Date·|·null with ⏎················status:·string;⏎················issuedOn:·Date·|·null;⏎················createdOn:·Date·|·null;⏎················updatedOn:·Date·|·null;⏎·············
(prettier/prettier)
[error] 670-670: Replace ``${row.courseId}${row.userId},·parseInt(row.attempt_count)·||·0 with `⏎················`${row.courseId}${row.userId}`,⏎················parseInt(row.attempt_count)·||·0,⏎··············`
(prettier/prettier)
🤖 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 `@src/modules/tracking_content/tracking_content.service.ts` around lines 632 -
705, The assessment tracking write paths must also invalidate the course
dashboard cache because getOrLoad caches attempt_count under course:${tenantId}.
Update createAssessmentTracking, deleteAssessmentTracking, and any
assessment-tracking update path that changes a course/user’s attempts to
invalidate course:${tenantId} alongside assessment:${tenantId}, reusing the
existing tenant-scoped cache invalidation mechanism.
| this.dataSource.query(certificateQuery, [courseIdArray, userIdArray, tenantId]), | ||
| this.dataSource.query(attemptQuery, [courseIdArray, userIdArray, tenantId]), | ||
| ]); | ||
|
|
||
| const statusMap = new Map< | ||
| string, | ||
| { status: string; issuedOn: Date | null; createdOn: Date | null; updatedOn: Date | null } | ||
| >(); | ||
| for (const row of certificateResults) { | ||
| statusMap.set(`${row.courseId}_${row.userId}`, { | ||
| status: row.status, | ||
| issuedOn: row.issuedOn, | ||
| createdOn: row.createdOn, | ||
| updatedOn: row.updatedOn, | ||
| }); | ||
| } | ||
|
|
||
| const data = courseIdArray.map((courseId) => { | ||
| const userStatusMap: Record< | ||
| string, | ||
| { | ||
| status: string; | ||
| highestAttempt: number; | ||
| issuedOn: Date | null; | ||
| createdOn: Date | null; | ||
| updatedOn: Date | null; | ||
| const attemptMap = new Map<string, number>(); | ||
| for (const row of attemptResults) { | ||
| attemptMap.set(`${row.courseId}_${row.userId}`, parseInt(row.attempt_count) || 0); |
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick win
Fix the lint errors reported on the new dashboard loader.
ESLint reports prettier/prettier errors on lines 651, 652, 657, and 670. These lines are new. Run the formatter to keep the lint job green.
🧰 Tools
🪛 ESLint
[error] 651-651: Replace courseIdArray,·userIdArray,·tenantId with ⏎················courseIdArray,⏎················userIdArray,⏎················tenantId,⏎··············
(prettier/prettier)
[error] 652-652: Replace courseIdArray,·userIdArray,·tenantId with ⏎················courseIdArray,⏎················userIdArray,⏎················tenantId,⏎··············
(prettier/prettier)
[error] 657-657: Replace ·status:·string;·issuedOn:·Date·|·null;·createdOn:·Date·|·null;·updatedOn:·Date·|·null with ⏎················status:·string;⏎················issuedOn:·Date·|·null;⏎················createdOn:·Date·|·null;⏎················updatedOn:·Date·|·null;⏎·············
(prettier/prettier)
[error] 670-670: Replace ``${row.courseId}${row.userId},·parseInt(row.attempt_count)·||·0 with `⏎················`${row.courseId}${row.userId}`,⏎················parseInt(row.attempt_count)·||·0,⏎··············`
(prettier/prettier)
🤖 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 `@src/modules/tracking_content/tracking_content.service.ts` around lines 651 -
670, Run Prettier on the new dashboard loader code around the
certificateResults, statusMap, and attemptMap processing in the tracking
service, correcting the reported formatting violations on the query and map
lines while preserving the existing logic.
Source: Linters/SAST tools
| Object.keys(filters).forEach((key) => { | ||
| const value = filters[key]; | ||
|
|
||
| if (Array.isArray(value) && value.length > 0) { | ||
| // Array filter - use IN clause | ||
| queryBuilder.andWhere( | ||
| `UserCourseCertificate.${key} IN (:...${key})`, | ||
| { | ||
| [key]: value, | ||
| }, | ||
| ); | ||
| } else if (value) { | ||
| // Single value filter - use equality | ||
| queryBuilder.andWhere(`UserCourseCertificate.${key} = :${key}`, { | ||
| [key]: value, | ||
| if (Array.isArray(value) && value.length > 0) { | ||
| // Array filter - use IN clause | ||
| queryBuilder.andWhere( | ||
| `UserCourseCertificate.${key} IN (:...${key})`, | ||
| { | ||
| [key]: value, | ||
| }, | ||
| ); | ||
| } else if (value) { | ||
| // Single value filter - use equality | ||
| queryBuilder.andWhere(`UserCourseCertificate.${key} = :${key}`, { | ||
| [key]: value, | ||
| }); | ||
| } | ||
| }); |
There was a problem hiding this comment.
🔒 Security & Privacy | 🟠 Major | ⚡ Quick win
Restrict filters keys to an allowlist.
Lines 250-260 interpolate key into the SQL fragment and into the parameter name. filters comes from the request body, so a caller controls the fragment text. TypeORM passes the fragment to the query planner after property resolution, so an unexpected key can produce an error or an unintended condition. The cache layer also stores the resulting rows for 120 seconds under a hashed key, which extends the effect of any accepted malformed filter.
Validate key against the allowed column names of UserCourseCertificate before you add the condition.
🔒 Proposed fix
+ const allowedFilterKeys = [
+ 'userId',
+ 'courseId',
+ 'status',
+ 'certificateId',
+ 'usercertificateId',
+ ];
Object.keys(filters).forEach((key) => {
+ if (!allowedFilterKeys.includes(key)) {
+ throw new Error(`Invalid filter key: ${key}`);
+ }
const value = filters[key];📝 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.
| Object.keys(filters).forEach((key) => { | |
| const value = filters[key]; | |
| if (Array.isArray(value) && value.length > 0) { | |
| // Array filter - use IN clause | |
| queryBuilder.andWhere( | |
| `UserCourseCertificate.${key} IN (:...${key})`, | |
| { | |
| [key]: value, | |
| }, | |
| ); | |
| } else if (value) { | |
| // Single value filter - use equality | |
| queryBuilder.andWhere(`UserCourseCertificate.${key} = :${key}`, { | |
| [key]: value, | |
| if (Array.isArray(value) && value.length > 0) { | |
| // Array filter - use IN clause | |
| queryBuilder.andWhere( | |
| `UserCourseCertificate.${key} IN (:...${key})`, | |
| { | |
| [key]: value, | |
| }, | |
| ); | |
| } else if (value) { | |
| // Single value filter - use equality | |
| queryBuilder.andWhere(`UserCourseCertificate.${key} = :${key}`, { | |
| [key]: value, | |
| }); | |
| } | |
| }); | |
| const allowedFilterKeys = [ | |
| 'userId', | |
| 'courseId', | |
| 'status', | |
| 'certificateId', | |
| 'usercertificateId', | |
| ]; | |
| Object.keys(filters).forEach((key) => { | |
| if (!allowedFilterKeys.includes(key)) { | |
| throw new Error(`Invalid filter key: ${key}`); | |
| } | |
| const value = filters[key]; | |
| if (Array.isArray(value) && value.length > 0) { | |
| // Array filter - use IN clause | |
| queryBuilder.andWhere( | |
| `UserCourseCertificate.${key} IN (:...${key})`, | |
| { | |
| [key]: value, | |
| }, | |
| ); | |
| } else if (value) { | |
| // Single value filter - use equality | |
| queryBuilder.andWhere(`UserCourseCertificate.${key} = :${key}`, { | |
| [key]: value, | |
| }); | |
| } | |
| }); |
🤖 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 `@src/modules/user_certificate/user_certificate.service`..ts around lines 245 -
262, Restrict the filter iteration in the UserCourseCertificate query-building
flow to an explicit allowlist of valid UserCourseCertificate column names before
interpolating key into SQL or parameter names. Skip or reject any unapproved
key, while preserving the existing array IN and scalar equality handling for
allowed keys.
| await this.cacheService.invalidate( | ||
| `usercert:${tenantId}`, | ||
| 'importUserDataForCertificate', | ||
| ); | ||
|
|
There was a problem hiding this comment.
🗄️ Data Integrity & Integration | 🟠 Major | ⚡ Quick win
Invalidate the namespace of the tenant that owns the saved record.
Line 318 looks up the record with createUserCertificateDto.tenantId, and line 330 saves the DTO. The invalidation at Line 335 uses request.tenantId. If the two values differ, this call bumps the wrong namespace and fetchUserStatusForCourse and searchUsersCourses keep serving stale data for the record's real tenant.
enrollUserForCourse also invalidates course:${tenantId}. Import writes the same table, so include that namespace for consistency.
🐛 Proposed fix
+ const recordTenantId = result.tenantId ?? tenantId;
await this.cacheService.invalidate(
- `usercert:${tenantId}`,
+ [`usercert:${recordTenantId}`, `course:${recordTenantId}`],
'importUserDataForCertificate',
);📝 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.
| await this.cacheService.invalidate( | |
| `usercert:${tenantId}`, | |
| 'importUserDataForCertificate', | |
| ); | |
| const recordTenantId = result.tenantId ?? tenantId; | |
| await this.cacheService.invalidate( | |
| [`usercert:${recordTenantId}`, `course:${recordTenantId}`], | |
| 'importUserDataForCertificate', | |
| ); |
🤖 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 `@src/modules/user_certificate/user_certificate.service`..ts around lines 334 -
338, Update the cache invalidation in the import flow around the user
certificate record lookup and save to use the saved record’s tenantId (from
createUserCertificateDto) rather than request.tenantId. Also invalidate the
corresponding course:${tenantId} namespace, matching enrollUserForCourse, so
both usercert and course caches are cleared for the record’s owning tenant.
|
![github-advanced-security[bot]](https://avatars.githubusercontent.com/in/57789?s=60&v=4){kind=small}
|
|
||
| export function hashCacheParts(...parts: unknown[]): string { | ||
| const normalized = JSON.stringify(parts); | ||
| return createHash('sha1').update(normalized).digest('hex'); |
2 participants
No description provided.