5bafd92edf
The slack sub-tests in TestDiscordOnly_CreateRejectsNonDiscord and TestBlocker3_CreateProviderRejectsNonDiscordWithSecurityEvents were omitting the required token field from their request payloads. CreateProvider enforces that Slack providers must have a non-empty token (the webhook URL) at creation time. Without it the service returns "slack webhook URL is required", which the handler does not classify as a 400 validation error, so it falls through to 500. Add a token field to each test struct, populate it for the slack case with a valid-format Slack webhook URL, and use WithSlackURLValidator to bypass the real format check in unit tests — matching the pattern used in all existing service-level Slack tests.
302 lines
11 KiB
Markdown
302 lines
11 KiB
Markdown
# Fix Plan: Slack Sub-Test Failures in Handler-Level Provider Create Tests
|
|
|
|
**Status:** Active
|
|
**Created:** 2026-03-15
|
|
**Branch:** `feature/beta-release`
|
|
**Scope:** Two failing `go test` sub-tests in the handlers package
|
|
**Previous Plan:** Backed up to `docs/plans/current_spec.md.bak`
|
|
|
|
---
|
|
|
|
## 1. Failing Tests
|
|
|
|
| Test | File | Sub-test | Expected | Actual |
|
|
|------|------|----------|----------|--------|
|
|
| `TestBlocker3_CreateProviderRejectsNonDiscordWithSecurityEvents` | `notification_provider_blocker3_test.go` | `slack` | `201 Created` | `500 Internal Server Error` |
|
|
| `TestDiscordOnly_CreateRejectsNonDiscord` | `notification_provider_discord_only_test.go` | `slack` | `201 Created` | `500 Internal Server Error` |
|
|
|
|
---
|
|
|
|
## 2. Root Cause Analysis
|
|
|
|
### 2.1 Execution Path
|
|
|
|
Both tests are table-driven. For the `slack` row, the request payload is built as:
|
|
|
|
```go
|
|
payload := map[string]interface{}{
|
|
"name": "Test Provider",
|
|
"type": "slack",
|
|
"url": "https://example.com/webhook",
|
|
"enabled": true,
|
|
// ... (no "token" field)
|
|
}
|
|
```
|
|
|
|
The handler `Create()` in `notification_provider_handler.go` passes the bound request to `service.CreateProvider()`.
|
|
|
|
### 2.2 Service Enforcement
|
|
|
|
`CreateProvider()` in `notification_service.go` (line ~793) has a mandatory token guard for Slack:
|
|
|
|
```go
|
|
if provider.Type == "slack" {
|
|
token := strings.TrimSpace(provider.Token)
|
|
if token == "" {
|
|
return fmt.Errorf("slack webhook URL is required") // ← triggered
|
|
}
|
|
if err := s.validateSlackURL(token); err != nil {
|
|
return err
|
|
}
|
|
}
|
|
```
|
|
|
|
Because the test payload omits `"token"`, `provider.Token` is empty, and `CreateProvider` returns the error `"slack webhook URL is required"`.
|
|
|
|
### 2.3 Handler Error Classifier Gap
|
|
|
|
The handler passes the error to `isProviderValidationError()` (line ~280):
|
|
|
|
```go
|
|
func isProviderValidationError(err error) bool {
|
|
errMsg := err.Error()
|
|
return strings.Contains(errMsg, "invalid custom template") ||
|
|
strings.Contains(errMsg, "rendered template") ||
|
|
strings.Contains(errMsg, "failed to parse template") ||
|
|
strings.Contains(errMsg, "failed to render template") ||
|
|
strings.Contains(errMsg, "invalid Discord webhook URL") ||
|
|
strings.Contains(errMsg, "invalid Slack webhook URL")
|
|
}
|
|
```
|
|
|
|
The string `"slack webhook URL is required"` matches none of these patterns. The handler falls through to the default 500 branch:
|
|
|
|
```go
|
|
respondSanitizedProviderError(c, http.StatusInternalServerError, "PROVIDER_CREATE_FAILED", "internal", "Failed to create provider")
|
|
```
|
|
|
|
### 2.4 Summary
|
|
|
|
The production code is correct. Slack providers must have a webhook URL (stored in `token`) at creation time. The Slack webhook URL is architecturally separate from `url` — it is a credential/secret and is stored in `token`, not `url`. The tests are wrong because they supply a `slack` provider type with no `token` field, which the service correctly rejects. The 500 response is a side-effect of `isProviderValidationError` not classifying `"slack webhook URL is required"` as a 400-class validation error, but that is a separate concern; the fix belongs in the tests.
|
|
|
|
---
|
|
|
|
## 3. Files Involved
|
|
|
|
### Production (no changes needed)
|
|
|
|
| File | Function | Role |
|
|
|------|----------|------|
|
|
| `backend/internal/api/handlers/notification_provider_handler.go` | `Create()` | Binds request, calls service, returns HTTP response |
|
|
| `backend/internal/api/handlers/notification_provider_handler.go` | `isProviderValidationError()` | Classifies service errors for 400 vs 500 routing |
|
|
| `backend/internal/services/notification_service.go` | `CreateProvider()` | Enforces Slack token requirement at line ~793 |
|
|
| `backend/internal/services/notification_service.go` | `validateSlackWebhookURL()` | Validates `https://hooks.slack.com/services/T.../B.../xxx` regex |
|
|
| `backend/internal/services/notification_service.go` | `WithSlackURLValidator()` | Service option to override URL validator in tests |
|
|
|
|
### Tests (require fixes)
|
|
|
|
| File | Test | Change |
|
|
|------|------|--------|
|
|
| `backend/internal/api/handlers/notification_provider_discord_only_test.go` | `TestDiscordOnly_CreateRejectsNonDiscord` | Add `token` to struct + payload; use `WithSlackURLValidator` |
|
|
| `backend/internal/api/handlers/notification_provider_blocker3_test.go` | `TestBlocker3_CreateProviderRejectsNonDiscordWithSecurityEvents` | Add `token` to struct + payload; use `WithSlackURLValidator` |
|
|
|
|
---
|
|
|
|
## 4. Minimal Fix
|
|
|
|
No production code changes are required. All changes are confined to the two test files.
|
|
|
|
### 4.1 Established Pattern
|
|
|
|
The `services` package already uses `WithSlackURLValidator` in 7 tests to bypass the real URL format check in unit tests (`notification_service_test.go` lines 1456, 1482, 1507, 3304, 3334, 3370, 3397 and `notification_service_json_test.go` line 196). The handler tests adopt the same pattern.
|
|
|
|
### 4.2 Fix for `notification_provider_discord_only_test.go`
|
|
|
|
**Change 1 — service constructor** (line ~27):
|
|
|
|
```go
|
|
// Before
|
|
service := services.NewNotificationService(db, nil)
|
|
|
|
// After
|
|
service := services.NewNotificationService(db, nil,
|
|
services.WithSlackURLValidator(func(string) error { return nil }),
|
|
)
|
|
```
|
|
|
|
**Change 2 — test struct** (line ~33):
|
|
|
|
```go
|
|
// Before
|
|
testCases := []struct {
|
|
name string
|
|
providerType string
|
|
wantStatus int
|
|
wantCode string
|
|
}{
|
|
{"webhook", "webhook", http.StatusCreated, ""},
|
|
{"gotify", "gotify", http.StatusCreated, ""},
|
|
{"slack", "slack", http.StatusCreated, ""},
|
|
{"telegram", "telegram", http.StatusCreated, ""},
|
|
{"generic", "generic", http.StatusBadRequest, "UNSUPPORTED_PROVIDER_TYPE"},
|
|
{"email", "email", http.StatusCreated, ""},
|
|
}
|
|
|
|
// After
|
|
testCases := []struct {
|
|
name string
|
|
providerType string
|
|
token string
|
|
wantStatus int
|
|
wantCode string
|
|
}{
|
|
{"webhook", "webhook", "", http.StatusCreated, ""},
|
|
{"gotify", "gotify", "", http.StatusCreated, ""},
|
|
{"slack", "slack", "https://hooks.slack.com/services/T1234567890/B1234567890/XXXXXXXXXXXXXXXXXXXX", http.StatusCreated, ""},
|
|
{"telegram", "telegram", "", http.StatusCreated, ""},
|
|
{"generic", "generic", "", http.StatusBadRequest, "UNSUPPORTED_PROVIDER_TYPE"},
|
|
{"email", "email", "", http.StatusCreated, ""},
|
|
}
|
|
```
|
|
|
|
**Change 3 — payload map** (line ~48):
|
|
|
|
```go
|
|
// Before
|
|
payload := map[string]interface{}{
|
|
"name": "Test Provider",
|
|
"type": tc.providerType,
|
|
"url": "https://example.com/webhook",
|
|
"enabled": true,
|
|
"notify_proxy_hosts": true,
|
|
}
|
|
|
|
// After
|
|
payload := map[string]interface{}{
|
|
"name": "Test Provider",
|
|
"type": tc.providerType,
|
|
"url": "https://example.com/webhook",
|
|
"token": tc.token,
|
|
"enabled": true,
|
|
"notify_proxy_hosts": true,
|
|
}
|
|
```
|
|
|
|
### 4.3 Fix for `notification_provider_blocker3_test.go`
|
|
|
|
**Change 1 — service constructor** (line ~32):
|
|
|
|
```go
|
|
// Before
|
|
service := services.NewNotificationService(db, nil)
|
|
|
|
// After
|
|
service := services.NewNotificationService(db, nil,
|
|
services.WithSlackURLValidator(func(string) error { return nil }),
|
|
)
|
|
```
|
|
|
|
**Change 2 — test struct** (line ~35):
|
|
|
|
```go
|
|
// Before
|
|
testCases := []struct {
|
|
name string
|
|
providerType string
|
|
wantStatus int
|
|
}{
|
|
{"webhook", "webhook", http.StatusCreated},
|
|
{"gotify", "gotify", http.StatusCreated},
|
|
{"slack", "slack", http.StatusCreated},
|
|
{"email", "email", http.StatusCreated},
|
|
}
|
|
|
|
// After
|
|
testCases := []struct {
|
|
name string
|
|
providerType string
|
|
token string
|
|
wantStatus int
|
|
}{
|
|
{"webhook", "webhook", "", http.StatusCreated},
|
|
{"gotify", "gotify", "", http.StatusCreated},
|
|
{"slack", "slack", "https://hooks.slack.com/services/T1234567890/B1234567890/XXXXXXXXXXXXXXXXXXXX", http.StatusCreated},
|
|
{"email", "email", "", http.StatusCreated},
|
|
}
|
|
```
|
|
|
|
**Change 3 — payload map** (line ~50):
|
|
|
|
```go
|
|
// Before
|
|
payload := map[string]interface{}{
|
|
"name": "Test Provider",
|
|
"type": tc.providerType,
|
|
"url": "https://example.com/webhook",
|
|
"enabled": true,
|
|
"notify_security_waf_blocks": true,
|
|
}
|
|
|
|
// After
|
|
payload := map[string]interface{}{
|
|
"name": "Test Provider",
|
|
"type": tc.providerType,
|
|
"url": "https://example.com/webhook",
|
|
"token": tc.token,
|
|
"enabled": true,
|
|
"notify_security_waf_blocks": true,
|
|
}
|
|
```
|
|
|
|
---
|
|
|
|
## 5. What Is Not Changing
|
|
|
|
- **`notification_provider_handler.go`** — `Create()` and `isProviderValidationError()` are correct as written.
|
|
- **`notification_service.go`** — The Slack token requirement in `CreateProvider()` is correct behavior; Slack webhook URLs are secrets and belong in `token`, not `url`.
|
|
- **No new tests** — Existing test coverage is sufficient. The tests just need correct input data that reflects the real API contract.
|
|
|
|
---
|
|
|
|
## 6. Commit Slicing Strategy
|
|
|
|
A single commit and single PR is appropriate. The entire change is two test files, three small hunks each (service constructor, struct definition, payload map). There are no production code changes and no risk of merge conflicts.
|
|
|
|
**Suggested commit message:**
|
|
|
|
```
|
|
test: supply slack webhook token in handler create sub-tests
|
|
|
|
The slack sub-tests in TestDiscordOnly_CreateRejectsNonDiscord and
|
|
TestBlocker3_CreateProviderRejectsNonDiscordWithSecurityEvents were
|
|
omitting the required token field from their request payloads.
|
|
CreateProvider enforces that Slack providers must have a non-empty
|
|
token (the webhook URL) at creation time. Without it the service
|
|
returns "slack webhook URL is required", which the handler does not
|
|
classify as a 400 validation error, so it falls through to 500.
|
|
|
|
Add a token field to each test struct, populate it for the slack
|
|
case with a valid-format Slack webhook URL, and use
|
|
WithSlackURLValidator to bypass the real format check in unit tests —
|
|
matching the pattern used in all existing service-level Slack tests.
|
|
```
|
|
|
|
---
|
|
|
|
## 7. Verification
|
|
|
|
After applying the fix, run the two previously failing sub-tests:
|
|
|
|
```bash
|
|
cd /projects/Charon/backend
|
|
go test ./internal/api/handlers/... \
|
|
-run "TestBlocker3_CreateProviderRejectsNonDiscordWithSecurityEvents/slack|TestDiscordOnly_CreateRejectsNonDiscord/slack" \
|
|
-v
|
|
```
|
|
|
|
Both sub-tests must exit `PASS`. Then run the full handlers suite to confirm no regressions:
|
|
|
|
```bash
|
|
go test ./internal/api/handlers/... -count=1
|
|
```
|