rebase from node-redis #1
Conversation
* Add SETINFO support to client connection, with the ability to disable sending the user agent if the end user desires. * Also enables modifying the user-agent with a tag to enable distinguishing different usages.
|
Important Auto Review SkippedReview was skipped due to path filters Files ignored due to path filters (3)
WalkthroughThe updates focus on enhancing error handling with new types ( Changes
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? 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 as PR comments)
Additionally, you can add CodeRabbit Configration File (
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Review Status
Actionable comments generated: 1
Configuration used: CodeRabbit UI
Files ignored due to filter (7)
- package-lock.json
- package.json
- packages/client/package.json
- packages/client/tsconfig.json
- packages/graph/package.json
- packages/search/package.json
- tsconfig.base.json
Files selected for processing (12)
- packages/client/lib/client/index.spec.ts (3 hunks)
- packages/client/lib/client/index.ts (3 hunks)
- packages/client/lib/client/socket.ts (2 hunks)
- packages/client/lib/cluster/index.ts (8 hunks)
- packages/client/lib/commands/CLIENT_INFO.ts (2 hunks)
- packages/client/lib/errors.ts (2 hunks)
- packages/client/lib/multi-command.ts (3 hunks)
- packages/client/lib/test-utils.ts (1 hunks)
- packages/graph/lib/commands/EXPLAIN.ts (1 hunks)
- packages/graph/lib/commands/PROFILE.ts (1 hunks)
- packages/graph/lib/graph.ts (1 hunks)
- packages/search/lib/index.ts (1 hunks)
Files skipped from review due to trivial changes (3)
- packages/client/lib/cluster/index.ts
- packages/graph/lib/commands/EXPLAIN.ts
- packages/graph/lib/commands/PROFILE.ts
Additional comments: 24
packages/client/lib/client/index.spec.ts (5)
6-6: The import of
ErrorReplyandMultiErrorReplyaligns with the PR's focus on enhancing error handling mechanisms. Ensure that these new error types are used correctly in the error handling logic within the test suite.13-13: The import of
versionfrompackage.jsonis new and should be used to reflect the current version of the library in client information or other relevant areas.124-160: New tests have been added to verify the client information handling, including setting default library name and version, disabling the sending of this information, and sending a client name tag. Ensure that these tests are comprehensive and cover all the new scenarios introduced by the changes.
606-620: The test case for handling error replies in multi-command scenarios is crucial for ensuring the new error types work as expected. Verify that the assertions correctly test the behavior of
MultiErrorReplyand associated error handling.120-163: > Note: This review was outside the patches, so it was mapped to the patch with the greatest overlap. Original lines [3-621]
Ensure that all new and modified tests pass by running
npm testas per the PR checklist. This will confirm that the changes do not introduce regressions and that the new features work as intended.packages/client/lib/client/index.ts (4)
14-14: The import of
ErrorReplyis correctly added to handle new error types.19-19: The import of
versionfrompackage.jsonis correctly added. Ensure that theversionis used appropriately in the codebase and that the import does not introduce any side effects.Verification successful
The import of
versionfrompackage.jsoninpackages/client/lib/client/index.tsis used as part of a command to set client information, which is consistent with the PR's objectives. There is no evidence from the provided output to suggest that the import introduces any side effects. Based on this, the usage ofversionappears to be appropriate.* 71-78: The addition of `disableClientInfo` and `clientInfoTag` to the `RedisClientOptions` interface is correctly implemented. Ensure that these new options are documented and that their usage is consistent across the codebase.Scripts Executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Verify that the version import is used correctly and does not cause side effects. rg --type typescript --files-with-matches 'version' packages/client/lib/client/index.tsLength of output: 122
Script:
#!/bin/bash # Verify that the version import is used correctly and does not cause side effects. rg 'version' packages/client/lib/client/index.tsLength of output: 161
Verification successful
The addition of
disableClientInfoandclientInfoTagto theRedisClientOptionsinterface has been verified. The options are documented in theindex.tsfile and are consistently used within the same file. Tests inindex.spec.tsalso include these options, indicating that their behavior is covered by the testing suite. No issues with documentation or inconsistent usage have been found.* 287-311: The new logic to conditionally send client information based on the `disableClientInfo` option is correctly implemented. However, ensure that the error handling for `ErrorReply` is consistent with the rest of the codebase and that the new client information logic does not introduce any regressions.Scripts Executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Verify that the new options are documented and used consistently. rg --type typescript --files-with-matches 'disableClientInfo|clientInfoTag' packages/client/lib/client/Length of output: 140
Script:
#!/bin/bash # Verify that the new options are documented and used consistently. rg --glob '*.ts' 'disableClientInfo|clientInfoTag' packages/client/lib/client/Length of output: 617
packages/client/lib/client/socket.ts (5)
202-204: The condition in line 202 checks if the socket closed unexpectedly without an error and if the socket is still marked as open. This is a good check to ensure that the error handling logic is only triggered for unexpected closures. However, it's important to ensure that
this.#isOpenis managed correctly throughout the socket's lifecycle to avoid false positives or negatives.236-239: The logic in lines 236-239 checks if the socket was ready and open before attempting to reconnect. This is a sensible approach, as it avoids unnecessary reconnect attempts when the socket was not ready or already closed. However, it's crucial to ensure that the
#shouldReconnectmethod returns the expected values to control the flow correctly.232-234: The error handling in line 234 emits the error to the event listeners. It's important to ensure that this error emission is consistent with the overall error handling strategy of the library and that listeners are set up to handle these errors appropriately.
229-239: > Note: This review was outside the patches, so it was mapped to the patch with the greatest overlap. Original lines [199-239]
The reconnect loop within the
#connectmethod is designed to keep trying until the socket is open and ready. It's important to ensure that there are adequate safeguards to prevent an infinite loop in case of persistent connection issues. Additionally, the error handling and retry logic should be reviewed to confirm that they are in line with the intended behavior and that they provide a clear strategy for when to stop attempting to reconnect.
- 229-239: > Note: This review was outside the patches, so it was mapped to the patch with the greatest overlap. Original lines [199-239]
The changes within
socket.tsappear to be internal and do not seem to alter the declarations of exported or public entities. It's important to ensure that these changes do not inadvertently change the behavior expected by public API consumers.packages/client/lib/commands/CLIENT_INFO.ts (2)
31-36: The addition of
libNameandlibVerto theClientInfoReplyinterface is consistent with the PR objectives to manage client information more effectively. Ensure that the newly added fields are documented and that any external documentation is updated accordingly.68-70: The changes to the
transformReplyfunction correctly parse the newlibNameandlibVerfields from the raw reply. This is in line with the modifications to theClientInfoReplyinterface.packages/client/lib/errors.ts (2)
1-1: The addition of the
importstatement forRedisCommandRawReplyis noted and appears to be used in the newMultiErrorReplyclass.69-84: The
MultiErrorReplyclass is well-structured and follows the convention of extending the baseErrorclass throughErrorReply. The use of a generator function for iterating over errors is a good practice for lazy evaluation. Ensure that the consumers of this class are updated to handle the new structure and iteration pattern.packages/client/lib/multi-command.ts (2)
72-75: Ensure that the handling of
nullas the last reply inhandleExecRepliesis consistent with the expected behavior of the Redis server and the client's error handling strategy.81-93: The logic for capturing error indexes and throwing a
MultiErrorReplyseems correct. However, verify that theerrorIndexesarray correctly corresponds to the indexes ofErrorReplyinstances within therawRepliesarray and that theMultiErrorReplyis constructed with the appropriate parameters.packages/client/lib/test-utils.ts (2)
5-11: The addition of
defaultDockerVersionto theTestUtilsclass instance is a clear and straightforward update. Ensure that this new property is utilized appropriately whereverTestUtilsinstances are created or used.5-11: Verify that the new
defaultDockerVersionproperty is properly integrated and used in the codebase whereTestUtilsinstances are involved.Verification successful
The new
defaultDockerVersionproperty is present in theTestUtilsclass definition and is being utilized in varioustest-utils.tsfiles across different packages. This suggests that the property is properly integrated into the codebase.Scripts Executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Search for usage of TestUtils instances to ensure the new property is integrated correctly. rg --type typescript "new TestUtils"Length of output: 73
Script:
#!/bin/bash # Search for usage of TestUtils instances to ensure the new property is integrated correctly. rg "new TestUtils"Length of output: 410
Script:
#!/bin/bash # Check if the `defaultDockerVersion` property is being accessed or utilized in the files where TestUtils is instantiated. rg "defaultDockerVersion"Length of output: 447
packages/graph/lib/graph.ts (1)
- 129-136: The types
GraphReplyandGraphClientTypeare now exported, which changes the public API of this module. Confirm that this change is intentional and check for any potential downstream effects on the codebase that may rely on these types.Verification successful
The previous script did not execute as expected due to an unrecognized file type flag. I will modify the script to remove the
--type typescriptflag and rerun the search to find usages of 'GraphReply' and 'GraphClientType' across the entire codebase.
The verification process did not find any instances of 'GraphReply' or 'GraphClientType' being used outside of the
graph.tsfile. This suggests that the change to export these types currently does not have any downstream effects within the codebase. However, by exporting these types, they become part of the public API and could be used by external consumers in the future.Scripts Executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Search for files that import 'GraphReply' or 'GraphClientType' from 'graph.ts' rg --type typescript --files-with-matches 'from.*graph' | xargs rg 'GraphReply|GraphClientType'Length of output: 522
Script:
#!/bin/bash # Search for files that import 'GraphReply' or 'GraphClientType' from 'graph.ts' across all files rg --files-with-matches 'from.*graph' | xargs rg 'GraphReply|GraphClientType'Length of output: 79
packages/search/lib/index.ts (1)
- 3-4: The addition of
RedisSearchLanguagesandAggregateGroupByReducersto the exports is confirmed. Ensure that corresponding documentation updates are included if these are new public APIs.
packages/client/lib/errors.ts
Outdated
| export class MultiErrorReply extends ErrorReply { | ||
| replies; | ||
| errorIndexes; | ||
|
|
||
| constructor(replies: Array<RedisCommandRawReply | ErrorReply>, errorIndexes: Array<number>) { | ||
| super(`${errorIndexes.length} commands failed, see .replies and .errorIndexes for more information`); | ||
| this.replies = replies; | ||
| this.errorIndexes = errorIndexes; | ||
| } | ||
|
|
||
| *errors() { | ||
| for (const index of this.errorIndexes) { | ||
| yield this.replies[index]; | ||
| } | ||
| } | ||
| } |
There was a problem hiding this comment.
The MultiErrorReply class does not explicitly set the stack property to undefined as seen in the ErrorReply class. If this is intentional, no action is needed, but if it's an oversight, consider aligning the behavior with ErrorReply.
class MultiErrorReply extends ErrorReply {
// ...
constructor(replies: Array<RedisCommandRawReply | ErrorReply>, errorIndexes: Array<number>) {
super(`${errorIndexes.length} commands failed, see .replies and .errorIndexes for more information`);
+ this.stack = undefined;
// ...
}
// ...
}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.
| export class MultiErrorReply extends ErrorReply { | |
| replies; | |
| errorIndexes; | |
| constructor(replies: Array<RedisCommandRawReply | ErrorReply>, errorIndexes: Array<number>) { | |
| super(`${errorIndexes.length} commands failed, see .replies and .errorIndexes for more information`); | |
| this.replies = replies; | |
| this.errorIndexes = errorIndexes; | |
| } | |
| *errors() { | |
| for (const index of this.errorIndexes) { | |
| yield this.replies[index]; | |
| } | |
| } | |
| } | |
| export class MultiErrorReply extends ErrorReply { | |
| replies; | |
| errorIndexes; | |
| constructor(replies: Array<RedisCommandRawReply | ErrorReply>, errorIndexes: Array<number>) { | |
| super(`${errorIndexes.length} commands failed, see .replies and .errorIndexes for more information`); | |
| this.stack = undefined; | |
| this.replies = replies; | |
| this.errorIndexes = errorIndexes; | |
| } | |
| *errors() { | |
| for (const index of this.errorIndexes) { | |
| yield this.replies[index]; | |
| } | |
| } | |
| } |
There was a problem hiding this comment.
Review Status
Actionable comments generated: 0
Configuration used: CodeRabbit UI
Files ignored due to filter (3)
- package-lock.json
- package.json
- packages/client/package.json
Files selected for processing (1)
- README.md (1 hunks)
Files skipped from review due to trivial changes (1)
- README.md
…ent.disconnect()` when unsubscribing (#2687) * Confirm the client isOpen before disconnecting * Write tests * fix tests * fix tests --------- Co-authored-by: Leibale Eidelman <[email protected]>
There was a problem hiding this comment.
Review Status
Actionable comments generated: 0
Configuration used: CodeRabbit UI
Files selected for processing (2)
- packages/client/lib/cluster/cluster-slots.ts (2 hunks)
- packages/client/lib/cluster/index.spec.ts (2 hunks)
Additional comments: 4
packages/client/lib/cluster/index.spec.ts (2)
- 239-249: The test case for concurrent
UNSUBSCRIBEoperations is well-structured and follows best practices for asynchronous testing withPromise.all. This ensures that both unsubscribe operations are initiated simultaneously, which is a realistic scenario for testing the robustness of the unsubscribe mechanism in a concurrent environment.- 338-351: The test case for concurrent
SUNSUBSCRIBEoperations mirrors the structure and intent of the previous test forUNSUBSCRIBE, targeting the sharded unsubscribe functionality. It's correctly implemented to test concurrent operations, ensuring the system's stability and correctness under simultaneous unsubscribe requests.packages/client/lib/cluster/cluster-slots.ts (2)
- 565-565: The addition of a condition to check if the client is open before disconnecting it in the
executeUnsubscribeCommandmethod is a good practice. This ensures that the client is only disconnected if it is both inactive in pub/sub and still open, preventing unnecessary disconnection attempts on already closed clients.- 616-616: Similarly, the condition added to the
executeShardedUnsubscribeCommandmethod to check if the client is open before disconnecting it follows the same good practice as the previous comment. It ensures that unnecessary disconnection attempts are avoided, which is crucial for maintaining the stability and efficiency of the system.
dosubot
bot
added
the
size:L
There was a problem hiding this comment.
Review Status
Actionable comments generated: 0
Configuration used: CodeRabbit UI
Files selected for processing (1)
- packages/json/lib/test-utils.ts (1 hunks)
Additional comments: 1
packages/json/lib/test-utils.ts (1)
- 6-7: The addition of
defaultDockerVersionto theTestUtilsobject initialization is correctly implemented.
…2733) * Parser support with all commands * remove "dist" from all imports for consistency * address most of my review comments * small tweak to multi type mapping handling * tweak multi commands / fix addScript cases * nits * addressed all in person review comments * revert addCommand/addScript changes to multi-commands addCommand needs to be there for sendCommand like ability within a multi. If its there, it might as well be used by createCommand() et al, to avoid repeating code. addScript is there (even though only used once), but now made private to keep the logic for bookkeeping near each other.
* fix sentinel generics * comment nit
* shallow copy of this.#options.defaults.socket * shallow copy of this.#options.defaults.socket * nit * fix redis create cluster client again --------- Co-authored-by: Max Gruenfelder <[email protected]> Co-authored-by: Leibale Eidelman <[email protected]>
- Add node 22 - Update actions/setup-node from v3 to v4 - Ignore .md files from triggering the workflow
…nt authentication (#2877) * feat(auth): refactor authentication mechanism to use CredentialsProvider - Introduce new credential providers: AsyncCredentialsProvider, StreamingCredentialsProvider - Update client handshake process to use the new CredentialsProviders and to support async credentials fetch / credentials refresh - Internal conversion of username/password to a CredentialsProvider - Modify URL parsing to accommodate the new authentication structure - Tests * feat(auth): auth extensions Introduces TokenManager and supporting classes to handle token acquisition, automatic refresh, and updates via identity providers. This foundation enables consistent authentication token management across different identity provider implementations. Key additions: - Add TokenManager to obtain and maintain auth tokens from identity providers with automated refresh scheduling based on TTL and configurable thresholds - Add IdentityProvider interface for token acquisition from auth providers - Implement Token class for managing token state and TTL tracking - Include configurable retry mechanism with exponential backoff and jitter - Add comprehensive test suite covering refresh cycles and error handling This change establishes the core infrastructure needed for reliable token lifecycle management across different authentication providers. * feat(auth): add Entra ID identity provider integration Introduces Entra ID (former Azure AD) authentication support with multiple authentication flows and automated token lifecycle management. Key additions: - Add EntraIdCredentialsProvider for handling Entra ID authentication flows - Implement MSALIdentityProvider to integrate with MSAL/EntraID authentication library - Add support for multiple authentication methods: - Managed identities (system and user-assigned) - Client credentials with certificate - Client credentials with secret - Authorization Code flow with PKCE - Add factory class with builder methods for each authentication flow - Include sample Express server implementation for Authorization Code flow - Add comprehensive configuration options for authority and token management * feat(test-utils): improve cluster testing - Add support for configuring replica authentication with 'masterauth' - Allow default client configuration during test cluster creation This improves the testing framework's flexibility by automatically configuring replica authentication when '--requirepass' is used and enabling custom client configurations across cluster nodes. * feat(auth): add EntraId integration tests - Add integration tests for token renewal and re-authentication flows - Update credentials provider to use uniqueId as username instead of account username - Add test utilities for loading Redis endpoint configurations - Split TypeScript configs into separate files for samples and integration tests - Remove `@redis/authx` package and nest it under `@`
Description
Checklist
npm testpass with this change (including linting)?Summary by CodeRabbit
New Features
versionfrompackage.jsonfor external use.Bug Fixes
transfromReplyto `transformReply.Documentation
Refactor
Tests
Chores
versionfrompackage.jsonfor external use.