Backend redis fixes #1489
Backend redis fixes #1489
Conversation
There was a problem hiding this comment.
Pull request overview
This PR implements fixes and improvements for Redis backend functionality, introducing support for standalone Redis keys (keys without colons) as a new collection type and improving error handling for disabled Redis KEYS commands.
Key Changes:
- Added STANDALONE_COLLECTION table type to handle Redis keys without prefixes
- Implemented fallback from KEYS to SCAN command when KEYS is disabled
- Removed maxTokens configuration from Amazon Bedrock AI provider
Reviewed changes
Copilot reviewed 2 out of 3 changed files in this pull request and generated 9 comments.
| File | Description |
|---|---|
| shared-code/src/data-access-layer/data-access-objects/data-access-object-redis.ts | Major refactoring adding STANDALONE_COLLECTION support, new getKeysWithPattern helper method for KEYS command fallback, and whitespace normalization (spaces to tabs) |
| backend/src/interceptors/timeout.interceptor.ts | Whitespace normalization only (spaces to tabs) |
| backend/src/entities/ai/amazon-bedrock/amazon-bedrock.ai.provider.ts | Removed maxTokens configuration and reordered imports |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| const tnlCachedObj = { | ||
| server: server, | ||
| client: client, | ||
| redis: redisClient, | ||
| }; | ||
| LRUStorage.setTunnelCache(connectionCopy, tnlCachedObj); | ||
| resolve(tnlCachedObj.redis); |
There was a problem hiding this comment.
The variable name 'tnlCachedObj' uses an unclear abbreviation. Consider renaming to 'tunnelCachedObj' or 'cachedTunnel' for better readability and maintainability.
| const tnlCachedObj = { | |
| server: server, | |
| client: client, | |
| redis: redisClient, | |
| }; | |
| LRUStorage.setTunnelCache(connectionCopy, tnlCachedObj); | |
| resolve(tnlCachedObj.redis); | |
| const tunnelCachedObj = { | |
| server: server, | |
| client: client, | |
| redis: redisClient, | |
| }; | |
| LRUStorage.setTunnelCache(connectionCopy, tunnelCachedObj); | |
| resolve(tunnelCachedObj.redis); |
| const cachedTnl = LRUStorage.getTunnelCache(connectionCopy); | ||
| if (cachedTnl?.redis && cachedTnl.server && cachedTnl.client) { | ||
| resolve(cachedTnl.redis); |
There was a problem hiding this comment.
The variable name 'cachedTnl' uses an unclear abbreviation. Consider renaming to 'cachedTunnel' for better readability.
| const cachedTnl = LRUStorage.getTunnelCache(connectionCopy); | |
| if (cachedTnl?.redis && cachedTnl.server && cachedTnl.client) { | |
| resolve(cachedTnl.redis); | |
| const cachedTunnel = LRUStorage.getTunnelCache(connectionCopy); | |
| if (cachedTunnel?.redis && cachedTunnel.server && cachedTunnel.client) { | |
| resolve(cachedTunnel.redis); |
| try { | ||
| let value: unknown; | ||
| switch (keyType) { | ||
| case 'string': | ||
| value = await redisClient.get(key); | ||
| break; | ||
| case 'list': | ||
| value = `[list: ${await redisClient.lLen(key)} items]`; | ||
| break; | ||
| case 'set': | ||
| value = `[set: ${await redisClient.sCard(key)} members]`; | ||
| break; | ||
| case 'zset': | ||
| value = `[zset: ${await redisClient.zCard(key)} members]`; | ||
| break; | ||
| case 'hash': | ||
| value = `[hash: ${await redisClient.hLen(key)} fields]`; | ||
| break; | ||
| default: | ||
| value = `[${keyType}]`; | ||
| } | ||
|
|
||
| allRows.push({ | ||
| key: key, | ||
| type: keyType, | ||
| value: value, | ||
| }); | ||
| } catch (_error) {} |
There was a problem hiding this comment.
The getStandaloneCollectionRows method processes all standalone keys in batches of 1000, which is good. However, for each key in the batch, it makes individual Redis calls (redisClient.get, redisClient.lLen, etc.) inside the loop after the pipeline execution. This could be optimized by using pipelines for these operations as well, especially when dealing with large numbers of keys. Consider batching these operations to reduce the number of round trips to Redis.
|
|
||
| const command = new ConverseCommand({ | ||
| modelId: this.modelId, | ||
| messages: conversation, |
There was a problem hiding this comment.
Removing the maxTokens configuration from inferenceConfig means the model will use its default token limit. This could lead to unexpectedly long responses or increased costs. If this change is intentional, consider documenting why the limit was removed. If not, the maxTokens parameter should be restored to control response length and costs.
| messages: conversation, | |
| messages: conversation, | |
| inferenceConfig: { | |
| maxTokens: 1024, // Limit response length to control cost and latency | |
| }, |
| if (settings.ordering_field && settings.ordering) { | ||
| allRows.sort((a, b) => { | ||
| const aVal = a[settings.ordering_field]; | ||
| const bVal = b[settings.ordering_field]; | ||
| const modifier = settings.ordering === 'ASC' ? 1 : -1; | ||
|
|
||
| if (aVal < bVal) return -1 * modifier; | ||
| if (aVal > bVal) return 1 * modifier; | ||
| return 0; | ||
| }); |
There was a problem hiding this comment.
The sorting logic doesn't handle null or undefined values properly. When comparing values with the less than or greater than operators, null or undefined values could cause unexpected sorting behavior. Consider adding explicit null/undefined checks in the comparison function to handle these edge cases consistently.
| } | ||
|
|
||
| const totalRows = allRows.length; | ||
| console.log('🚀 ~ DataAccessObjectRedis ~ getStandaloneCollectionRows ~ allRows:', allRows); |
There was a problem hiding this comment.
Debug console.log statement should be removed before merging to production. This line outputs potentially large datasets to the console which can impact performance and clutter logs.
| console.log('🚀 ~ DataAccessObjectRedis ~ getStandaloneCollectionRows ~ allRows:', allRows); |
| if (error?.message?.includes('KEYS command is disabled') || error?.message?.includes('please use SCAN')) { | ||
| return this.getAllKeysWithScan(redisClient, pattern); |
There was a problem hiding this comment.
The getKeysWithPattern method includes error handling that catches disabled KEYS commands and falls back to SCAN, which is good. However, the error detection logic uses optional chaining on error?.message which may not catch all error types. Consider checking if error is an Error instance first, or using a more robust error type check.
|
|
||
| return result; | ||
| } catch (error) { | ||
| if (error?.message?.includes('KEYS command is disabled') || error?.message?.includes('please use SCAN')) { |
There was a problem hiding this comment.
Similar error handling issue as in getKeysWithPattern. The optional chaining on error?.message may not work correctly for all error types. Consider verifying that error is an Error instance before accessing its message property.
| result = Number(rowValue) <= Number(value); | ||
| break; | ||
| case FilterCriteriaEnum.empty: | ||
| result = rowValue === null || rowValue === undefined || rowValue === ''; |
There was a problem hiding this comment.
This guard always evaluates to false.
| result = rowValue === null || rowValue === undefined || rowValue === ''; | |
| result = rowValue === null || rowValue === ''; |
No description provided.