Introduce project event webhook #1131
Conversation
Co-authored-by: coderabbitai[bot] <136622811+coderabbitai[bot]@users.noreply.github.com>
WalkthroughThis pull request introduces comprehensive support for project event webhooks in the Yorkie system. The changes span multiple files and packages, adding new configuration options, data structures, and methods to handle webhook events for document creation and removal. The implementation allows projects to configure a webhook URL and specify which events should trigger the webhook, providing a flexible mechanism for external services to receive notifications about document lifecycle events. Changes
Sequence DiagramsequenceDiagram
participant Client
participant YorkieServer
participant WebhookEndpoint
alt Document Creation
Client->>YorkieServer: AttachDocument
YorkieServer->>YorkieServer: Check Project Webhook Config
YorkieServer->>WebhookEndpoint: Send DocumentCreated Webhook
end
alt Document Removal
Client->>YorkieServer: RemoveDocument
YorkieServer->>YorkieServer: Check Project Webhook Config
YorkieServer->>WebhookEndpoint: Send DocumentRemoved Webhook
end
Possibly related issues
Possibly related PRs
Suggested reviewers
✨ Finishing Touches
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: 8
🔭 Outside diff range comments (1)
api/docs/yorkie/v1/cluster.openapi.yaml (1)
Line range hint
313-316: Add HMAC signature security scheme.The PR objectives mention HMAC signature authentication in the
X-Signatureheader, but it's not defined in the security schemes.Add the HMAC signature security scheme:
ApiKeyAuth: in: header name: Authorization type: apiKey + HmacSignature: + type: apiKey + in: header + name: X-Signature + description: "HMAC SHA-256 signature of the request payload using the project's secret key"
🧹 Nitpick comments (13)
api/types/project_event.go (2)
20-27: Duplicate comment for ProjectEvent type.The comment "ProjectEvent represents the event of the project." appears twice - once for the type and once for the constants block.
Apply this diff to fix the duplicate comment:
-// ProjectEvent represents the event of the project. +// List of supported project events. const (
29-37: Consider using a map for O(1) lookup.The current implementation iterates through all events for each check. For better performance, consider using a map when dealing with a larger set of events.
+var projectEventSet = map[string]bool{ + string(DocumentCreated): true, + string(DocumentRemoved): true, +} func IsProjectEvent(event string) bool { - for _, e := range ProjectEvents() { - if event == string(e) { - return true - } - } - return false + return projectEventSet[event] }server/backend/config_test.go (1)
58-71: Add test cases for invalid duration formats.The current test cases only verify basic invalid formats. Consider adding more edge cases for duration validation.
Add test cases for:
- Negative durations
- Zero durations
- Very large durations
- Invalid units (e.g., "10y")
server/backend/database/project_info_test.go (1)
37-37: Enhance test coverage for event webhook fields.The current tests only verify basic field updates. Consider adding test cases for:
- Invalid webhook URLs
- Invalid event types
- Empty event lists
- Duplicate events
Add test cases:
// Test invalid webhook URL invalidURL := "not-a-url" project.UpdateFields(&types.UpdatableProjectFields{EventWebhookURL: &invalidURL}) assert.Equal(t, invalidURL, project.EventWebhookURL) // Verify storage // URL validation should happen at the API level // Test invalid event type invalidEvents := []string{"InvalidEvent"} project.UpdateFields(&types.UpdatableProjectFields{EventWebhookEvents: &invalidEvents}) assert.Equal(t, invalidEvents, project.EventWebhookEvents) // Verify storage // Event validation should happen at the API levelAlso applies to: 50-54
pkg/webhook/retry.go (1)
38-81: Consider adding jitter to the exponential backoff.While the implementation is solid, adding jitter to the backoff intervals would help prevent the "thundering herd" problem when multiple clients retry simultaneously.
func waitInterval(retries uint64, baseInterval, maxWaitInterval gotime.Duration) gotime.Duration { interval := gotime.Duration(math.Pow(2, float64(retries))) * baseInterval + // Add jitter by multiplying by a random value between 0.5 and 1.5 + jitter := 0.5 + rand.Float64() + interval = gotime.Duration(float64(interval) * jitter) if maxWaitInterval < interval { return maxWaitInterval } return interval }api/types/updatable_project_fields.go (1)
86-104: Consider adding error details to invalid event messages.While the validation is correct, the error message could be more helpful by including the list of valid events.
- if err := validation.RegisterTranslation("invalid_event_webhook_events", "given {0} is invalid event"); err != nil { + validEvents := strings.Join(GetValidProjectEvents(), ", ") + if err := validation.RegisterTranslation("invalid_event_webhook_events", fmt.Sprintf("given {0} is invalid event. Valid events are: %s", validEvents)); err != nil { fmt.Fprintln(os.Stderr, "updatable project fields: ", err) os.Exit(1) }server/rpc/projectevent/webhook.go (3)
48-48: Consider using RFC3339 format for timestamp.The
IssuedAtfield usestime.Now().String()which may not be ideal for webhook consumers.- IssuedAt: gotime.Now().String(), + IssuedAt: gotime.Now().UTC().Format(time.RFC3339),
58-60: Consider implementing event recovery mechanism.The comment suggests implementing event recovery through the dashboard. This is a good idea for handling failed events.
Consider implementing:
- Event persistence for failed deliveries
- Dashboard UI for viewing and manually retrying failed events
- Automated retry queue for failed events
118-122: Consider logging response body for non-2xx responses.When receiving non-2xx responses, it would be helpful to log the response body for debugging purposes.
if resp.StatusCode != http.StatusOK && resp.StatusCode != http.StatusUnauthorized && resp.StatusCode != http.StatusForbidden { + body, _ := io.ReadAll(resp.Body) + logging.From(ctx).With( + "status_code", resp.StatusCode, + "response_body", string(body), + ).Error("unexpected webhook response") return resp.StatusCode, webhook.ErrUnexpectedStatusCode }cmd/yorkie/project/update.go (1)
246-263: Consider improving flag descriptions.The flag descriptions could be more descriptive to help users understand the expected values and format.
- "project-event-webhook update url", + "URL endpoint where project events will be sent (e.g., https://example.com/webhook)", - "project-event-webhook methods to add ('ALL' for all events)", + "Event types to subscribe to. Use 'ALL' for all events or specify individual events: document-created, document-removed", - "project-event-webhook events to remove ('ALL' for all events)", + "Event types to unsubscribe from. Use 'ALL' to remove all events or specify individual events to remove",server/config.go (1)
68-71: Consider adding comments explaining configuration values.The default values would benefit from comments explaining why these specific values were chosen.
+// DefaultEventWebhookMaxRetries is set to 10 to provide sufficient retry attempts while avoiding excessive load const DefaultEventWebhookMaxRetries = 10 +// DefaultEventWebhookBaseWaitInterval is set to 3s to provide a reasonable initial delay between retries const DefaultEventWebhookBaseWaitInterval = 3000 * time.Millisecond +// DefaultEventWebhookMaxWaitInterval caps the maximum delay to 3s to ensure timely delivery const DefaultEventWebhookMaxWaitInterval = 3000 * time.Millisecond +// DefaultEventWebhookRequestTimeout is set to 10s to allow for slower webhook endpoints while preventing hanging requests const DefaultEventWebhookRequestTimeout = 10 * time.Secondserver/backend/config.go (1)
135-157: Consider consolidating duplicate validation logic.The validation logic for duration parsing is repeated for each webhook configuration field. Consider extracting this into a helper function to improve maintainability.
+func validateDuration(value, flagName string) error { + if _, err := time.ParseDuration(value); err != nil { + return fmt.Errorf( + `invalid argument "%s" for "--%s" flag: %w`, + value, + flagName, + err, + ) + } + return nil +} func (c *Config) Validate() error { - if _, err := time.ParseDuration(c.EventWebhookBaseWaitInterval); err != nil { - return fmt.Errorf( - `invalid argument "%s" for "--event-webhook-base-wait-interval" flag: %w`, - c.EventWebhookBaseWaitInterval, - err, - ) - } + if err := validateDuration(c.EventWebhookBaseWaitInterval, "event-webhook-base-wait-interval"); err != nil { + return err + }test/helper/helper.go (1)
73-85: Consider using constants for magic numbers in test configurations.The test configuration values use magic numbers. Consider defining these as named constants for better maintainability.
+const ( + defaultTestTimeout = 10 * gotime.Second + defaultTestWaitInterval = 3 * gotime.Millisecond +) -AdminTokenDuration = "10s" -ClientDeactivateThreshold = "10s" -AuthWebhookCacheAuthTTL = 10 * gotime.Second -AuthWebhookCacheUnauthTTL = 10 * gotime.Second -EventWebhookRequestTimeout = 10 * gotime.Second +AdminTokenDuration = defaultTestTimeout.String() +ClientDeactivateThreshold = defaultTestTimeout.String() +AuthWebhookCacheAuthTTL = defaultTestTimeout +AuthWebhookCacheUnauthTTL = defaultTestTimeout +EventWebhookRequestTimeout = defaultTestTimeout
📜 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 (24)
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)pkg/webhook/client.go(1 hunks)pkg/webhook/retry.go(1 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)server/rpc/projectevent/webhook.go(1 hunks)server/rpc/yorkie_server.go(3 hunks)test/helper/helper.go(2 hunks)
✅ Files skipped from review due to trivial changes (3)
- api/docs/yorkie/v1/yorkie.openapi.yaml
- api/docs/yorkie/v1/resources.openapi.yaml
- api/docs/yorkie/v1/admin.openapi.yaml
⏰ Context from checks skipped due to timeout of 90000ms (3)
- GitHub Check: complex-test
- GitHub Check: bench
- GitHub Check: build
🔇 Additional comments (29)
server/backend/config_test.go (1)
34-36: Verify interval relationships.The test configuration shows:
EventWebhookBaseWaitInterval: "10ms"EventWebhookMaxWaitInterval: "10ms"These values being equal might not test the expected relationship where max interval should be greater than base interval.
Consider adding test cases to verify the relationship between intervals:
conf9 := validConf conf9.EventWebhookMaxWaitInterval = "5ms" // Less than base interval assert.Error(t, conf9.Validate(), "max wait interval should be greater than base interval")api/types/project.go (2)
41-45: LGTM! Well-documented fields for event webhook configuration.The new fields are properly documented and follow the existing pattern in the struct.
83-100: LGTM! Clean implementation of RequireEventWebhook.The method follows the same pattern as RequireAuth, making it consistent and easy to understand. It properly checks both the URL and events list before determining if a webhook is required.
pkg/webhook/retry.go (2)
29-36: LGTM! Clear error definitions.The error variables are well-defined and properly exported for external use.
93-106: LGTM! Well-documented retry conditions.The shouldRetry function follows Kubernetes patterns and handles appropriate HTTP status codes for retries.
api/types/updatable_project_fields.go (2)
42-46: LGTM! Well-structured field additions.The new fields follow the existing pattern and include proper validation tags.
54-59: LGTM! Comprehensive null check in Validate method.The validation logic properly includes the new fields in the emptiness check.
server/config_test.go (1)
64-64: LGTM! Comprehensive test coverage for event webhook configuration.The test cases properly verify all new configuration parameters including max retries, base wait interval, max wait interval, and request timeout.
Also applies to: 82-92
server/rpc/projectevent/webhook.go (1)
94-96: LGTM: Good use of configuration for timeouts.The implementation properly uses configurable timeouts from the config, which is a good practice for webhook clients.
server/backend/database/project_info.go (1)
60-64: LGTM: Clean integration of new webhook fields.The new fields are well-documented and follow the existing pattern of the codebase.
cmd/yorkie/project/update.go (1)
186-218: LGTM: Robust string slice update implementation.The
updateStringSlicefunction is well-implemented with proper handling of the "ALL" special case.server/config.go (1)
201-215: LGTM: Proper default value handling.The implementation correctly ensures default values are set when not provided in the configuration.
server/backend/config.go (1)
76-86: LGTM! Well-structured configuration fields for event webhooks.The new configuration fields are well-documented and follow the existing pattern. The YAML tags are correctly defined.
cmd/yorkie/server.go (3)
50-56: LGTM! Well-organized variable declarations.The new event webhook variables are properly declared alongside related configuration variables.
73-75: LGTM! Proper configuration assignments.The event webhook configurations are correctly assigned to the backend configuration.
336-359: LGTM! Well-documented command-line flags.The new command-line flags are well-documented with clear descriptions and proper default values.
test/helper/helper.go (1)
261-279: LGTM! Proper test configuration setup.The test configuration is properly set up with all required fields.
api/converter/to_pb.go (1)
564-575: LGTM! Event webhook fields are handled correctly.The implementation follows the established pattern for handling optional fields in the
ToUpdatableProjectFieldsfunction, using appropriate wrapper types and handling nil cases correctly.api/converter/from_pb.go (2)
57-58: LGTM! Event webhook fields are correctly mapped in FromProject.The implementation correctly maps the event webhook fields from the Protobuf format to the model format.
926-931: LGTM! Event webhook fields are handled correctly in FromUpdatableProjectFields.The implementation follows the established pattern for handling optional fields, checking for nil before setting the values.
server/backend/database/testcases/testcases.go (5)
770-774: LGTM! Test data is well-defined.The test data for event webhook fields is properly defined with realistic values.
789-790: LGTM! Test case covers updating all fields.The test case verifies that event webhook fields can be updated along with other fields.
802-803: LGTM! Test assertions are comprehensive.The test assertions verify that event webhook fields are correctly updated and persisted.
841-858: LGTM! Test case covers individual event webhook URL updates.The test case verifies that the event webhook URL can be updated independently while preserving other fields.
859-876: LGTM! Test case covers individual event webhook events updates.The test case verifies that the event webhook events can be updated independently while preserving other fields.
api/yorkie/v1/resources.proto (3)
300-304: LGTM! Project message fields are well-defined.The event webhook fields are correctly added to the
Projectmessage with appropriate field numbers and types.
311-313: LGTM! EventWebhookEvents message is well-defined.The
EventWebhookEventsmessage is correctly defined with a repeated string field for events.
318-320: LGTM! UpdatableProjectFields message fields are well-defined.The event webhook fields are correctly added to the
UpdatableProjectFieldsmessage with appropriate field numbers and wrapper types.api/docs/yorkie/v1/cluster.openapi.yaml (1)
3-11: Consider updating the version number.Given the significant addition of webhook functionality, consider if the version number (v0.5.8) should be incremented according to your versioning strategy.
| // WebhookAttribute represents the attribute of the webhook. | ||
| type WebhookAttribute struct { | ||
| DocumentKey string `json:"documentKey"` | ||
| ClientKey string `json:"clientKey"` | ||
| IssuedAt string `json:"issuedAt"` | ||
| } |
There was a problem hiding this comment.
💡 Codebase verification
Use the internal validation framework for WebhookAttribute validation
The codebase consistently uses an internal validation framework. Instead of implementing a standalone validation method, add field validation tags and use validation.ValidateStruct():
type WebhookAttribute struct {
DocumentKey string `json:"documentKey" validate:"required"`
ClientKey string `json:"clientKey" validate:"required"`
IssuedAt string `json:"issuedAt" validate:"required,datetime=2006-01-02T15:04:05Z07:00"`
}
func (w *WebhookAttribute) Validate() error {
return validation.ValidateStruct(w)
}🔗 Analysis chain
Add validation for WebhookAttribute fields.
The WebhookAttribute struct should include validation for required fields and proper time format for IssuedAt.
Consider adding a validation method:
func (w *WebhookAttribute) Validate() error {
if w.DocumentKey == "" {
return errors.New("documentKey is required")
}
if w.ClientKey == "" {
return errors.New("clientKey is required")
}
if _, err := time.Parse(time.RFC3339, w.IssuedAt); err != nil {
return fmt.Errorf("invalid issuedAt format: %w", err)
}
return nil
}🏁 Scripts executed
The following scripts were executed for the analysis:
Error: No shell scripts found
Script:
#!/bin/bash
# Check for existing validation methods for WebhookAttribute
echo "=== Checking WebhookAttribute validation ==="
ast-grep --pattern 'func (w *WebhookAttribute) Validate() $_'
echo -e "\n=== Checking similar validation patterns ==="
# Look for other Validate methods to understand validation patterns
ast-grep --pattern 'func ($_ *$_) Validate() error'
echo -e "\n=== Checking time format usage ==="
# Search for time.Parse usage to understand time format conventions
rg "time\.Parse\(" -A 1
echo -e "\n=== Checking validation frameworks ==="
# Look for common validation imports
rg "validator|validation" --type go
Length of output: 16939
| func NewClient(timeout time.Duration, privateKey string) *http.Client { | ||
| return &http.Client{ | ||
| Timeout: timeout, | ||
| Transport: &HMACTransport{ | ||
| PrivateKey: privateKey, | ||
| }, | ||
| } | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Consider adding request retry mechanism.
The client should handle transient failures with retries, especially for webhook delivery.
Consider implementing retries with exponential backoff:
type RetryConfig struct {
MaxRetries int
BaseDelay time.Duration
MaxDelay time.Duration
}
func NewClientWithRetry(timeout time.Duration, privateKey string, retryConfig RetryConfig) *http.Client {
return &http.Client{
Timeout: timeout,
Transport: &RetryTransport{
Base: &HMACTransport{
PrivateKey: privateKey,
},
RetryConfig: retryConfig,
},
}
}| func (t *HMACTransport) RoundTrip(r *http.Request) (*http.Response, error) { | ||
| reqCopy := r.Clone(r.Context()) | ||
|
|
||
| rawBody, err := io.ReadAll(r.Body) | ||
| if err != nil { | ||
| return nil, fmt.Errorf("failed to read request: %w", err) | ||
| } | ||
| reqCopy.Body = io.NopCloser(bytes.NewBuffer(rawBody)) | ||
|
|
||
| signature := GenerateHMACSignature(t.PrivateKey, rawBody) | ||
| reqCopy.Header.Set("X-Signature", signature) | ||
|
|
||
| resp, err := http.DefaultTransport.RoundTrip(reqCopy) | ||
| if err != nil { | ||
| return nil, fmt.Errorf("failed to send request: %w", err) | ||
| } | ||
|
|
||
| return resp, nil | ||
| } |
There was a problem hiding this comment.
Potential memory leak in RoundTrip implementation.
The original request body is not being closed after reading.
Apply this diff to fix the memory leak:
func (t *HMACTransport) RoundTrip(r *http.Request) (*http.Response, error) {
reqCopy := r.Clone(r.Context())
rawBody, err := io.ReadAll(r.Body)
+ if r.Body != nil {
+ defer r.Body.Close()
+ }
if err != nil {
return nil, fmt.Errorf("failed to read request: %w", err)
}
reqCopy.Body = io.NopCloser(bytes.NewBuffer(rawBody))📝 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.
| func (t *HMACTransport) RoundTrip(r *http.Request) (*http.Response, error) { | |
| reqCopy := r.Clone(r.Context()) | |
| rawBody, err := io.ReadAll(r.Body) | |
| if err != nil { | |
| return nil, fmt.Errorf("failed to read request: %w", err) | |
| } | |
| reqCopy.Body = io.NopCloser(bytes.NewBuffer(rawBody)) | |
| signature := GenerateHMACSignature(t.PrivateKey, rawBody) | |
| reqCopy.Header.Set("X-Signature", signature) | |
| resp, err := http.DefaultTransport.RoundTrip(reqCopy) | |
| if err != nil { | |
| return nil, fmt.Errorf("failed to send request: %w", err) | |
| } | |
| return resp, nil | |
| } | |
| func (t *HMACTransport) RoundTrip(r *http.Request) (*http.Response, error) { | |
| reqCopy := r.Clone(r.Context()) | |
| rawBody, err := io.ReadAll(r.Body) | |
| if r.Body != nil { | |
| defer r.Body.Close() | |
| } | |
| if err != nil { | |
| return nil, fmt.Errorf("failed to read request: %w", err) | |
| } | |
| reqCopy.Body = io.NopCloser(bytes.NewBuffer(rawBody)) | |
| signature := GenerateHMACSignature(t.PrivateKey, rawBody) | |
| reqCopy.Header.Set("X-Signature", signature) | |
| resp, err := http.DefaultTransport.RoundTrip(reqCopy) | |
| if err != nil { | |
| return nil, fmt.Errorf("failed to send request: %w", err) | |
| } | |
| return resp, nil | |
| } |
| // 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 | ||
| } | ||
|
|
||
| // 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.
Error messages need correction in ParseProjectWebhook methods.
The error messages in ParseProjectWebhookBaseWaitInterval and ParseProjectWebhookTimeout incorrectly mention "max wait interval".
Apply this diff to fix the error messages:
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.Fprintln(os.Stderr, "parse project webhook base wait interval: %w", 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.Fprintln(os.Stderr, "parse project webhook request timeout: %w", 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.
| // 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 | |
| } | |
| // 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 | |
| } | |
| // 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 | |
| } | |
| // 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 base 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 request timeout: %w", err) | |
| os.Exit(1) | |
| } | |
| return result | |
| } |
| projectevent.DocumentRemoved(ctx, s.backend, docInfo.Key.String(), clientInfo.Key) | ||
|
|
There was a problem hiding this comment.
🛠️ Refactor suggestion
Add error handling for document removal event.
Similar to the document creation event, the document removal event should include error handling.
-projectevent.DocumentRemoved(ctx, s.backend, docInfo.Key.String(), clientInfo.Key)
+if err := projectevent.DocumentRemoved(ctx, s.backend, docInfo.Key.String(), clientInfo.Key); err != nil {
+ logging.From(ctx).Error(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.
| projectevent.DocumentRemoved(ctx, s.backend, docInfo.Key.String(), clientInfo.Key) | |
| if err := projectevent.DocumentRemoved(ctx, s.backend, docInfo.Key.String(), clientInfo.Key); err != nil { | |
| logging.From(ctx).Error(err) | |
| } | |
| if docInfo.ServerSeq == 0 { | ||
| projectevent.DocumentCreated(ctx, s.backend, docInfo.Key.String(), clientInfo.Key) | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Consider error handling for event webhook calls.
The DocumentCreated event is triggered without error handling. Consider handling potential errors to ensure system stability.
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).Error(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.
| if docInfo.ServerSeq == 0 { | |
| projectevent.DocumentCreated(ctx, s.backend, docInfo.Key.String(), clientInfo.Key) | |
| } | |
| if docInfo.ServerSeq == 0 { | |
| if err := projectevent.DocumentCreated(ctx, s.backend, docInfo.Key.String(), clientInfo.Key); err != nil { | |
| logging.From(ctx).Error(err) | |
| } | |
| } |
| paths: | ||
| /yorkie.v1.ClusterService/DetachDocument: | ||
| post: | ||
| description: "" | ||
| requestBody: | ||
| $ref: "#/components/requestBodies/yorkie.v1.ClusterService.DetachDocument.yorkie.v1.ClusterServiceDetachDocumentRequest" | ||
| $ref: '#/components/requestBodies/yorkie.v1.ClusterService.DetachDocument.yorkie.v1.ClusterServiceDetachDocumentRequest' | ||
| responses: | ||
| "200": | ||
| $ref: "#/components/responses/yorkie.v1.ClusterService.DetachDocument.yorkie.v1.ClusterServiceDetachDocumentResponse" | ||
| $ref: '#/components/responses/yorkie.v1.ClusterService.DetachDocument.yorkie.v1.ClusterServiceDetachDocumentResponse' | ||
| default: | ||
| $ref: "#/components/responses/connect.error" | ||
| $ref: '#/components/responses/connect.error' | ||
| tags: | ||
| - yorkie.v1.ClusterService | ||
| - yorkie.v1.ClusterService |
There was a problem hiding this comment.
Add webhook-related endpoint specifications.
The PR objectives mention webhook functionality and HMAC signature authentication, but the API specification is missing:
- Webhook configuration endpoints
- HMAC signature header definition (
X-Signature) - Webhook event payload schemas
Would you like me to help generate the missing API specifications for the webhook functionality?
Also applies to: 313-321
| 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
Enhance webhook property definitions.
The new webhook properties need more detailed specifications:
- Add descriptions explaining the purpose and usage of each property
- Add URL format validation for
eventWebhookUrl - Define allowed values for
eventWebhookEventsusing an enum
Apply this diff to enhance the property definitions:
eventWebhookEvents:
additionalProperties: false
- description: ""
+ description: "List of event types that trigger webhook notifications (e.g., document.created, document.removed)"
items:
- type: string
+ type: string
+ enum:
+ - document.created
+ - document.removed
title: event_webhook_events
type: array
eventWebhookUrl:
additionalProperties: false
- description: ""
+ description: "HTTPS URL that will receive webhook event notifications"
title: event_webhook_url
type: string
+ format: uri
+ pattern: "^https://"📝 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 | |
| eventWebhookUrl: | |
| additionalProperties: false | |
| description: "" | |
| title: event_webhook_url | |
| type: string | |
| eventWebhookEvents: | |
| additionalProperties: false | |
| description: "List of event types that trigger webhook notifications (e.g., document.created, document.removed)" | |
| items: | |
| type: string | |
| enum: | |
| - document.created | |
| - document.removed | |
| title: event_webhook_events | |
| type: array | |
| eventWebhookUrl: | |
| additionalProperties: false | |
| description: "HTTPS URL that will receive webhook event notifications" | |
| title: event_webhook_url | |
| type: string | |
| format: uri | |
| pattern: "^https://" |
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #1131 +/- ##
==========================================
- Coverage 46.82% 46.73% -0.10%
==========================================
Files 84 85 +1
Lines 12282 12417 +135
==========================================
+ Hits 5751 5803 +52
- Misses 5954 6035 +81
- Partials 577 579 +2 ☔ View full report in Codecov by Sentry. |
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
Based on the comprehensive changes, here are the release notes:
Release Notes
New Features
Improvements
Configuration
Documentation
These release notes summarize the key additions and improvements related to the new event webhook functionality across the project.