New Upstash Redis implementation #8350
Conversation
📝 WalkthroughWalkthroughThe pull request introduces a new implementation for Upstash Redis caching in the GraphQL Mesh framework. This includes the addition of the Changes
Possibly related PRs
Suggested reviewers
Poem
📜 Recent review detailsConfiguration used: CodeRabbit UI ⛔ Files ignored due to path filters (1)
📒 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 (14)
🪧 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://ed8d36a1.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.
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; +}
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
3768101 to
7cfcce7
Compare
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (1)
e2e/cache-control/gateway.config.ts (1)
7-14:⚠️ Potential issueAdd type safety and environment variable validation.
The current implementation has several areas for improvement:
- Using type assertion is unsafe and can lead to runtime errors
- Missing validation for CACHE_STORAGE environment variable
Consider this safer implementation:
+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 (process.env.CACHE_STORAGE === 'upstash-redis') { +if (cacheStorage === 'upstash-redis') { config.cache = new UpstashRedisCache(); } else { config.cache = { - type: process.env.CACHE_STORAGE as 'redis', + type: cacheStorage as CacheType, }; }
🧹 Nitpick comments (2)
e2e/cache-control/gateway.config.ts (2)
1-5: Add environment variable validation.Consider validating required environment variables early to fail fast and provide clear error messages.
import { defineConfig } from '@graphql-hive/gateway'; import UpstashRedisCache from '@graphql-mesh/cache-upstash-redis'; import useHTTPCache from '@graphql-mesh/plugin-http-cache'; +if (!process.env.CACHE_STORAGE) { + throw new Error('CACHE_STORAGE environment variable is required'); +} +if (!process.env.CACHE_PLUGIN) { + throw new Error('CACHE_PLUGIN environment variable is required'); +} + const config: ReturnType<typeof defineConfig> = {};
15-26: Replace console.log with proper logging.Using console.log for configuration information is not recommended in production code. Consider using a proper logging library that supports different log levels and structured logging.
+import { logger } from './logger'; // Add proper logging setup + -console.log(`Using cache storage: ${process.env.CACHE_STORAGE}`); +logger.info('Cache configuration', { storage: process.env.CACHE_STORAGE }); -console.log(`Using cache plugin: ${process.env.CACHE_PLUGIN}`); +logger.info('Plugin configuration', { plugin: process.env.CACHE_PLUGIN }); if (process.env.CACHE_PLUGIN === 'HTTP Caching') { config.plugins = ctx => [useHTTPCache(ctx)]; } else if (process.env.CACHE_PLUGIN === 'Response Caching') { config.responseCaching = { session: () => null, }; } else { - throw new Error(`Unknown caching plugin: ${process.env.CACHE_PLUGIN}`); + logger.error('Invalid plugin configuration', { plugin: process.env.CACHE_PLUGIN }); + throw new Error(`Unknown caching plugin: ${process.env.CACHE_PLUGIN}`); }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
e2e/cache-control/gateway.config.ts(1 hunks)
⏰ Context from checks skipped due to timeout of 90000ms (14)
- GitHub Check: e2e / node v22
- GitHub Check: e2e / node v20
- GitHub Check: e2e / node v18
- GitHub Check: unit / node 22
- GitHub Check: integration / node 22
- GitHub Check: unit / node 20
- GitHub Check: apollo-federation-compatibility
- GitHub Check: integration / node 20
- GitHub Check: unit / node 18
- GitHub Check: release / snapshot
- GitHub Check: integration / node 18
- GitHub Check: deployment
- GitHub Check: check
- GitHub Check: Analyze (javascript-typescript)
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (2)
e2e/cache-control/gateway.config.ts (2)
7-29: Improve type safety for cache storage configuration.While the validation logic is good, we can further improve type safety:
-const VALID_CACHE_STORAGES = ['upstash-redis', 'inmemory-lru', 'redis']; +const VALID_CACHE_STORAGES = ['upstash-redis', 'inmemory-lru', 'redis'] as const; +type CacheStorageType = typeof VALID_CACHE_STORAGES[number]; -const cacheStorage = process.env.CACHE_STORAGE; +const cacheStorage = process.env.CACHE_STORAGE as string; if (cacheStorage === 'upstash-redis') { config.cache = new UpstashRedisCache(); } else { config.cache = { - type: cacheStorage as 'redis', + type: cacheStorage as CacheStorageType, }; }
30-49: Improve type safety for cache plugin configuration.Similar to the cache storage configuration, we can improve type safety:
-const VALID_CACHE_PLUGINS = ['HTTP Caching', 'Response Caching']; +const VALID_CACHE_PLUGINS = ['HTTP Caching', 'Response Caching'] as const; +type CachePluginType = typeof VALID_CACHE_PLUGINS[number]; -const cachePlugin = process.env.CACHE_PLUGIN; +const cachePlugin = process.env.CACHE_PLUGIN as string;
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (1)
.yarn/patches/jest-message-util-npm-29.7.0-7f88b6e8d1.patchis excluded by!**/.yarn/**
📒 Files selected for processing (3)
e2e/cache-control/cache-control.test.ts(1 hunks)e2e/cache-control/gateway.config.ts(1 hunks)package.json(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- e2e/cache-control/cache-control.test.ts
⏰ Context from checks skipped due to timeout of 90000ms (1)
- GitHub Check: Analyze (javascript-typescript)
🔇 Additional comments (4)
e2e/cache-control/gateway.config.ts (3)
1-6: LGTM!Clean initialization with proper imports and type safety using ReturnType.
46-48: Verify the session callback implementation.The session callback always returns null. Please verify if this is the intended behavior or if it should be implementing some session-based logic.
52-53: LGTM!The config export is properly typed using defineConfig.
package.json (1)
124-126: LGTM!The resolutions ensure consistent behavior across different versions of jest-message-util by applying the same patch.
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
e2e/utils/tenv.ts (1)
822-827: Add TypeScript type annotations for better type safety.Consider adding explicit type annotations to improve type safety and code documentation.
-export function applyMaskServicePorts(result: string, subgraphs: Service[]) { +export function applyMaskServicePorts(result: string, subgraphs: Service[]): string {
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (1)
e2e/cache-control/__snapshots__/cache-control.test.ts.snapis excluded by!**/*.snap
📒 Files selected for processing (1)
e2e/utils/tenv.ts(2 hunks)
⏰ Context from checks skipped due to timeout of 90000ms (1)
- GitHub Check: Analyze (javascript-typescript)
🔇 Additional comments (1)
e2e/utils/tenv.ts (1)
349-350: LGTM! Good refactoring of port masking logic.The extraction of port masking logic into a separate function improves code organization and maintainability.
Related graphql-hive/gateway#576
Summary by CodeRabbit
Summary by CodeRabbit
New Features
Dependencies
@graphql-mesh/cache-upstash-redispackage.Improvements