Enhance webhook tests and error handling for authentication #930
Conversation
WalkthroughThe changes in this pull request focus on enhancing error handling and authentication management within the Changes
Possibly related PRs
Suggested reviewers
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
packages/sdk/src/client/client.tsOops! Something went wrong! :( ESLint: 8.19.0 ESLint couldn't find the plugin "@typescript-eslint/eslint-plugin". (The package "@typescript-eslint/eslint-plugin" was not found when loaded as a Node module from the directory "/packages/sdk".) It's likely that the plugin isn't installed correctly. Try reinstalling by running the following: The plugin "@typescript-eslint/eslint-plugin" was referenced from the config file in "packages/sdk/.eslintrc.js » ../../.eslintrc.js". If you still can't figure out the problem, please stop by https://eslint.org/chat/help to chat with the team. 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 using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Outside diff range and nitpick comments (5)
packages/sdk/src/client/client.ts (2)
730-744: Consider refactoring error handling for better maintainability.The error handling logic could be extracted into a separate method to improve readability and maintainability.
Consider this refactor:
- if (await this.handleConnectError(err)) { - if ( - err instanceof ConnectError && - errorCodeOf(err) === Code.ErrUnauthenticated - ) { - attachment.doc.publish([ - { - type: DocEventType.AuthError, - value: { - reason: errorMetadataOf(err).reason, - method: 'Broadcast', - }, - }, - ]); - } + if (await this.handleBroadcastError(err, attachment)) {And add a new method:
private async handleBroadcastError<T, P extends Indexable>( err: unknown, attachment: Attachment<T, P> ): Promise<boolean> { if (!(await this.handleConnectError(err))) { return false; } if ( err instanceof ConnectError && errorCodeOf(err) === Code.ErrUnauthenticated ) { attachment.doc.publish([ { type: DocEventType.AuthError, value: { reason: errorMetadataOf(err).reason, method: 'Broadcast', }, }, ]); } return true; }
730-744: Consider creating a shared error handling utility.The authentication error handling pattern is duplicated across multiple methods (broadcast, syncInternal, runWatchLoop). Consider extracting this into a shared utility method.
Consider adding a utility method:
private publishAuthError<T, P extends Indexable>( err: unknown, attachment: Attachment<T, P>, method: string ): void { if ( err instanceof ConnectError && errorCodeOf(err) === Code.ErrUnauthenticated ) { attachment.doc.publish([ { type: DocEventType.AuthError, value: { reason: errorMetadataOf(err).reason, method, }, }, ]); } }This can then be used across all methods:
this.publishAuthError(err, attachment, 'Broadcast'); this.publishAuthError(err, attachment, 'PushPull'); this.publishAuthError(err, attachment, 'WatchDocuments');packages/sdk/test/integration/webhook_test.ts (3)
345-368: Refactor duplicated project setup code into a helper functionThe code for creating and updating a project is duplicated in multiple test cases (lines 345–368 and 581–605). Refactoring this into a helper function will improve maintainability and reduce code duplication.
You can create a helper function like
createProjectWithWebhook:+async function createProjectWithWebhook(methods: string[]): Promise<{ projectId: string; apiKey: string }> { + const projectResponse = await axios.post( + `${testRPCAddr}/yorkie.v1.AdminService/CreateProject`, + { name: `auth-webhook-${new Date().getTime()}` }, + { + headers: { Authorization: adminToken }, + }, + ); + const projectId = projectResponse.data.project.id; + const apiKey = projectResponse.data.project.publicKey; + + await axios.post( + `${testRPCAddr}/yorkie.v1.AdminService/UpdateProject`, + { + id: projectId, + fields: { + auth_webhook_url: `http://${webhookServerAddress}:${webhookServerPort}/auth-webhook`, + auth_webhook_methods: { methods }, + }, + }, + { + headers: { Authorization: adminToken }, + }, + ); + return { projectId, apiKey }; +}Then replace the duplicated code in your tests:
-// Create new project -const projectResponse = await axios.post( - `${testRPCAddr}/yorkie.v1.AdminService/CreateProject`, - { name: `auth-webhook-${new Date().getTime()}` }, - { - headers: { Authorization: adminToken }, - }, -); -const projectId = projectResponse.data.project.id; -apiKey = projectResponse.data.project.publicKey; - -// Update project with webhook URL and methods -await axios.post( - `${testRPCAddr}/yorkie.v1.AdminService/UpdateProject`, - { - id: projectId, - fields: { - auth_webhook_url: `http://${webhookServerAddress}:${webhookServerPort}/auth-webhook`, - auth_webhook_methods: { methods: ['RemoveDocument'] }, - }, - }, - { - headers: { Authorization: adminToken }, - }, -); +const { projectId, apiKey } = await createProjectWithWebhook(['RemoveDocument']);Also applies to: 581-605
145-150: Reuse theauthTokenInjectorfunction for multiple clientsYou have identical
authTokenInjectorfunctions forc1andc2. Consider defining it once and reusing it to simplify the code.Refactor as follows:
+const authTokenInjector = async () => { + return `token-${Date.now() + 1000 * 60 * 60}`; // expire in 1 hour +}; const c1 = new yorkie.Client(testRPCAddr, { apiKey, - authTokenInjector: async () => { - return `token-${Date.now() + 1000 * 60 * 60}`; // expire in 1 hour - }, + authTokenInjector, }); const c2 = new yorkie.Client(testRPCAddr, { apiKey, - authTokenInjector: async () => { - return `token-${Date.now() + 1000 * 60 * 60}`; // expire in 1 hour - }, + authTokenInjector, });
371-376: Handle potential negative token expiration timesIn your
authTokenInjectorfunction (lines 371–376 and 607–612), you return expired tokens by subtractingTokenExpirationMsfromDate.now(). Ensure thatTokenExpirationMsis always positive to prevent potential issues with negative expiration times.Consider adding a check or using absolute values:
-return `token-${Date.now() - TokenExpirationMs}`; // token expired +return `token-${Date.now() - Math.abs(TokenExpirationMs)}`; // token expiredAlso applies to: 607-612
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (3)
packages/sdk/src/client/client.ts(1 hunks)packages/sdk/src/document/document.ts(1 hunks)packages/sdk/test/integration/webhook_test.ts(5 hunks)
🔇 Additional comments (2)
packages/sdk/src/client/client.ts (1)
730-744: LGTM! Well-structured error handling for authentication failures.
The implementation correctly:
- Checks for authentication errors before retry attempts
- Publishes AuthError events with proper context
- Maintains the existing retry mechanism
packages/sdk/src/document/document.ts (1)
428-428: LGTM! The AuthErrorEvent interface extension is well-aligned.
The addition of 'Broadcast' to the method types is consistent with the enhanced error handling for broadcast operations.
Let's verify the broadcast authentication error handling implementation:
✅ Verification successful
Based on the search results, I can now generate the final response:
The AuthErrorEvent interface extension is correctly implemented and consistent with the codebase.
The search results show that:
- The AuthErrorEvent interface is properly defined with DocEventType.AuthError and includes 'Broadcast' as a valid method type
- The auth error event handling is implemented in the event subscription system
- The broadcast method exists and can trigger events through the publish mechanism
The addition of 'Broadcast' to the method types is a valid extension that aligns with the existing auth error handling infrastructure.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Verify the broadcast authentication error handling implementation
# Expected: Find the broadcast method that triggers AuthError events
# Search for broadcast method implementation that handles auth errors
ast-grep --pattern 'broadcast($$$) {
$$$
type: DocEventType.AuthError,
$$$
}'
# Search for broadcast-related authentication error handling
rg -A 5 'DocEventType\.AuthError.*Broadcast'
Length of output: 132
Script:
#!/bin/bash
# Let's try a different approach to find broadcast-related error handling
# Search for broadcast method implementation
ast-grep --pattern 'broadcast($$$) {
$$$
}'
# Search for AuthError event emissions
rg -B 5 -A 2 'emit.*AuthError'
# Search for broadcast-related error handling
rg -B 3 -A 3 'broadcast.*error|error.*broadcast' -i
# Look for any authentication checks in broadcast-related code
rg -B 3 -A 3 'broadcast.*auth|auth.*broadcast' -i
Length of output: 109885
Script:
#!/bin/bash
# Let's search for broadcast-related authentication code more broadly
# Search for broadcast method implementation and auth-related code
ast-grep --pattern 'broadcast($$$) {
$$$
auth
$$$
}'
# Search for auth error handling in broadcast context
rg -B 5 -A 5 'broadcast.*error|error.*broadcast' -i
# Look for auth error event emissions
rg -B 3 -A 3 'emit.*AuthError.*method.*Broadcast'
# Search for broadcast method with error handling
rg -B 5 -A 5 'broadcast.*\{' -i
Length of output: 110354
Script:
#!/bin/bash
# Let's search for auth error handling in the document.ts file specifically
# Search for auth error event type definition and usage
rg -B 3 -A 3 'AuthError.*event' packages/sdk/src/document/document.ts
# Search for broadcast method implementation
rg -B 10 -A 10 'broadcast.*\{' packages/sdk/src/document/document.ts
# Search for auth error handling in broadcast context
rg -B 5 -A 5 'AuthError.*Broadcast|Broadcast.*AuthError' packages/sdk/src/document/document.ts
# Look for event emission related to broadcast auth
rg -B 5 -A 5 'emit.*auth.*broadcast|broadcast.*auth.*emit' -i packages/sdk/src/document/document.ts
Length of output: 1914
Script:
#!/bin/bash
# Let's search for auth error handling more broadly
# Search for AuthError type definition
rg -B 5 -A 5 'type.*AuthError' packages/sdk/src/document/document.ts
# Search for DocEventType.AuthError usage
rg -B 5 -A 5 'DocEventType\.AuthError' packages/sdk/src/document/document.ts
# Search for auth error handling in event types
rg -B 5 -A 5 'interface.*AuthError.*Event' packages/sdk/src/document/document.ts
# Look for auth error event emission
rg -B 5 -A 5 'emit.*AuthError|publish.*AuthError' packages/sdk/src/document/document.ts
Length of output: 1842
hackerwins
changed the title
What this PR does / why we need it?
Any background context you want to provide?
What are the relevant tickets?
Related yorkie-team/yorkie#1082
Checklist
Summary by CodeRabbit
New Features
'Broadcast', in theAuthErrorEventinterface.Bug Fixes
Tests