Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Upgrade redis to version 5 and update expiration strategy to 'EXAT' in cache handler documentation and logic #825

Draft
wants to merge 1 commit into
base: canary
Choose a base branch
from

Conversation

better-salmon
Copy link
Contributor

@better-salmon better-salmon commented Oct 21, 2024

Summary by CodeRabbit

  • New Features

    • Introduced a new delete method in the custom Redis handler for cache entry deletion.
  • Bug Fixes

    • Corrected the default value for keyExpirationStrategy in multiple documentation files and implementations from EXPIREAT to EXAT.
  • Documentation

    • Updated documentation to clarify the default values and requirements for keyExpirationStrategy.
  • Chores

    • Updated dependency version for redis in multiple package.json files.
    • Removed the getTimeoutRedisCommandOptions function to streamline command options handling.

Copy link

changeset-bot bot commented Oct 21, 2024

⚠️ No Changeset found

Latest commit: 923f4eb

Merging this PR will not cause a version bump for any packages. If these changes should not result in a new version, you're good to go. If these changes should result in a version bump, you need to add a changeset.

This PR includes no changesets

When changesets are added to this PR, you'll see the packages that this PR includes changesets for and the associated semver types

Click here to learn what changesets are, and how to add one.

Click here if you're a maintainer who wants to add a changeset to this PR

Copy link

coderabbitai bot commented Oct 21, 2024

Walkthrough

The changes involve updates to several files related to the @repo/cache-testing and @neshca/cache-handler projects. Key modifications include updating the redis dependency version in package.json files, correcting documentation regarding the keyExpirationStrategy parameter, and enhancing the custom Redis handler implementation by simplifying timeout management and adding a new delete method. Additionally, the handling of Redis commands has been standardized, and some functions have been removed to streamline the codebase.

Changes

File Change Summary
apps/cache-testing/package.json Updated redis dependency version from 4.7.0 to 5.0.0-next.5.
docs/cache-handler-docs/src/pages/handlers/experimental-redis-cluster.mdx Corrected keyExpirationStrategy default value from EXPRIREAT to EXAT.
`docs/cache-handler-docs/src/pages/handlers/redis-strings.mdx Updated keyExpirationStrategy default value from EXPIREAT to EXAT and added Redis version requirements.
docs/cache-handler-docs/src/pages/usage/creating-a-custom-handler.mdx Simplified Redis client operations and added delete method for cache entries.
packages/cache-handler/package.json Updated version to 1.8.1, changed redis peer dependency from >= 4.6 to 5.0.0-next.5, added optional dependency meta.
packages/cache-handler/src/common-types.ts Updated keyExpirationStrategy default value from 'EXPIREAT' to 'EXAT'.
packages/cache-handler/src/handlers/experimental-redis-cluster.ts Changed keyExpirationStrategy default value to 'EXAT', updated command options handling.
packages/cache-handler/src/handlers/redis-stack.ts Updated error handling and command options to use abort signals.
packages/cache-handler/src/handlers/redis-strings.ts Updated keyExpirationStrategy to 'EXAT', modified command handling to use abort signals.
packages/cache-handler/src/helpers/get-timeout-redis-command-options.ts Removed getTimeoutRedisCommandOptions function.
packages/cache-handler/src/helpers/helpers.ts Removed export of getTimeoutRedisCommandOptions function.

Poem

🐇 In the garden where changes bloom,
Redis whispers, dispelling gloom.
With EXAT set, our keys will thrive,
A new delete method keeps it alive!
No more timeout fuss, just swift delight,
Hopping forward, our code shines bright! 🌼


Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on your favorite social media?

❤️ Share
🪧 Tips

Chat

There are 3 ways to chat with CodeRabbit:

  • Review comments: Directly reply to a review comment made by CodeRabbit. Example:
    • I pushed a fix in commit <commit_id>, please review it.
    • Generate unit testing code for this file.
    • Open a follow-up GitHub issue for this discussion.
  • Files and specific lines of code (under the "Files changed" tab): Tag @coderabbitai in a new review comment at the desired location with your query. Examples:
    • @coderabbitai generate unit testing code for this file.
    • @coderabbitai modularize this function.
  • PR comments: Tag @coderabbitai in a new PR comment to ask questions about the PR branch. For the best results, please provide a very specific query, as very limited context is provided in this mode. Examples:
    • @coderabbitai gather interesting stats about this repository and render them as a table. Additionally, render a pie chart showing the language distribution in the codebase.
    • @coderabbitai read src/utils.ts and generate unit testing code.
    • @coderabbitai read the files in the src/scheduler package and generate a class diagram using mermaid and a README in the markdown format.
    • @coderabbitai help me debug CodeRabbit configuration file.

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)

  • @coderabbitai pause to pause the reviews on a PR.
  • @coderabbitai resume to resume the paused reviews.
  • @coderabbitai review to trigger an incremental review. This is useful when automatic reviews are disabled for the repository.
  • @coderabbitai full review to do a full review from scratch and review all the files again.
  • @coderabbitai summary to regenerate the summary of the PR.
  • @coderabbitai resolve resolve all the CodeRabbit review comments.
  • @coderabbitai configuration to show the current CodeRabbit configuration for the repository.
  • @coderabbitai help to get help.

Other keywords and placeholders

  • Add @coderabbitai ignore anywhere in the PR description to prevent this PR from being reviewed.
  • Add @coderabbitai summary to generate the high-level summary at a specific location in the PR description.
  • Add @coderabbitai anywhere in the PR title to generate the title automatically.

CodeRabbit Configuration File (.coderabbit.yaml)

  • You can programmatically configure CodeRabbit by adding a .coderabbit.yaml file to the root of your repository.
  • Please see the configuration documentation for more information.
  • If your editor has YAML language server enabled, you can add the path at the top of this file to enable auto-completion and validation: # yaml-language-server: $schema=https://coderabbit.ai/integrations/schema.v2.json

Documentation and Community

  • Visit our Documentation for detailed information on how to use CodeRabbit.
  • Join our Discord Community to get help, request features, and share feedback.
  • Follow us on X/Twitter for updates and announcements.

Copy link

@coderabbitai coderabbitai bot left a comment

Choose a reason for hiding this comment

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

Actionable comments posted: 5

🧹 Outside diff range comments (4)
docs/cache-handler-docs/src/pages/handlers/experimental-redis-cluster.mdx (1)

Line range hint 1-100: Consider adding information about Redis version upgrade.

While the document correctly updates the default expiration strategy to 'EXAT', it doesn't mention the upgrade to Redis version 5, which is part of the PR objectives. Consider adding a note about the minimum Redis version required for this handler, especially if it's related to the version 5 upgrade.

docs/cache-handler-docs/src/pages/usage/creating-a-custom-handler.mdx (1)

Line range hint 162-226: LGTM: Enhanced revalidateTag method and new delete functionality.

The changes in this segment improve the custom Redis handler:

  1. The revalidateTag method now consistently uses withAbortSignal for Redis operations, aligning with the new timeout handling approach.
  2. The cursor for hScan is correctly initialized as a string '0', which is the expected data type for the Redis client.
  3. A new delete method has been added, providing a direct way to delete cache entries through the handler.

These modifications enhance the functionality and consistency of the cache handler.

Consider adding a comment explaining the purpose of the delete method, especially its internal use by the CacheHandler class:

async delete(key) {
  // This method is used internally by the CacheHandler class to delete specific cache entries
  // It provides a way to manually invalidate cache items when needed
  await client.withAbortSignal(AbortSignal.timeout(timeoutMs)).unlink(key);
},
packages/cache-handler/src/handlers/redis-strings.ts (2)

Line range hint 57-81: Add error handling for potential AbortError exceptions

When using withAbortSignal(AbortSignal.timeout(timeoutMs)), operations may throw an AbortError if they exceed the timeout. Consider wrapping these calls in try-catch blocks to handle exceptions gracefully and provide meaningful error messages.

Apply this diff to handle exceptions:

- const result = await client.withAbortSignal(AbortSignal.timeout(timeoutMs)).get(keyPrefix + key);
+ try {
+   const result = await client.withAbortSignal(AbortSignal.timeout(timeoutMs)).get(keyPrefix + key);
+ } catch (error) {
+   if (error.name === 'AbortError') {
+     // Handle timeout-specific logic here
+     return null;
+   }
+   throw error; // Re-throw other unexpected errors
+ }

Repeat similar error handling for other operations using withAbortSignal.

Also applies to: 92-117, 128-130, 141-141, 175-179, 184-184


Line range hint 92-117: Avoid reusing the same AbortSignal across multiple operations

The signal variable created with AbortSignal.timeout(timeoutMs) is shared among multiple operations (setOperation, expireOperation, setTagsOperation). Reusing an AbortSignal can lead to unintended behavior since aborting one operation may affect the others. Create a new AbortSignal for each operation to ensure isolation.

Apply this diff to create separate signals:

- const signal = AbortSignal.timeout(timeoutMs);

  let setOperation: Promise<string | null>;

  let expireOperation: Promise<number> | undefined;

  switch (keyExpirationStrategy) {
    case 'EXAT': {
-     setOperation = client.withAbortSignal(signal).set(
+     setOperation = client.withAbortSignal(AbortSignal.timeout(timeoutMs)).set(
        keyPrefix + key,
        JSON.stringify(cacheHandlerValue),
        // Additional logic...
      );
      break;
    }
    case 'EXPIREAT': {
-     setOperation = client.withAbortSignal(signal).set(keyPrefix + key, JSON.stringify(cacheHandlerValue));
+     setOperation = client.withAbortSignal(AbortSignal.timeout(timeoutMs)).set(keyPrefix + key, JSON.stringify(cacheHandlerValue));

      expireOperation = cacheHandlerValue.lifespan
        ? client
-           .withAbortSignal(signal)
+           .withAbortSignal(AbortSignal.timeout(timeoutMs))
            .expireAt(keyPrefix + key, cacheHandlerValue.lifespan.expireAt)
        : undefined;
      break;
    }
    // Other cases...
  }

  const setTagsOperation =
    cacheHandlerValue.tags.length > 0
      ? client
-         .withAbortSignal(signal)
+         .withAbortSignal(AbortSignal.timeout(timeoutMs))
          .hSet(keyPrefix + sharedTagsKey, key, JSON.stringify(cacheHandlerValue.tags))
      : undefined;
📜 Review details

Configuration used: CodeRabbit UI
Review profile: ASSERTIVE

📥 Commits

Files that changed from the base of the PR and between e55cfbd and aab256f.

⛔ Files ignored due to path filters (1)
  • package-lock.json is excluded by !**/package-lock.json
📒 Files selected for processing (11)
  • apps/cache-testing/package.json (1 hunks)
  • docs/cache-handler-docs/src/pages/handlers/experimental-redis-cluster.mdx (1 hunks)
  • docs/cache-handler-docs/src/pages/handlers/redis-strings.mdx (1 hunks)
  • docs/cache-handler-docs/src/pages/usage/creating-a-custom-handler.mdx (7 hunks)
  • packages/cache-handler/package.json (1 hunks)
  • packages/cache-handler/src/common-types.ts (1 hunks)
  • packages/cache-handler/src/handlers/experimental-redis-cluster.ts (7 hunks)
  • packages/cache-handler/src/handlers/redis-stack.ts (6 hunks)
  • packages/cache-handler/src/handlers/redis-strings.ts (8 hunks)
  • packages/cache-handler/src/helpers/get-timeout-redis-command-options.ts (0 hunks)
  • packages/cache-handler/src/helpers/helpers.ts (0 hunks)
💤 Files with no reviewable changes (2)
  • packages/cache-handler/src/helpers/get-timeout-redis-command-options.ts
  • packages/cache-handler/src/helpers/helpers.ts
🧰 Additional context used
🔇 Additional comments (39)
apps/cache-testing/package.json (1)

23-23: Verify compatibility with Redis 5.0.0-next.5

The upgrade from Redis 4.7.0 to 5.0.0-next.5 is a significant change that aligns with the PR objectives. However, this is a major version upgrade to a pre-release version, which may introduce breaking changes or unstable features.

  1. Ensure that all Redis-related code in the project is compatible with version 5.x.x.
  2. Update any Redis client initialization or usage that may have changed between versions 4 and 5.
  3. Review the Redis 5.x changelog for any deprecated features or new APIs that should be utilized.
  4. Consider adding a comment in the package.json file indicating the reason for using a pre-release version.

To help verify the impact of this change, run the following script:

This script will help identify areas of the code that might need attention due to the Redis upgrade.

docs/cache-handler-docs/src/pages/handlers/experimental-redis-cluster.mdx (2)

40-40: LGTM: Default value for keyExpirationStrategy updated correctly.

The default value for keyExpirationStrategy has been updated from 'EXPRIREAT' to 'EXAT'. This change aligns with the PR objectives and is consistent with updates in other files.


Line range hint 1-100: LGTM: Change is consistent throughout the document.

The update to the default value of keyExpirationStrategy is consistently reflected in the code example and the parameter descriptions. The document maintains information about both 'EXAT' and 'EXPIREAT' strategies, which is helpful for users.

packages/cache-handler/package.json (2)

98-101: Approve optional redis dependency and verify error handling

Making redis an optional peer dependency is a good change that allows for more flexibility in projects that may not require redis.

To ensure robustness, please verify that the code properly handles cases where redis is not available. This might include:

  1. Graceful fallback to alternative caching mechanisms
  2. Clear error messages when attempting to use redis features without the dependency installed

Consider running the following script to check for proper error handling:

#!/bin/bash
# Description: Check for error handling when redis is not available
# Test: Expect to see error handling code for cases where redis is not installed

rg -i 'redis' -A 5 -B 5 | rg -i 'try|catch|if.*require.*redis'

96-96: Verify stability of redis 5.0.0-next.5

The upgrade to redis version 5 aligns with the PR objectives. However, using a specific pre-release version (5.0.0-next.5) might lead to compatibility issues if not managed carefully.

Please confirm:

  1. Is this pre-release version stable enough for production use?
  2. Are there any known issues or breaking changes in this version that might affect the project?
  3. Is there a more stable release of redis 5 available that we could use instead?
docs/cache-handler-docs/src/pages/usage/creating-a-custom-handler.mdx (4)

22-22: LGTM: Redis client import added.

The addition of the createClient import from the 'redis' package is correct and necessary for creating a Redis client. This change aligns with the PR objective of upgrading Redis.


104-106: LGTM: Consistent timeout handling for hmGet operation.

The addition of withAbortSignal(AbortSignal.timeout(timeoutMs)) to the hmGet operation is consistent with the new timeout handling approach. This ensures that the operation doesn't hang indefinitely, improving the overall reliability of the cache handler.


110-113: LGTM: Improved revalidation check and consistent timeout handling.

The changes in this segment are appropriate:

  1. The condition for checking revalidation time has been slightly adjusted, likely for improved clarity.
  2. The unlink operation now uses withAbortSignal(AbortSignal.timeout(timeoutMs)), consistent with the new timeout handling approach throughout the code.

These modifications enhance the reliability and consistency of the cache invalidation process.


Line range hint 1-226: Overall assessment: Significant improvements to the custom Redis handler

The changes in this file successfully achieve the PR objectives and enhance the custom Redis handler implementation:

  1. The Redis client creation and operations have been updated to align with the upgraded Redis version.
  2. A consistent timeout handling approach using withAbortSignal has been implemented across all Redis operations, improving reliability.
  3. The expiration strategy has been updated to use 'EXAT' instead of 'EXPIREAT', as intended.
  4. A new delete method has been added, providing more flexibility in cache management.
  5. Various optimizations and code improvements have been made, enhancing overall performance and readability.

These changes collectively result in a more robust, efficient, and maintainable custom Redis handler for the @neshca/cache-handler package.

packages/cache-handler/src/handlers/redis-stack.ts (11)

58-58: Index schema definition is correctly configured

The index is properly set up to index the tags field as TEXT with an alias tag.


81-83: Improved timeout handling with abort signals

The use of withAbortSignal(AbortSignal.timeout(timeoutMs)) enhances timeout management by integrating it directly into the Redis json.get command.


97-99: Consistent timeout application in tag revalidation retrieval

Including withAbortSignal(AbortSignal.timeout(timeoutMs)) ensures consistent timeout behavior when retrieving revalidation times with hmGet.


103-103: Timeout handling added to cache invalidation

Applying withAbortSignal to the unlink operation ensures that cache invalidation respects the configured timeout.


116-116: Refactored abort signal into a reusable variable

Defining const signal = AbortSignal.timeout(timeoutMs); simplifies the code by reusing the signal across multiple Redis commands.


118-120: Consistent timeout usage in cache set operation

Using the signal with withAbortSignal for the json.set command ensures the set operation adheres to the timeout policy.


123-123: Timeout applied to key expiration

Including withAbortSignal(signal) with the expireAt command ensures that key expiration operations are subject to the timeout setting.


138-140: Timeout management in implicit tag revalidation

Applying withAbortSignal(AbortSignal.timeout(timeoutMs)) to the hSet command ensures timely execution when marking implicit tags as revalidated.


170-170: Timeout enforced for bulk key deletion

Using withAbortSignal with the unlink command for multiple keys ensures that the bulk deletion operation respects the timeout setting.


173-173: Timeout handling integrated into single key deletion

Applying withAbortSignal to the unlink command for individual key deletion aligns with the overall timeout strategy.


148-153: Potential redundancy in timeout specifications for search

Both withAbortSignal(AbortSignal.timeout(timeoutMs)) and the TIMEOUT option are used in the ft.searchNoContent command. Verify if specifying the timeout in both places is necessary or if one suffices to manage the timeout effectively.

Run the following script to check if both timeout settings are required or if one can be omitted:

#!/bin/bash
# Description: Search for documentation or examples regarding the use of both `withAbortSignal` and `TIMEOUT` in `ft.searchNoContent`.

# Expectation: Determine if it's standard practice to use both timeout mechanisms together.
rg 'ft\.searchNoContent' -A 5 | rg -C 3 'withAbortSignal|TIMEOUT'
packages/cache-handler/src/handlers/redis-strings.ts (2)

41-41: Verify 'EXAT' expiration strategy compatibility with Redis versions

The default keyExpirationStrategy has been changed to 'EXAT'. Ensure that all environments using this handler are running Redis version 6.2 or higher, as the 'EXAT' option was introduced in Redis 6.2.


146-158: Ensure correct cursor handling in hScan operation

The cursor is initialized as a string '0' and comparisons are made using string comparison (cursor !== '0'). This aligns with Redis's hScan behavior, where cursors are returned as strings. Good job ensuring type consistency for accurate iteration.

packages/cache-handler/src/handlers/experimental-redis-cluster.ts (17)

75-75: Ensure intentional change of default keyExpirationStrategy to 'EXAT'

Changing the default keyExpirationStrategy from 'EXPIREAT' to 'EXAT' may affect expiration behavior. Confirm that this change is intentional and compatible with the rest of the codebase.


83-85: Good practice: Added timeout to Redis get command

Including an abort signal with a timeout in the get command improves reliability by preventing indefinite waits.


103-105: Good practice: Added timeout to Redis hmGet command

Consistently adding timeout enhances the robustness of the hmGet operation.


109-111: Good practice: Added timeout to Redis unlink command

Including timeout options ensures that the unlink operation does not hang indefinitely.


120-120: Refactored timeout options into a variable

Defining { abortSignal: AbortSignal.timeout(timeoutMs) } once as options improves code readability and maintainability.


124-124: Type correction for expireOperation

Updating expireOperation to Promise<number> | undefined aligns with the return type of the expireAt command.


128-128: Applied timeout options to set command

Using .withCommandOptions(options) when setting the key ensures consistent timeout handling.


140-142: Applied timeout options to set command in 'EXPIREAT' case

Ensuring that timeouts are included in the set operation for the 'EXPIREAT' strategy maintains consistency.


145-147: Applied timeout options to expireAt command

Including timeout options ensures the expireAt command is timely and does not block indefinitely.


158-160: Applied timeout options to hSet command for tags

Including timeouts when setting tags enhances reliability and prevents potential delays.


169-171: Applied timeout options to hSet in revalidateTag for implicit tags

Good practice to include timeout in the hSet operation for revalidating implicit tags.


176-176: Changed cursor type to string '0'

Changing cursor from a number to a string aligns with the expected type for the Redis hScan command, ensuring correct scanning behavior.


181-183: Applied timeout options to hScan command

Including a timeout in hScan enhances the robustness of the scanning operation.


190-190: Corrected cursor comparison to string

Comparing cursor to '0' as a string is necessary after changing the cursor type, ensuring proper loop termination.


219-221: Applied timeout options to unlink commands per slot

Including timeouts in the unlink operations across slots improves reliability and prevents potential blocking.


228-230: Applied timeout options to hDel for updating tags

Ensuring that the hDel operation has a timeout enhances stability.


235-235: Applied timeout options to unlink in delete method

Including a timeout in the unlink operation of the delete method ensures consistent command execution.

Comment on lines +67 to +73
* @default 'EXAT'
*
* @since 1.3.0
*
* @remarks
* - The `'EXPIREAT'` strategy requires Redis server 4.0.0 or newer.
* - The `'EXAT'` strategy requires Redis server 6.2.0 or newer.
* - The `'EXPIREAT'` strategy requires Redis server 4.0.0 or newer.
Copy link

Choose a reason for hiding this comment

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

🧹 Nitpick (assertive)

LGTM! Consider adding a note about performance.

The update to the default keyExpirationStrategy and its documentation looks good. It aligns with the PR objectives and is consistent with changes in other files.

Consider adding a brief note about the performance benefits of using 'EXAT' over 'EXPIREAT' in the documentation. This would help users understand why 'EXAT' is now the default choice. For example:

 * @remarks
 * - The `'EXAT'` strategy requires Redis server 6.2.0 or newer.
 * - The `'EXPIREAT'` strategy requires Redis server 4.0.0 or newer.
+ * - The `'EXAT'` strategy is more efficient as it sets the expiration time in a single operation,
+ *   while `'EXPIREAT'` requires an additional command call.
 */

Committable suggestion was skipped due to low confidence.

@@ -30,7 +30,7 @@ The `redis-strings` Handler uses plain Redis as the cache store. It is a simple
- `client` - A Redis client instance. The client must be ready before creating the Handler.
- `keyPrefix` - Optional. Prefix for all keys, useful for namespacing. Defaults to an empty string.
- `timeoutMs` - Optional. Timeout in milliseconds for Redis operations. Defaults to `5000`. For disabling timeouts, set it to `0`.
- `keyExpirationStrategy` - Optional. It allows you to choose the expiration strategy for cache keys. Defaults to `EXPRIREAT`.
- `keyExpirationStrategy` - Optional. It allows you to choose the expiration strategy for cache keys. Defaults to `EXAT`.
Copy link

Choose a reason for hiding this comment

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

🧹 Nitpick (assertive)

💡 Codebase verification

Clarify Redis version requirements to reflect EXAT dependency.

The installation.mdx currently states that Redis version 4.6.0 or newer is supported. However, the redis-strings.mdx specifies that the EXAT expiration strategy requires Redis 6.2.0 or newer. To avoid confusion:

  • Update installation.mdx to mention that while Redis 4.6.0 or newer is generally supported, certain features like EXAT require Redis 6.2.0 or newer.
  • Review other documentation sections to ensure version requirements are consistently and accurately described.
🔗 Analysis chain

Update related sections for consistency with the new default value.

The change of the default keyExpirationStrategy from EXPIREAT to EXAT is correct. However, to ensure complete and consistent documentation:

  1. Update the code example at the beginning of the file to reflect this change.
  2. In the Parameters section, explicitly mention that EXAT is now the default value.
  3. Verify if the Redis version requirement (6.2.0 or newer for EXAT) is accurate and consistent with other parts of the codebase.

Here's a suggested diff for the code example:

 const redisHandler = createRedisHandler({
   client,
   keyPrefix: 'prefix:',
   timeoutMs: 1000,
-  keyExpirationStrategy: 'EXAT',
   sharedTagsKey: '__sharedTags__',
   revalidateTagQuerySize: 100,
 });

And for the Parameters section:

-  - `keyExpirationStrategy` - Optional. It allows you to choose the expiration strategy for cache keys. Defaults to `EXAT`.
+  - `keyExpirationStrategy` - Optional. It allows you to choose the expiration strategy for cache keys. Defaults to `EXAT` (new default).

To verify the Redis version requirement, please run:

🏁 Scripts executed

The following scripts were executed for the analysis:

Script:

#!/bin/bash
# Description: Check for Redis version requirements across the codebase

# Test: Search for mentions of Redis versions
rg -i 'redis.*version|version.*redis'

Length of output: 138

Comment on lines +75 to +79
// Add a new AbortSignal with a timeout to the Redis operation.
// By default, redis client operations will wait indefinitely.
const options = commandOptions({ signal: AbortSignal.timeout(timeoutMs) });

// Get the value from Redis.
// We use the key prefix to avoid key collisions with other data in Redis.
const result = await client.get(options, keyPrefix + key);
const result = await client.withAbortSignal(AbortSignal.timeout(timeoutMs)).get(keyPrefix + key);
Copy link

Choose a reason for hiding this comment

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

🧹 Nitpick (assertive)

LGTM: Improved timeout handling and key prefixing.

The changes improve the code by:

  1. Using withAbortSignal with AbortSignal.timeout(timeoutMs) for better timeout management.
  2. Including the key prefix in the get operation to avoid key collisions.

These modifications align with the simplification of timeout handling mentioned in the PR summary.

Consider extracting the AbortSignal.timeout(timeoutMs) into a separate variable for reusability and improved readability:

const timeoutSignal = AbortSignal.timeout(timeoutMs);
const result = await client.withAbortSignal(timeoutSignal).get(keyPrefix + key);

Comment on lines +128 to +152
const signal = AbortSignal.timeout(timeoutMs);

// Redis stores strings, so we need to stringify the JSON.
const setOperation = client.set(options, keyPrefix + key, JSON.stringify(cacheHandlerValue));

// If the cacheHandlerValue has a lifespan, set the automatic expiration.
// cacheHandlerValue.lifespan can be null if the value is the page from the Pages Router without getStaticPaths or with `fallback: false`
// so, we need to check if it exists before using it
const expireOperation = cacheHandlerValue.lifespan
? client.expireAt(options, keyPrefix + key, cacheHandlerValue.lifespan.expireAt)
: undefined;
const setOperation = client.withAbortSignal(signal).set(
keyPrefix + key,
JSON.stringify(cacheHandlerValue),
// If the cacheHandlerValue has a lifespan, set the automatic expiration.
// cacheHandlerValue.lifespan can be null if the value is the page from the Pages Router without getStaticPaths or with `fallback: false`
// so, we need to check if it exists before using it
typeof cacheHandlerValue.lifespan?.expireAt === 'number'
? {
EXAT: cacheHandlerValue.lifespan.expireAt,
}
: undefined,
);

// If the cache handler value has tags, set the tags.
// We store them separately to save time to retrieve them in the `revalidateTag` method.
const setTagsOperation = cacheHandlerValue.tags.length
? client.hSet(options, keyPrefix + sharedTagsKey, key, JSON.stringify(cacheHandlerValue.tags))
: undefined;
const setTagsOperation =
cacheHandlerValue.tags.length > 0
? client.withAbortSignal(signal).hSet(keyPrefix + sharedTagsKey, key, JSON.stringify(cacheHandlerValue.tags))
: undefined;

// Wait for all operations to complete.
await Promise.all([setOperation, expireOperation, setTagsOperation]);
await Promise.all([setOperation, setTagsOperation]);
Copy link

Choose a reason for hiding this comment

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

🧹 Nitpick (assertive)

LGTM: Improved set operations with consistent timeout handling and expiration strategy.

The changes in this segment significantly improve the set method:

  1. Creation of an AbortSignal for timeout handling is consistent with the new approach.
  2. The set operation now includes the expiration logic directly, using EXAT instead of EXPIREAT, aligning with the PR objective.
  3. The hSet operation for tags now uses withAbortSignal, ensuring consistent timeout handling.
  4. Using Promise.all to wait for both set operations is a good performance optimization.

These modifications enhance the reliability and efficiency of the cache setting process.

Consider extracting the expiration logic into a separate function for improved readability:

const getExpirationOptions = (lifespan) =>
  typeof lifespan?.expireAt === 'number'
    ? { EXAT: lifespan.expireAt }
    : undefined;

const setOperation = client.withAbortSignal(signal).set(
  keyPrefix + key,
  JSON.stringify(cacheHandlerValue),
  getExpirationOptions(cacheHandlerValue.lifespan)
);

Comment on lines +66 to 68
if (error instanceof Error && error.message === 'Index already exists') {
return;
}
Copy link

Choose a reason for hiding this comment

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

💡 Codebase verification

Error Handling Relies on Error Messages

The current error handling for existing index checks relies on comparing the error message string. It's recommended to use specific error codes or constants to ensure reliability.

  • File: packages/cache-handler/src/handlers/redis-stack.ts
    • Lines: 66-68
🔗 Analysis chain

Verify error handling for existing index check

Ensure that checking error.message === 'Index already exists' reliably captures the intended error. Consider using a specific error code or constant if available, to avoid potential issues with message variations.

Run the following script to check for usage of error codes or constants in index creation error handling:

🏁 Scripts executed

The following scripts were executed for the analysis:

Script:

#!/bin/bash
# Description: Search for instances where index creation errors are handled, looking for specific error codes or constants.

rg 'client\.ft\.create' -A 5 | rg 'error\.(code|name)'

Length of output: 668

@better-salmon better-salmon marked this pull request as draft October 21, 2024 15:33
@tadeumaia
Copy link

What problems are you guys hitting with next 15? I've been running an redis-string on next 15 with no problems for a while. Is it any specific feature?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants