New Upstash Redis implementation #8350
Conversation
|
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
e2e/cache-control/cache-control.test.tsOops! Something went wrong! :( ESLint: 9.19.0 ESLint couldn't find an eslint.config.(js|mjs|cjs) file. From ESLint v9.0.0, the default configuration file is now eslint.config.js. https://eslint.org/docs/latest/use/configure/migration-guide If you still have problems after following the migration guide, please stop by 📝 WalkthroughWalkthroughThe pull request introduces a new implementation for Upstash Redis caching in the GraphQL Mesh framework. This includes the addition of the Changes
Sequence DiagramsequenceDiagram
participant Client
participant UpstashRedisCache
participant UpstashRedis
Client->>UpstashRedisCache: Initialize with config
Client->>UpstashRedisCache: set(key, value, options)
UpstashRedisCache->>UpstashRedis: Store key-value
Client->>UpstashRedisCache: get(key)
UpstashRedisCache->>UpstashRedis: Retrieve value
UpstashRedisCache-->>Client: Return value
Client->>UpstashRedisCache: delete(key)
UpstashRedisCache->>UpstashRedis: Remove key
Poem
📜 Recent review detailsConfiguration used: CodeRabbit UI 📒 Files selected for processing (1)
🚧 Files skipped from review as they are similar to previous changes (1)
⏰ Context from checks skipped due to timeout of 90000ms (13)
🪧 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 (
|
Apollo Federation Subgraph Compatibility Results
Learn more: |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 3
🧹 Nitpick comments (5)
packages/cache/upstash-redis/src/index.ts (1)
26-34: Optimize TTL conversion.The TTL conversion to milliseconds could be more explicit and safer.
Consider this improvement:
async set<T>(key: string, value: T, options?: { ttl?: number }) { if (options?.ttl) { + const ttlMs = Math.max(1, Math.floor(options.ttl * 1000)); await this.redis.set(key, value, { - px: options.ttl * 1000, + px: ttlMs, }); } else { await this.redis.set(key, value); } }e2e/cache-control/gateway.config.ts (1)
7-14: Add type safety and error handling for cache configuration.The current implementation could be more robust with proper type checking and error handling.
Consider this improvement:
+type CacheType = 'upstash-redis' | 'redis' | 'inmemory-lru'; + +function isValidCacheType(type: string): type is CacheType { + return ['upstash-redis', 'redis', 'inmemory-lru'].includes(type); +} + +const cacheType = process.env.CACHE_STORAGE; +if (!cacheType || !isValidCacheType(cacheType)) { + throw new Error(`Invalid cache type: ${cacheType}`); +} + -if (process.env.CACHE_STORAGE === 'upstash-redis') { +if (cacheType === 'upstash-redis') { config.cache = new UpstashRedisCache(); } else { config.cache = { // @ts-expect-error - We know it - type: process.env.CACHE_STORAGE, + type: cacheType, }; }e2e/cache-control/cache-control.test.ts (2)
102-102: Enhance container health check.The current health check only verifies HTTP availability but not Redis functionality through the HTTP interface.
Consider this improvement:
- healthcheck: ['CMD', 'curl', '-f', 'http://localhost'], + healthcheck: ['CMD', 'curl', '-f', 'http://localhost/health' '-H', 'Authorization: Bearer example_token'],
110-113: Add error handling for container disposal.The disposal of containers should handle potential errors gracefully.
Consider this improvement:
async [DisposableSymbols.asyncDispose]() { - await redis[DisposableSymbols.asyncDispose](); - await srh[DisposableSymbols.asyncDispose](); + try { + await Promise.all([ + redis[DisposableSymbols.asyncDispose](), + srh[DisposableSymbols.asyncDispose](), + ]); + } catch (error) { + console.error('Error disposing containers:', error); + throw error; + } }packages/cache/upstash-redis/package.json (1)
34-36: Consider constraining the graphql peer dependency version range.Using
"*"for the graphql peer dependency might lead to compatibility issues. Consider specifying a minimum version or version range that has been tested with this package.- "graphql": "*" + "graphql": "^16.0.0"
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (1)
yarn.lockis excluded by!**/yarn.lock,!**/*.lock
📒 Files selected for processing (6)
.changeset/sixty-rivers-shave.md(1 hunks)e2e/cache-control/cache-control.test.ts(1 hunks)e2e/cache-control/gateway.config.ts(1 hunks)e2e/cache-control/package.json(1 hunks)packages/cache/upstash-redis/package.json(1 hunks)packages/cache/upstash-redis/src/index.ts(1 hunks)
✅ Files skipped from review due to trivial changes (1)
- .changeset/sixty-rivers-shave.md
⏰ Context from checks skipped due to timeout of 90000ms (13)
- GitHub Check: e2e / node v22
- GitHub Check: unit / node 22
- GitHub Check: e2e / node v20
- GitHub Check: integration / node 22
- GitHub Check: unit / node 20
- GitHub Check: integration / node 20
- GitHub Check: release / snapshot
- GitHub Check: unit / node 18
- GitHub Check: integration / node 18
- GitHub Check: e2e / node v18
- GitHub Check: check
- GitHub Check: deployment
- GitHub Check: apollo-federation-compatibility
🔇 Additional comments (5)
e2e/cache-control/package.json (1)
8-8: Update version specification.The version
^0.0.0is not a proper semantic version and might cause issues with dependency resolution.Please verify and update to a proper version number following semantic versioning.
packages/cache/upstash-redis/package.json (4)
14-32: LGTM! Excellent dual CJS/ESM support configuration.The exports field is well-structured with proper type definitions for both CommonJS and ES modules.
44-48: LGTM! Publishing configuration is well-structured.The publishing configuration correctly specifies public access and the dist directory, with sideEffects properly set to false for better tree-shaking.
49-51: LGTM! TypeScript configuration is consistent.The TypeScript definition path matches the exports configuration.
37-43: Verify @upstash/redis version compatibility and security.Let's check if version 1.34.3 of @upstash/redis is the latest stable version and if it has any known security vulnerabilities.
✅ Verification successful
@upstash/redis version 1.34.3 is verified as secure and up-to-date
The package is using the latest stable version, is actively maintained by the Upstash team, and has no known security vulnerabilities. While version 1.35.0-canary exists, it's a pre-release version not recommended for production use.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Check @upstash/redis version and security advisories # Check NPM for latest version echo "Latest version from NPM:" npm view @upstash/redis version # Check for security advisories echo -e "\nSecurity advisories:" npm audit @upstash/[email protected]Length of output: 539
Script:
#!/bin/bash # Get detailed package information echo "Package details:" npm view @upstash/redis # Check GitHub security advisories echo -e "\nGitHub security advisories:" gh api \ -H "Accept: application/vnd.github+json" \ -H "X-GitHub-Api-Version: 2022-11-28" \ /repos/upstash/upstash-redis/security-advisoriesLength of output: 1184
💻 Website PreviewThe latest changes are available as preview in: https://a2287e78.graphql-mesh.pages.dev |
There was a problem hiding this comment.
Actionable comments posted: 2
♻️ Duplicate comments (1)
packages/cache/upstash-redis/src/index.ts (1)
10-20:⚠️ Potential issueAdd environment variable validation.
The constructor should validate the required environment variables to prevent runtime errors.
Add validation as follows:
constructor(config?: Partial<RedisConfigNodejs>) { this.abortCtrl = new AbortController(); + const url = process.env.UPSTASH_REDIS_REST_URL; + const token = process.env.UPSTASH_REDIS_REST_TOKEN; + if (!url || !token) { + throw new Error('UPSTASH_REDIS_REST_URL and UPSTASH_REDIS_REST_TOKEN must be set'); + } this.redis = new Redis({ - url: process.env.UPSTASH_REDIS_REST_URL, - token: process.env.UPSTASH_REDIS_REST_TOKEN, + url, + token, enableAutoPipelining: true, signal: this.abortCtrl.signal, latencyLogging: !!process.env.DEBUG, ...config, }); }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
packages/cache/upstash-redis/src/index.ts(1 hunks)
🧰 Additional context used
🪛 GitHub Check: e2e / node v22
packages/cache/upstash-redis/src/index.ts
[failure] 49-49:
Type 'string' is not assignable to type 'number'.
🪛 GitHub Check: e2e / node v20
packages/cache/upstash-redis/src/index.ts
[failure] 49-49:
Type 'string' is not assignable to type 'number'.
🪛 GitHub Check: e2e / node v18
packages/cache/upstash-redis/src/index.ts
[failure] 49-49:
Type 'string' is not assignable to type 'number'.
🪛 GitHub Check: apollo-federation-compatibility
packages/cache/upstash-redis/src/index.ts
[failure] 49-49:
Type 'string' is not assignable to type 'number'.
🪛 GitHub Check: integration / node 22
packages/cache/upstash-redis/src/index.ts
[failure] 49-49:
Type 'string' is not assignable to type 'number'.
🪛 GitHub Check: integration / node 20
packages/cache/upstash-redis/src/index.ts
[failure] 49-49:
Type 'string' is not assignable to type 'number'.
🪛 GitHub Check: integration / node 18
packages/cache/upstash-redis/src/index.ts
[failure] 49-49:
Type 'string' is not assignable to type 'number'.
🪛 GitHub Check: release / snapshot
packages/cache/upstash-redis/src/index.ts
[failure] 49-49:
Type 'string' is not assignable to type 'number'.
🪛 GitHub Check: deployment
packages/cache/upstash-redis/src/index.ts
[failure] 49-49:
Type 'string' is not assignable to type 'number'.
🪛 GitHub Actions: website
packages/cache/upstash-redis/src/index.ts
[error] 49-49: Type 'string' is not assignable to type 'number'
🪛 GitHub Actions: pr
packages/cache/upstash-redis/src/index.ts
[error] 49-49: Type 'string' is not assignable to type 'number'
⏰ Context from checks skipped due to timeout of 90000ms (4)
- GitHub Check: unit / node 22
- GitHub Check: unit / node 20
- GitHub Check: unit / node 18
- GitHub Check: check
🔇 Additional comments (4)
packages/cache/upstash-redis/src/index.ts (4)
1-9: LGTM!The imports and class declaration are well-structured and follow best practices.
22-24: LGTM!The get method is correctly implemented with proper type safety.
36-39: LGTM!The delete method is correctly implemented, properly handling the Redis response.
55-57: LGTM!The dispose method correctly implements the disposal pattern.
Co-authored-by: coderabbitai[bot] <136622811+coderabbitai[bot]@users.noreply.github.com>
Co-authored-by: coderabbitai[bot] <136622811+coderabbitai[bot]@users.noreply.github.com>
ardatan
force-pushed
the
upstash-redis
branch
from
0679797 to
0194643
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (1)
yarn.lockis excluded by!**/yarn.lock,!**/*.lock
📒 Files selected for processing (6)
.changeset/sixty-rivers-shave.md(1 hunks)e2e/cache-control/cache-control.test.ts(1 hunks)e2e/cache-control/gateway.config.ts(1 hunks)e2e/cache-control/package.json(1 hunks)packages/cache/upstash-redis/package.json(1 hunks)packages/cache/upstash-redis/src/index.ts(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (5)
- e2e/cache-control/package.json
- .changeset/sixty-rivers-shave.md
- e2e/cache-control/cache-control.test.ts
- packages/cache/upstash-redis/package.json
- packages/cache/upstash-redis/src/index.ts
⏰ Context from checks skipped due to timeout of 90000ms (13)
- GitHub Check: unit / node 22
- GitHub Check: unit / node 20
- GitHub Check: e2e / node v22
- GitHub Check: unit / node 18
- GitHub Check: e2e / node v20
- GitHub Check: e2e / node v18
- GitHub Check: apollo-federation-compatibility
- GitHub Check: check
- GitHub Check: integration / node 22
- GitHub Check: integration / node 20
- GitHub Check: integration / node 18
- GitHub Check: deployment
- GitHub Check: release / snapshot
🔇 Additional comments (3)
e2e/cache-control/gateway.config.ts (3)
2-2: LGTM! Clean import and configuration initialization.The import of UpstashRedisCache and empty configuration initialization look good.
Also applies to: 5-5
Line range hint
16-29: LGTM! Clear plugin configuration with proper error handling.The logging and plugin configuration logic is well-structured with appropriate error handling for unknown plugin types.
8-8: 🛠️ Refactor suggestionAdd configuration options and error handling for Redis connection.
The UpstashRedisCache is initialized without configuration options or error handling.
Please verify if UpstashRedisCache requires configuration options:
Consider adding configuration and error handling:
-config.cache = new UpstashRedisCache(); +try { + config.cache = new UpstashRedisCache({ + // Add necessary configuration options based on environment variables + url: process.env.UPSTASH_REDIS_URL, + token: process.env.UPSTASH_REDIS_TOKEN, + }); +} catch (error) { + console.error('Failed to initialize Upstash Redis cache:', error); + throw error; +}
| if (process.env.CACHE_STORAGE === 'upstash-redis') { | ||
| config.cache = new UpstashRedisCache(); | ||
| } else { | ||
| config.cache = { | ||
| // @ts-expect-error - We know it | ||
| type: process.env.CACHE_STORAGE, | ||
| }; | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Add type safety and environment variable validation.
The current implementation has several areas for improvement:
- Using @ts-expect-error is a code smell that indicates potential type safety issues
- Missing validation for CACHE_STORAGE environment variable
Consider this safer implementation:
-if (process.env.CACHE_STORAGE === 'upstash-redis') {
+const VALID_CACHE_TYPES = ['upstash-redis', 'memory', 'redis'] as const;
+type CacheType = typeof VALID_CACHE_TYPES[number];
+
+const cacheStorage = process.env.CACHE_STORAGE as string;
+
+if (!VALID_CACHE_TYPES.includes(cacheStorage as CacheType)) {
+ throw new Error(`Invalid cache storage type: ${cacheStorage}. Valid types are: ${VALID_CACHE_TYPES.join(', ')}`);
+}
+
+if (cacheStorage === 'upstash-redis') {
config.cache = new UpstashRedisCache();
} else {
config.cache = {
- // @ts-expect-error - We know it
- type: process.env.CACHE_STORAGE,
+ type: cacheStorage as CacheType,
};
}📝 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.
| if (process.env.CACHE_STORAGE === 'upstash-redis') { | |
| config.cache = new UpstashRedisCache(); | |
| } else { | |
| config.cache = { | |
| // @ts-expect-error - We know it | |
| type: process.env.CACHE_STORAGE, | |
| }; | |
| } | |
| const VALID_CACHE_TYPES = ['upstash-redis', 'memory', 'redis'] as const; | |
| type CacheType = typeof VALID_CACHE_TYPES[number]; | |
| const cacheStorage = process.env.CACHE_STORAGE as string; | |
| if (!VALID_CACHE_TYPES.includes(cacheStorage as CacheType)) { | |
| throw new Error(`Invalid cache storage type: ${cacheStorage}. Valid types are: ${VALID_CACHE_TYPES.join(', ')}`); | |
| } | |
| if (cacheStorage === 'upstash-redis') { | |
| config.cache = new UpstashRedisCache(); | |
| } else { | |
| config.cache = { | |
| type: cacheStorage as CacheType, | |
| }; | |
| } |
Closes #8350
Summary by CodeRabbit
New Features
Dependencies
@graphql-mesh/cache-upstash-redispackage.Improvements