Introduce project event webhook #1113
Conversation
WalkthroughThis pull request introduces an event webhook system for Yorkie, enabling notifications to external services about document creation and removal events. It includes modifications across multiple files, adding new fields and methods for managing webhooks in the Changes
Assessment against linked issues
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 (
|
|
I notice that the
The first task has been completed, and the second task has been worked on #563. but third task is ongoing in PR #718. I'm currently considering when we should publish the Two possible approaches:
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (19)
api/types/project_event.go (3)
20-21: Clarify the docstring or remove the duplication
You've declared the typeProjectEventat line 21 and also have a comment at line 20 that says "ProjectEvent represents the event of the project." The same comment repeats at line 23. Consider consolidating the comment sections to avoid duplicate docstring for the same entity.
25-27: Extend or future-proof event enums
Currently, only two constantsDocumentCreatedandDocumentRemovedare declared. Before merging, consider whether future events (e.g., DocumentUpdated, DocumentAccessed) might soon be needed. It may be beneficial to outline how to add additionalProjectEventconstants in the future.
29-34: Consider adding validation logic
WebhookAttributeincludesDocumentKey,ClientKey, andIssuedAt, which are all strings. Depending on your use cases, it might be beneficial to apply constraints or validations (for example, ensuringIssuedAtis a valid timestamp).pkg/webhook/client.go (4)
31-35: Lock down or rotate private key
HMACTransportholds aPrivateKeyas a simple string. In production, consider stricter key management practices, such as using environment variables or credential managers (e.g. Vault).
37-43: Ensure constant-time comparison
GenerateHMACSignatureuses a standard HMAC approach. For improved security, ensure you’re also using a constant-time string comparison function (likehmac.Equal) whenever you verify signatures. This is less about generation and more about how you will compare the header on the receiver side.
49-55: Handle large request bodies carefully
Reading the full request body (io.ReadAll) into memory can be risky for large payloads. Consider setting a maximum limit or using a streaming approach if you anticipate very large payloads to avoid potential memory pressure.
72-78: Allow specifying a custom transport
NewClientalways wrapshttp.DefaultTransportinsideHMACTransport. If a user wants to customize TLS settings, proxies, or other details, they can’t easily plug in their own transport. Consider adding a parameter or exposing a method to inject a custom underlying transport.api/types/project.go (1)
83-100: Consider more expressive naming for method
RequireEventWebhookreturns a boolean but doesn't convey the specific reason for requiring the webhook. If expanded to handle multiple event types or statuses, a more descriptive name likeShouldTriggerEventWebhookcould make the intent clearer.pkg/webhook/retry.go (2)
37-64: Distinguish between both HTTP and transport errors
WithExponentialBackoffmerges status code checks with error checks, returning eitherErrUnexpectedStatusCodeor the underlying error. This is reasonable but consider a structured approach to error handling (e.g., typed errors), which would help when diagnosing failures (like DNS resolution errors, timeouts, etc.) vs. unexpected status codes.
66-74: Add jitter to exponential backoff
Currently,waitIntervaldoubles the wait time. For production systems, especially if many clients or goroutines use the same retry logic, consider adding jitter to reduce the risk of thundering herd problems (when many clients retry simultaneously).server/rpc/projectevent/webhook.go (2)
36-62: Log response body on failures
While errors are logged when JSON marshaling or sending the webhook fails, capturing and logging at least a portion of the response body in cases of non-2xx responses may help operators debug webhook-related issues more easily.- logging.From(ctx).Error(err) + body, _ := io.ReadAll(resp.Body) + logging.From(ctx).Errorf("webhook error: %v, response: %s", err, string(body))
91-126: Improve error message consistency
Your error check treats statuses other than OK, Unauthorized, and Forbidden the same. Depending on your system’s needs, you might treat “Service Unavailable (503)” or “Too Many Requests (429)” distinctly. If you prefer generic handling, consider clarifying that these unexpected statuses are retried.server/config.go (2)
63-71: No input validation for negative durations
Although defaults are applied, if a user manually provides negative intervals or durations, it may introduce undefined behavior. Consider adding checks to ensure thatEventWebhookMaxRetries≥ 0 and intervals ≥ 0.
201-215: Align naming consistency
Currently, some fields reference “EventWebhook,” others “ProjectWebhook” (for example, in the parsing functions). Ensure that the naming is consistent to avoid confusion when reading or configuring the code.server/backend/config.go (1)
76-87: Clarify in-field documentation
The docstring states “is the time that waits time for response” which is a bit unclear. Consider rephrasing to “is the timeout duration for waiting for a webhook response.”cmd/yorkie/server.go (3)
50-56: Utilize consistent naming
Variables introduced here reference “eventWebhook...”, but the user-facing flags reference “project-event-webhook...”. Aligning these might provide better clarity and ease of maintenance.
336-341: Ensure no integer overflow
eventWebhookMaxRetriesis auint64. Although unlikely, verify that excessively large values won’t cause issues (e.g., indefinite retry). A safeguard or maximum recommended value might be beneficial.
342-359: Validate non-zero intervals
Ensure that zero or negative durations forproject-event-webhook-base-wait-interval,project-event-webhook-max-wait-interval, orproject-event-webhook-request-timeoutare handled gracefully instead of leading to potential indefinite blocking or error states.server/rpc/yorkie_server.go (1)
161-164: Consider adding error handling forDocumentCreated.
Ifprojectevent.DocumentCreatedfails, it would be useful to capture or log the error to ensure that the system is aware of potential failures delivering creation events.if docInfo.ServerSeq == 0 { - projectevent.DocumentCreated(ctx, s.backend, docInfo.Key.String(), clientInfo.Key) + if err := projectevent.DocumentCreated(ctx, s.backend, docInfo.Key.String(), clientInfo.Key); err != nil { + logging.From(ctx).Errorf("failed to create document event: %v", err) + } }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (9)
api/types/project.go(2 hunks)api/types/project_event.go(1 hunks)cmd/yorkie/server.go(3 hunks)pkg/webhook/client.go(1 hunks)pkg/webhook/retry.go(1 hunks)server/backend/config.go(2 hunks)server/config.go(2 hunks)server/rpc/projectevent/webhook.go(1 hunks)server/rpc/yorkie_server.go(3 hunks)
🔇 Additional comments (8)
api/types/project_event.go (1)
36-40: Check for optional vs required fields
WebhookRequest includes the Type and Attributes. Confirm if all these fields should always be present in the webhook payload. If any are optional, consider using pointers or adding default values.
api/types/project.go (1)
41-45: Maintain naming consistency
EventWebhookURL and event_webhook_url, similarly EventWebhookMethods and event_webhook_methods follow the same pattern as AuthWebhookURL and auth_webhook_url. This consistency looks good. Keep an eye on naming consistency in future expansions as well.
server/rpc/projectevent/webhook.go (2)
20-33: Consider tracking context lifespan when launching goroutines
Since you're dispatching these webhook events in a background goroutine, ensure that the passed ctx won't be canceled prematurely once the original request completes. If the ctx is tied to a client request, triggering this goroutine after the request returns could lead to unexpected cancellation.
Would you like me to generate a shell script to check goroutine usage in the codebase for potential context misuses?
64-89: Confirm the timing of "DocumentRemoved"
Given the discussion about soft vs. hard deletion in the PR objectives, please confirm that calling DocumentRemoved at this stage aligns with your intended approach. If documents can be revived, you may wish to defer this call to ensure the correct event timing.
cmd/yorkie/server.go (1)
73-75: Confirm max-wait intervals
Double-check that eventWebhookMaxWaitInterval and eventWebhookBaseWaitInterval make sense together (e.g., base <= max). It might be worth validating them or logging an error if the base is larger than the max.
server/rpc/yorkie_server.go (3)
39-39: New import is well-introduced and consistent with the rest of the file.
The import statement aligns with the newly introduced event-based architecture and seems properly scoped.
161-164: Confirm the event firing strictly on ServerSeq == 0.
This condition implies that the creation event is only triggered the first time a document is attached. If future usage requires re-creations or re-attachments, you may need an alternative condition.
566-567: Provide clarity around soft vs. hard deletion for document removal.
Currently, DocumentRemoved is triggered on any remove operation. If you plan to support soft deletions, ensure that the event’s name and timing align with actual removal semantics to avoid user confusion.
| // shouldRetry returns true if the given error should be retried. | ||
| // Refer to https://github.com/kubernetes/kubernetes/search?q=DefaultShouldRetry | ||
| func shouldRetry(statusCode int, err error) bool { | ||
| // If the connection is reset, we should retry. | ||
| var errno syscall.Errno | ||
| if errors.As(err, &errno) { | ||
| return errno == syscall.ECONNRESET | ||
| } | ||
|
|
||
| return statusCode == http.StatusInternalServerError || | ||
| statusCode == http.StatusServiceUnavailable || | ||
| statusCode == http.StatusGatewayTimeout || | ||
| statusCode == http.StatusTooManyRequests | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Add 4xx handling
shouldRetry currently returns true for only high-level server errors like 500 or 503. In certain scenarios (e.g., 429 Too Many Requests), the function also returns true, which is fine. However, you may need logic for other 4xx status codes if your service wants clients to back off, especially if a rate-limit or temporary ban is in place.
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (2)
pkg/webhook/retry.go (2)
37-44: Add input validation for WithExponentialBackoff parameters.The function should validate its input parameters to ensure proper operation.
Consider adding these checks:
func WithExponentialBackoff( ctx context.Context, maxRetries uint64, baseInterval, maxInterval gotime.Duration, webhookFn func() (int, error), ) error { + if ctx == nil { + return fmt.Errorf("context cannot be nil") + } + if webhookFn == nil { + return fmt.Errorf("webhook function cannot be nil") + } + if baseInterval <= 0 { + return fmt.Errorf("base interval must be positive") + } + if maxInterval < baseInterval { + return fmt.Errorf("max interval must be greater than or equal to base interval") + }
71-72: Incomplete function comment.The comment for
waitIntervalis incomplete and doesn't fully describe the function's behavior.Consider this improvement:
-// waitInterval returns the interval of given retries. it returns maxWaitInterval +// waitInterval calculates the wait interval using exponential backoff based on the number +// of retries. It returns the calculated interval or maxWaitInterval if the calculated +// interval would exceed it.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
pkg/webhook/retry.go(1 hunks)server/backend/config.go(2 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- server/backend/config.go
🔇 Additional comments (3)
pkg/webhook/retry.go (3)
1-27: LGTM! Clean package structure and imports.
The file structure follows good practices with proper license headers and well-organized imports. The gotime alias is a good choice to avoid confusion in time-related operations.
29-35: LGTM! Well-defined error types.
The error variables are properly defined with clear, descriptive messages that align with their intended use cases in the retry logic.
81-94: Enhance error handling for 4xx status codes.
While the current implementation handles common server errors and rate limiting (429), it could benefit from handling additional 4xx status codes based on your webhook service's requirements.
Consider this enhancement:
func shouldRetry(statusCode int, err error) bool {
var errno syscall.Errno
if errors.As(err, &errno) {
return errno == syscall.ECONNRESET
}
+ // 4xx client errors that indicate temporary conditions
+ if statusCode == http.StatusTooManyRequests ||
+ statusCode == http.StatusRequestTimeout ||
+ statusCode == http.StatusConflict {
+ return true
+ }
+
+ // 5xx server errors
return statusCode == http.StatusInternalServerError ||
statusCode == http.StatusServiceUnavailable ||
- statusCode == http.StatusGatewayTimeout ||
- statusCode == http.StatusTooManyRequests
+ statusCode == http.StatusGatewayTimeout
}Also, consider making the Kubernetes reference more specific:
-// Refer to https://github.com/kubernetes/kubernetes/search?q=DefaultShouldRetry
+// Retry logic inspired by Kubernetes' DefaultShouldRetry:
+// https://github.com/kubernetes/kubernetes/blob/master/staging/src/k8s.io/client-go/util/retry/util.go
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (1)
pkg/webhook/retry.go (1)
93-106: 🛠️ Refactor suggestionAdd comprehensive 4xx handling.
The shouldRetry function currently handles only specific status codes. Consider adding handling for other relevant 4xx status codes and documenting the retry policy for each status code.
Consider this enhanced implementation:
func shouldRetry(statusCode int, err error) bool { var errno syscall.Errno if errors.As(err, &errno) { return errno == syscall.ECONNRESET } + // Retry on server errors return statusCode == http.StatusInternalServerError || statusCode == http.StatusServiceUnavailable || statusCode == http.StatusGatewayTimeout || - statusCode == http.StatusTooManyRequests + // Retry on specific client errors + statusCode == http.StatusTooManyRequests || + statusCode == http.StatusRequestTimeout || + // Consider adding other relevant status codes based on your webhook service's behavior + false }
🧹 Nitpick comments (2)
pkg/webhook/retry.go (2)
29-36: Consider grouping related errors under a custom error type.The error declarations are well-documented and follow Go conventions. However, for better error handling by consumers, consider creating a custom error type that wraps these errors and provides type assertions.
Example implementation:
type WebhookError struct { Err error StatusCode int } func (e *WebhookError) Error() string { return e.Err.Error() } func (e *WebhookError) Unwrap() error { return e.Err }
83-91: Improve documentation for waitInterval function.The function comment is incomplete. It should explain the exponential backoff calculation and document the parameters.
Suggested documentation:
// waitInterval calculates the wait duration using exponential backoff. // It doubles the wait interval for each retry attempt, starting from baseInterval. // The returned duration is capped at maxWaitInterval. // Parameters: // - retries: number of retry attempts so far // - baseInterval: initial wait duration // - maxWaitInterval: maximum wait duration
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
pkg/webhook/retry.go(1 hunks)
🔇 Additional comments (2)
pkg/webhook/retry.go (2)
1-27: LGTM! Clean package structure and imports.
The package structure follows Go conventions with proper license header and well-organized imports.
38-45: Verify webhook timeout configuration alignment.
The implementation supports the PR's goal of handling timeouts through the retry mechanism. However, let's verify that the timeout values align with the requirements specified in the PR objectives.
Run this script to check timeout configurations across the codebase:
✅ Verification successful
Let me generate another verification script to check the webhook timeout configurations more specifically:
The previous results show some timeout configurations but we need to verify specifically how they are used with the retry mechanism in the webhook package.
Let me check the full implementation of the retry mechanism:
Let me check how the retry mechanism is used in the project event webhook:
Webhook timeout configuration is properly implemented and aligned with requirements
The implementation properly handles timeouts and unreachable events through:
-
A robust exponential backoff retry mechanism in
pkg/webhook/retry.gothat:- Takes configurable base and max intervals
- Supports maximum retry attempts
- Handles transient errors and specific HTTP status codes (500, 503, 504, 429)
- Includes connection reset detection
-
Proper webhook client configuration in
pkg/webhook/client.gowith:- Configurable request timeout
- HMAC authentication
- Error handling
-
Well-structured webhook event sending in
server/rpc/projectevent/webhook.gothat:- Uses the retry mechanism with configurable parameters
- Runs in background to avoid blocking
- Logs failures for potential recovery
- Handles authentication errors separately from retryable errors
The implementation aligns with the PR objectives by providing a comprehensive solution for handling timeouts and unreachable events with configurable retry parameters.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Check for consistency in timeout configurations
# Look for timeout-related configurations
echo "Searching for timeout configurations..."
rg -i "timeout|interval|retry" -g "*.{go,yaml,json}"
# Look for webhook client configurations
echo -e "\nSearching for webhook client configurations..."
ast-grep --pattern 'type $_ struct {
$$$
timeout $_
$$$
}'
Length of output: 77576
Script:
#!/bin/bash
# Search for webhook timeout and retry related code
echo "Searching for webhook timeout configurations..."
rg -A 5 "EventWebhookRequestTimeout|EventWebhookBaseWaitInterval|EventWebhookMaxWaitInterval|AuthWebhookMaxWaitInterval" pkg/webhook/
# Search for retry mechanism implementation
echo -e "\nSearching for retry mechanism implementation..."
rg -A 10 "WithExponentialBackoff" pkg/webhook/
Length of output: 939
Script:
#!/bin/bash
# Get the full implementation of retry mechanism
echo "Full retry mechanism implementation:"
cat pkg/webhook/retry.go
# Check where this retry mechanism is used
echo -e "\nUsage of retry mechanism:"
rg "WithExponentialBackoff\(" --type go
Length of output: 3236
Script:
#!/bin/bash
# Check project event webhook implementation
echo "Project event webhook implementation:"
cat server/rpc/projectevent/webhook.go
# Check webhook client implementation
echo -e "\nWebhook client implementation:"
cat pkg/webhook/client.go
Length of output: 6504
Co-authored-by: coderabbitai[bot] <136622811+coderabbitai[bot]@users.noreply.github.com>
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
pkg/webhook/retry.go (1)
38-81: Add input validation and improve error handling.While the implementation is solid, consider these improvements:
Add input validation for the parameters:
- baseInterval should be > 0
- maxInterval should be >= baseInterval
- maxRetries should have a reasonable upper limit
Consider adding more context to error wrapping for better debugging.
Here's a suggested improvement:
func WithExponentialBackoff( ctx context.Context, maxRetries uint64, baseInterval, maxInterval gotime.Duration, webhookFn func() (int, error), ) error { + if baseInterval <= 0 { + return fmt.Errorf("baseInterval must be positive: %v", baseInterval) + } + if maxInterval < baseInterval { + return fmt.Errorf("maxInterval (%v) must be >= baseInterval (%v)", + maxInterval, baseInterval) + } + if maxRetries > 100 { + return fmt.Errorf("maxRetries (%d) exceeds maximum allowed (100)", maxRetries) + } + start := gotime.Now()
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
pkg/webhook/retry.go(1 hunks)
🔇 Additional comments (4)
pkg/webhook/retry.go (4)
17-27: LGTM! Clean package structure and imports.
The package structure is well-organized with clear naming and necessary imports. Good practice using the gotime alias to avoid confusion with the time package.
29-36: LGTM! Well-defined error types.
The error variables are well-documented and follow Go conventions. They provide clear distinction between different error scenarios.
83-91: LGTM! Correct implementation of exponential backoff calculation.
The waitInterval function correctly implements exponential backoff with proper boundary checking.
93-106: Enhance error handling in shouldRetry function.
While the current implementation covers basic scenarios, consider enhancing it with:
-
Additional network-related errors:
syscall.ETIMEDOUTsyscall.ECONNREFUSEDsyscall.ENETUNREACH
-
Common HTTP client errors that might warrant retries:
http.StatusBadGateway(502)http.StatusNotExtended(510)
Here's a suggested improvement:
func shouldRetry(statusCode int, err error) bool {
var errno syscall.Errno
if errors.As(err, &errno) {
- return errno == syscall.ECONNRESET
+ switch errno {
+ case syscall.ECONNRESET,
+ syscall.ETIMEDOUT,
+ syscall.ECONNREFUSED,
+ syscall.ENETUNREACH:
+ return true
+ }
}
return statusCode == http.StatusInternalServerError ||
statusCode == http.StatusServiceUnavailable ||
statusCode == http.StatusGatewayTimeout ||
- statusCode == http.StatusTooManyRequests
+ statusCode == http.StatusTooManyRequests ||
+ statusCode == http.StatusBadGateway ||
+ statusCode == http.StatusNotExtended
}Note: This comment partially addresses the previous review comment about 4xx handling.
There was a problem hiding this comment.
Actionable comments posted: 9
🧹 Nitpick comments (8)
server/backend/database/project_info_test.go (1)
37-37: Consider using more realistic test data.While the test structure is correct, consider using more meaningful test values:
- Replace "testEvent" with actual event types (e.g., "document.created", "document.removed")
- Use a valid URL format for testURL (e.g., "https://api.example.com/webhooks")
-testURL := "testUrl" -testEvents := []string{"testEvent"} +testURL := "https://api.example.com/webhooks" +testEvents := []string{"document.created", "document.removed"}Also applies to: 50-54
api/converter/from_pb.go (1)
57-58: Add validation for webhook URL format.The
EventWebhookURLis assigned directly from the protobuf field without validating the URL format. Consider adding URL validation to prevent invalid webhook configurations.+import "net/url" + func FromProject(pbProject *api.Project) *types.Project { + var eventWebhookURL string + if pbProject.EventWebhookUrl != "" { + if _, err := url.Parse(pbProject.EventWebhookUrl); err == nil { + eventWebhookURL = pbProject.EventWebhookUrl + } + } return &types.Project{ ID: types.ID(pbProject.Id), Name: pbProject.Name, AuthWebhookURL: pbProject.AuthWebhookUrl, AuthWebhookMethods: pbProject.AuthWebhookMethods, - EventWebhookURL: pbProject.EventWebhookUrl, + EventWebhookURL: eventWebhookURL, EventWebhookEvents: pbProject.EventWebhookEvents,server/backend/database/testcases/testcases.go (1)
841-876: Add test cases for invalid webhook URLs.The test suite should include cases for invalid webhook URLs and empty event arrays to ensure proper validation.
// 04. Update eventWebhookURL test newEventWebhookURL2 := newEventWebhookURL + "2" + invalidURL := "not-a-url" fields = &types.UpdatableProjectFields{ EventWebhookURL: &newEventWebhookURL2, } assert.NoError(t, fields.Validate()) res, err = db.UpdateProjectInfo(ctx, dummyOwnerID, id, fields) assert.NoError(t, err) + + // Test invalid URL + fields = &types.UpdatableProjectFields{ + EventWebhookURL: &invalidURL, + } + assert.Error(t, fields.Validate())api/types/project.go (1)
83-100: Consider adding validation for webhook URL format.The
RequireEventWebhookmethod could benefit from URL validation to ensure the webhook URL is properly formatted.func (p *Project) RequireEventWebhook(event ProjectEvent) bool { if len(p.EventWebhookURL) == 0 { return false } + if _, err := url.Parse(p.EventWebhookURL); err != nil { + return false + } if len(p.EventWebhookEvents) == 0 { return false }server/config.go (1)
68-71: Consider increasing the base wait interval.The base wait interval (3000ms) equals the max wait interval, which eliminates the exponential backoff effect. Consider using a smaller base interval.
- DefaultEventWebhookBaseWaitInterval = 3000 * time.Millisecond - DefaultEventWebhookMaxWaitInterval = 3000 * time.Millisecond + DefaultEventWebhookBaseWaitInterval = 100 * time.Millisecond + DefaultEventWebhookMaxWaitInterval = 3000 * time.Millisecondserver/backend/config.go (1)
135-157: Add validation for interval relationships.Consider adding validation to ensure:
- Base wait interval is less than max wait interval
- Request timeout is reasonable (e.g., not longer than max wait interval)
if _, err := time.ParseDuration(c.EventWebhookMaxWaitInterval); err != nil { return fmt.Errorf( `invalid argument "%s" for "--event-webhook-max-wait-interval" flag: %w`, c.EventWebhookMaxWaitInterval, err, ) } + +baseWait, err := time.ParseDuration(c.EventWebhookBaseWaitInterval) +if err != nil { + return fmt.Errorf( + `invalid argument "%s" for "--event-webhook-base-wait-interval" flag: %w`, + c.EventWebhookBaseWaitInterval, + err, + ) +} + +maxWait, err := time.ParseDuration(c.EventWebhookMaxWaitInterval) +if err != nil { + return fmt.Errorf( + `invalid argument "%s" for "--event-webhook-max-wait-interval" flag: %w`, + c.EventWebhookMaxWaitInterval, + err, + ) +} + +if baseWait >= maxWait { + return fmt.Errorf( + "base wait interval (%s) must be less than max wait interval (%s)", + c.EventWebhookBaseWaitInterval, + c.EventWebhookMaxWaitInterval, + ) +}cmd/yorkie/server.go (1)
336-359: Enhance help messages with examples.Consider adding examples to the help messages to make the configuration more user-friendly.
cmd.Flags().Uint64Var( &conf.Backend.EventWebhookMaxRetries, "project-event-webhook-max-retries", server.DefaultEventWebhookMaxRetries, - "Maximum number of retries for a project webhook.", + "Maximum number of retries for a project webhook. Example: 3", ) cmd.Flags().DurationVar( &eventWebhookBaseWaitInterval, "project-event-webhook-base-wait-interval", server.DefaultEventWebhookBaseWaitInterval, - "Base wait interval for retrying exponential backoff the project webhook.", + "Base wait interval for retrying exponential backoff the project webhook. Example: 1s", ) cmd.Flags().DurationVar( &eventWebhookMaxWaitInterval, "project-event-webhook-max-wait-interval", server.DefaultEventWebhookMaxWaitInterval, - "Maximum wait interval for project webhook.", + "Maximum wait interval for project webhook. Example: 1m", ) cmd.Flags().DurationVar( &eventWebhookRequestTimeout, "project-event-webhook-request-timeout", server.DefaultEventWebhookRequestTimeout, - "Time to wait for a response from the project webhook.", + "Time to wait for a response from the project webhook. Example: 5s", )cmd/yorkie/project/update.go (1)
186-218: LGTM! Well-implemented string slice update utility.The function efficiently handles string slice updates with proper deduplication and special case handling.
Consider adding sorting to the output slice for consistent ordering:
updated := make([]string, 0, len(stringSet)) for s := range stringSet { updated = append(updated, s) } + sort.Strings(updated) return updated
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (1)
api/yorkie/v1/resources.pb.gois excluded by!**/*.pb.go
📒 Files selected for processing (20)
api/converter/from_pb.go(2 hunks)api/converter/to_pb.go(1 hunks)api/docs/yorkie/v1/admin.openapi.yaml(58 hunks)api/docs/yorkie/v1/cluster.openapi.yaml(7 hunks)api/docs/yorkie/v1/resources.openapi.yaml(67 hunks)api/docs/yorkie/v1/yorkie.openapi.yaml(52 hunks)api/types/project.go(2 hunks)api/types/project_event.go(1 hunks)api/types/updatable_project_fields.go(3 hunks)api/yorkie/v1/resources.proto(1 hunks)cmd/yorkie/project/update.go(6 hunks)cmd/yorkie/server.go(3 hunks)server/backend/config.go(3 hunks)server/backend/config_test.go(2 hunks)server/backend/database/project_info.go(4 hunks)server/backend/database/project_info_test.go(2 hunks)server/backend/database/testcases/testcases.go(6 hunks)server/config.go(2 hunks)server/config_test.go(2 hunks)test/helper/helper.go(2 hunks)
✅ Files skipped from review due to trivial changes (2)
- api/docs/yorkie/v1/yorkie.openapi.yaml
- api/docs/yorkie/v1/admin.openapi.yaml
🚧 Files skipped from review as they are similar to previous changes (1)
- api/types/project_event.go
⏰ Context from checks skipped due to timeout of 90000ms (3)
- GitHub Check: build
- GitHub Check: bench
- GitHub Check: complex-test
🔇 Additional comments (19)
server/backend/config_test.go (2)
34-36: LGTM! Configuration values are well-structured.The new event webhook configuration fields follow the established pattern and use standard duration formats.
58-71: LGTM! Validation test cases are comprehensive.The test cases properly verify invalid duration formats for all new event webhook fields.
api/types/updatable_project_fields.go (2)
42-46: LGTM! Field documentation is clear and consistent.The new event webhook fields are well-documented and follow the established pattern.
86-104: Verify the IsProjectEvent function implementation.The validation logic references
IsProjectEvent, but its implementation is not visible in the current context. Please ensure it properly validates all supported event types.✅ Verification successful
IsProjectEvent implementation is correct and complete
The function properly validates against two supported events:
DocumentCreatedandDocumentRemoved. The implementation is type-safe and consistently used across webhook-related functionality.🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Search for IsProjectEvent implementation ast-grep --pattern 'func IsProjectEvent($_) bool { $$$ }'Length of output: 443
Script:
#!/bin/bash # Search for ProjectEvents implementation and related type definitions ast-grep --pattern 'func ProjectEvents() $_$ { $$$ }' # Search for type definitions and constants ast-grep --pattern 'type ProjectEvent string' # Search for const block containing project events ast-grep --pattern 'const ( $$$ $_$ ProjectEvent = $_$ $$$ )'Length of output: 230
Script:
#!/bin/bash # Read the entire project_event.go file cat api/types/project_event.go # Alternative search for const declarations ast-grep --pattern 'const $_$ ProjectEvent = $_$'Length of output: 1744
server/config_test.go (1)
82-92: LGTM! Configuration tests are thorough.The tests properly verify all new event webhook configuration parameters against their default values, with appropriate error handling.
server/backend/database/testcases/testcases.go (1)
770-774: LGTM! Good test setup for event webhook fields.The test data setup is clear and includes both URL and events array, providing good coverage for the new fields.
api/types/project.go (1)
41-46: LGTM! Well-documented fields with clear purpose.The new fields are properly documented and follow the existing pattern of the codebase.
server/config.go (1)
201-215: LGTM! Proper initialization of event webhook configuration.The configuration initialization follows the established pattern and includes all necessary fields.
server/backend/config.go (1)
76-87: LGTM! Well-structured webhook configuration.The new event webhook configuration fields are well-designed, covering essential aspects:
- Retry mechanism with max retries
- Exponential backoff with base and max intervals
- Request timeout
api/yorkie/v1/resources.proto (1)
311-320: LGTM! Well-structured updatable fields.The event webhook fields in
UpdatableProjectFieldsare well-designed:
- Consistent with the
Projectmessage structure- Follows the pattern of other updatable fields
- Uses appropriate wrapper types for optional fields
cmd/yorkie/server.go (1)
53-56: LGTM! Well-defined configuration variables.The event webhook configuration variables are properly defined with appropriate types and consistent naming.
server/backend/database/project_info.go (4)
60-64: LGTM! Well-structured field additions.The new webhook fields are properly documented and follow the existing patterns for field naming and BSON tags.
103-104: LGTM! Proper deep copy implementation.The new fields are correctly included in the deep copy operation. Since these are string types, direct assignment is safe.
122-127: LGTM! Safe field update implementation.The implementation properly checks for nil values before updating the fields, preventing accidental field clearing.
141-142: LGTM! Proper type conversion.The new fields are correctly included in the conversion to the Project type.
cmd/yorkie/project/update.go (2)
40-42: LGTM! Well-defined flag variables and event types.The new flag variables follow the existing naming convention, and the event types are clearly defined.
Also applies to: 58-61
246-263: LGTM! Well-documented flag initialization.The new flags are properly initialized with clear descriptions that indicate special values like 'ALL'.
test/helper/helper.go (1)
73-84: LGTM! Well-defined test configuration.The new event webhook configuration variables are consistent with existing timeout patterns and have reasonable test values.
api/converter/to_pb.go (1)
564-576: LGTM! Proper protobuf conversion implementation.The new fields are correctly converted to protobuf format with proper nil checks and wrapper types.
| if pbProjectFields.EventWebhookUrl != nil { | ||
| updatableProjectFields.EventWebhookURL = &pbProjectFields.EventWebhookUrl.Value | ||
| } | ||
| if pbProjectFields.EventWebhookEvents != nil { | ||
| updatableProjectFields.EventWebhookEvents = &pbProjectFields.EventWebhookEvents.Events | ||
| } |
There was a problem hiding this comment.
Add null pointer check before dereferencing.
The code directly dereferences Value and Events fields without checking if EventWebhookUrl or EventWebhookEvents is nil, which could lead to a panic.
if pbProjectFields.EventWebhookUrl != nil {
+ if pbProjectFields.EventWebhookUrl.Value != "" {
+ if _, err := url.Parse(pbProjectFields.EventWebhookUrl.Value); err == nil {
updatableProjectFields.EventWebhookURL = &pbProjectFields.EventWebhookUrl.Value
+ }
+ }
}
if pbProjectFields.EventWebhookEvents != nil {
+ if len(pbProjectFields.EventWebhookEvents.Events) > 0 {
updatableProjectFields.EventWebhookEvents = &pbProjectFields.EventWebhookEvents.Events
+ }
}📝 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 pbProjectFields.EventWebhookUrl != nil { | |
| updatableProjectFields.EventWebhookURL = &pbProjectFields.EventWebhookUrl.Value | |
| } | |
| if pbProjectFields.EventWebhookEvents != nil { | |
| updatableProjectFields.EventWebhookEvents = &pbProjectFields.EventWebhookEvents.Events | |
| } | |
| if pbProjectFields.EventWebhookUrl != nil { | |
| if pbProjectFields.EventWebhookUrl.Value != "" { | |
| if _, err := url.Parse(pbProjectFields.EventWebhookUrl.Value); err == nil { | |
| updatableProjectFields.EventWebhookURL = &pbProjectFields.EventWebhookUrl.Value | |
| } | |
| } | |
| } | |
| if pbProjectFields.EventWebhookEvents != nil { | |
| if len(pbProjectFields.EventWebhookEvents.Events) > 0 { | |
| updatableProjectFields.EventWebhookEvents = &pbProjectFields.EventWebhookEvents.Events | |
| } | |
| } |
| // ParseProjectWebhookMaxWaitInterval returns max wait interval. | ||
| func (c *Config) ParseProjectWebhookMaxWaitInterval() time.Duration { | ||
| result, err := time.ParseDuration(c.EventWebhookMaxWaitInterval) | ||
| if err != nil { | ||
| fmt.Fprintln(os.Stderr, "parse project webhook max wait interval: %w", err) | ||
| os.Exit(1) | ||
| } | ||
|
|
||
| return result | ||
| } |
There was a problem hiding this comment.
Fix error message formatting.
The error logging statement uses the wrong format:
fmt.Fprintlndoesn't support%wverb- The error is passed as a separate argument
- fmt.Fprintln(os.Stderr, "parse project webhook max wait interval: %w", err)
+ fmt.Fprintf(os.Stderr, "parse project webhook max wait interval: %v\n", err)📝 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.
| // ParseProjectWebhookMaxWaitInterval returns max wait interval. | |
| func (c *Config) ParseProjectWebhookMaxWaitInterval() time.Duration { | |
| result, err := time.ParseDuration(c.EventWebhookMaxWaitInterval) | |
| if err != nil { | |
| fmt.Fprintln(os.Stderr, "parse project webhook max wait interval: %w", err) | |
| os.Exit(1) | |
| } | |
| return result | |
| } | |
| // ParseProjectWebhookMaxWaitInterval returns max wait interval. | |
| func (c *Config) ParseProjectWebhookMaxWaitInterval() time.Duration { | |
| result, err := time.ParseDuration(c.EventWebhookMaxWaitInterval) | |
| if err != nil { | |
| fmt.Fprintf(os.Stderr, "parse project webhook max wait interval: %v\n", err) | |
| os.Exit(1) | |
| } | |
| return result | |
| } |
| // ParseProjectWebhookBaseWaitInterval returns base wait interval. | ||
| func (c *Config) ParseProjectWebhookBaseWaitInterval() time.Duration { | ||
| result, err := time.ParseDuration(c.EventWebhookBaseWaitInterval) | ||
| if err != nil { | ||
| fmt.Fprintln(os.Stderr, "parse project webhook max wait interval: %w", err) | ||
| os.Exit(1) | ||
| } | ||
|
|
||
| return result | ||
| } | ||
|
|
||
| // ParseProjectWebhookTimeout returns timeout for request. | ||
| func (c *Config) ParseProjectWebhookTimeout() time.Duration { | ||
| result, err := time.ParseDuration(c.EventWebhookRequestTimeout) | ||
| if err != nil { | ||
| fmt.Fprintln(os.Stderr, "parse project webhook max wait interval: %w", err) | ||
| os.Exit(1) | ||
| } | ||
|
|
||
| return result | ||
| } |
There was a problem hiding this comment.
Fix incorrect error messages and formatting.
Both methods have incorrect error messages and formatting issues:
func (c *Config) ParseProjectWebhookBaseWaitInterval() time.Duration {
result, err := time.ParseDuration(c.EventWebhookBaseWaitInterval)
if err != nil {
- fmt.Fprintln(os.Stderr, "parse project webhook max wait interval: %w", err)
+ fmt.Fprintf(os.Stderr, "parse project webhook base wait interval: %v\n", err)
os.Exit(1)
}
return result
}
func (c *Config) ParseProjectWebhookTimeout() time.Duration {
result, err := time.ParseDuration(c.EventWebhookRequestTimeout)
if err != nil {
- fmt.Fprintln(os.Stderr, "parse project webhook max wait interval: %w", err)
+ fmt.Fprintf(os.Stderr, "parse project webhook timeout: %v\n", err)
os.Exit(1)
}
return result
}📝 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.
| // ParseProjectWebhookBaseWaitInterval returns base wait interval. | |
| func (c *Config) ParseProjectWebhookBaseWaitInterval() time.Duration { | |
| result, err := time.ParseDuration(c.EventWebhookBaseWaitInterval) | |
| if err != nil { | |
| fmt.Fprintln(os.Stderr, "parse project webhook max wait interval: %w", err) | |
| os.Exit(1) | |
| } | |
| return result | |
| } | |
| // ParseProjectWebhookTimeout returns timeout for request. | |
| func (c *Config) ParseProjectWebhookTimeout() time.Duration { | |
| result, err := time.ParseDuration(c.EventWebhookRequestTimeout) | |
| if err != nil { | |
| fmt.Fprintln(os.Stderr, "parse project webhook max wait interval: %w", err) | |
| os.Exit(1) | |
| } | |
| return result | |
| } | |
| // ParseProjectWebhookBaseWaitInterval returns base wait interval. | |
| func (c *Config) ParseProjectWebhookBaseWaitInterval() time.Duration { | |
| result, err := time.ParseDuration(c.EventWebhookBaseWaitInterval) | |
| if err != nil { | |
| fmt.Fprintf(os.Stderr, "parse project webhook base wait interval: %v\n", err) | |
| os.Exit(1) | |
| } | |
| return result | |
| } | |
| // ParseProjectWebhookTimeout returns timeout for request. | |
| func (c *Config) ParseProjectWebhookTimeout() time.Duration { | |
| result, err := time.ParseDuration(c.EventWebhookRequestTimeout) | |
| if err != nil { | |
| fmt.Fprintf(os.Stderr, "parse project webhook timeout: %v\n", err) | |
| os.Exit(1) | |
| } | |
| return result | |
| } |
| string event_webhook_url = 7; | ||
| repeated string event_webhook_events = 8; | ||
| string client_deactivate_threshold = 9; | ||
| google.protobuf.Timestamp created_at = 10; | ||
| google.protobuf.Timestamp updated_at = 11; |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Document valid event types.
The event_webhook_events field accepts any string value. Consider:
- Adding comments to document valid event types
- Using an enum to restrict values to valid event types (e.g.,
document_created,document_removed)
+// Valid event types for event_webhook_events:
+// - document_created: Triggered when a new document is created
+// - document_removed: Triggered when a document is removed
message Project {
string id = 1;
string name = 2;
string public_key = 3;
string secret_key = 4;
string auth_webhook_url = 5;
repeated string auth_webhook_methods = 6;
string event_webhook_url = 7;
repeated string event_webhook_events = 8;
string client_deactivate_threshold = 9;
google.protobuf.Timestamp created_at = 10;
google.protobuf.Timestamp updated_at = 11;
}📝 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.
| string event_webhook_url = 7; | |
| repeated string event_webhook_events = 8; | |
| string client_deactivate_threshold = 9; | |
| google.protobuf.Timestamp created_at = 10; | |
| google.protobuf.Timestamp updated_at = 11; | |
| // Valid event types for event_webhook_events: | |
| // - document_created: Triggered when a new document is created | |
| // - document_removed: Triggered when a document is removed | |
| message Project { | |
| string id = 1; | |
| string name = 2; | |
| string public_key = 3; | |
| string secret_key = 4; | |
| string auth_webhook_url = 5; | |
| repeated string auth_webhook_methods = 6; | |
| string event_webhook_url = 7; | |
| repeated string event_webhook_events = 8; | |
| string client_deactivate_threshold = 9; | |
| google.protobuf.Timestamp created_at = 10; | |
| google.protobuf.Timestamp updated_at = 11; | |
| } |
| eventWebhookEvents: | ||
| additionalProperties: false | ||
| description: "" | ||
| items: | ||
| type: string | ||
| title: event_webhook_events | ||
| type: array |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Add documentation and validation for event webhook events.
The eventWebhookEvents property lacks documentation about supported event types and validation constraints.
Add description and enum validation:
eventWebhookEvents:
additionalProperties: false
- description: ""
+ description: "List of event types that trigger the webhook. Supported values: documentCreated, documentRemoved"
items:
+ enum:
+ - documentCreated
+ - documentRemoved
type: string
title: event_webhook_events
type: array📝 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.
| eventWebhookEvents: | |
| additionalProperties: false | |
| description: "" | |
| items: | |
| type: string | |
| title: event_webhook_events | |
| type: array | |
| eventWebhookEvents: | |
| additionalProperties: false | |
| description: "List of event types that trigger the webhook. Supported values: documentCreated, documentRemoved" | |
| items: | |
| enum: | |
| - documentCreated | |
| - documentRemoved | |
| type: string | |
| title: event_webhook_events | |
| type: array |
| eventWebhookUrl: | ||
| additionalProperties: false | ||
| description: "" | ||
| title: event_webhook_url | ||
| type: string |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Add URL format validation for webhook endpoint.
The eventWebhookUrl property should validate the URL format to prevent configuration errors.
Add format validation:
eventWebhookUrl:
additionalProperties: false
- description: ""
+ description: "HTTPS URL endpoint where webhook events will be delivered"
+ format: uri
+ pattern: "^https://.+"
title: event_webhook_url
type: string📝 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.
| eventWebhookUrl: | |
| additionalProperties: false | |
| description: "" | |
| title: event_webhook_url | |
| type: string | |
| eventWebhookUrl: | |
| additionalProperties: false | |
| description: "HTTPS URL endpoint where webhook events will be delivered" | |
| format: uri | |
| pattern: "^https://.+" | |
| title: event_webhook_url | |
| type: string |
| eventWebhookEvents: | ||
| additionalProperties: false | ||
| description: "" | ||
| items: | ||
| type: string | ||
| title: event_webhook_events | ||
| type: array | ||
| eventWebhookUrl: | ||
| additionalProperties: false | ||
| description: "" | ||
| title: event_webhook_url | ||
| type: string |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Maintain consistency with cluster.openapi.yaml schema.
The Project schema in resources.openapi.yaml should maintain the same validation constraints and documentation as cluster.openapi.yaml.
Apply the same changes suggested for cluster.openapi.yaml to maintain consistency across API definitions.
| yorkie.v1.UpdatableProjectFields.EventWebhookEvents: | ||
| additionalProperties: false | ||
| description: "" | ||
| properties: | ||
| events: | ||
| additionalProperties: false | ||
| description: "" | ||
| items: | ||
| type: string | ||
| title: events | ||
| type: array | ||
| title: EventWebhookEvents | ||
| type: object |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Add validation for updatable event webhook events.
The EventWebhookEvents schema for updatable fields should include the same validation constraints as the base schema.
Add validation:
yorkie.v1.UpdatableProjectFields.EventWebhookEvents:
additionalProperties: false
- description: ""
+ description: "List of event types that trigger the webhook"
properties:
events:
additionalProperties: false
- description: ""
+ description: "Supported values: documentCreated, documentRemoved"
items:
+ enum:
+ - documentCreated
+ - documentRemoved
type: string
title: events
type: array📝 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.
| yorkie.v1.UpdatableProjectFields.EventWebhookEvents: | |
| additionalProperties: false | |
| description: "" | |
| properties: | |
| events: | |
| additionalProperties: false | |
| description: "" | |
| items: | |
| type: string | |
| title: events | |
| type: array | |
| title: EventWebhookEvents | |
| type: object | |
| yorkie.v1.UpdatableProjectFields.EventWebhookEvents: | |
| additionalProperties: false | |
| description: "List of event types that trigger the webhook" | |
| properties: | |
| events: | |
| additionalProperties: false | |
| description: "Supported values: documentCreated, documentRemoved" | |
| items: | |
| enum: | |
| - documentCreated | |
| - documentRemoved | |
| type: string | |
| title: events | |
| type: array | |
| title: EventWebhookEvents | |
| type: object |
| eventWebhookUrl: | ||
| $ref: '#/components/schemas/google.protobuf.StringValue' | ||
| additionalProperties: false | ||
| description: "" | ||
| title: event_webhook_url | ||
| type: object |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Add validation for updatable webhook URL.
The eventWebhookUrl field in updatable fields should include URL format validation.
Add validation:
eventWebhookUrl:
$ref: '#/components/schemas/google.protobuf.StringValue'
additionalProperties: false
- description: ""
+ description: "HTTPS URL endpoint where webhook events will be delivered"
+ format: uri
+ pattern: "^https://.+"
title: event_webhook_url
type: object📝 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.
| eventWebhookUrl: | |
| $ref: '#/components/schemas/google.protobuf.StringValue' | |
| additionalProperties: false | |
| description: "" | |
| title: event_webhook_url | |
| type: object | |
| eventWebhookUrl: | |
| $ref: '#/components/schemas/google.protobuf.StringValue' | |
| additionalProperties: false | |
| description: "HTTPS URL endpoint where webhook events will be delivered" | |
| format: uri | |
| pattern: "^https://.+" | |
| title: event_webhook_url | |
| type: object |
What this PR does / why we need it:
This PR implements document creation and document removal events for a new project event webhook. Currently, users who uses Yorkie as a SaaS are not always aware of document status changes. By introducing project-level events—rather than relying solely on doc-level events like
attachorpush-pull—we can more clearly represent the lifecycle of documents and make those events available to external systems.Which issue(s) this PR fixes: Fixes #1002
Special notes for your reviewer:
DocEvent, but instead, I'm introducing aProject Event. We already haveDocEventevents (DocumentChangedDocumentWatched,DocumentUnwatched,broadcast), and using the same naming might be confusing.attach,document-changed,watched,unwatched) don't clearly convey CRUD semantics. For instances:documentAttachedalone doesn't explicitly indicate creation.push-pullmay imply either reading or updating a document, and it's not always obvious which. (AChangecan contain bothpresenceandoperations; if it only haspresence, it might not mean a real update of document's content.)removeDocumentfunction only marks a document as "removed" without fully deleting it. In contrast, theremoveDocumentByAdminfunction permanently deletes the document. When aremoveDocumentaction occurs, Yorkie sends webhook events, allowing the customer's server to decide the next steps—such as moving the document to a recycle bin or directly deleting it usingremoveDocumentByAdmin.X-Signatureheader, generated using the project's secret key. The receiving server can verify this signature to ensure the request is from Yorkie.projectKeyin our request attributes.Handling timeouts and unreachable events
[immediately, 5s, 5m, 30m, 2h, 5h, 10h], etc.).Add an HTTP client with timeout
http.POSTdoes not allow specifying a request timeout directly, using an HTTP client with a configurable timeout is more predictable. Although the auth webhook still useshttp.POST, we may consider migrating it to an HTTP client for consistency.In this PR
This design proposal differs somewhat from the original issue’s requirements. If the idea seems sound, I’ll proceed with the remaining tasks:
event_webhooktestsUpdateProjectRequestAPI specpkg/webhookIn a subsequent PR
document-changedevent (with caching)Does this PR introduce a user-facing change?
Additional documentation
There are four configuration options related to timeouts, which can be confusing. Below is a brief explanation:
Checklist
Summary by CodeRabbit
New Features
Improvements