-
Notifications
You must be signed in to change notification settings - Fork 3
Closed
Description
Summary
Config validation has three separate validation files with overlapping responsibilities and duplicated validation patterns for ports, timeouts, and field constraints.
Duplication Details
Pattern 1: Port Range Validation (3 instances)
Files:
internal/config/env_validation.go:275-276internal/config/schema_validation.go:240-248internal/config/validation.go:201-210
Duplicated Code:
// env_validation.go:275-276
if port < 1 || port > 65535 {
return 0, fmt.Errorf("MCP_GATEWAY_PORT must be between 1 and 65535, got %d", port)
}
// schema_validation.go:240-248
if port < 1 || port > 65535 {
return &ValidationError{
Field: "port",
Message: fmt.Sprintf("port must be between 1 and 65535, got %d", port),
JSONPath: "gateway.port",
Suggestion: "Use a valid port number (e.g., 8080)",
}
}
// validation.go:201-210
if *gateway.Port < 1 || *gateway.Port > 65535 {
return &ValidationError{
Field: "port",
Message: fmt.Sprintf("port must be between 1 and 65535, got %d", *gateway.Port),
JSONPath: "gateway.port",
Suggestion: "Use a valid port number (e.g., 8080)",
}
}Pattern 2: Timeout Validation (4 instances)
Files:
internal/config/schema_validation.go:265-281internal/config/validation.go:213-229
Duplicated Code:
// schema_validation.go:265-272
if stdinCfg.Gateway.StartupTimeout != nil && *stdinCfg.Gateway.StartupTimeout < 1 {
return &ValidationError{
Field: "startupTimeout",
Message: fmt.Sprintf("startupTimeout must be at least 1, got %d", *stdinCfg.Gateway.StartupTimeout),
JSONPath: "gateway.startupTimeout",
Suggestion: "Use a positive number of seconds (e.g., 30)",
}
}
// validation.go:213-220 (identical pattern)Both files repeat this for StartupTimeout and ToolTimeout.
Pattern 3: ValidationError Creation (15+ instances)
Pattern across both schema_validation.go and validation.go:
return &ValidationError{
Field: "fieldName",
Message: fmt.Sprintf("...", value),
JSONPath: "path.to.field",
Suggestion: "...",
}Instances:
- schema_validation.go: 7 instances (lines 192, 203, 215, 226, 242, 266, 274)
- validation.go: 8 instances (lines 49, 85, 96, 107, 117, 144, 154, 163)
Pattern 4: Mount Validation
Files:
internal/config/schema_validation.go:201-210internal/config/validation.go:78-124
Both validate mount format source:dest:mode with nearly identical logic.
schema_validation.go:201-210
for i, mount := range server.Mounts {
if !mountPattern.MatchString(mount) {
return &ValidationError{
Field: "mounts",
Message: fmt.Sprintf("mount '%s' does not match required pattern", mount),
JSONPath: fmt.Sprintf("%s.mounts[%d]", jsonPath, i),
Suggestion: "Use format 'source:dest:mode' where mode is 'ro' or 'rw'",
}
}
}validation.go:78-124 (46 lines)
func validateMounts(mounts []string, jsonPath string) error {
for i, mount := range mounts {
parts := strings.Split(mount, ":")
if len(parts) != 3 {
return &ValidationError{
Field: "mounts",
Message: fmt.Sprintf("invalid mount format '%s' (expected 'source:dest:mode')", mount),
JSONPath: fmt.Sprintf("%s.mounts[%d]", jsonPath, i),
Suggestion: "Use format 'source:dest:mode' where mode is 'ro' (read-only) or 'rw' (read-write)",
}
}
source, dest, mode := parts[0], parts[1], parts[2]
// Validate source is not empty
if source == "" {
return &ValidationError{...}
}
// Validate dest is not empty
if dest == "" {
return &ValidationError{...}
}
// Validate mode
if mode != "ro" && mode != "rw" {
return &ValidationError{...}
}
}
return nil
}File Responsibilities (Overlap)
env_validation.go
- Environment variable validation
- Docker accessibility checks
- Container environment validation
- PORT VALIDATION (overlap)
schema_validation.go
- JSON schema validation
- String pattern validation
- PORT VALIDATION (overlap)
- TIMEOUT VALIDATION (overlap)
- MOUNT VALIDATION (overlap)
validation.go
- Variable expansion
- Server config validation
- Gateway config validation
- PORT VALIDATION (overlap)
- TIMEOUT VALIDATION (overlap)
- MOUNT VALIDATION (overlap)
Issues
1. Unclear Validation Flow
- Three files validate the same fields
- Unclear which validator runs when
- Risk of conflicting validation logic
2. Inconsistent Error Messages
Port validation produces different error types:
- env_validation:
fmt.Errorf - schema_validation:
&ValidationError - validation:
&ValidationError
3. Maintenance Burden
- Changing port range requires updating 3 files
- Easy to miss one instance
- No single source of truth
Severity Assessment: MEDIUM
Impact:
- Maintenance burden: Update 3 files for constraint changes
- Inconsistency risk: Different error formats for same validation
- Testing complexity: Must test same validation in 3 places
- Code bloat: ~120 lines of duplicated validation logic
Evidence:
- Port validation: 3 instances
- Timeout validation: 4 instances (2 fields × 2 files)
- Mount validation: 2 instances (different detail levels)
- ValidationError creation: 15+ instances with similar structure
Refactoring Recommendations
1. Consolidate Validation Rules (Recommended)
// internal/config/rules/rules.go
package rules
// PortRange validates port is in valid range (1-65535)
func PortRange(port int, jsonPath string) *ValidationError {
if port < 1 || port > 65535 {
return &ValidationError{
Field: "port",
Message: fmt.Sprintf("port must be between 1 and 65535, got %d", port),
JSONPath: jsonPath,
Suggestion: "Use a valid port number (e.g., 8080)",
}
}
return nil
}
// TimeoutPositive validates timeout is >= 1
func TimeoutPositive(timeout int, fieldName, jsonPath string) *ValidationError {
if timeout < 1 {
return &ValidationError{
Field: fieldName,
Message: fmt.Sprintf("%s must be at least 1, got %d", fieldName, timeout),
JSONPath: jsonPath,
Suggestion: "Use a positive number of seconds",
}
}
return nil
}
// MountFormat validates mount format and components
func MountFormat(mount, jsonPath string, index int) *ValidationError {
// Single implementation of mount validation
}2. Update Validation Files
// env_validation.go
func GetGatewayPortFromEnv() (int, error) {
port, err := strconv.Atoi(portStr)
if err != nil {
return 0, err
}
if err := rules.PortRange(port, "MCP_GATEWAY_PORT"); err != nil {
return 0, fmt.Errorf("%s", err.Message)
}
return port, nil
}// validation.go
func validateGatewayConfig(gateway *StdinGatewayConfig) error {
if gateway.Port != nil {
if err := rules.PortRange(*gateway.Port, "gateway.port"); err != nil {
return err
}
}
if gateway.StartupTimeout != nil {
if err := rules.TimeoutPositive(*gateway.StartupTimeout, "startupTimeout", "gateway.startupTimeout"); err != nil {
return err
}
}
return nil
}3. Alternative: Validation Pipeline
// internal/config/validation_pipeline.go
type Validator interface {
Validate(cfg *StdinConfig) error
}
type ValidationPipeline struct {
validators []Validator
}
func (p *ValidationPipeline) Validate(cfg *StdinConfig) error {
for _, v := range p.validators {
if err := v.Validate(cfg); err != nil {
return err
}
}
return nil
}
// Usage:
pipeline := &ValidationPipeline{
validators: []Validator{
&SchemaValidator{},
&SemanticValidator{},
&EnvironmentValidator{},
},
}Estimated Effort
- Refactoring: 3-4 hours
- Testing: 2-3 hours (21 existing validation tests to update)
- Risk: Medium (central to config parsing)
Related Issues
- Error message formatting (potential shared formatter)
- Variable expansion validation (currently in validation.go only)
AI generated by Duplicate Code Detector
Reactions are currently unavailable