Fix MessagingConfig code review issues (gt-mrqiz, gt-ngoe6, gt-akc4m)
- Add Type field for schema consistency (gt-mrqiz) - Fix error message inconsistency: all validation errors now wrap sentinel (gt-ngoe6) - Add missing tests: wrong type, future version, malformed JSON (gt-akc4m) 🤖 Generated with [Claude Code](https://claude.com/claude-code) Co-Authored-By: Claude Opus 4.5 <noreply@anthropic.com>
This commit is contained in:
@@ -612,6 +612,9 @@ func SaveMessagingConfig(path string, config *MessagingConfig) error {
|
|||||||
|
|
||||||
// validateMessagingConfig validates a MessagingConfig.
|
// validateMessagingConfig validates a MessagingConfig.
|
||||||
func validateMessagingConfig(c *MessagingConfig) error {
|
func validateMessagingConfig(c *MessagingConfig) error {
|
||||||
|
if c.Type != "messaging" && c.Type != "" {
|
||||||
|
return fmt.Errorf("%w: expected type 'messaging', got '%s'", ErrInvalidType, c.Type)
|
||||||
|
}
|
||||||
if c.Version > CurrentMessagingVersion {
|
if c.Version > CurrentMessagingVersion {
|
||||||
return fmt.Errorf("%w: got %d, max supported %d", ErrInvalidVersion, c.Version, CurrentMessagingVersion)
|
return fmt.Errorf("%w: got %d, max supported %d", ErrInvalidVersion, c.Version, CurrentMessagingVersion)
|
||||||
}
|
}
|
||||||
@@ -637,20 +640,20 @@ func validateMessagingConfig(c *MessagingConfig) error {
|
|||||||
// Validate queues have at least one worker
|
// Validate queues have at least one worker
|
||||||
for name, queue := range c.Queues {
|
for name, queue := range c.Queues {
|
||||||
if len(queue.Workers) == 0 {
|
if len(queue.Workers) == 0 {
|
||||||
return fmt.Errorf("%w: queue '%s' has no workers", ErrMissingField, name)
|
return fmt.Errorf("%w: queue '%s' workers", ErrMissingField, name)
|
||||||
}
|
}
|
||||||
if queue.MaxClaims < 0 {
|
if queue.MaxClaims < 0 {
|
||||||
return fmt.Errorf("queue '%s': max_claims must be non-negative", name)
|
return fmt.Errorf("%w: queue '%s' max_claims must be non-negative", ErrMissingField, name)
|
||||||
}
|
}
|
||||||
}
|
}
|
||||||
|
|
||||||
// Validate announces have at least one reader
|
// Validate announces have at least one reader
|
||||||
for name, announce := range c.Announces {
|
for name, announce := range c.Announces {
|
||||||
if len(announce.Readers) == 0 {
|
if len(announce.Readers) == 0 {
|
||||||
return fmt.Errorf("%w: announce '%s' has no readers", ErrMissingField, name)
|
return fmt.Errorf("%w: announce '%s' readers", ErrMissingField, name)
|
||||||
}
|
}
|
||||||
if announce.RetainCount < 0 {
|
if announce.RetainCount < 0 {
|
||||||
return fmt.Errorf("announce '%s': retain_count must be non-negative", name)
|
return fmt.Errorf("%w: announce '%s' retain_count must be non-negative", ErrMissingField, name)
|
||||||
}
|
}
|
||||||
}
|
}
|
||||||
|
|
||||||
|
|||||||
@@ -1,6 +1,7 @@
|
|||||||
package config
|
package config
|
||||||
|
|
||||||
import (
|
import (
|
||||||
|
"os"
|
||||||
"path/filepath"
|
"path/filepath"
|
||||||
"testing"
|
"testing"
|
||||||
"time"
|
"time"
|
||||||
@@ -575,6 +576,9 @@ func TestMessagingConfigRoundTrip(t *testing.T) {
|
|||||||
t.Fatalf("LoadMessagingConfig: %v", err)
|
t.Fatalf("LoadMessagingConfig: %v", err)
|
||||||
}
|
}
|
||||||
|
|
||||||
|
if loaded.Type != "messaging" {
|
||||||
|
t.Errorf("Type = %q, want 'messaging'", loaded.Type)
|
||||||
|
}
|
||||||
if loaded.Version != CurrentMessagingVersion {
|
if loaded.Version != CurrentMessagingVersion {
|
||||||
t.Errorf("Version = %d, want %d", loaded.Version, CurrentMessagingVersion)
|
t.Errorf("Version = %d, want %d", loaded.Version, CurrentMessagingVersion)
|
||||||
}
|
}
|
||||||
@@ -618,6 +622,7 @@ func TestMessagingConfigValidation(t *testing.T) {
|
|||||||
{
|
{
|
||||||
name: "valid config with lists",
|
name: "valid config with lists",
|
||||||
config: &MessagingConfig{
|
config: &MessagingConfig{
|
||||||
|
Type: "messaging",
|
||||||
Version: 1,
|
Version: 1,
|
||||||
Lists: map[string][]string{
|
Lists: map[string][]string{
|
||||||
"oncall": {"mayor/", "gastown/witness"},
|
"oncall": {"mayor/", "gastown/witness"},
|
||||||
@@ -625,6 +630,22 @@ func TestMessagingConfigValidation(t *testing.T) {
|
|||||||
},
|
},
|
||||||
wantErr: false,
|
wantErr: false,
|
||||||
},
|
},
|
||||||
|
{
|
||||||
|
name: "wrong type",
|
||||||
|
config: &MessagingConfig{
|
||||||
|
Type: "wrong",
|
||||||
|
Version: 1,
|
||||||
|
},
|
||||||
|
wantErr: true,
|
||||||
|
},
|
||||||
|
{
|
||||||
|
name: "future version rejected",
|
||||||
|
config: &MessagingConfig{
|
||||||
|
Type: "messaging",
|
||||||
|
Version: 999,
|
||||||
|
},
|
||||||
|
wantErr: true,
|
||||||
|
},
|
||||||
{
|
{
|
||||||
name: "list with no recipients",
|
name: "list with no recipients",
|
||||||
config: &MessagingConfig{
|
config: &MessagingConfig{
|
||||||
@@ -694,6 +715,21 @@ func TestLoadMessagingConfigNotFound(t *testing.T) {
|
|||||||
}
|
}
|
||||||
}
|
}
|
||||||
|
|
||||||
|
func TestLoadMessagingConfigMalformedJSON(t *testing.T) {
|
||||||
|
dir := t.TempDir()
|
||||||
|
path := filepath.Join(dir, "messaging.json")
|
||||||
|
|
||||||
|
// Write malformed JSON
|
||||||
|
if err := os.WriteFile(path, []byte("{not valid json"), 0644); err != nil {
|
||||||
|
t.Fatalf("writing test file: %v", err)
|
||||||
|
}
|
||||||
|
|
||||||
|
_, err := LoadMessagingConfig(path)
|
||||||
|
if err == nil {
|
||||||
|
t.Fatal("expected error for malformed JSON")
|
||||||
|
}
|
||||||
|
}
|
||||||
|
|
||||||
func TestLoadOrCreateMessagingConfig(t *testing.T) {
|
func TestLoadOrCreateMessagingConfig(t *testing.T) {
|
||||||
// Test creating default when not found
|
// Test creating default when not found
|
||||||
config, err := LoadOrCreateMessagingConfig("/nonexistent/path.json")
|
config, err := LoadOrCreateMessagingConfig("/nonexistent/path.json")
|
||||||
|
|||||||
@@ -258,7 +258,8 @@ func DefaultAccountsConfigDir() string {
|
|||||||
// MessagingConfig represents the messaging configuration (config/messaging.json).
|
// MessagingConfig represents the messaging configuration (config/messaging.json).
|
||||||
// This defines mailing lists, work queues, and announcement channels.
|
// This defines mailing lists, work queues, and announcement channels.
|
||||||
type MessagingConfig struct {
|
type MessagingConfig struct {
|
||||||
Version int `json:"version"` // schema version
|
Type string `json:"type"` // "messaging"
|
||||||
|
Version int `json:"version"` // schema version
|
||||||
|
|
||||||
// Lists are static mailing lists. Messages are fanned out to all recipients.
|
// Lists are static mailing lists. Messages are fanned out to all recipients.
|
||||||
// Each recipient gets their own copy of the message.
|
// Each recipient gets their own copy of the message.
|
||||||
@@ -302,6 +303,7 @@ const CurrentMessagingVersion = 1
|
|||||||
// NewMessagingConfig creates a new MessagingConfig with defaults.
|
// NewMessagingConfig creates a new MessagingConfig with defaults.
|
||||||
func NewMessagingConfig() *MessagingConfig {
|
func NewMessagingConfig() *MessagingConfig {
|
||||||
return &MessagingConfig{
|
return &MessagingConfig{
|
||||||
|
Type: "messaging",
|
||||||
Version: CurrentMessagingVersion,
|
Version: CurrentMessagingVersion,
|
||||||
Lists: make(map[string][]string),
|
Lists: make(map[string][]string),
|
||||||
Queues: make(map[string]QueueConfig),
|
Queues: make(map[string]QueueConfig),
|
||||||
|
|||||||
Reference in New Issue
Block a user