refactor(webhook): move webhook to subdirectory and simplify naming #31
Conversation
dosubot
bot
added
the
size:L
WalkthroughThe changes in this pull request involve a comprehensive refactoring of the SDK, transitioning from the Changes
Poem
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 (
|
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## master #31 +/- ##
=======================================
Coverage 53.75% 53.75%
=======================================
Files 21 21
Lines 1025 1025
=======================================
Hits 551 551
Misses 384 384
Partials 90 90 ☔ View full report in Codecov by Sentry. |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 3
🧹 Outside diff range and nitpick comments (11)
webhook/dispatcher_test.go (1)
10-14: Consider enhancing the loadData helper functionWhile the implementation is functional, consider these improvements:
- Add filepath validation to prevent potential path traversal
- Make it more reusable by returning both content and error instead of asserting in the function
Consider this alternative implementation:
-func loadData(t *testing.T, filepath string) []byte { +func loadData(t *testing.T, filepath string) ([]byte, error) { + if filepath == "" { + return nil, fmt.Errorf("filepath cannot be empty") + } content, err := os.ReadFile(filepath) - assert.NoError(t, err) - return content + if err != nil { + return nil, fmt.Errorf("failed to read file %s: %w", filepath, err) + } + return content, nil }Usage example:
content, err := loadData(t, filepath) assert.NoError(t, err)webhook/event_story_test.go (2)
Line range hint
12-21: Consider using table-driven tests for better coverageWhile the current test is thorough, consider using table-driven tests to:
- Test multiple scenarios (different field changes, empty fields, etc.)
- Make it easier to add new test cases
Example refactor:
func TestWebhookEvent_Story_StoryUpdateEvent(t *testing.T) { tests := []struct { name string filepath string expectedEvent string expectedFields []string wantErr bool }{ { name: "normal update", filepath: "../.testdata/webhook/story_update_event.json", expectedEvent: EventTypeStoryUpdate, expectedFields: []string{"owner", "modified"}, wantErr: false, }, // Add more test cases here } for _, tt := range tests { t.Run(tt.name, func(t *testing.T) { var event StoryUpdateEvent content, err := loadData(t, tt.filepath) if tt.wantErr { assert.Error(t, err) return } assert.NoError(t, err) assert.NoError(t, json.Unmarshal(content, &event)) assert.Equal(t, tt.expectedEvent, event.Event) assert.Equal(t, tt.expectedFields, event.ChangeFields) }) } }
Line range hint
19-21: Enhance JSON structure verificationThe current JSON structure verification could be more thorough by:
- Checking the exact JSON structure instead of just containing a string
- Verifying all relevant fields are properly marshaled
Consider using a golden file approach:
// Compare against a golden file expected, err := loadData(t, "../.testdata/webhook/story_update_event_golden.json") assert.NoError(t, err) assert.JSONEq(t, string(expected), string(bytes))webhook/event_test.go (1)
44-44: Update test data path to use testdata directoryThe test data path has been updated to
"../.testdata/webhook/". Consider moving the test data to atestdatadirectory (without the dot prefix) as per Go's convention for test data organization.- payload := loadData(t, "../.testdata/webhook/"+tt.filename) + payload := loadData(t, "testdata/webhook/"+tt.filename)webhook/event.go (1)
Line range hint
39-48: Address TODO comment and improve error handlingA few suggestions for improvement:
- The TODO comment about adding more event types should be addressed
- Consider adding support for
EventTypeTaskUpdate,EventTypeBugUpdate, andEventTypeBugCommentUpdatewhich are defined but not handled- Consider using custom error types for better error handling
Would you like me to help implement the missing event type handlers or create custom error types?
README.md (2)
Line range hint
88-93: Improve error handling in webhook handlerThe current error handling simply logs the error and continues. Consider:
- Setting appropriate HTTP status codes based on the error
- Providing more detailed error responses
log.Println("Received webhook request") if err := dispatcher.DispatchRequest(r); err != nil { log.Println(err) + http.Error(w, "Failed to process webhook", http.StatusInternalServerError) + return } w.Write([]byte("ok"))🧰 Tools
🪛 Markdownlint (0.35.0)
67-67: Column: 1
Hard tabs(MD010, no-hard-tabs)
68-68: Column: 1
Hard tabs(MD010, no-hard-tabs)
70-70: Column: 1
Hard tabs(MD010, no-hard-tabs)
76-76: Column: 1
Hard tabs(MD010, no-hard-tabs)
77-77: Column: 1
Hard tabs(MD010, no-hard-tabs)
81-81: Column: 1
Hard tabs(MD010, no-hard-tabs)
82-82: Column: 1
Hard tabs(MD010, no-hard-tabs)
83-83: Column: 1
Hard tabs(MD010, no-hard-tabs)
84-84: Column: 1
Hard tabs(MD010, no-hard-tabs)
86-86: Column: 1
Hard tabs(MD010, no-hard-tabs)
87-87: Column: 1
Hard tabs(MD010, no-hard-tabs)
88-88: Column: 1
Hard tabs(MD010, no-hard-tabs)
89-89: Column: 1
Hard tabs(MD010, no-hard-tabs)
90-90: Column: 1
Hard tabs(MD010, no-hard-tabs)
91-91: Column: 1
Hard tabs(MD010, no-hard-tabs)
Line range hint
70-93: Consider adding security measures to the webhook endpointThe current example doesn't show any authentication or validation of incoming webhook requests. Consider adding:
- Webhook secret validation
- Request signature verification
- IP allowlisting
Would you like me to provide an example of how to implement these security measures?
🧰 Tools
🪛 Markdownlint (0.35.0)
67-67: Column: 1
Hard tabs(MD010, no-hard-tabs)
68-68: Column: 1
Hard tabs(MD010, no-hard-tabs)
70-70: Column: 1
Hard tabs(MD010, no-hard-tabs)
76-76: Column: 1
Hard tabs(MD010, no-hard-tabs)
77-77: Column: 1
Hard tabs(MD010, no-hard-tabs)
81-81: Column: 1
Hard tabs(MD010, no-hard-tabs)
82-82: Column: 1
Hard tabs(MD010, no-hard-tabs)
83-83: Column: 1
Hard tabs(MD010, no-hard-tabs)
84-84: Column: 1
Hard tabs(MD010, no-hard-tabs)
86-86: Column: 1
Hard tabs(MD010, no-hard-tabs)
87-87: Column: 1
Hard tabs(MD010, no-hard-tabs)
88-88: Column: 1
Hard tabs(MD010, no-hard-tabs)
89-89: Column: 1
Hard tabs(MD010, no-hard-tabs)
90-90: Column: 1
Hard tabs(MD010, no-hard-tabs)
91-91: Column: 1
Hard tabs(MD010, no-hard-tabs)
webhook/event_bug.go (1)
3-55: Consider improving field types and adding documentationSeveral fields could benefit from more appropriate types:
- Boolean fields (
IsNewStatus,IsReplicate,IsJenkins) should useboolinstead ofstring- Time-related fields (
Created,InProgressTime, etc.) should usetime.Time- Numeric fields should use appropriate numeric types
Example refactor for some fields:
type BugCreateEvent struct { - IsNewStatus string `json:"is_new_status,omitempty"` - IsReplicate string `json:"is_replicate,omitempty"` - IsJenkins string `json:"is_jenkins,omitempty"` - Created string `json:"created,omitempty"` + IsNewStatus bool `json:"is_new_status,omitempty"` + IsReplicate bool `json:"is_replicate,omitempty"` + IsJenkins bool `json:"is_jenkins,omitempty"` + Created time.Time `json:"created,omitempty"`webhook/dispatcher.go (2)
Line range hint
54-65: Update error message to reflect new package nameThe error message still references the old package name "tapd".
- return errors.New("tapd: webhook dispatcher unsupported event") + return errors.New("webhook: unsupported event")🧰 Tools
🪛 GitHub Check: codecov/patch
[warning] 54-54: webhook/dispatcher.go#L54
Added line #L54 was not covered by tests
Line range hint
21-126: Add test coverage for dispatcher methodsThe static analysis indicates missing test coverage for several methods including:
WithRegistersNewDispatcherRegistersDispatchand related methods- Event processing methods
Would you like me to help generate comprehensive test cases for these methods?
🧰 Tools
🪛 GitHub Check: codecov/patch
[warning] 67-67: webhook/dispatcher.go#L67
Added line #L67 was not covered by tests
[warning] 75-75: webhook/dispatcher.go#L75
Added line #L75 was not covered by tests
[warning] 83-83: webhook/dispatcher.go#L83
Added line #L83 was not covered by tests
[warning] 87-87: webhook/dispatcher.go#L87
Added line #L87 was not covered by tests
[warning] 91-91: webhook/dispatcher.go#L91
Added line #L91 was not covered by tests
[warning] 95-95: webhook/dispatcher.go#L95
Added line #L95 was not covered by testswebhook/event_story.go (1)
Line range hint
53-255: Consider organizing custom fields into separate structsThe large number of custom fields could be better organized for maintainability.
Consider creating separate structs for groups of related fields:
type CustomFields struct { FieldOne *string `json:"custom_field_one,omitempty"` FieldTwo *string `json:"custom_field_two,omitempty"` // ... other fields } type CustomPlanFields struct { FieldOne *string `json:"custom_plan_field_1,omitempty"` FieldTwo *string `json:"custom_plan_field_2,omitempty"` // ... other fields } type StoryUpdateEventOldFields struct { // ... core fields ... CustomFields CustomPlanFields }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (11)
README.md(1 hunks)webhook/dispatcher.go(6 hunks)webhook/dispatcher_test.go(1 hunks)webhook/event.go(2 hunks)webhook/event_bug.go(1 hunks)webhook/event_bug_test.go(2 hunks)webhook/event_story.go(1 hunks)webhook/event_story_test.go(2 hunks)webhook/event_test.go(5 hunks)webhook/listeners.go(1 hunks)webhook_event_bug.go(0 hunks)
💤 Files with no reviewable changes (1)
- webhook_event_bug.go
✅ Files skipped from review due to trivial changes (1)
- webhook/listeners.go
🧰 Additional context used
🪛 Markdownlint (0.35.0)
README.md
70-70: Column: 1
Hard tabs
(MD010, no-hard-tabs)
76-76: Column: 1
Hard tabs
(MD010, no-hard-tabs)
77-77: Column: 1
Hard tabs
(MD010, no-hard-tabs)
81-81: Column: 1
Hard tabs
(MD010, no-hard-tabs)
82-82: Column: 1
Hard tabs
(MD010, no-hard-tabs)
83-83: Column: 1
Hard tabs
(MD010, no-hard-tabs)
84-84: Column: 1
Hard tabs
(MD010, no-hard-tabs)
86-86: Column: 1
Hard tabs
(MD010, no-hard-tabs)
87-87: Column: 1
Hard tabs
(MD010, no-hard-tabs)
88-88: Column: 1
Hard tabs
(MD010, no-hard-tabs)
🪛 GitHub Check: codecov/patch
webhook/dispatcher.go
[warning] 21-23: webhook/dispatcher.go#L21-L23
Added lines #L21 - L23 were not covered by tests
[warning] 28-29: webhook/dispatcher.go#L28-L29
Added lines #L28 - L29 were not covered by tests
[warning] 36-36: webhook/dispatcher.go#L36
Added line #L36 was not covered by tests
[warning] 54-54: webhook/dispatcher.go#L54
Added line #L54 was not covered by tests
[warning] 67-67: webhook/dispatcher.go#L67
Added line #L67 was not covered by tests
[warning] 75-75: webhook/dispatcher.go#L75
Added line #L75 was not covered by tests
[warning] 83-83: webhook/dispatcher.go#L83
Added line #L83 was not covered by tests
[warning] 87-87: webhook/dispatcher.go#L87
Added line #L87 was not covered by tests
[warning] 91-91: webhook/dispatcher.go#L91
Added line #L91 was not covered by tests
[warning] 95-95: webhook/dispatcher.go#L95
Added line #L95 was not covered by tests
[warning] 106-106: webhook/dispatcher.go#L106
Added line #L106 was not covered by tests
[warning] 117-117: webhook/dispatcher.go#L117
Added line #L117 was not covered by tests
🔇 Additional comments (1)
webhook/event_story.go (1)
4-32: LGTM! Well-structured event definitions
The event structures are well-organized with:
- Consistent field naming and JSON tags
- Appropriate use of omitempty
- Clear separation of concerns between create and update events
Also applies to: 36-52
| @@ -9,7 +9,7 @@ import ( | |||
|
|
|||
| func TestWebhookEvent_Bug_BugCreateEvent(t *testing.T) { | |||
| var event BugCreateEvent | |||
| assert.NoError(t, json.Unmarshal(loadData(t, ".testdata/webhook/bug_create_event.json"), &event)) | |||
| assert.NoError(t, json.Unmarshal(loadData(t, "../.testdata/webhook/bug_create_event.json"), &event)) | |||
There was a problem hiding this comment.
💡 Codebase verification
Path should be updated to use local .testdata directory
The test data file exists at .testdata/webhook/bug_create_event.json and this is the correct path to use. The current relative path "../.testdata/webhook/bug_create_event.json" should be updated to ".testdata/webhook/bug_create_event.json" as:
- The
.testdatadirectory exists at the repository root - The same test file is already being used correctly in
webhook/event_test.go - Using
../makes the test more fragile to directory structure changes
🔗 Analysis chain
Verify test data path and enhance test coverage
The relative path "../.testdata" might be fragile if the project structure changes. Consider:
- Using a test helper to resolve test data paths
- Adding more assertions to verify other fields in the BugCreateEvent struct
Let's verify the test data path exists and check for other potential test files:
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Verify test data structure and identify potential missing test cases
# Check if the test data directory exists
if [ -d ".testdata" ]; then
echo "Test data directory found at root"
elif [ -d "../.testdata" ]; then
echo "Test data directory found one level up"
else
echo "Test data directory not found"
fi
# Look for other bug event test files that might need test coverage
fd -e json . | grep -i "bug.*event.*json"
Length of output: 143
Script:
#!/bin/bash
# Let's verify the actual path structure and content more thoroughly
# First, let's check the content of .testdata directory
echo "Content of .testdata directory:"
ls -R .testdata/
# Check if the specific file exists in both locations
echo -e "\nChecking specific file locations:"
[ -f ".testdata/webhook/bug_create_event.json" ] && echo "File exists at .testdata/webhook/bug_create_event.json"
[ -f "../.testdata/webhook/bug_create_event.json" ] && echo "File exists at ../.testdata/webhook/bug_create_event.json"
# Let's also check how this test file is used in other tests
echo -e "\nUsage of bug_create_event.json in tests:"
rg "bug_create_event.json" -l
Length of output: 1808
| event, ok := raw["event"].(string) | ||
| if !ok { | ||
| return "", nil, errors.New("tapd: webhook event type not found") | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Update error message prefix
Since the package has been renamed from tapd to webhook, the error message prefix should be updated accordingly.
- return "", nil, errors.New("tapd: webhook event type not found")
+ return "", nil, errors.New("webhook: event type not found")📝 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.
| event, ok := raw["event"].(string) | |
| if !ok { | |
| return "", nil, errors.New("tapd: webhook event type not found") | |
| } | |
| event, ok := raw["event"].(string) | |
| if !ok { | |
| return "", nil, errors.New("webhook: event type not found") | |
| } |
| dispatcher := webhook.NewDispatcher( | ||
| webhook.WithRegisters(&StoreUpdateListener{}), | ||
| ) | ||
| dispatcher.Register(&StoreUpdateListener{}) | ||
| dispatcher.Registers(&StoreUpdateListener{}) |
There was a problem hiding this comment.
Remove redundant listener registration
The StoreUpdateListener is being registered twice:
- Through
WithRegistersoption during initialization - Through a separate
Registerscall
Remove one of these registrations to avoid duplicate handling of events.
dispatcher := webhook.NewDispatcher(
webhook.WithRegisters(&StoreUpdateListener{}),
)
- dispatcher.Registers(&StoreUpdateListener{})📝 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.
| dispatcher := webhook.NewDispatcher( | |
| webhook.WithRegisters(&StoreUpdateListener{}), | |
| ) | |
| dispatcher.Register(&StoreUpdateListener{}) | |
| dispatcher.Registers(&StoreUpdateListener{}) | |
| dispatcher := webhook.NewDispatcher( | |
| webhook.WithRegisters(&StoreUpdateListener{}), | |
| ) |
🧰 Tools
🪛 Markdownlint (0.35.0)
81-81: Column: 1
Hard tabs
(MD010, no-hard-tabs)
82-82: Column: 1
Hard tabs
(MD010, no-hard-tabs)
83-83: Column: 1
Hard tabs
(MD010, no-hard-tabs)
84-84: Column: 1
Hard tabs
(MD010, no-hard-tabs)
Summary by CodeRabbit
Release Notes
New Features
BugCreateEventstruct to encapsulate attributes related to bug creation events.Refactor
tapdtowebhook, reflecting a reorganization of the codebase.Bug Fixes
Documentation