refactor(config): split validation into per-notifier Validate methods - #5395
refactor(config): split validation into per-notifier Validate methods#5395ayushi-work wants to merge 3 commits into
Conversation
📝 WalkthroughWalkthroughNotifier configuration validation is split into dedicated ChangesNotifier validation centralization
Estimated code review effort: 3 (Moderate) | ~25 minutes Possibly related PRs
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (4)
notify/mattermost/config.go (1)
141-147: 🎯 Functional Correctness | 🟡 Minor | ⚡ Quick winConsider adding a requirement check for the Mattermost webhook URL.
Unlike other webhook-based notifiers (such as Discord and MSTeams), this configuration does not verify that at least one of
webhook_urlorwebhook_url_fileis provided. If there is no global fallback for Mattermost endpoints, omitting both could lead to errors during message delivery rather than being caught at startup.🛡️ Proposed fix
func (c *MattermostConfig) Validate() error { + if c.WebhookURL == nil && len(c.WebhookURLFile) == 0 { + return errors.New("one of webhook_url or webhook_url_file must be configured") + } + if c.WebhookURL != nil && len(c.WebhookURLFile) > 0 { return errors.New("at most one of webhook_url & webhook_url_file must be configured") }🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@notify/mattermost/config.go` around lines 141 - 147, Update MattermostConfig.Validate to require at least one of WebhookURL or WebhookURLFile, while preserving the existing mutual-exclusion validation when both are configured. Return a clear configuration error when neither endpoint is provided.config/notifiers.go (2)
491-493: 🩺 Stability & Availability | 🟡 Minor | ⚡ Quick winPrevent potential panic if
APIURLis nil.If
UpdateMessageis true andAPIURLisnil, calling.String()could cause a nil-pointer dereference panic (depending onSecretURL's implementation). Since the validation requires the URL to be a specific Slack API endpoint, explicitly checking fornilguarantees safety.🛡️ Proposed fix
- if c.UpdateMessage && c.APIURL.String() != "https://slack.com/api/chat.postMessage" { + if c.UpdateMessage && (c.APIURL == nil || c.APIURL.String() != "https://slack.com/api/chat.postMessage") { return errors.New("update_message can only be used with bot tokens. api_url must be set to https://slack.com/api/chat.postMessage") }🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@config/notifiers.go` around lines 491 - 493, Update the validation condition in the notifier configuration path around UpdateMessage so it checks APIURL for nil before calling APIURL.String(). Treat a nil URL as invalid and return the existing validation error, while preserving acceptance only for the required Slack chat.postMessage endpoint.
688-693: 🎯 Functional Correctness | 🟡 Minor | ⚡ Quick winFix the logic to reject configs where all three fields are provided.
The boolean logic
(A != B) != C(which corresponds toA XOR B XOR C) correctly rejects configs with 0 or 2 provided fields. However, it incorrectly evaluates tofalse(no error) when all three fields are provided, bypassing the mutually exclusive requirement.Consider using an explicit count to ensure exactly one field is configured.
🐛 Proposed fix
func (c *SNSConfig) Validate() error { - if (c.TargetARN == "") != (c.TopicARN == "") != (c.PhoneNumber == "") { + provided := 0 + if c.TargetARN != "" { + provided++ + } + if c.TopicARN != "" { + provided++ + } + if c.PhoneNumber != "" { + provided++ + } + if provided != 1 { return errors.New("must provide either a Target ARN, Topic ARN, or Phone Number for SNS config") } return nil }🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@config/notifiers.go` around lines 688 - 693, Update SNSConfig.Validate to count the non-empty values among TargetARN, TopicARN, and PhoneNumber, and return the existing validation error unless exactly one field is configured. Remove the chained XOR condition so configurations with all three fields, none, or multiple fields are rejected.notify/incidentio/config.go (1)
83-85: 🎯 Functional Correctness | 🟠 Major | ⚡ Quick winFix logic error when
HTTPConfigis nil.The condition
c.HTTPConfig != nilprevents this validation from triggering whenHTTPConfigis completely omitted (i.e.,nil). This incorrectly allows a configuration with no authentication at all to pass validation. Change!=to==so the requirement is properly enforced whenHTTPConfigis missing.🐛 Proposed fix
- if (c.HTTPConfig != nil && c.HTTPConfig.Authorization == nil) && c.AlertSourceToken == "" && c.AlertSourceTokenFile == "" { + if (c.HTTPConfig == nil || c.HTTPConfig.Authorization == nil) && c.AlertSourceToken == "" && c.AlertSourceTokenFile == "" { return errors.New("at least one of alert_source_token, alert_source_token_file or http_config.authorization must be configured") }🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@notify/incidentio/config.go` around lines 83 - 85, Update the authentication validation condition in the configuration validation logic to treat a nil HTTPConfig as missing authorization, while still checking AlertSourceToken and AlertSourceTokenFile. Change the HTTPConfig nil comparison so configurations without any authentication are rejected with the existing error.
🧹 Nitpick comments (1)
config/notifiers.go (1)
348-369: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueMove the "missing name or url" validation to
Validate().To fully align with the PR's objective of separating data mutation from pure validation, consider moving the check for missing
nameorurlinto the newValidate()method.♻️ Proposed refactor
if c.URL != "" { // Clear all message action fields. c.Name = "" c.Value = "" c.ConfirmField = nil } else if c.Name != "" { c.URL = "" - } else { - return errors.New("missing name or url in Slack action configuration") } return c.Validate() } func (c *SlackAction) Validate() error { + if c.URL == "" && c.Name == "" { + return errors.New("missing name or url in Slack action configuration") + } if c.Type == "" {🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@config/notifiers.go` around lines 348 - 369, Move the missing name-or-URL check from the mutation logic into SlackAction.Validate, keeping the existing error message. Ensure the mutating method only clears conflicting fields and then delegates validation, while Validate rejects configurations where both Name and URL are empty.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Outside diff comments:
In `@config/notifiers.go`:
- Around line 491-493: Update the validation condition in the notifier
configuration path around UpdateMessage so it checks APIURL for nil before
calling APIURL.String(). Treat a nil URL as invalid and return the existing
validation error, while preserving acceptance only for the required Slack
chat.postMessage endpoint.
- Around line 688-693: Update SNSConfig.Validate to count the non-empty values
among TargetARN, TopicARN, and PhoneNumber, and return the existing validation
error unless exactly one field is configured. Remove the chained XOR condition
so configurations with all three fields, none, or multiple fields are rejected.
In `@notify/incidentio/config.go`:
- Around line 83-85: Update the authentication validation condition in the
configuration validation logic to treat a nil HTTPConfig as missing
authorization, while still checking AlertSourceToken and AlertSourceTokenFile.
Change the HTTPConfig nil comparison so configurations without any
authentication are rejected with the existing error.
In `@notify/mattermost/config.go`:
- Around line 141-147: Update MattermostConfig.Validate to require at least one
of WebhookURL or WebhookURLFile, while preserving the existing mutual-exclusion
validation when both are configured. Return a clear configuration error when
neither endpoint is provided.
---
Nitpick comments:
In `@config/notifiers.go`:
- Around line 348-369: Move the missing name-or-URL check from the mutation
logic into SlackAction.Validate, keeping the existing error message. Ensure the
mutating method only clears conflicting fields and then delegates validation,
while Validate rejects configurations where both Name and URL are empty.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Enterprise
Run ID: 0da194d9-9bae-4371-8216-3f9714fbe06b
📒 Files selected for processing (11)
config/common/notifierconfig.goconfig/notifiers.gonotify/discord/config.gonotify/incidentio/config.gonotify/jira/config.gonotify/mattermost/config.gonotify/msteams/config.gonotify/msteamsv2/config.gonotify/opsgenie/config.gonotify/telegram/config.gonotify/webhook/config.go
|
@ayushi-work that PR is maybe a bit pre mature, the proposal I made in the issue was just an idea. I am not sure it is a good one. And if this is the way to go the interface would probably be less sepcific eg |
|
@TheMeier Hi! I kept the interface small because Validate() is really the only behavior I need for #4989, and it felt cleaner to keep validation separate from other concerns. That said, I'm not attached to the interface itself, if we'd rather see a different interface, I'm happy to change it. For the file layout, the external notifier Validate() methods are already in notify//config.go. The ones in config/notifiers.go remained there because those notifier types have always lived in the config package, and moving them felt like a separate cleanup from what I was trying to do here. This is mostly just the mechanical extraction to make the follow-up work in #4989 easier. |
|
oh sorry I overlooked that this was already done in notify/$NAME/config.go where available. i am currently moving ther configs out of notifiy/config.go one by one. so actually it looks quite ok to me |
alright great! let me know if anything needs to be changed! |
|
Hi! just bumping this up! |
Spaceman1701
left a comment
Spaceman1701
left a comment
There was a problem hiding this comment.
I quite like this change - decoupling unmarshaling from validation is a good idea.
| for h, v := range c.Headers { | ||
| normalized := textproto.CanonicalMIMEHeaderKey(h) | ||
| if _, ok := normalizedHeaders[normalized]; ok { | ||
| return fmt.Errorf("duplicate header %q in email config", normalized) |
There was a problem hiding this comment.
I know it's obvious right now, but would you mind adding a comment that explains why this validation is not in Validate? For a future someone who isn't familiar with the history of this code, it might be rather surprising.
There was a problem hiding this comment.
Done, added a comment explaining why the duplicate detection stays in UnmarshalYAML.
…prometheus#4992) Signed-off-by: ayushi-work <ayushi.work007@gmail.com>
…halYAML Signed-off-by: ayushi-work <ayushi.work007@gmail.com>
Signed-off-by: ayushi-work <ayushi.work007@gmail.com>
ab02362 to
2710fb9
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
config/notifiers.go (2)
254-279: 🎯 Functional Correctness | 🟠 Major | ⚡ Quick winMove the name-or-URL requirement into
Validate.Line 267 leaves a pure validation rule in
UnmarshalYAML;SlackAction{Type: "...", Text: "..."}.Validate()currently succeeds without eithernameorurl. Keep field clearing in unmarshalling, but enforce this invariant inValidate.Proposed fix
- } else { - return errors.New("missing name or url in Slack action configuration") } return c.Validate() } func (c *SlackAction) Validate() error { + if c.Name == "" && c.URL == "" { + return errors.New("missing name or url in Slack action configuration") + } if c.Type == "" {🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@config/notifiers.go` around lines 254 - 279, Move the missing name-or-URL validation from SlackAction.UnmarshalYAML into SlackAction.Validate so direct Validate calls enforce the same invariant. Keep UnmarshalYAML responsible for unmarshalling and clearing conflicting fields, while Validate rejects configurations where both URL and Name are empty alongside its existing Type and Text checks.
391-404: 🩺 Stability & Availability | 🟠 Major | ⚡ Quick winReject nil
api_urlbefore validatingupdate_message.
SlackConfig.Validate()callsc.APIURL.String()afterUpdateMessage, butAPIURLis a zero*commoncfg.SecretURLby default. That value has noString()method, so the call panics instead of returning a config error. Checkc.APIURL != nilfirst; the subsequent URL comparison can then return the nil/unknown URL as the invalid value.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@config/notifiers.go` around lines 391 - 404, Update SlackConfig.Validate so the update_message check validates c.APIURL is non-nil before calling c.APIURL.String(). When UpdateMessage is enabled with a nil APIURL, return the configuration error; only perform the URL comparison after the nil check.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@config/notifiers.go`:
- Around line 599-602: Update SNSConfig.Validate to explicitly count the
non-empty values among TargetARN, TopicARN, and PhoneNumber, and return the
existing validation error unless exactly one target identifier is configured.
Remove the chained != expression while preserving the current error message.
---
Outside diff comments:
In `@config/notifiers.go`:
- Around line 254-279: Move the missing name-or-URL validation from
SlackAction.UnmarshalYAML into SlackAction.Validate so direct Validate calls
enforce the same invariant. Keep UnmarshalYAML responsible for unmarshalling and
clearing conflicting fields, while Validate rejects configurations where both
URL and Name are empty alongside its existing Type and Text checks.
- Around line 391-404: Update SlackConfig.Validate so the update_message check
validates c.APIURL is non-nil before calling c.APIURL.String(). When
UpdateMessage is enabled with a nil APIURL, return the configuration error; only
perform the URL comparison after the nil check.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro Plus
Run ID: cc6a83ed-92d9-4454-9907-cce564212d2c
📒 Files selected for processing (12)
config/common/notifierconfig.goconfig/notifiers.gonotify/discord/config.gonotify/incidentio/config.gonotify/jira/config.gonotify/mattermost/config.gonotify/msteams/config.gonotify/msteamsv2/config.gonotify/opsgenie/config.gonotify/pagerduty/config.gonotify/telegram/config.gonotify/webhook/config.go
🚧 Files skipped from review as they are similar to previous changes (7)
- config/common/notifierconfig.go
- notify/opsgenie/config.go
- notify/mattermost/config.go
- notify/msteams/config.go
- notify/incidentio/config.go
- notify/webhook/config.go
- notify/telegram/config.go
| func (c *SNSConfig) Validate() error { | ||
| if (c.TargetARN == "") != (c.TopicARN == "") != (c.PhoneNumber == "") { | ||
| return errors.New("must provide either a Target ARN, Topic ARN, or Phone Number for SNS config") | ||
| } |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟠 Major | ⚡ Quick win
Count configured SNS targets explicitly.
Chained != evaluates left-to-right, so all three target fields configured passes validation. Count non-empty target identifiers and require the count to equal one.
Proposed fix
func (c *SNSConfig) Validate() error {
- if (c.TargetARN == "") != (c.TopicARN == "") != (c.PhoneNumber == "") {
+ targets := 0
+ for _, target := range []string{c.TargetARN, c.TopicARN, c.PhoneNumber} {
+ if target != "" {
+ targets++
+ }
+ }
+ if targets != 1 {
return errors.New("must provide either a Target ARN, Topic ARN, or Phone Number for SNS config")
}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| func (c *SNSConfig) Validate() error { | |
| if (c.TargetARN == "") != (c.TopicARN == "") != (c.PhoneNumber == "") { | |
| return errors.New("must provide either a Target ARN, Topic ARN, or Phone Number for SNS config") | |
| } | |
| func (c *SNSConfig) Validate() error { | |
| targets := 0 | |
| for _, target := range []string{c.TargetARN, c.TopicARN, c.PhoneNumber} { | |
| if target != "" { | |
| targets++ | |
| } | |
| } | |
| if targets != 1 { | |
| return errors.New("must provide either a Target ARN, Topic ARN, or Phone Number for SNS config") | |
| } |
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@config/notifiers.go` around lines 599 - 602, Update SNSConfig.Validate to
explicitly count the non-empty values among TargetARN, TopicARN, and
PhoneNumber, and return the existing validation error unless exactly one target
identifier is configured. Remove the chained != expression while preserving the
current error message.
3 participants
Extract validation logic from
UnmarshalYAMLmethods into standaloneValidate() errormethods for every notifier config type. Adds aValidatorinterface inconfig/common.This is a prerequisite for #4989 — once validation lives in separate methods, config loading can call
Validate()on each notifier independently and collect all errors witherrors.Joininstead of failing on the first error.Validate() errorUnmarshalYAMLValidate()Closes #4992
Pull Request Checklist