Skip to content

Commit

Permalink
Handle case where webhook event may not be set
Browse files Browse the repository at this point in the history
  • Loading branch information
shydefoo committed Jun 5, 2024
1 parent 5f8ef30 commit 36280c9
Show file tree
Hide file tree
Showing 4 changed files with 102 additions and 2 deletions.
18 changes: 18 additions & 0 deletions api/pkg/webhooks/README.md
Original file line number Diff line number Diff line change
Expand Up @@ -48,6 +48,24 @@ InvokeWebhooks(context.Context, EventType, payload interface{}, onSuccess func([

method in the caller code based on the event.

#### Optional webhooks events

In the event that there are multiple events to be configured, for example `OnProjectCreated` and `OnProjectUpdated`, and only `OnProjectCreated` webhooks should be fired, use the `IsEventConfigured()` method provided by the `WebhookManager` to check if the event is set before calling `InvokeWebhooks()`

For example:

```go
if webhookManager == nil || !webhookManager.IsEventConfigured(ProjectUpdatedEvent) {
// do step if webhooks disabled, or event not set
...
} else {
err := webhookManager.InvokeWebhooks(ctx, ProjectUpdatedEvent, project, func(p []byte) error {
// onSuccess steps
...
}, webhooks.NoOpErrorHandler)
}
```

### Single Webhook Configuration

```yaml
Expand Down
10 changes: 10 additions & 0 deletions api/pkg/webhooks/manager.go
Original file line number Diff line number Diff line change
Expand Up @@ -14,13 +14,23 @@ type WebhookManager interface {
func(payload []byte) error,
func(error) error,
) error

IsEventConfigured(EventType) bool
}

type SimpleWebhookManager struct {
SyncClients map[EventType][]WebhookClient
AsyncClients map[EventType][]WebhookClient
}

// IsEventConfigured checks if the event is configured in the webhook manager
// Use this method before calling InvokeWebhooks if it is optional to set webhooks for an event
func (w *SimpleWebhookManager) IsEventConfigured(event EventType) bool {
_, ok := w.SyncClients[event]
_, ok1 := w.AsyncClients[event]
return ok || ok1
}

// InvokeWebhooks iterates through the webhooks for a given event and invokes them.
// Sync webhooks are called first, and only after all of them succeed, the async webhooks are called.
// Sync webhooks are called in the order that they are defined. The call order of async webhooks are
Expand Down
4 changes: 2 additions & 2 deletions api/service/projects_service.go
Original file line number Diff line number Diff line change
Expand Up @@ -81,7 +81,7 @@ func (service *projectsService) CreateProject(ctx context.Context, project *mode
}
}

if service.webhookManager != nil {
if service.webhookManager != nil && service.webhookManager.IsEventConfigured(ProjectUpdatedEvent) {
err = service.webhookManager.InvokeWebhooks(ctx, ProjectCreatedEvent, project, func(p []byte) error {
// Expects webhook output to be a project object
var tmpproject models.Project
Expand Down Expand Up @@ -122,7 +122,7 @@ func (service *projectsService) UpdateProject(ctx context.Context, project *mode
return nil, fmt.Errorf("error while updating authorization policy for project %s", project.Name)
}
}
if service.webhookManager == nil {
if service.webhookManager == nil || !service.webhookManager.IsEventConfigured(ProjectUpdatedEvent) {
return service.save(project)
}
err := service.webhookManager.InvokeWebhooks(ctx, ProjectUpdatedEvent, project, func(p []byte) error {
Expand Down
72 changes: 72 additions & 0 deletions api/service/projects_service_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -499,3 +499,75 @@ func TestProjectsService_UpdateProjectWithWebhook(t *testing.T) {
})
}
}

func TestProjectsService_UpdateProjectWithWebhookEventNotSet(t *testing.T) {
tests := []struct {
name string
arg *models.Project
expResult *models.Project
wantError bool
wantErrorMsg string
whResponse []byte
}{
{
"success: webhook event ignored",
&models.Project{
ID: 1,
Name: "my-project",
Administrators: []string{"[email protected]"},
Readers: nil,
Team: "team-1",
},
&models.Project{
ID: 1,
Name: "my-project",
MLFlowTrackingURL: MLFlowTrackingURL,
Administrators: []string{"[email protected]"},
Readers: nil,
Team: "team-1",
},
false,
"",
[]byte(`{
"id": 1,
"name": "my-project",
"mlflow_tracking_url": "http://localhost:5555",
"administrators": ["[email protected]"],
"team": "team-1",
"stream": "team-2-modified-by-webhook"
}`),
},
}
for _, tt := range tests {
t.Run(tt.name, func(t *testing.T) {
storage := &mocks.ProjectRepository{}
storage.On("Save", tt.expResult).Return(tt.expResult, nil)

authEnforcer := &enforcerMock.Enforcer{}
mockClient1 := &webhooks.MockWebhookClient{}
mockClient1.On("IsAsync").Return(false)
mockClient1.On("GetName").Return("webhook1")
mockClient1.On("IsFinalResponse").Return(true)
mockClient1.On("GetUseDataFrom").Return("")
mockClient1.On("Invoke", mock.Anything, mock.Anything).Return(tt.whResponse, nil)
whManager := &webhooks.SimpleWebhookManager{
SyncClients: map[webhooks.EventType][]webhooks.WebhookClient{
ProjectCreatedEvent: {
mockClient1,
},
},
}
mockClient1.On("Invoke", mock.Anything, mock.Anything).Return(tt.whResponse, nil)
projectsService, err := NewProjectsService(MLFlowTrackingURL, storage, authEnforcer, false, whManager)

assert.NoError(t, err)

res, err := projectsService.UpdateProject(context.Background(), tt.arg)
require.NoError(t, err)
require.Equal(t, tt.expResult, res)

storage.AssertExpectations(t)
authEnforcer.AssertExpectations(t)
})
}
}

0 comments on commit 36280c9

Please sign in to comment.