Add Event Webhook Support with DocRootChanged Event #1156
Conversation
- types package can't import events package
- add `EventWebhookURL` - add `EventWebhookTypes`
- add event webhook options to updatable_project_fields
- valid event webhook type is empty, not empty `""`
- RFC3339 to ISO 8601
WalkthroughThis change integrates support for event webhooks across the codebase. New fields and logic were added to convert, store, and validate event webhook configurations in both project and updatable project field structures. The API conversion functions, OpenAPI specifications, types, tests, and CLI commands were updated accordingly. In addition, backend configuration parameters, server flags, and webhook client functionality were extended to include event webhook handling, along with a new package responsible for sending event notifications. These modifications provide comprehensive support for event-driven webhook operations. Changes
Sequence Diagram(s)sequenceDiagram
participant D as Document Service
participant P as Packs Module
participant E as Event Webhook Sender
participant C as Webhook Client
participant W as Webhook Endpoint
D->>P: Trigger document change with operations
P->>E: Call SendEvent(docKey, eventType)
E->>C: Build request body with event webhook data
C->>W: POST request with JSON payload
W-->>C: Return HTTP response (status)
C-->>E: Forward status code
E-->>P: Return result of SendEvent
✨ 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: 4
🧹 Nitpick comments (18)
api/types/event_webhook.go (4)
1-2: Update the copyright year.The copyright year 2025 is incorrect as it's in the future. Consider updating it to the current year 2024.
-* Copyright 2025 The Yorkie Authors. All rights reserved. +* Copyright 2024 The Yorkie Authors. All rights reserved.
19-25: Enhance documentation for event types.Consider adding more comprehensive documentation:
- List all valid event types (currently only DocRootChanged)
- Add a note about potential future event types
- Document the string value mapping ("DocumentRootChanged")
// EventWebhookType represents event webhook type +// Currently supported event types: +// - DocRootChanged ("DocumentRootChanged"): Indicates document content modification +// +// Note: Additional event types may be added in future versions. type EventWebhookType string
27-30: Improve event type validation for future scalability.The current implementation works for a single event type but could be enhanced for better maintainability as more event types are added.
Consider using a map for O(1) lookup and type safety:
+var validEventTypes = map[EventWebhookType]bool{ + DocRootChanged: true, +} + // IsValidEventType checks whether the given event type is valid. -func IsValidEventType(eventType string) bool { - return eventType == string(DocRootChanged) +func IsValidEventType(eventType string) bool { + return validEventTypes[EventWebhookType(eventType)] +}
32-42: Add timestamp validation and improve documentation.The webhook structures look good, but consider these improvements:
- Document the expected format of
IssuedAt(e.g., ISO 8601)- Add validation for the timestamp format
// EventWebhookAttribute represents the attribute of the webhook. +// IssuedAt should be an ISO 8601 formatted timestamp (e.g., "2024-02-20T15:04:05Z") type EventWebhookAttribute struct { Key string `json:"key"` IssuedAt string `json:"issuedAt"` } +// ValidateTimestamp validates the IssuedAt timestamp format +func (a *EventWebhookAttribute) ValidateTimestamp() error { + _, err := time.Parse(time.RFC3339, a.IssuedAt) + if err != nil { + return fmt.Errorf("invalid timestamp format: %v", err) + } + return nil +}server/backend/config_test.go (1)
36-40: Consider adding edge case tests for webhook configurations.While the current tests cover basic validation, consider adding tests for:
- Maximum allowed durations
- Relative ordering (e.g., MaxWaitInterval > MinWaitInterval)
- Zero or negative durations
Example additional test cases:
conf11 := validConf conf11.EventWebhookMaxWaitInterval = "0s" conf11.EventWebhookMinWaitInterval = "1s" assert.Error(t, conf11.Validate(), "max wait interval should be greater than min wait interval") conf12 := validConf conf12.EventWebhookRequestTimeout = "-1s" assert.Error(t, conf12.Validate(), "negative duration should be invalid")Also applies to: 67-82
server/rpc/webhook/event.go (1)
77-93: Extract time format constant and improve error message.The function uses a hardcoded time format string and could have a more descriptive error message.
Apply this diff to improve the implementation:
+const ( + // ISO8601UTCFormat is the ISO 8601 format with millisecond precision in UTC + ISO8601UTCFormat = "2006-01-02T15:04:05.000Z" +) func buildEventWebhookBody(docKey string, webhookType types.EventWebhookType) ([]byte, error) { req := types.EventWebhookRequest{ Type: webhookType, Attributes: types.EventWebhookAttribute{ Key: docKey, - IssuedAt: gotime.Now().UTC().Format("2006-01-02T15:04:05.000Z"), + IssuedAt: gotime.Now().UTC().Format(ISO8601UTCFormat), }, } body, err := json.Marshal(req) if err != nil { - return nil, fmt.Errorf("marshal event webhook request: %w", err) + return nil, fmt.Errorf("failed to marshal event webhook request for doc %s: %w", docKey, err) } return body, nil }api/types/events/events.go (1)
50-58: Enhance method documentation.The
WebhookTypemethod would benefit from more detailed documentation explaining the mapping between event types and webhook types.Apply this diff to improve the documentation:
-// WebhookType returns a matched event webhook type. +// WebhookType returns the corresponding webhook type for the event. +// Currently, only DocRootChangedEvent maps to DocRootChanged webhook type. +// All other events return an empty string, indicating no webhook should be triggered. func (t DocEventType) WebhookType() types.EventWebhookType {api/types/updatable_project_fields.go (1)
88-107: Consider extracting the validation rule name as a constant.The validation rule name "invalid_webhook_event" is used in multiple places. Consider extracting it as a constant to improve maintainability.
+const ( + ValidationRuleInvalidWebhookEvent = "invalid_webhook_event" +) if err := validation.RegisterValidation( - "invalid_webhook_event", + ValidationRuleInvalidWebhookEvent, func(level validation.FieldLevel) bool { eventTypes := level.Field().Interface().([]string) for _, eventType := range eventTypes { if !IsValidEventType(eventType) { return false } } return true }, ); err != nil { fmt.Fprintln(os.Stderr, "updatable project fields: ", err) os.Exit(1) } if err := validation.RegisterTranslation( - "invalid_webhook_event", + ValidationRuleInvalidWebhookEvent, "given {0} is invalid event type", ); err != nil { fmt.Fprintln(os.Stderr, "updatable project fields: ", err) os.Exit(1) }api/types/updatable_project_fields_test.go (1)
37-40: Add test cases for empty webhook URL and event types.Consider adding test cases for:
- Empty event webhook URL validation
- Empty event types validation
- Multiple event types validation
t.Run("validation test", func(t *testing.T) { // ... existing test code ... + // Test empty event webhook URL + emptyURL := "" + fields = &types.UpdatableProjectFields{ + EventWebhookURL: &emptyURL, + EventWebhookTypes: &newEventWebhookTypes, + } + assert.NoError(t, fields.Validate()) + + // Test empty event types + emptyTypes := []string{} + fields = &types.UpdatableProjectFields{ + EventWebhookURL: &newEventWebhookURL, + EventWebhookTypes: &emptyTypes, + } + assert.NoError(t, fields.Validate()) + + // Test multiple event types + multipleTypes := []string{ + string(types.DocRootChanged), + "InvalidType", + } + fields = &types.UpdatableProjectFields{ + EventWebhookURL: &newEventWebhookURL, + EventWebhookTypes: &multipleTypes, + } + assert.ErrorAs(t, fields.Validate(), &formErr) })Also applies to: 72-75
server/backend/database/project_info.go (1)
122-127: Consider adding a validation method for event webhook configuration.Similar to
ClientDeactivateThresholdAsTimeDuration, consider adding a method to validate the event webhook configuration.+// ValidateEventWebhookConfig validates the event webhook configuration. +func (i *ProjectInfo) ValidateEventWebhookConfig() error { + if i.EventWebhookURL == "" && len(i.EventWebhookTypes) > 0 { + return errors.New("event webhook URL is required when event types are specified") + } + if i.EventWebhookURL != "" && len(i.EventWebhookTypes) == 0 { + return errors.New("event types are required when webhook URL is specified") + } + return nil +}server/backend/backend.go (2)
56-60: Consider adding more detailed documentation for event webhook fields.While the implementation is correct, the documentation could be more descriptive about:
- The purpose and lifecycle of the cache
- The expected format of requests and responses
- The retry behavior and error handling
111-122: Consider adding error handling for cache initialization.While the implementation is correct, consider adding error handling for cache initialization failures, similar to how database initialization is handled.
Apply this diff to add error handling:
- eventWebhookCache := cache.NewLRUExpireCache[string, int]( - conf.EventWebhookCacheSize, - ) + eventWebhookCache, err := cache.NewLRUExpireCache[string, int]( + conf.EventWebhookCacheSize, + ) + if err != nil { + return nil, fmt.Errorf("create event webhook cache: %w", err) + }cmd/yorkie/project/update.go (2)
58-61: Consider adding documentation for event webhook types.While the implementation is correct, consider adding documentation that explains:
- The purpose and behavior of each event type
- When each event type is triggered
- The payload format for each event type
245-262: Consider improving the flag descriptions.The flag descriptions could be more descriptive:
- "event-webhook methods to add" should be "event-webhook types to add"
- Consider adding examples in the descriptions
Apply this diff to improve the descriptions:
- "event-webhook methods to add ('ALL' for all events)", + "event-webhook types to add (e.g., 'DocRootChanged' or 'ALL' for all types)", - "event-webhook types to remove ('ALL' for all events)", + "event-webhook types to remove (e.g., 'DocRootChanged' or 'ALL' for all types)",server/config.go (1)
74-79: Consider grouping related webhook constants together.For better code organization and readability, consider grouping all webhook-related constants (both auth and event) together.
Apply this diff to group the constants:
-const ( - DefaultAuthWebhookRequestTimeout = 3 * time.Second - DefaultAuthWebhookMaxRetries = 10 - DefaultAuthWebhookMaxWaitInterval = 3000 * time.Millisecond - DefaultAuthWebhookMinWaitInterval = 100 * time.Millisecond - DefaultAuthWebhookCacheSize = 5000 - DefaultAuthWebhookCacheTTL = 10 * time.Second - - DefaultEventWebhookRequestTimeout = 3 * time.Second - DefaultEventWebhookMaxRetries = 10 - DefaultEventWebhookMaxWaitInterval = 3000 * time.Millisecond - DefaultEventWebhookMinWaitInterval = 100 * time.Millisecond - DefaultEventWebhookCacheSize = 5000 - DefaultEventWebhookCacheTTL = 10 * time.Second -) +// Webhook related constants +const ( + // Auth webhook constants + DefaultAuthWebhookRequestTimeout = 3 * time.Second + DefaultAuthWebhookMaxRetries = 10 + DefaultAuthWebhookMaxWaitInterval = 3000 * time.Millisecond + DefaultAuthWebhookMinWaitInterval = 100 * time.Millisecond + DefaultAuthWebhookCacheSize = 5000 + DefaultAuthWebhookCacheTTL = 10 * time.Second + + // Event webhook constants + DefaultEventWebhookRequestTimeout = 3 * time.Second + DefaultEventWebhookMaxRetries = 10 + DefaultEventWebhookMaxWaitInterval = 3000 * time.Millisecond + DefaultEventWebhookMinWaitInterval = 100 * time.Millisecond + DefaultEventWebhookCacheSize = 5000 + DefaultEventWebhookCacheTTL = 10 * time.Second +)server/packs/packs.go (1)
199-211: Improve error handling and condition clarity.
- The error handling could be more specific about what went wrong.
- The condition for sending events could be more explicit about its purpose.
Apply this diff to improve the code:
- if reqPack.OperationsLen() > 0 { + // Send webhook event when document root content changes + if numOps := reqPack.OperationsLen(); numOps > 0 { if err := webhook.SendEvent( ctx, be, project, docInfo.Key.String(), events.DocRootChangedEvent, ); err != nil { - logging.From(ctx).Error(err) + logging.From(ctx).Error("failed to send DocRootChanged event", zap.Error(err)) return } }test/integration/event_webhook_test.go (1)
59-80: Consider adding request validation and error handling.The webhook server implementation could be enhanced with additional validations:
- Content-Type header validation
- Request method validation (e.g., POST only)
- Error response handling for invalid requests
srv := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { + if r.Method != http.MethodPost { + w.WriteHeader(http.StatusMethodNotAllowed) + return + } + if r.Header.Get("Content-Type") != "application/json" { + w.WriteHeader(http.StatusUnsupportedMediaType) + return + } atomic.AddInt32(&reqCnt, 1) signatureHeader := r.Header.Get("X-Signature-256") assert.NotZero(t, len(signatureHeader))api/yorkie/v1/resources.proto (1)
307-322: Enhance UpdatableProjectFields with Event Webhook ConfigThe
UpdatableProjectFieldsmessage now defines a nestedEventWebhookTypesmessage (with a repeated string fieldtypes) and adds new fields forevent_webhook_urlandevent_webhook_types(positions 4 and 5 respectively). This integration is consistent with the new attributes in theProjectmessage.Consider adding inline documentation (comments) describing the expected format or usage of these new webhook fields for improved maintainability.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (4)
api/yorkie/v1/admin.pb.gois excluded by!**/*.pb.goapi/yorkie/v1/cluster.pb.gois excluded by!**/*.pb.goapi/yorkie/v1/resources.pb.gois excluded by!**/*.pb.goapi/yorkie/v1/yorkie.pb.gois excluded by!**/*.pb.go
📒 Files selected for processing (37)
api/converter/from_pb.go(2 hunks)api/converter/to_pb.go(2 hunks)api/docs/yorkie/v1/admin.openapi.yaml(4 hunks)api/docs/yorkie/v1/cluster.openapi.yaml(1 hunks)api/docs/yorkie/v1/resources.openapi.yaml(6 hunks)api/docs/yorkie/v1/yorkie.openapi.yaml(2 hunks)api/types/event_webhook.go(1 hunks)api/types/events/events.go(2 hunks)api/types/project.go(2 hunks)api/types/project_test.go(1 hunks)api/types/updatable_project_fields.go(2 hunks)api/types/updatable_project_fields_test.go(3 hunks)api/yorkie/v1/resources.proto(1 hunks)api/yorkie/v1/v1connect/admin.connect.go(1 hunks)api/yorkie/v1/v1connect/cluster.connect.go(1 hunks)api/yorkie/v1/v1connect/yorkie.connect.go(1 hunks)cmd/yorkie/project/list.go(2 hunks)cmd/yorkie/project/update.go(6 hunks)cmd/yorkie/server.go(3 hunks)pkg/webhook/client.go(1 hunks)server/backend/backend.go(3 hunks)server/backend/config.go(4 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.sample.yml(1 hunks)server/config_test.go(2 hunks)server/packs/packs.go(2 hunks)server/packs/packs_test.go(1 hunks)server/rpc/server_test.go(1 hunks)server/rpc/webhook/event.go(1 hunks)test/bench/grpc_bench_test.go(1 hunks)test/complex/main_test.go(1 hunks)test/helper/helper.go(2 hunks)test/integration/event_webhook_test.go(1 hunks)
✅ Files skipped from review due to trivial changes (4)
- api/yorkie/v1/v1connect/cluster.connect.go
- api/yorkie/v1/v1connect/yorkie.connect.go
- api/yorkie/v1/v1connect/admin.connect.go
- api/docs/yorkie/v1/yorkie.openapi.yaml
🧰 Additional context used
🪛 GitHub Check: bench
test/bench/grpc_bench_test.go
[failure] 111-111:
undefined: eventWebhookURL
[failure] 112-112:
undefined: eventWebhookTypes
⏰ Context from checks skipped due to timeout of 90000ms (2)
- GitHub Check: build
- GitHub Check: complex-test
🔇 Additional comments (28)
server/backend/database/project_info_test.go (1)
37-54: LGTM! Test coverage looks good.The test cases for event webhook fields follow the same pattern as existing tests and provide good coverage for the new functionality.
api/types/project_test.go (1)
53-74: LGTM! Comprehensive test coverage for event webhook requirements.The test cases thoroughly cover all scenarios and mirror the structure of the auth webhook tests, making the code consistent and easy to understand.
server/rpc/webhook/event.go (1)
95-98: LGTM!The cache key generation is efficient and follows a clear pattern.
api/types/project.go (1)
41-46: LGTM!The implementation of
RequireEventWebhookfollows the same pattern asRequireAuth, maintaining consistency in the codebase. The new fields are properly documented and integrated.Also applies to: 83-100
cmd/yorkie/project/list.go (1)
89-90: LGTM!The new columns are properly integrated into the table output, maintaining consistency with the existing format.
Also applies to: 101-102
api/types/updatable_project_fields.go (1)
42-47: LGTM! Field declarations are well-structured.The new fields follow the established pattern with proper BSON tags and validation rules.
server/config_test.go (1)
64-64: LGTM! Test coverage is comprehensive.The test cases for event webhook configuration are well-structured and cover all default values.
Also applies to: 86-100
server/backend/database/project_info.go (1)
60-65: LGTM! Field declarations are well-documented.The new fields are properly defined with clear documentation and BSON tags.
pkg/webhook/client.go (1)
107-109: LGTM! The type check for integer response is well-implemented.The conditional check for integer response type is a clean way to handle different response types, allowing the method to return the HTTP status code directly without JSON decoding.
test/complex/main_test.go (1)
86-90: LGTM! Event webhook configuration follows the same pattern as auth webhook.The configuration parameters for event webhooks are consistently structured, following the established pattern of auth webhooks, which makes the code more maintainable.
cmd/yorkie/project/update.go (1)
185-217: LGTM! TheupdateStringSlicefunction is well-implemented.The function is:
- Generic enough to handle both auth and event webhook types
- Efficiently uses a map for deduplication
- Correctly handles the 'ALL' special case
server/rpc/server_test.go (1)
81-85: LGTM!The event webhook configuration parameters are correctly added, following the same pattern as the existing auth webhook configuration.
server/config.go (1)
221-243: LGTM!The event webhook configuration handling is correctly implemented, following the same pattern as the auth webhook configuration.
test/integration/event_webhook_test.go (1)
48-57: LGTM! Secure signature verification implementation.The
verifySignaturefunction correctly implements HMAC-SHA256 signature verification with constant-time comparison usinghmac.Equalto prevent timing attacks.server/backend/config.go (1)
79-96: LGTM! Well-structured event webhook configuration.The event webhook configuration fields follow the same pattern as the auth webhook configuration, maintaining consistency in the codebase.
server/packs/packs_test.go (1)
107-111: LGTM! Consistent test configuration for event webhooks.The event webhook test configuration follows the same pattern as the auth webhook configuration, maintaining consistency in the test setup.
cmd/yorkie/server.go (1)
84-87: LGTM! Consistent configuration assignment for event webhooks.The event webhook configuration assignments follow the same pattern as the auth webhook configuration, maintaining consistency in the server setup.
server/config.sample.yml (1)
83-97: New Event Webhook Configuration ParametersThe new parameters (EventWebhookRequestTimeout, EventWebhookMaxRetries, EventWebhookMinWaitInterval, EventWebhookMaxWaitInterval, and EventWebhookCacheTTL) have been added under the Backend section. Their values (e.g. "3s", 10, "100ms", "3s", "10s") look consistent with the corresponding AuthWebhook settings.
api/yorkie/v1/resources.proto (1)
293-305: Extend Project Message with Webhook FieldsThe
Projectmessage now includes the new fieldsevent_webhook_url(at position 7) andevent_webhook_types(at position 8), and the subsequent fields have been renumbered accordingly. This change correctly expands the data model to support event webhooks. Please verify that downstream conversion functions (in from_pb.go and to_pb.go) and any existing clients have been updated to handle the new signature.api/docs/yorkie/v1/cluster.openapi.yaml (1)
272-283: New Webhook Fields in Cluster OpenAPI SpecThe OpenAPI schema for
yorkie.v1.Projectnow includes the propertieseventWebhookTypes(an array of strings) andeventWebhookUrl(a string). Their definitions (e.g.additionalProperties: false, empty description, and proper title) align with the Protobuf definitions and the data model.api/docs/yorkie/v1/resources.openapi.yaml (1)
272-283: Project Schema Webhook Field AdditionsThe modifications add
eventWebhookTypesandeventWebhookUrlto theyorkie.v1.Projectschema. The definitions mirror the changes made in the Protobuf and the cluster OpenAPI spec. The use of type array (for webhook types) and string (for webhook URL) is appropriate.api/docs/yorkie/v1/admin.openapi.yaml (1)
2166-2192: Incorporation of Event Webhook Fields in Admin APIThe
UpdatableProjectFieldsschema in the admin OpenAPI file now includes references foreventWebhookTypes(using the new schemayorkie.v1.UpdatableProjectFields.EventWebhookTypes) andeventWebhookUrl(referencinggoogle.protobuf.StringValue). This ensures that admin endpoints (e.g. UpdateProject) accept and update these new webhook configurations. Verify consistency with other OpenAPI documents to maintain uniformity across the API.test/helper/helper.go (1)
82-86: LGTM! Event webhook configuration values are well-defined.The new event webhook configuration values mirror the existing auth webhook pattern and use appropriate test values.
api/converter/to_pb.go (2)
62-63: LGTM! Event webhook fields are correctly added to Protobuf conversion.The implementation follows the established pattern and correctly handles the new event webhook fields.
567-576: LGTM! Event webhook fields are properly handled in updatable fields conversion.The implementation correctly handles the conditional conversion of event webhook fields, following the same pattern as other fields.
api/converter/from_pb.go (2)
58-59: LGTM! Event webhook fields are correctly added to model conversion.The implementation follows the established pattern and correctly handles the new event webhook fields.
927-932: LGTM! Event webhook fields are properly handled in updatable fields conversion.The implementation correctly handles the conditional conversion of event webhook fields, following the same pattern as other fields.
server/backend/database/testcases/testcases.go (1)
770-771: LGTM! Test cases comprehensively cover event webhook functionality.The test cases are well-structured and cover important scenarios:
- Update all fields
- Update individual fields
- Verify field values after updates
- Test error cases
Also applies to: 786-787, 800-801, 815-817, 821-825, 836-837, 840-858
| }))) | ||
|
|
||
| waitWebhookReceived := 20 * time.Millisecond | ||
| t.Run("throttling event test", func(t *testing.T) { |
There was a problem hiding this comment.
💡 Verification agent
🧩 Analysis chain
Consider adding test coverage for throttling.
The skipped test for throttling event updates should be implemented to ensure proper rate limiting behavior.
Would you like me to help implement the throttling test case?
🏁 Script executed:
#!/bin/bash
# Search for any existing rate limiting or throttling implementations
rg -A 5 "throttl|rate.?limit"Length of output: 14411
Implement Throttling Test Coverage
The "throttling event test" in test/integration/event_webhook_test.go is currently skipped with the message "remove this after implement advanced event cache control". Please add active test coverage to simulate throttling event updates and verify that the rate limiting behavior (as configured via the Helm charts) is working as expected.
- Remove or replace the
t.Skipcall once the advanced event cache control mechanism is in place. - Implement assertions to ensure that the number of updates is throttled according to the configuration parameters (for example, the effective requests per unit).
Would you like assistance with designing or implementing this test case?
There was a problem hiding this comment.
Would it be a good idea to cache webhook results for event trigger controlling?
- When an event occurs, it is basic to convey everything to the user.
- However, if too many calls are made, it may be unnecessary.
- This seems to be intended to debouncing or throttle event control.
- We can achieve a similar effect by caching the webhook results, but it makes the intent appear ambiguous.
There was a problem hiding this comment.
@hackerwins You raise excellent points. Using cache for throttling does make the intent unclear, as caching and rate limiting serve different purposes. Instead of using cache, we could consider:
- Explicit Rate Limiting: Implement a proper rate limiter (e.g., token bucket, leaky bucket) that clearly expresses the intent of controlling event frequency.
- Debouncing: For rapid-fire events, implement a debouncing mechanism that waits for a "quiet period" before sending the webhook.
- Event Aggregation: Batch events within time windows and send them together, reducing the total number of webhook calls while ensuring no events are lost.
Would you like to explore any of these alternatives?
✏️ Learnings added
Learnt from: hackerwins
PR: yorkie-team/yorkie#1156
File: test/integration/event_webhook_test.go:274-274
Timestamp: 2025-02-18T10:51:18.828Z
Learning: In the Yorkie project, caching should not be used as a mechanism for rate limiting or throttling webhook events, as it makes the intent unclear. Instead, use explicit rate limiting, debouncing, or event aggregation mechanisms.
Note: Learnings are effective only in the context of similar code segments. To apply general coding guidelines, please configure review instructions. You can manage existing learnings in the app.
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #1156 +/- ##
==========================================
- Coverage 38.56% 38.37% -0.19%
==========================================
Files 170 172 +2
Lines 25441 25796 +355
==========================================
+ Hits 9812 9900 +88
- Misses 14808 15063 +255
- Partials 821 833 +12 ☔ View full report in Codecov by Sentry. |
- `WebhookCache` -> `AuthWebhookCache` - `WebhookClient` -> `AuthWebhookClient`
|
There are too many changed files, making it difficult to identify the important parts. So, I've left a self-review detailing my considerations and concerns. |
- `rpc` -> `server`
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (2)
server/webhook/event.go (2)
39-42: Include additional context in the error message.
WhenwebhookTypeis empty or invalid, consider including thedocKeyin the returned error for improved debugging and traceability of failing events.An example enhancement:
-if webhookType == "" { - return fmt.Errorf("invalid event webhook type: %s", eventType) +if webhookType == "" { + return fmt.Errorf("invalid event webhook type: %s (docKey=%s)", eventType, docKey) }
53-57: Ensure unique cache keys when docKey may contain “:” or other special characters.
IfdocKeycontains colons or other special characters, it could collide with the delimiter-based format (publicKey:event:docKey:webhookType). Consider URL-encoding or hashing to avoid collisions.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
server/packs/packs.go(2 hunks)server/webhook/event.go(1 hunks)
⏰ Context from checks skipped due to timeout of 90000ms (3)
- GitHub Check: bench
- GitHub Check: complex-test
- GitHub Check: build
🔇 Additional comments (2)
server/packs/packs.go (2)
40-40: Import statement looks good.
No issues found with addinggithub.com/yorkie-team/yorkie/server/webhook.
199-210: Avoid skipping snapshot creation if the webhook call fails.
Currently, this goroutine returns immediately upon a webhook error, preventing snapshot creation. That might delay or skip crucial snapshot storage if the webhook is unresponsive or fails. Consider logging the error but continuing with the subsequent snapshot logic to maintain data integrity.[refactor_suggestion_essential, verify]
A possible approach:
if err := webhook.SendEvent( ctx, be, project, docInfo.Key.String(), events.DocRootChangedEvent, ); err != nil { - logging.From(ctx).Error(err) - return + logging.From(ctx).Error( + fmt.Errorf("webhook send failed, but proceeding with snapshot creation: %w", err), + ) }Would you like to confirm if skipping the snapshot step is intentional?
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
There was a problem hiding this comment.
Go Benchmark
| Benchmark suite | Current: da1fda0 | Previous: d1bc63e | Ratio |
|---|---|---|---|
BenchmarkDocument/constructor_test |
1440 ns/op 1385 B/op 24 allocs/op |
1434 ns/op 1385 B/op 24 allocs/op |
1.00 |
BenchmarkDocument/constructor_test - ns/op |
1440 ns/op |
1434 ns/op |
1.00 |
BenchmarkDocument/constructor_test - B/op |
1385 B/op |
1385 B/op |
1 |
BenchmarkDocument/constructor_test - allocs/op |
24 allocs/op |
24 allocs/op |
1 |
BenchmarkDocument/status_test |
1037 ns/op 1353 B/op 22 allocs/op |
1017 ns/op 1353 B/op 22 allocs/op |
1.02 |
BenchmarkDocument/status_test - ns/op |
1037 ns/op |
1017 ns/op |
1.02 |
BenchmarkDocument/status_test - B/op |
1353 B/op |
1353 B/op |
1 |
BenchmarkDocument/status_test - allocs/op |
22 allocs/op |
22 allocs/op |
1 |
BenchmarkDocument/equals_test |
7812 ns/op 7562 B/op 129 allocs/op |
8577 ns/op 7562 B/op 129 allocs/op |
0.91 |
BenchmarkDocument/equals_test - ns/op |
7812 ns/op |
8577 ns/op |
0.91 |
BenchmarkDocument/equals_test - B/op |
7562 B/op |
7562 B/op |
1 |
BenchmarkDocument/equals_test - allocs/op |
129 allocs/op |
129 allocs/op |
1 |
BenchmarkDocument/nested_update_test |
16838 ns/op 12307 B/op 258 allocs/op |
17152 ns/op 12306 B/op 258 allocs/op |
0.98 |
BenchmarkDocument/nested_update_test - ns/op |
16838 ns/op |
17152 ns/op |
0.98 |
BenchmarkDocument/nested_update_test - B/op |
12307 B/op |
12306 B/op |
1.00 |
BenchmarkDocument/nested_update_test - allocs/op |
258 allocs/op |
258 allocs/op |
1 |
BenchmarkDocument/delete_test |
23033 ns/op 15788 B/op 339 allocs/op |
22757 ns/op 15788 B/op 339 allocs/op |
1.01 |
BenchmarkDocument/delete_test - ns/op |
23033 ns/op |
22757 ns/op |
1.01 |
BenchmarkDocument/delete_test - B/op |
15788 B/op |
15788 B/op |
1 |
BenchmarkDocument/delete_test - allocs/op |
339 allocs/op |
339 allocs/op |
1 |
BenchmarkDocument/object_test |
9394 ns/op 7033 B/op 118 allocs/op |
8652 ns/op 7033 B/op 118 allocs/op |
1.09 |
BenchmarkDocument/object_test - ns/op |
9394 ns/op |
8652 ns/op |
1.09 |
BenchmarkDocument/object_test - B/op |
7033 B/op |
7033 B/op |
1 |
BenchmarkDocument/object_test - allocs/op |
118 allocs/op |
118 allocs/op |
1 |
BenchmarkDocument/array_test |
28357 ns/op 12139 B/op 273 allocs/op |
28380 ns/op 12139 B/op 273 allocs/op |
1.00 |
BenchmarkDocument/array_test - ns/op |
28357 ns/op |
28380 ns/op |
1.00 |
BenchmarkDocument/array_test - B/op |
12139 B/op |
12139 B/op |
1 |
BenchmarkDocument/array_test - allocs/op |
273 allocs/op |
273 allocs/op |
1 |
BenchmarkDocument/text_test |
32323 ns/op 15188 B/op 484 allocs/op |
31961 ns/op 15188 B/op 484 allocs/op |
1.01 |
BenchmarkDocument/text_test - ns/op |
32323 ns/op |
31961 ns/op |
1.01 |
BenchmarkDocument/text_test - B/op |
15188 B/op |
15188 B/op |
1 |
BenchmarkDocument/text_test - allocs/op |
484 allocs/op |
484 allocs/op |
1 |
BenchmarkDocument/text_composition_test |
32297 ns/op 18701 B/op 501 allocs/op |
32008 ns/op 18701 B/op 501 allocs/op |
1.01 |
BenchmarkDocument/text_composition_test - ns/op |
32297 ns/op |
32008 ns/op |
1.01 |
BenchmarkDocument/text_composition_test - B/op |
18701 B/op |
18701 B/op |
1 |
BenchmarkDocument/text_composition_test - allocs/op |
501 allocs/op |
501 allocs/op |
1 |
BenchmarkDocument/rich_text_test |
85983 ns/op 39350 B/op 1146 allocs/op |
86149 ns/op 39351 B/op 1146 allocs/op |
1.00 |
BenchmarkDocument/rich_text_test - ns/op |
85983 ns/op |
86149 ns/op |
1.00 |
BenchmarkDocument/rich_text_test - B/op |
39350 B/op |
39351 B/op |
1.00 |
BenchmarkDocument/rich_text_test - allocs/op |
1146 allocs/op |
1146 allocs/op |
1 |
BenchmarkDocument/counter_test |
18073 ns/op 11810 B/op 253 allocs/op |
18211 ns/op 11810 B/op 253 allocs/op |
0.99 |
BenchmarkDocument/counter_test - ns/op |
18073 ns/op |
18211 ns/op |
0.99 |
BenchmarkDocument/counter_test - B/op |
11810 B/op |
11810 B/op |
1 |
BenchmarkDocument/counter_test - allocs/op |
253 allocs/op |
253 allocs/op |
1 |
BenchmarkDocument/text_edit_gc_100 |
1401435 ns/op 864910 B/op 17282 allocs/op |
1403630 ns/op 864889 B/op 17281 allocs/op |
1.00 |
BenchmarkDocument/text_edit_gc_100 - ns/op |
1401435 ns/op |
1403630 ns/op |
1.00 |
BenchmarkDocument/text_edit_gc_100 - B/op |
864910 B/op |
864889 B/op |
1.00 |
BenchmarkDocument/text_edit_gc_100 - allocs/op |
17282 allocs/op |
17281 allocs/op |
1.00 |
BenchmarkDocument/text_edit_gc_1000 |
52156664 ns/op 46839357 B/op 185590 allocs/op |
53094806 ns/op 46839316 B/op 185584 allocs/op |
0.98 |
BenchmarkDocument/text_edit_gc_1000 - ns/op |
52156664 ns/op |
53094806 ns/op |
0.98 |
BenchmarkDocument/text_edit_gc_1000 - B/op |
46839357 B/op |
46839316 B/op |
1.00 |
BenchmarkDocument/text_edit_gc_1000 - allocs/op |
185590 allocs/op |
185584 allocs/op |
1.00 |
BenchmarkDocument/text_split_gc_100 |
2104826 ns/op 1580998 B/op 15950 allocs/op |
2122117 ns/op 1581046 B/op 15950 allocs/op |
0.99 |
BenchmarkDocument/text_split_gc_100 - ns/op |
2104826 ns/op |
2122117 ns/op |
0.99 |
BenchmarkDocument/text_split_gc_100 - B/op |
1580998 B/op |
1581046 B/op |
1.00 |
BenchmarkDocument/text_split_gc_100 - allocs/op |
15950 allocs/op |
15950 allocs/op |
1 |
BenchmarkDocument/text_split_gc_1000 |
127351818 ns/op 137791362 B/op 185013 allocs/op |
128674156 ns/op 137789660 B/op 185008 allocs/op |
0.99 |
BenchmarkDocument/text_split_gc_1000 - ns/op |
127351818 ns/op |
128674156 ns/op |
0.99 |
BenchmarkDocument/text_split_gc_1000 - B/op |
137791362 B/op |
137789660 B/op |
1.00 |
BenchmarkDocument/text_split_gc_1000 - allocs/op |
185013 allocs/op |
185008 allocs/op |
1.00 |
BenchmarkDocument/text_delete_all_10000 |
16818853 ns/op 10575636 B/op 56133 allocs/op |
17323202 ns/op 10576550 B/op 56135 allocs/op |
0.97 |
BenchmarkDocument/text_delete_all_10000 - ns/op |
16818853 ns/op |
17323202 ns/op |
0.97 |
BenchmarkDocument/text_delete_all_10000 - B/op |
10575636 B/op |
10576550 B/op |
1.00 |
BenchmarkDocument/text_delete_all_10000 - allocs/op |
56133 allocs/op |
56135 allocs/op |
1.00 |
BenchmarkDocument/text_delete_all_100000 |
296837292 ns/op 105492784 B/op 565986 allocs/op |
286783438 ns/op 105503508 B/op 566016 allocs/op |
1.04 |
BenchmarkDocument/text_delete_all_100000 - ns/op |
296837292 ns/op |
286783438 ns/op |
1.04 |
BenchmarkDocument/text_delete_all_100000 - B/op |
105492784 B/op |
105503508 B/op |
1.00 |
BenchmarkDocument/text_delete_all_100000 - allocs/op |
565986 allocs/op |
566016 allocs/op |
1.00 |
BenchmarkDocument/text_100 |
231011 ns/op 120906 B/op 5181 allocs/op |
245669 ns/op 120941 B/op 5181 allocs/op |
0.94 |
BenchmarkDocument/text_100 - ns/op |
231011 ns/op |
245669 ns/op |
0.94 |
BenchmarkDocument/text_100 - B/op |
120906 B/op |
120941 B/op |
1.00 |
BenchmarkDocument/text_100 - allocs/op |
5181 allocs/op |
5181 allocs/op |
1 |
BenchmarkDocument/text_1000 |
2473976 ns/op 1156079 B/op 51084 allocs/op |
2583529 ns/op 1156080 B/op 51084 allocs/op |
0.96 |
BenchmarkDocument/text_1000 - ns/op |
2473976 ns/op |
2583529 ns/op |
0.96 |
BenchmarkDocument/text_1000 - B/op |
1156079 B/op |
1156080 B/op |
1.00 |
BenchmarkDocument/text_1000 - allocs/op |
51084 allocs/op |
51084 allocs/op |
1 |
BenchmarkDocument/array_1000 |
1237703 ns/op 1088184 B/op 11879 allocs/op |
1308608 ns/op 1088143 B/op 11879 allocs/op |
0.95 |
BenchmarkDocument/array_1000 - ns/op |
1237703 ns/op |
1308608 ns/op |
0.95 |
BenchmarkDocument/array_1000 - B/op |
1088184 B/op |
1088143 B/op |
1.00 |
BenchmarkDocument/array_1000 - allocs/op |
11879 allocs/op |
11879 allocs/op |
1 |
BenchmarkDocument/array_10000 |
13390119 ns/op 9889292 B/op 120736 allocs/op |
13702040 ns/op 9888182 B/op 120731 allocs/op |
0.98 |
BenchmarkDocument/array_10000 - ns/op |
13390119 ns/op |
13702040 ns/op |
0.98 |
BenchmarkDocument/array_10000 - B/op |
9889292 B/op |
9888182 B/op |
1.00 |
BenchmarkDocument/array_10000 - allocs/op |
120736 allocs/op |
120731 allocs/op |
1.00 |
BenchmarkDocument/array_gc_100 |
132535 ns/op 99895 B/op 1266 allocs/op |
137387 ns/op 99897 B/op 1266 allocs/op |
0.96 |
BenchmarkDocument/array_gc_100 - ns/op |
132535 ns/op |
137387 ns/op |
0.96 |
BenchmarkDocument/array_gc_100 - B/op |
99895 B/op |
99897 B/op |
1.00 |
BenchmarkDocument/array_gc_100 - allocs/op |
1266 allocs/op |
1266 allocs/op |
1 |
BenchmarkDocument/array_gc_1000 |
1424355 ns/op 1140914 B/op 12926 allocs/op |
1490623 ns/op 1141017 B/op 12927 allocs/op |
0.96 |
BenchmarkDocument/array_gc_1000 - ns/op |
1424355 ns/op |
1490623 ns/op |
0.96 |
BenchmarkDocument/array_gc_1000 - B/op |
1140914 B/op |
1141017 B/op |
1.00 |
BenchmarkDocument/array_gc_1000 - allocs/op |
12926 allocs/op |
12927 allocs/op |
1.00 |
BenchmarkDocument/counter_1000 |
202763 ns/op 178135 B/op 5771 allocs/op |
216155 ns/op 178135 B/op 5771 allocs/op |
0.94 |
BenchmarkDocument/counter_1000 - ns/op |
202763 ns/op |
216155 ns/op |
0.94 |
BenchmarkDocument/counter_1000 - B/op |
178135 B/op |
178135 B/op |
1 |
BenchmarkDocument/counter_1000 - allocs/op |
5771 allocs/op |
5771 allocs/op |
1 |
BenchmarkDocument/counter_10000 |
2176694 ns/op 2068939 B/op 59778 allocs/op |
2222630 ns/op 2068970 B/op 59778 allocs/op |
0.98 |
BenchmarkDocument/counter_10000 - ns/op |
2176694 ns/op |
2222630 ns/op |
0.98 |
BenchmarkDocument/counter_10000 - B/op |
2068939 B/op |
2068970 B/op |
1.00 |
BenchmarkDocument/counter_10000 - allocs/op |
59778 allocs/op |
59778 allocs/op |
1 |
BenchmarkDocument/object_1000 |
1390461 ns/op 1436936 B/op 9924 allocs/op |
1474710 ns/op 1437214 B/op 9925 allocs/op |
0.94 |
BenchmarkDocument/object_1000 - ns/op |
1390461 ns/op |
1474710 ns/op |
0.94 |
BenchmarkDocument/object_1000 - B/op |
1436936 B/op |
1437214 B/op |
1.00 |
BenchmarkDocument/object_1000 - allocs/op |
9924 allocs/op |
9925 allocs/op |
1.00 |
BenchmarkDocument/object_10000 |
14646409 ns/op 12348571 B/op 101221 allocs/op |
14598808 ns/op 12351158 B/op 101230 allocs/op |
1.00 |
BenchmarkDocument/object_10000 - ns/op |
14646409 ns/op |
14598808 ns/op |
1.00 |
BenchmarkDocument/object_10000 - B/op |
12348571 B/op |
12351158 B/op |
1.00 |
BenchmarkDocument/object_10000 - allocs/op |
101221 allocs/op |
101230 allocs/op |
1.00 |
BenchmarkDocument/tree_100 |
1016742 ns/op 951021 B/op 6102 allocs/op |
1083304 ns/op 951029 B/op 6102 allocs/op |
0.94 |
BenchmarkDocument/tree_100 - ns/op |
1016742 ns/op |
1083304 ns/op |
0.94 |
BenchmarkDocument/tree_100 - B/op |
951021 B/op |
951029 B/op |
1.00 |
BenchmarkDocument/tree_100 - allocs/op |
6102 allocs/op |
6102 allocs/op |
1 |
BenchmarkDocument/tree_1000 |
75502314 ns/op 86582305 B/op 60112 allocs/op |
80425809 ns/op 86582548 B/op 60112 allocs/op |
0.94 |
BenchmarkDocument/tree_1000 - ns/op |
75502314 ns/op |
80425809 ns/op |
0.94 |
BenchmarkDocument/tree_1000 - B/op |
86582305 B/op |
86582548 B/op |
1.00 |
BenchmarkDocument/tree_1000 - allocs/op |
60112 allocs/op |
60112 allocs/op |
1 |
BenchmarkDocument/tree_10000 |
9418115101 ns/op 8581189200 B/op 600196 allocs/op |
10123294087 ns/op 8581187408 B/op 600190 allocs/op |
0.93 |
BenchmarkDocument/tree_10000 - ns/op |
9418115101 ns/op |
10123294087 ns/op |
0.93 |
BenchmarkDocument/tree_10000 - B/op |
8581189200 B/op |
8581187408 B/op |
1.00 |
BenchmarkDocument/tree_10000 - allocs/op |
600196 allocs/op |
600190 allocs/op |
1.00 |
BenchmarkDocument/tree_delete_all_1000 |
77680884 ns/op 87566499 B/op 75289 allocs/op |
84232656 ns/op 87566174 B/op 75287 allocs/op |
0.92 |
BenchmarkDocument/tree_delete_all_1000 - ns/op |
77680884 ns/op |
84232656 ns/op |
0.92 |
BenchmarkDocument/tree_delete_all_1000 - B/op |
87566499 B/op |
87566174 B/op |
1.00 |
BenchmarkDocument/tree_delete_all_1000 - allocs/op |
75289 allocs/op |
75287 allocs/op |
1.00 |
BenchmarkDocument/tree_edit_gc_100 |
3793358 ns/op 4147883 B/op 15146 allocs/op |
4015027 ns/op 4147754 B/op 15146 allocs/op |
0.94 |
BenchmarkDocument/tree_edit_gc_100 - ns/op |
3793358 ns/op |
4015027 ns/op |
0.94 |
BenchmarkDocument/tree_edit_gc_100 - B/op |
4147883 B/op |
4147754 B/op |
1.00 |
BenchmarkDocument/tree_edit_gc_100 - allocs/op |
15146 allocs/op |
15146 allocs/op |
1 |
BenchmarkDocument/tree_edit_gc_1000 |
313157354 ns/op 384041498 B/op 154934 allocs/op |
339117183 ns/op 384035298 B/op 154923 allocs/op |
0.92 |
BenchmarkDocument/tree_edit_gc_1000 - ns/op |
313157354 ns/op |
339117183 ns/op |
0.92 |
BenchmarkDocument/tree_edit_gc_1000 - B/op |
384041498 B/op |
384035298 B/op |
1.00 |
BenchmarkDocument/tree_edit_gc_1000 - allocs/op |
154934 allocs/op |
154923 allocs/op |
1.00 |
BenchmarkDocument/tree_split_gc_100 |
2631634 ns/op 2407262 B/op 11131 allocs/op |
2772059 ns/op 2407216 B/op 11131 allocs/op |
0.95 |
BenchmarkDocument/tree_split_gc_100 - ns/op |
2631634 ns/op |
2772059 ns/op |
0.95 |
BenchmarkDocument/tree_split_gc_100 - B/op |
2407262 B/op |
2407216 B/op |
1.00 |
BenchmarkDocument/tree_split_gc_100 - allocs/op |
11131 allocs/op |
11131 allocs/op |
1 |
BenchmarkDocument/tree_split_gc_1000 |
190822951 ns/op 222497704 B/op 122062 allocs/op |
206630555 ns/op 222499982 B/op 122068 allocs/op |
0.92 |
BenchmarkDocument/tree_split_gc_1000 - ns/op |
190822951 ns/op |
206630555 ns/op |
0.92 |
BenchmarkDocument/tree_split_gc_1000 - B/op |
222497704 B/op |
222499982 B/op |
1.00 |
BenchmarkDocument/tree_split_gc_1000 - allocs/op |
122062 allocs/op |
122068 allocs/op |
1.00 |
BenchmarkRPC/client_to_server |
423458161 ns/op 16412976 B/op 223632 allocs/op |
421420283 ns/op 16134125 B/op 223563 allocs/op |
1.00 |
BenchmarkRPC/client_to_server - ns/op |
423458161 ns/op |
421420283 ns/op |
1.00 |
BenchmarkRPC/client_to_server - B/op |
16412976 B/op |
16134125 B/op |
1.02 |
BenchmarkRPC/client_to_server - allocs/op |
223632 allocs/op |
223563 allocs/op |
1.00 |
BenchmarkRPC/client_to_client_via_server |
793005860 ns/op 37206352 B/op 477574 allocs/op |
787416932 ns/op 38529484 B/op 478163 allocs/op |
1.01 |
BenchmarkRPC/client_to_client_via_server - ns/op |
793005860 ns/op |
787416932 ns/op |
1.01 |
BenchmarkRPC/client_to_client_via_server - B/op |
37206352 B/op |
38529484 B/op |
0.97 |
BenchmarkRPC/client_to_client_via_server - allocs/op |
477574 allocs/op |
478163 allocs/op |
1.00 |
BenchmarkRPC/attach_large_document |
1271262478 ns/op 1898139640 B/op 12283 allocs/op |
1281173852 ns/op 1910678808 B/op 12176 allocs/op |
0.99 |
BenchmarkRPC/attach_large_document - ns/op |
1271262478 ns/op |
1281173852 ns/op |
0.99 |
BenchmarkRPC/attach_large_document - B/op |
1898139640 B/op |
1910678808 B/op |
0.99 |
BenchmarkRPC/attach_large_document - allocs/op |
12283 allocs/op |
12176 allocs/op |
1.01 |
BenchmarkRPC/adminCli_to_server |
554722948 ns/op 21350448 B/op 316689 allocs/op |
542227868 ns/op 22933544 B/op 316715 allocs/op |
1.02 |
BenchmarkRPC/adminCli_to_server - ns/op |
554722948 ns/op |
542227868 ns/op |
1.02 |
BenchmarkRPC/adminCli_to_server - B/op |
21350448 B/op |
22933544 B/op |
0.93 |
BenchmarkRPC/adminCli_to_server - allocs/op |
316689 allocs/op |
316715 allocs/op |
1.00 |
BenchmarkLocker |
86.19 ns/op 32 B/op 1 allocs/op |
83.05 ns/op 32 B/op 1 allocs/op |
1.04 |
BenchmarkLocker - ns/op |
86.19 ns/op |
83.05 ns/op |
1.04 |
BenchmarkLocker - B/op |
32 B/op |
32 B/op |
1 |
BenchmarkLocker - allocs/op |
1 allocs/op |
1 allocs/op |
1 |
BenchmarkLockerParallel |
45.54 ns/op 0 B/op 0 allocs/op |
45.68 ns/op 0 B/op 0 allocs/op |
1.00 |
BenchmarkLockerParallel - ns/op |
45.54 ns/op |
45.68 ns/op |
1.00 |
BenchmarkLockerParallel - B/op |
0 B/op |
0 B/op |
1 |
BenchmarkLockerParallel - allocs/op |
0 allocs/op |
0 allocs/op |
1 |
BenchmarkLockerMoreKeys |
178.9 ns/op 31 B/op 0 allocs/op |
179.4 ns/op 31 B/op 0 allocs/op |
1.00 |
BenchmarkLockerMoreKeys - ns/op |
178.9 ns/op |
179.4 ns/op |
1.00 |
BenchmarkLockerMoreKeys - B/op |
31 B/op |
31 B/op |
1 |
BenchmarkLockerMoreKeys - allocs/op |
0 allocs/op |
0 allocs/op |
1 |
BenchmarkRWLocker/RWLock_rate_2 |
49.36 ns/op 0 B/op 0 allocs/op |
49.55 ns/op 0 B/op 0 allocs/op |
1.00 |
BenchmarkRWLocker/RWLock_rate_2 - ns/op |
49.36 ns/op |
49.55 ns/op |
1.00 |
BenchmarkRWLocker/RWLock_rate_2 - B/op |
0 B/op |
0 B/op |
1 |
BenchmarkRWLocker/RWLock_rate_2 - allocs/op |
0 allocs/op |
0 allocs/op |
1 |
BenchmarkRWLocker/RWLock_rate_10 |
43.86 ns/op 0 B/op 0 allocs/op |
43.6 ns/op 0 B/op 0 allocs/op |
1.01 |
BenchmarkRWLocker/RWLock_rate_10 - ns/op |
43.86 ns/op |
43.6 ns/op |
1.01 |
BenchmarkRWLocker/RWLock_rate_10 - B/op |
0 B/op |
0 B/op |
1 |
BenchmarkRWLocker/RWLock_rate_10 - allocs/op |
0 allocs/op |
0 allocs/op |
1 |
BenchmarkRWLocker/RWLock_rate_100 |
59.47 ns/op 2 B/op 0 allocs/op |
60.32 ns/op 2 B/op 0 allocs/op |
0.99 |
BenchmarkRWLocker/RWLock_rate_100 - ns/op |
59.47 ns/op |
60.32 ns/op |
0.99 |
BenchmarkRWLocker/RWLock_rate_100 - B/op |
2 B/op |
2 B/op |
1 |
BenchmarkRWLocker/RWLock_rate_100 - allocs/op |
0 allocs/op |
0 allocs/op |
1 |
BenchmarkRWLocker/RWLock_rate_1000 |
88.31 ns/op 8 B/op 0 allocs/op |
88.83 ns/op 8 B/op 0 allocs/op |
0.99 |
BenchmarkRWLocker/RWLock_rate_1000 - ns/op |
88.31 ns/op |
88.83 ns/op |
0.99 |
BenchmarkRWLocker/RWLock_rate_1000 - B/op |
8 B/op |
8 B/op |
1 |
BenchmarkRWLocker/RWLock_rate_1000 - allocs/op |
0 allocs/op |
0 allocs/op |
1 |
BenchmarkChange/Push_10_Changes |
4504876 ns/op 150668 B/op 1623 allocs/op |
4538868 ns/op 150377 B/op 1623 allocs/op |
0.99 |
BenchmarkChange/Push_10_Changes - ns/op |
4504876 ns/op |
4538868 ns/op |
0.99 |
BenchmarkChange/Push_10_Changes - B/op |
150668 B/op |
150377 B/op |
1.00 |
BenchmarkChange/Push_10_Changes - allocs/op |
1623 allocs/op |
1623 allocs/op |
1 |
BenchmarkChange/Push_100_Changes |
16236087 ns/op 776300 B/op 8509 allocs/op |
16825394 ns/op 777642 B/op 8512 allocs/op |
0.96 |
BenchmarkChange/Push_100_Changes - ns/op |
16236087 ns/op |
16825394 ns/op |
0.96 |
BenchmarkChange/Push_100_Changes - B/op |
776300 B/op |
777642 B/op |
1.00 |
BenchmarkChange/Push_100_Changes - allocs/op |
8509 allocs/op |
8512 allocs/op |
1.00 |
BenchmarkChange/Push_1000_Changes |
130305456 ns/op 7160562 B/op 79324 allocs/op |
129494120 ns/op 7275446 B/op 79326 allocs/op |
1.01 |
BenchmarkChange/Push_1000_Changes - ns/op |
130305456 ns/op |
129494120 ns/op |
1.01 |
BenchmarkChange/Push_1000_Changes - B/op |
7160562 B/op |
7275446 B/op |
0.98 |
BenchmarkChange/Push_1000_Changes - allocs/op |
79324 allocs/op |
79326 allocs/op |
1.00 |
BenchmarkChange/Pull_10_Changes |
3661233 ns/op 124783 B/op 1456 allocs/op |
3718987 ns/op 124887 B/op 1457 allocs/op |
0.98 |
BenchmarkChange/Pull_10_Changes - ns/op |
3661233 ns/op |
3718987 ns/op |
0.98 |
BenchmarkChange/Pull_10_Changes - B/op |
124783 B/op |
124887 B/op |
1.00 |
BenchmarkChange/Pull_10_Changes - allocs/op |
1456 allocs/op |
1457 allocs/op |
1.00 |
BenchmarkChange/Pull_100_Changes |
5261338 ns/op 354915 B/op 5183 allocs/op |
5373170 ns/op 354806 B/op 5181 allocs/op |
0.98 |
BenchmarkChange/Pull_100_Changes - ns/op |
5261338 ns/op |
5373170 ns/op |
0.98 |
BenchmarkChange/Pull_100_Changes - B/op |
354915 B/op |
354806 B/op |
1.00 |
BenchmarkChange/Pull_100_Changes - allocs/op |
5183 allocs/op |
5181 allocs/op |
1.00 |
BenchmarkChange/Pull_1000_Changes |
10659241 ns/op 2196083 B/op 44681 allocs/op |
10873819 ns/op 2196656 B/op 44682 allocs/op |
0.98 |
BenchmarkChange/Pull_1000_Changes - ns/op |
10659241 ns/op |
10873819 ns/op |
0.98 |
BenchmarkChange/Pull_1000_Changes - B/op |
2196083 B/op |
2196656 B/op |
1.00 |
BenchmarkChange/Pull_1000_Changes - allocs/op |
44681 allocs/op |
44682 allocs/op |
1.00 |
BenchmarkSnapshot/Push_3KB_snapshot |
18647169 ns/op 907318 B/op 8511 allocs/op |
19111288 ns/op 900894 B/op 8515 allocs/op |
0.98 |
BenchmarkSnapshot/Push_3KB_snapshot - ns/op |
18647169 ns/op |
19111288 ns/op |
0.98 |
BenchmarkSnapshot/Push_3KB_snapshot - B/op |
907318 B/op |
900894 B/op |
1.01 |
BenchmarkSnapshot/Push_3KB_snapshot - allocs/op |
8511 allocs/op |
8515 allocs/op |
1.00 |
BenchmarkSnapshot/Push_30KB_snapshot |
133084233 ns/op 8178582 B/op 89106 allocs/op |
135763402 ns/op 8191528 B/op 88273 allocs/op |
0.98 |
BenchmarkSnapshot/Push_30KB_snapshot - ns/op |
133084233 ns/op |
135763402 ns/op |
0.98 |
BenchmarkSnapshot/Push_30KB_snapshot - B/op |
8178582 B/op |
8191528 B/op |
1.00 |
BenchmarkSnapshot/Push_30KB_snapshot - allocs/op |
89106 allocs/op |
88273 allocs/op |
1.01 |
BenchmarkSnapshot/Pull_3KB_snapshot |
7496526 ns/op 1116506 B/op 20066 allocs/op |
7464985 ns/op 1115898 B/op 20063 allocs/op |
1.00 |
BenchmarkSnapshot/Pull_3KB_snapshot - ns/op |
7496526 ns/op |
7464985 ns/op |
1.00 |
BenchmarkSnapshot/Pull_3KB_snapshot - B/op |
1116506 B/op |
1115898 B/op |
1.00 |
BenchmarkSnapshot/Pull_3KB_snapshot - allocs/op |
20066 allocs/op |
20063 allocs/op |
1.00 |
BenchmarkSnapshot/Pull_30KB_snapshot |
19751697 ns/op 9302907 B/op 193609 allocs/op |
20013500 ns/op 9302880 B/op 193606 allocs/op |
0.99 |
BenchmarkSnapshot/Pull_30KB_snapshot - ns/op |
19751697 ns/op |
20013500 ns/op |
0.99 |
BenchmarkSnapshot/Pull_30KB_snapshot - B/op |
9302907 B/op |
9302880 B/op |
1.00 |
BenchmarkSnapshot/Pull_30KB_snapshot - allocs/op |
193609 allocs/op |
193606 allocs/op |
1.00 |
BenchmarkSplayTree/stress_test_100000 |
0.1943 ns/op 0 B/op 0 allocs/op |
0.1916 ns/op 0 B/op 0 allocs/op |
1.01 |
BenchmarkSplayTree/stress_test_100000 - ns/op |
0.1943 ns/op |
0.1916 ns/op |
1.01 |
BenchmarkSplayTree/stress_test_100000 - B/op |
0 B/op |
0 B/op |
1 |
BenchmarkSplayTree/stress_test_100000 - allocs/op |
0 allocs/op |
0 allocs/op |
1 |
BenchmarkSplayTree/stress_test_200000 |
0.4105 ns/op 0 B/op 0 allocs/op |
0.3796 ns/op 0 B/op 0 allocs/op |
1.08 |
BenchmarkSplayTree/stress_test_200000 - ns/op |
0.4105 ns/op |
0.3796 ns/op |
1.08 |
BenchmarkSplayTree/stress_test_200000 - B/op |
0 B/op |
0 B/op |
1 |
BenchmarkSplayTree/stress_test_200000 - allocs/op |
0 allocs/op |
0 allocs/op |
1 |
BenchmarkSplayTree/stress_test_300000 |
0.5636 ns/op 0 B/op 0 allocs/op |
0.5706 ns/op 0 B/op 0 allocs/op |
0.99 |
BenchmarkSplayTree/stress_test_300000 - ns/op |
0.5636 ns/op |
0.5706 ns/op |
0.99 |
BenchmarkSplayTree/stress_test_300000 - B/op |
0 B/op |
0 B/op |
1 |
BenchmarkSplayTree/stress_test_300000 - allocs/op |
0 allocs/op |
0 allocs/op |
1 |
BenchmarkSplayTree/random_access_100000 |
0.0133 ns/op 0 B/op 0 allocs/op |
0.0125 ns/op 0 B/op 0 allocs/op |
1.06 |
BenchmarkSplayTree/random_access_100000 - ns/op |
0.0133 ns/op |
0.0125 ns/op |
1.06 |
BenchmarkSplayTree/random_access_100000 - B/op |
0 B/op |
0 B/op |
1 |
BenchmarkSplayTree/random_access_100000 - allocs/op |
0 allocs/op |
0 allocs/op |
1 |
BenchmarkSplayTree/random_access_200000 |
0.02925 ns/op 0 B/op 0 allocs/op |
0.03033 ns/op 0 B/op 0 allocs/op |
0.96 |
BenchmarkSplayTree/random_access_200000 - ns/op |
0.02925 ns/op |
0.03033 ns/op |
0.96 |
BenchmarkSplayTree/random_access_200000 - B/op |
0 B/op |
0 B/op |
1 |
BenchmarkSplayTree/random_access_200000 - allocs/op |
0 allocs/op |
0 allocs/op |
1 |
BenchmarkSplayTree/random_access_300000 |
0.04443 ns/op 0 B/op 0 allocs/op |
0.04175 ns/op 0 B/op 0 allocs/op |
1.06 |
BenchmarkSplayTree/random_access_300000 - ns/op |
0.04443 ns/op |
0.04175 ns/op |
1.06 |
BenchmarkSplayTree/random_access_300000 - B/op |
0 B/op |
0 B/op |
1 |
BenchmarkSplayTree/random_access_300000 - allocs/op |
0 allocs/op |
0 allocs/op |
1 |
BenchmarkSplayTree/editing_trace_bench |
0.002084 ns/op 0 B/op 0 allocs/op |
0.002447 ns/op 0 B/op 0 allocs/op |
0.85 |
BenchmarkSplayTree/editing_trace_bench - ns/op |
0.002084 ns/op |
0.002447 ns/op |
0.85 |
BenchmarkSplayTree/editing_trace_bench - B/op |
0 B/op |
0 B/op |
1 |
BenchmarkSplayTree/editing_trace_bench - allocs/op |
0 allocs/op |
0 allocs/op |
1 |
BenchmarkSync/memory_sync_10_test |
6923 ns/op 1341 B/op 35 allocs/op |
7098 ns/op 1341 B/op 35 allocs/op |
0.98 |
BenchmarkSync/memory_sync_10_test - ns/op |
6923 ns/op |
7098 ns/op |
0.98 |
BenchmarkSync/memory_sync_10_test - B/op |
1341 B/op |
1341 B/op |
1 |
BenchmarkSync/memory_sync_10_test - allocs/op |
35 allocs/op |
35 allocs/op |
1 |
BenchmarkSync/memory_sync_100_test |
55924 ns/op 9514 B/op 268 allocs/op |
55729 ns/op 9522 B/op 268 allocs/op |
1.00 |
BenchmarkSync/memory_sync_100_test - ns/op |
55924 ns/op |
55729 ns/op |
1.00 |
BenchmarkSync/memory_sync_100_test - B/op |
9514 B/op |
9522 B/op |
1.00 |
BenchmarkSync/memory_sync_100_test - allocs/op |
268 allocs/op |
268 allocs/op |
1 |
BenchmarkSync/memory_sync_1000_test |
618716 ns/op 75852 B/op 2110 allocs/op |
617823 ns/op 75483 B/op 2098 allocs/op |
1.00 |
BenchmarkSync/memory_sync_1000_test - ns/op |
618716 ns/op |
617823 ns/op |
1.00 |
BenchmarkSync/memory_sync_1000_test - B/op |
75852 B/op |
75483 B/op |
1.00 |
BenchmarkSync/memory_sync_1000_test - allocs/op |
2110 allocs/op |
2098 allocs/op |
1.01 |
BenchmarkSync/memory_sync_10000_test |
7899401 ns/op 758839 B/op 20445 allocs/op |
7698417 ns/op 762399 B/op 20548 allocs/op |
1.03 |
BenchmarkSync/memory_sync_10000_test - ns/op |
7899401 ns/op |
7698417 ns/op |
1.03 |
BenchmarkSync/memory_sync_10000_test - B/op |
758839 B/op |
762399 B/op |
1.00 |
BenchmarkSync/memory_sync_10000_test - allocs/op |
20445 allocs/op |
20548 allocs/op |
0.99 |
BenchmarkTextEditing |
5216441776 ns/op 3922673752 B/op 20619751 allocs/op |
5307830784 ns/op 3922717736 B/op 20619846 allocs/op |
0.98 |
BenchmarkTextEditing - ns/op |
5216441776 ns/op |
5307830784 ns/op |
0.98 |
BenchmarkTextEditing - B/op |
3922673752 B/op |
3922717736 B/op |
1.00 |
BenchmarkTextEditing - allocs/op |
20619751 allocs/op |
20619846 allocs/op |
1.00 |
BenchmarkTree/10000_vertices_to_protobuf |
4165840 ns/op 6363234 B/op 70025 allocs/op |
4541143 ns/op 6363275 B/op 70025 allocs/op |
0.92 |
BenchmarkTree/10000_vertices_to_protobuf - ns/op |
4165840 ns/op |
4541143 ns/op |
0.92 |
BenchmarkTree/10000_vertices_to_protobuf - B/op |
6363234 B/op |
6363275 B/op |
1.00 |
BenchmarkTree/10000_vertices_to_protobuf - allocs/op |
70025 allocs/op |
70025 allocs/op |
1 |
BenchmarkTree/10000_vertices_from_protobuf |
216250081 ns/op 442304235 B/op 290038 allocs/op |
228247159 ns/op 442304382 B/op 290039 allocs/op |
0.95 |
BenchmarkTree/10000_vertices_from_protobuf - ns/op |
216250081 ns/op |
228247159 ns/op |
0.95 |
BenchmarkTree/10000_vertices_from_protobuf - B/op |
442304235 B/op |
442304382 B/op |
1.00 |
BenchmarkTree/10000_vertices_from_protobuf - allocs/op |
290038 allocs/op |
290039 allocs/op |
1.00 |
BenchmarkTree/20000_vertices_to_protobuf |
8869321 ns/op 12890887 B/op 140028 allocs/op |
9325612 ns/op 12891042 B/op 140028 allocs/op |
0.95 |
BenchmarkTree/20000_vertices_to_protobuf - ns/op |
8869321 ns/op |
9325612 ns/op |
0.95 |
BenchmarkTree/20000_vertices_to_protobuf - B/op |
12890887 B/op |
12891042 B/op |
1.00 |
BenchmarkTree/20000_vertices_to_protobuf - allocs/op |
140028 allocs/op |
140028 allocs/op |
1 |
BenchmarkTree/20000_vertices_from_protobuf |
879554260 ns/op 1697470080 B/op 580043 allocs/op |
904962506 ns/op 1697478344 B/op 580045 allocs/op |
0.97 |
BenchmarkTree/20000_vertices_from_protobuf - ns/op |
879554260 ns/op |
904962506 ns/op |
0.97 |
BenchmarkTree/20000_vertices_from_protobuf - B/op |
1697470080 B/op |
1697478344 B/op |
1.00 |
BenchmarkTree/20000_vertices_from_protobuf - allocs/op |
580043 allocs/op |
580045 allocs/op |
1.00 |
BenchmarkTree/30000_vertices_to_protobuf |
13579998 ns/op 18976187 B/op 210029 allocs/op |
14244048 ns/op 18976105 B/op 210029 allocs/op |
0.95 |
BenchmarkTree/30000_vertices_to_protobuf - ns/op |
13579998 ns/op |
14244048 ns/op |
0.95 |
BenchmarkTree/30000_vertices_to_protobuf - B/op |
18976187 B/op |
18976105 B/op |
1.00 |
BenchmarkTree/30000_vertices_to_protobuf - allocs/op |
210029 allocs/op |
210029 allocs/op |
1 |
BenchmarkTree/30000_vertices_from_protobuf |
2021287482 ns/op 3751744656 B/op 870146 allocs/op |
2017247159 ns/op 3751752464 B/op 870145 allocs/op |
1.00 |
BenchmarkTree/30000_vertices_from_protobuf - ns/op |
2021287482 ns/op |
2017247159 ns/op |
1.00 |
BenchmarkTree/30000_vertices_from_protobuf - B/op |
3751744656 B/op |
3751752464 B/op |
1.00 |
BenchmarkTree/30000_vertices_from_protobuf - allocs/op |
870146 allocs/op |
870145 allocs/op |
1.00 |
BenchmarkVersionVector/clients_10 |
157781183 ns/op 745.0 1_changepack(bytes) 379.0 2_snapshot(bytes) 8.000 3_pushpull(ms) 6.000 4_attach(ms) 805.0 5_changepack_after_detach(bytes) 136.0 6_snapshot_after_detach(bytes) 8.000 7_pushpull_after_detach(ms) 21342946 B/op 83781 allocs/op |
160804913 ns/op 745.0 1_changepack(bytes) 379.0 2_snapshot(bytes) 8.000 3_pushpull(ms) 6.000 4_attach(ms) 805.0 5_changepack_after_detach(bytes) 136.0 6_snapshot_after_detach(bytes) 8.000 7_pushpull_after_detach(ms) 18308829 B/op 83708 allocs/op |
0.98 |
BenchmarkVersionVector/clients_10 - ns/op |
157781183 ns/op |
160804913 ns/op |
0.98 |
BenchmarkVersionVector/clients_10 - 1_changepack(bytes) |
745 1_changepack(bytes) |
745 1_changepack(bytes) |
1 |
BenchmarkVersionVector/clients_10 - 2_snapshot(bytes) |
379 2_snapshot(bytes) |
379 2_snapshot(bytes) |
1 |
BenchmarkVersionVector/clients_10 - 3_pushpull(ms) |
8 3_pushpull(ms) |
8 3_pushpull(ms) |
1 |
BenchmarkVersionVector/clients_10 - 4_attach(ms) |
6 4_attach(ms) |
6 4_attach(ms) |
1 |
BenchmarkVersionVector/clients_10 - 5_changepack_after_detach(bytes) |
805 5_changepack_after_detach(bytes) |
805 5_changepack_after_detach(bytes) |
1 |
BenchmarkVersionVector/clients_10 - 6_snapshot_after_detach(bytes) |
136 6_snapshot_after_detach(bytes) |
136 6_snapshot_after_detach(bytes) |
1 |
BenchmarkVersionVector/clients_10 - 7_pushpull_after_detach(ms) |
8 7_pushpull_after_detach(ms) |
8 7_pushpull_after_detach(ms) |
1 |
BenchmarkVersionVector/clients_10 - B/op |
21342946 B/op |
18308829 B/op |
1.17 |
BenchmarkVersionVector/clients_10 - allocs/op |
83781 allocs/op |
83708 allocs/op |
1.00 |
BenchmarkVersionVector/clients_100 |
1415014937 ns/op 6145 1_changepack(bytes) 3079 2_snapshot(bytes) 10.00 3_pushpull(ms) 10.00 4_attach(ms) 6210 5_changepack_after_detach(bytes) 137.0 6_snapshot_after_detach(bytes) 9.000 7_pushpull_after_detach(ms) 228741016 B/op 1539726 allocs/op |
1446448213 ns/op 6145 1_changepack(bytes) 3079 2_snapshot(bytes) 11.00 3_pushpull(ms) 11.00 4_attach(ms) 6210 5_changepack_after_detach(bytes) 137.0 6_snapshot_after_detach(bytes) 9.000 7_pushpull_after_detach(ms) 230333096 B/op 1539728 allocs/op |
0.98 |
BenchmarkVersionVector/clients_100 - ns/op |
1415014937 ns/op |
1446448213 ns/op |
0.98 |
BenchmarkVersionVector/clients_100 - 1_changepack(bytes) |
6145 1_changepack(bytes) |
6145 1_changepack(bytes) |
1 |
BenchmarkVersionVector/clients_100 - 2_snapshot(bytes) |
3079 2_snapshot(bytes) |
3079 2_snapshot(bytes) |
1 |
BenchmarkVersionVector/clients_100 - 3_pushpull(ms) |
10 3_pushpull(ms) |
11 3_pushpull(ms) |
0.91 |
BenchmarkVersionVector/clients_100 - 4_attach(ms) |
10 4_attach(ms) |
11 4_attach(ms) |
0.91 |
BenchmarkVersionVector/clients_100 - 5_changepack_after_detach(bytes) |
6210 5_changepack_after_detach(bytes) |
6210 5_changepack_after_detach(bytes) |
1 |
BenchmarkVersionVector/clients_100 - 6_snapshot_after_detach(bytes) |
137 6_snapshot_after_detach(bytes) |
137 6_snapshot_after_detach(bytes) |
1 |
BenchmarkVersionVector/clients_100 - 7_pushpull_after_detach(ms) |
9 7_pushpull_after_detach(ms) |
9 7_pushpull_after_detach(ms) |
1 |
BenchmarkVersionVector/clients_100 - B/op |
228741016 B/op |
230333096 B/op |
0.99 |
BenchmarkVersionVector/clients_100 - allocs/op |
1539726 allocs/op |
1539728 allocs/op |
1.00 |
BenchmarkVersionVector/clients_1000 |
60733772485 ns/op 60155 1_changepack(bytes) 30081 2_snapshot(bytes) 94.00 3_pushpull(ms) 222.0 4_attach(ms) 60217 5_changepack_after_detach(bytes) 139.0 6_snapshot_after_detach(bytes) 23.00 7_pushpull_after_detach(ms) 22065351584 B/op 111762517 allocs/op |
60983609373 ns/op 60155 1_changepack(bytes) 30081 2_snapshot(bytes) 95.00 3_pushpull(ms) 218.0 4_attach(ms) 60217 5_changepack_after_detach(bytes) 139.0 6_snapshot_after_detach(bytes) 23.00 7_pushpull_after_detach(ms) 22062927088 B/op 111759018 allocs/op |
1.00 |
BenchmarkVersionVector/clients_1000 - ns/op |
60733772485 ns/op |
60983609373 ns/op |
1.00 |
BenchmarkVersionVector/clients_1000 - 1_changepack(bytes) |
60155 1_changepack(bytes) |
60155 1_changepack(bytes) |
1 |
BenchmarkVersionVector/clients_1000 - 2_snapshot(bytes) |
30081 2_snapshot(bytes) |
30081 2_snapshot(bytes) |
1 |
BenchmarkVersionVector/clients_1000 - 3_pushpull(ms) |
94 3_pushpull(ms) |
95 3_pushpull(ms) |
0.99 |
BenchmarkVersionVector/clients_1000 - 4_attach(ms) |
222 4_attach(ms) |
218 4_attach(ms) |
1.02 |
BenchmarkVersionVector/clients_1000 - 5_changepack_after_detach(bytes) |
60217 5_changepack_after_detach(bytes) |
60217 5_changepack_after_detach(bytes) |
1 |
BenchmarkVersionVector/clients_1000 - 6_snapshot_after_detach(bytes) |
139 6_snapshot_after_detach(bytes) |
139 6_snapshot_after_detach(bytes) |
1 |
BenchmarkVersionVector/clients_1000 - 7_pushpull_after_detach(ms) |
23 7_pushpull_after_detach(ms) |
23 7_pushpull_after_detach(ms) |
1 |
BenchmarkVersionVector/clients_1000 - B/op |
22065351584 B/op |
22062927088 B/op |
1.00 |
BenchmarkVersionVector/clients_1000 - allocs/op |
111762517 allocs/op |
111759018 allocs/op |
1.00 |
This comment was automatically generated by workflow using github-action-benchmark.
There was a problem hiding this comment.
Actionable comments posted: 3
🧹 Nitpick comments (5)
server/webhook/event.go (3)
53-53: Address the TODO comment about status code handling.The comment indicates that status codes should be handled properly. Consider implementing retry logic for specific status codes (e.g., 429 Too Many Requests, 5xx Server Errors) and logging for monitoring purposes.
Would you like me to help implement proper status code handling?
48-64: Enhance error handling with more context.The error messages could be more descriptive to aid in debugging. Consider wrapping errors with additional context.
- return fmt.Errorf("invalid event webhook type: %s", eventType) + return fmt.Errorf("invalid event webhook type %q for document %q", eventType, docKey) - return fmt.Errorf("marshal event webhook request: %w", err) + return fmt.Errorf("failed to marshal event webhook request for document %q: %w", docKey, err) - return fmt.Errorf("send event webhook: %w", err) + return fmt.Errorf("failed to send event webhook for document %q to %q: %w", docKey, prj.EventWebhookURL, err)
72-72: Extract time format as a constant.The time format string "2006-01-02T15:04:05.000Z" should be extracted as a constant for better maintainability and reusability.
+const ( + // RFC3339MilliZ is the RFC3339 format with millisecond precision and UTC timezone + RFC3339MilliZ = "2006-01-02T15:04:05.000Z" +) - IssuedAt: gotime.Now().UTC().Format("2006-01-02T15:04:05.000Z"), + IssuedAt: gotime.Now().UTC().Format(RFC3339MilliZ),test/integration/event_webhook_test.go (2)
133-134: Extract magic numbers and durations as constants.The test uses several magic numbers and durations that should be extracted as constants for better maintainability.
+const ( + // waitWebhookReceived is the duration to wait for webhook requests to be received + waitWebhookReceived = 10 * time.Millisecond + + // webhookCacheTTL is the TTL for webhook cache in tests + webhookCacheTTL = 10 * time.Millisecond + + // projectCacheTTL is the TTL for project cache in tests + projectCacheTTL = 1 * time.Millisecond +)Also applies to: 211-212, 250-251
59-80: Enhance test helper function reusability.The
newWebhookServerfunction could be more reusable by accepting a callback for custom request handling.-func newWebhookServer(t *testing.T, secretKey, docKey string) (*httptest.Server, *int32) { +func newWebhookServer(t *testing.T, secretKey string, handler func(w http.ResponseWriter, r *http.Request)) (*httptest.Server, *int32) { var reqCnt int32 srv := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { atomic.AddInt32(&reqCnt, 1) signatureHeader := r.Header.Get("X-Signature-256") assert.NotZero(t, len(signatureHeader)) body, err := io.ReadAll(r.Body) assert.NoError(t, err) assert.NoError(t, verifySignature(signatureHeader, secretKey, body)) - req := &types.EventWebhookRequest{} - assert.NoError(t, gojson.Unmarshal(body, req)) - assert.Equal(t, types.DocRootChanged, req.Type) - assert.Equal(t, docKey, req.Attributes.Key) - - w.WriteHeader(http.StatusOK) + if handler != nil { + handler(w, r) + } else { + w.WriteHeader(http.StatusOK) + } })) t.Cleanup(func() { srv.Close() }) return srv, &reqCnt }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (14)
cmd/yorkie/server.go(3 hunks)pkg/webhook/client.go(2 hunks)server/backend/backend.go(4 hunks)server/backend/config.go(4 hunks)server/backend/config_test.go(2 hunks)server/config.go(2 hunks)server/config.sample.yml(1 hunks)server/config_test.go(2 hunks)server/packs/packs_test.go(1 hunks)server/rpc/server_test.go(1 hunks)server/webhook/event.go(1 hunks)test/complex/main_test.go(1 hunks)test/helper/helper.go(2 hunks)test/integration/event_webhook_test.go(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (9)
- pkg/webhook/client.go
- server/packs/packs_test.go
- server/config.sample.yml
- server/config.go
- server/config_test.go
- server/rpc/server_test.go
- test/helper/helper.go
- test/complex/main_test.go
- server/backend/config_test.go
🧰 Additional context used
🧠 Learnings (1)
test/integration/event_webhook_test.go (1)
Learnt from: hackerwins
PR: yorkie-team/yorkie#1156
File: test/integration/event_webhook_test.go:274-274
Timestamp: 2025-02-18T10:51:18.964Z
Learning: In the Yorkie project, caching should not be used as a mechanism for rate limiting or throttling webhook events, as it makes the intent unclear. Instead, use explicit rate limiting, debouncing, or event aggregation mechanisms.
⏰ Context from checks skipped due to timeout of 90000ms (3)
- GitHub Check: build
- GitHub Check: complex-test
- GitHub Check: bench
🔇 Additional comments (4)
server/backend/backend.go (1)
48-57: LGTM! Good separation of concerns.The renaming of webhook fields to
AuthWebhookCacheandAuthWebhookClientand the addition ofEventWebhookClientimproves code clarity by clearly distinguishing between authentication and event webhook functionalities.test/integration/event_webhook_test.go (1)
271-274: Implement proper rate limiting instead of using cache.Based on the retrieved learning from @hackerwins, caching should not be used as a mechanism for rate limiting webhook events. Instead, implement explicit rate limiting using token bucket, leaky bucket, or event aggregation.
Would you like me to help implement a proper rate limiting solution?
cmd/yorkie/server.go (2)
56-59: LGTM! Event webhook configuration variables follow the established pattern.The new event webhook configuration variables mirror the structure of the existing auth webhook variables, maintaining consistency in the codebase.
358-358: Fix flag name for auth webhook cache TTL.The flag name for auth webhook cache TTL contains "auth" twice, which appears to be a copy-paste error.
- "auth-webhook-cache-auth-ttl", + "auth-webhook-cache-ttl",
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (7)
api/types/project.go (1)
41-45: Consider adding URL format validation.While the field implementation is correct, consider adding validation to ensure the EventWebhookURL follows a valid URL format.
api/types/updatable_project_fields.go (1)
88-107: Consider improving error handling in init().The current implementation exits the program on validation registration errors. Consider logging the error and handling it gracefully instead of terminating the program.
- fmt.Fprintln(os.Stderr, "updatable project fields: ", err) - os.Exit(1) + log.Printf("Failed to register webhook event validation: %v", err) + // Consider implementing a fallback or recovery mechanismapi/types/updatable_project_fields_test.go (1)
73-75: Consider adding more edge cases.While the invalid event test is good, consider adding tests for:
- Empty event list
- Multiple invalid events
- Mixed valid and invalid events
api/docs/yorkie/v1/resources.openapi.yaml (4)
1338-1345: Enhance Documentation foreventWebhookEventsPropertyThe new
eventWebhookEventsproperty is added to theyorkie.v1.Projectschema as an array of strings withadditionalPropertiesset to false. While the structure is correct, the empty description may leave API consumers unclear about the intended use and expected values. Consider adding a more informative description to clarify its purpose and any accepted event types.
1346-1349: Clarify Description foreventWebhookUrlFieldThe
eventWebhookUrlproperty is integrated into theyorkie.v1.Projectschema correctly as a string withadditionalPropertiesdisabled. Similar to the previous property, the empty description could be enriched with details explaining its role (e.g., acceptable URL formats or usage context) to better assist API consumers.
1702-1713: Integrate New Webhook Fields in UpdatableProjectFieldsThe new fields
eventWebhookEventsandeventWebhookUrlare appropriately added to theyorkie.v1.UpdatableProjectFieldsschema using$refto corresponding definitions. This promotes schema reuse and consistency. However, both fields currently have empty descriptions. Providing more detailed documentation here would help developers understand their purpose and usage in webhook configuration.
1735-1748: DefineEventWebhookEventsSchema with Clear Usage GuidanceThe newly defined
yorkie.v1.UpdatableProjectFields.EventWebhookEventsschema is well-structured—it restricts properties withadditionalProperties: falseand defines aneventsproperty as an array of strings. To improve clarity for API consumers, consider adding a descriptive explanation outlining what events are expected and how this schema is leveraged in the webhook configuration.
📜 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 (17)
api/converter/from_pb.go(2 hunks)api/converter/to_pb.go(2 hunks)api/docs/yorkie/v1/admin.openapi.yaml(3 hunks)api/docs/yorkie/v1/cluster.openapi.yaml(1 hunks)api/docs/yorkie/v1/resources.openapi.yaml(3 hunks)api/types/project.go(2 hunks)api/types/project_test.go(1 hunks)api/types/updatable_project_fields.go(2 hunks)api/types/updatable_project_fields_test.go(3 hunks)api/yorkie/v1/resources.proto(1 hunks)cmd/yorkie/project/list.go(2 hunks)cmd/yorkie/project/update.go(6 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)test/bench/grpc_bench_test.go(2 hunks)test/integration/event_webhook_test.go(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (8)
- server/backend/database/project_info_test.go
- api/docs/yorkie/v1/cluster.openapi.yaml
- cmd/yorkie/project/list.go
- server/backend/database/project_info.go
- api/converter/to_pb.go
- cmd/yorkie/project/update.go
- api/yorkie/v1/resources.proto
- api/docs/yorkie/v1/admin.openapi.yaml
🧰 Additional context used
🧠 Learnings (1)
test/integration/event_webhook_test.go (1)
Learnt from: hackerwins
PR: yorkie-team/yorkie#1156
File: test/integration/event_webhook_test.go:274-274
Timestamp: 2025-02-18T10:51:18.964Z
Learning: In the Yorkie project, caching should not be used as a mechanism for rate limiting or throttling webhook events, as it makes the intent unclear. Instead, use explicit rate limiting, debouncing, or event aggregation mechanisms.
🪛 GitHub Check: bench
test/bench/grpc_bench_test.go
[failure] 114-114:
unknown field eventWebhookEvents in struct literal of type "github.com/yorkie-team/yorkie/api/types".UpdatableProjectFields, but does have EventWebhookEvents
⏰ Context from checks skipped due to timeout of 90000ms (2)
- GitHub Check: complex-test
- GitHub Check: build
🔇 Additional comments (8)
api/types/project_test.go (1)
53-74: LGTM! Well-structured test cases.The test cases comprehensively cover the event webhook functionality, including positive and negative scenarios. The pattern follows the existing auth webhook tests, maintaining consistency.
api/types/project.go (1)
83-100: LGTM! Well-implemented webhook check.The RequireEventWebhook method follows the same pattern as RequireAuth, making the code consistent and maintainable.
api/types/updatable_project_fields.go (1)
42-47: LGTM! Well-defined validation rules.The validation tags are properly configured for both fields, ensuring data integrity.
api/types/updatable_project_fields_test.go (1)
37-40: LGTM! Good test setup.The test data is well-structured and follows the established pattern.
test/integration/event_webhook_test.go (2)
48-57: LGTM! Secure signature verification implementation.The HMAC-SHA256 signature verification is correctly implemented using constant-time comparison.
59-80: LGTM! Well-structured webhook test server.The test server properly validates webhook signatures and request format, with atomic request counting for concurrent tests.
api/converter/from_pb.go (1)
58-59: LGTM! Clean protobuf conversion implementation.The event webhook field conversions are correctly implemented, maintaining consistency with the existing conversion patterns.
server/backend/database/testcases/testcases.go (1)
770-771: LGTM! Comprehensive test coverage for event webhook fields.The test cases thoroughly validate event webhook functionality:
- Test data initialization
- Field update verification
- Edge cases and validation
Also applies to: 800-801, 820-858
| t.Run("throttling event test", func(t *testing.T) { | ||
| t.Skip("remove this after implement advanced event timing control") | ||
|
|
||
| expectedUpdates := 5 |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Consider implementing rate limiting instead of caching.
Based on project learnings, caching should not be used for rate limiting webhook events. Instead, implement explicit rate limiting or event aggregation mechanisms.
Would you like me to help implement a proper rate limiting mechanism for the webhook events?
hackerwins
force-pushed
the
event-webhook
branch
from
2dbccd2 to
33cc60d
Compare
hackerwins
force-pushed
the
event-webhook
branch
from
33cc60d to
2b86a00
Compare
There was a problem hiding this comment.
Actionable comments posted: 2
🔭 Outside diff range comments (1)
api/docs/yorkie/v1/admin.openapi.yaml (1)
2312-2312:⚠️ Potential issueReview the Removal of the ApiKeyAuth Security Entry
The security section previously contained the line
ApiKeyAuth: [], which has been removed. Please verify that this removal is intentional and that the API’s security is handled appropriately elsewhere. If API key authentication is still required, you might need to reinstate or update this configuration.
🧹 Nitpick comments (5)
server/backend/database/testcases/testcases.go (1)
820-838: Add test case for concurrent webhook URL updates.The test verifies individual webhook URL updates but doesn't test concurrent updates from multiple clients.
Consider adding a test case for concurrent updates:
+t.Run("concurrent webhook URL updates test", func(t *testing.T) { + var wg sync.WaitGroup + for i := 0; i < 10; i++ { + wg.Add(1) + go func(i int) { + defer wg.Done() + newURL := fmt.Sprintf("http://localhost:400%d", i) + fields := &types.UpdatableProjectFields{ + EventWebhookURL: &newURL, + } + _, err := db.UpdateProjectInfo(ctx, dummyOwnerID, id, fields) + assert.NoError(t, err) + }(i) + } + wg.Wait() +})api/docs/yorkie/v1/resources.openapi.yaml (1)
1702-1713: Add examples and descriptions to webhook fields.The schema would benefit from examples and descriptions to improve API documentation.
Add examples and descriptions:
eventWebhookEvents: $ref: '#/components/schemas/yorkie.v1.UpdatableProjectFields.EventWebhookEvents' additionalProperties: false - description: "" + description: "List of event types that trigger webhook notifications" title: event_webhook_events type: object eventWebhookUrl: $ref: '#/components/schemas/google.protobuf.StringValue' additionalProperties: false - description: "" + description: "URL endpoint that receives webhook notifications" + example: "https://api.example.com/webhooks/yorkie" title: event_webhook_url type: objectapi/docs/yorkie/v1/admin.openapi.yaml (3)
1829-1840: New Event Webhook Attributes in the Project SchemaThe new fields
eventWebhookEventsandeventWebhookUrlare correctly added to theyorkie.v1.Projectschema. However, both fields have an empty description. Consider providing clear, user-friendly descriptions so that API consumers better understand their purpose and expected format.
2149-2160: New Event Webhook Fields in UpdatableProjectFieldsThe addition of
eventWebhookEvents(referencing the new schema) andeventWebhookUrl(usinggoogle.protobuf.StringValue) in theyorkie.v1.UpdatableProjectFieldsschema is implemented correctly. As with the Project schema, the descriptions for these fields are empty; adding detailed descriptions would improve documentation quality and clarity for API users.
2180-2192: Definition of EventWebhookEvents SchemaThe new schema
yorkie.v1.UpdatableProjectFields.EventWebhookEventsis defined as expected with aneventsproperty that accepts an array of strings. This schema will ensure that the webhook events are structured consistently. Consider enhancing the description to specify what event values are allowed or expected.
📜 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(2 hunks)api/docs/yorkie/v1/admin.openapi.yaml(3 hunks)api/docs/yorkie/v1/cluster.openapi.yaml(1 hunks)api/docs/yorkie/v1/resources.openapi.yaml(3 hunks)api/types/project.go(2 hunks)api/types/project_test.go(1 hunks)api/types/updatable_project_fields.go(2 hunks)api/types/updatable_project_fields_test.go(3 hunks)api/yorkie/v1/resources.proto(1 hunks)cmd/yorkie/project/list.go(2 hunks)cmd/yorkie/project/update.go(6 hunks)server/backend/backend.go(4 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/packs/packs.go(3 hunks)server/webhook/events.go(1 hunks)test/bench/grpc_bench_test.go(2 hunks)test/integration/event_webhook_test.go(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (13)
- server/backend/database/project_info_test.go
- test/bench/grpc_bench_test.go
- server/packs/packs.go
- api/docs/yorkie/v1/cluster.openapi.yaml
- api/types/project_test.go
- server/backend/database/project_info.go
- api/types/project.go
- api/types/updatable_project_fields_test.go
- api/types/updatable_project_fields.go
- api/yorkie/v1/resources.proto
- api/converter/to_pb.go
- server/backend/backend.go
- cmd/yorkie/project/list.go
🧰 Additional context used
🧠 Learnings (1)
test/integration/event_webhook_test.go (1)
Learnt from: hackerwins
PR: yorkie-team/yorkie#1156
File: test/integration/event_webhook_test.go:274-274
Timestamp: 2025-02-18T10:51:18.964Z
Learning: In the Yorkie project, caching should not be used as a mechanism for rate limiting or throttling webhook events, as it makes the intent unclear. Instead, use explicit rate limiting, debouncing, or event aggregation mechanisms.
⏰ Context from checks skipped due to timeout of 90000ms (3)
- GitHub Check: bench
- GitHub Check: build
- GitHub Check: complex-test
🔇 Additional comments (15)
server/webhook/events.go (2)
76-92: LGTM! Well-formatted request body builder.The function follows best practices by using UTC time and standard RFC3339 format for timestamps. Error handling for JSON marshaling is appropriate.
38-74:✅ Verification successful
LGTM! Well-structured event sending logic.
The function has proper error handling, validation, and context propagation. The error messages are descriptive and the status code check ensures webhook delivery.
Let's verify the error handling paths:
🏁 Script executed:
#!/bin/bash # Description: Check error handling coverage in tests rg -A 5 "ErrUnexpectedStatusCode|invalid event webhook type|marshal event webhook request|send event webhook"Length of output: 5190
LGTM – Error handling and validations are robust.
TheSendEventfunction correctly validates the webhook type, builds the request body, and handles errors from the webhook client with clear, descriptive messages. Additionally, our review confirms that tests cover error scenarios such as invalid event types, marshalling failures, and unexpected status codes.cmd/yorkie/project/update.go (5)
40-42: LGTM! Consistent flag naming.The new event webhook flags follow the same naming pattern as the existing auth webhook flags, maintaining consistency.
58-60: LGTM! Clear event type definition.The event types list is well-defined and aligns with the PR objective of introducing the
DocRootChangedevent.
185-222: LGTM! Well-designed reusable function.The function efficiently handles string slice updates with:
- Clear handling of "ALL" keyword
- Proper use of map for uniqueness
- Good reusability for both auth and event webhook configurations
250-255: LGTM! Consistent flag definition.The event webhook URL flag follows the same pattern as other URL flags, with clear description.
256-267: LGTM! Well-documented flag definitions.The event webhook events flags are consistently defined with clear descriptions of the 'ALL' option functionality.
test/integration/event_webhook_test.go (3)
48-80: LGTM! Robust webhook verification and test server setup.The implementation follows security best practices with:
- Proper HMAC signature verification
- Safe comparison using hmac.Equal
- Thread-safe request counting
- Proper test cleanup
110-245: LGTM! Comprehensive test coverage.The test cases thoroughly verify:
- Webhook registration/unregistration
- Event triggering for root changes
- Presence changes (correctly not triggering events)
- Combined root and presence changes
271-274: Consider implementing rate limiting instead of caching.Based on project learnings, caching should not be used for rate limiting webhook events. Instead, implement explicit rate limiting or event aggregation mechanisms.
Would you like me to help implement a proper rate limiting mechanism for the webhook events?
api/converter/from_pb.go (1)
58-59: LGTM! Consistent protobuf conversion handling.The event webhook field conversions follow the same pattern as existing fields, with proper null checks for updatable fields.
Also applies to: 927-932
server/backend/database/testcases/testcases.go (3)
770-771: LGTM: Event webhook test setup.The test setup correctly initializes event webhook test data with appropriate test values.
786-788: Verify event webhook fields validation.The test includes event webhook fields but doesn't verify validation of the webhook URL format or event types.
Consider adding test cases to verify:
- Invalid webhook URL formats
- Invalid event types
- Empty event types array
fields := &types.UpdatableProjectFields{ Name: &newName, AuthWebhookURL: &newAuthWebhookURL, AuthWebhookMethods: &newAuthWebhookMethods, EventWebhookURL: &newEventWebhookURL, EventWebhookEvents: &newEventWebhookEvents, ClientDeactivateThreshold: &newClientDeactivateThreshold, } +// Add validation test cases +invalidURL := "not-a-url" +fields = &types.UpdatableProjectFields{ + EventWebhookURL: &invalidURL, +} +err := fields.Validate() +assert.Error(t, err) + +invalidEvents := []string{"InvalidEvent"} +fields = &types.UpdatableProjectFields{ + EventWebhookEvents: &invalidEvents, +} +err = fields.Validate() +assert.Error(t, err)
800-801: LGTM: Event webhook assertions.The assertions correctly verify that the event webhook URL and events are updated and match the expected values.
api/docs/yorkie/v1/resources.openapi.yaml (1)
1338-1349: LGTM: Project schema event webhook properties.The schema correctly defines the event webhook properties with appropriate types and constraints.
| // 04. Update EventWebhookEvents test | ||
| var newEventWebhookEvents2 []string | ||
| newAuthWebhookMethods2 := []string{ | ||
| string(types.DetachDocument), | ||
| string(types.PushPull), | ||
| } | ||
| fields = &types.UpdatableProjectFields{ | ||
| AuthWebhookMethods: &newAuthWebhookMethods2, | ||
| EventWebhookEvents: &newEventWebhookEvents2, | ||
| } | ||
| assert.NoError(t, fields.Validate()) | ||
| res, err = db.UpdateProjectInfo(ctx, dummyOwnerID, id, fields) | ||
| assert.NoError(t, err) | ||
| updateInfo, err = db.FindProjectInfoByID(ctx, id) | ||
| assert.NoError(t, err) | ||
| assert.Equal(t, res, updateInfo) | ||
| assert.Equal(t, newEventWebhookEvents2, updateInfo.EventWebhookEvents) | ||
| assert.Equal(t, newAuthWebhookMethods2, updateInfo.AuthWebhookMethods) | ||
|
|
There was a problem hiding this comment.
🛠️ Refactor suggestion
Add test case for empty event webhook events array.
The test should verify behavior when setting an empty events array.
Add a test case to verify empty events array behavior:
var newEventWebhookEvents2 []string
newAuthWebhookMethods2 := []string{
string(types.DetachDocument),
string(types.PushPull),
}
fields = &types.UpdatableProjectFields{
AuthWebhookMethods: &newAuthWebhookMethods2,
EventWebhookEvents: &newEventWebhookEvents2,
}
assert.NoError(t, fields.Validate())
res, err = db.UpdateProjectInfo(ctx, dummyOwnerID, id, fields)
assert.NoError(t, err)
updateInfo, err = db.FindProjectInfoByID(ctx, id)
assert.NoError(t, err)
assert.Equal(t, res, updateInfo)
assert.Equal(t, newEventWebhookEvents2, updateInfo.EventWebhookEvents)
assert.Equal(t, newAuthWebhookMethods2, updateInfo.AuthWebhookMethods)
+// Test empty events array
+emptyEvents := make([]string, 0)
+fields = &types.UpdatableProjectFields{
+ EventWebhookEvents: &emptyEvents,
+}
+assert.NoError(t, fields.Validate())
+res, err = db.UpdateProjectInfo(ctx, dummyOwnerID, id, fields)
+assert.NoError(t, err)
+updateInfo, err = db.FindProjectInfoByID(ctx, id)
+assert.NoError(t, err)
+assert.Empty(t, updateInfo.EventWebhookEvents)📝 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.
| // 04. Update EventWebhookEvents test | |
| var newEventWebhookEvents2 []string | |
| newAuthWebhookMethods2 := []string{ | |
| string(types.DetachDocument), | |
| string(types.PushPull), | |
| } | |
| fields = &types.UpdatableProjectFields{ | |
| AuthWebhookMethods: &newAuthWebhookMethods2, | |
| EventWebhookEvents: &newEventWebhookEvents2, | |
| } | |
| assert.NoError(t, fields.Validate()) | |
| res, err = db.UpdateProjectInfo(ctx, dummyOwnerID, id, fields) | |
| assert.NoError(t, err) | |
| updateInfo, err = db.FindProjectInfoByID(ctx, id) | |
| assert.NoError(t, err) | |
| assert.Equal(t, res, updateInfo) | |
| assert.Equal(t, newEventWebhookEvents2, updateInfo.EventWebhookEvents) | |
| assert.Equal(t, newAuthWebhookMethods2, updateInfo.AuthWebhookMethods) | |
| // 04. Update EventWebhookEvents test | |
| var newEventWebhookEvents2 []string | |
| newAuthWebhookMethods2 := []string{ | |
| string(types.DetachDocument), | |
| string(types.PushPull), | |
| } | |
| fields = &types.UpdatableProjectFields{ | |
| AuthWebhookMethods: &newAuthWebhookMethods2, | |
| EventWebhookEvents: &newEventWebhookEvents2, | |
| } | |
| assert.NoError(t, fields.Validate()) | |
| res, err = db.UpdateProjectInfo(ctx, dummyOwnerID, id, fields) | |
| assert.NoError(t, err) | |
| updateInfo, err = db.FindProjectInfoByID(ctx, id) | |
| assert.NoError(t, err) | |
| assert.Equal(t, res, updateInfo) | |
| assert.Equal(t, newEventWebhookEvents2, updateInfo.EventWebhookEvents) | |
| assert.Equal(t, newAuthWebhookMethods2, updateInfo.AuthWebhookMethods) | |
| // Test empty events array | |
| emptyEvents := make([]string, 0) | |
| fields = &types.UpdatableProjectFields{ | |
| EventWebhookEvents: &emptyEvents, | |
| } | |
| assert.NoError(t, fields.Validate()) | |
| res, err = db.UpdateProjectInfo(ctx, dummyOwnerID, id, fields) | |
| assert.NoError(t, err) | |
| updateInfo, err = db.FindProjectInfoByID(ctx, id) | |
| assert.NoError(t, err) | |
| assert.Empty(t, updateInfo.EventWebhookEvents) |
hackerwins
changed the title
DocRootChanged Event
What this PR Does and Why We Need It
This PR introduces a basic implementation of event webhooks. The key changes include:
EventWebhookURLandEventWebhookTypesattributes.project_infoabout database.updatable_fieldfor yorkie admin serviceevent-webhookupdates.DocRootChangedEvent for Event WebhooksDocRootChangedevent.DocPresenceChangedorDocChangedfor this purpose, but their usage is ambiguous at the moment, so I did not include them.Webhook Request and Response
Request
{ "type": "string", // Only 'DocumentRootChanged' for now "attributes": { "key": "string", // Document Key "issuedAt": "2025-02-17T08:35:27.558Z" //ISO 8601 } }Response
Which Issue(s) This PR Fixes
This PR addresses issue: #1002
Special Notes for the Reviewer
1. Caching Concerns
2. Event Webhook Configuration
auth-webhook:EventWebhookMaxRetriesEventWebhookMaxWaitIntervalEventWebhookMinWaitIntervalEventWebhookRequestTimeoutAfter Discussing Caching with Reviewer
Does This PR Introduce a User-Facing Change?
Additional Documentation
Checklist
Summary by CodeRabbit
New Features
Documentation