Assist_Design/VALIDATION_AUDIT_REPORT.md

# Validation Schema & Logic Audit Report

**Date**: October 8, 2025  
**Scope**: Domain validation schemas and BFF validation logic  
**Status**: 🔴 Issues Found - Recommendations Provided

---

## Executive Summary

The codebase has undergone previous validation cleanup efforts (documented in `docs/validation/`), but there are still **critical overlaps and inconsistencies** that need to be addressed. This audit identifies:

- **5 major duplication issues** across validation utilities
- **1 architectural concern** with business logic placement
- **3 potential naming/organizational improvements**

---

## 🔴 Critical Issues

### 1. **UUID Validation Duplication** (Priority: HIGH)

**Problem**: Two different implementations of `isValidUuid()` exist:

| Location | Implementation | Issue |
|----------|---------------|--------|
| `packages/domain/common/validation.ts:76` | Uses Zod's `.uuid()` | ✅ Correct (Zod-based) |
| `packages/domain/toolkit/validation/helpers.ts:23` | Manual regex `/^[0-9a-f]{8}-...$/i` | ⚠️ Duplicate logic |

**Impact**: 
- Inconsistent validation behavior
- Maintenance burden (changes needed in two places)
- The regex version might accept invalid UUIDs that Zod would reject

**Recommendation**:
```typescript
// ❌ Remove from toolkit/validation/helpers.ts
export function isValidUuid(uuid: string): boolean {
  const uuidRegex = /^[0-9a-f]{8}-[0-9a-f]{4}-[0-9a-f]{4}-[0-9a-f]{4}-[0-9a-f]{12}$/i;
  return uuidRegex.test(uuid);
}

// ✅ Keep only in common/validation.ts (uses Zod)
export function isValidUuid(id: string): boolean {
  return uuidSchema.safeParse(id).success;
}
```

---

### 2. **UUID Schema Duplication** (Priority: HIGH)

**Problem**: `uuidSchema` is exported from two locations:

| Location | Definition |
|----------|------------|
| `packages/domain/common/validation.ts:13` | `z.string().uuid()` |
| `packages/domain/toolkit/validation/helpers.ts:202` | `z.string().uuid()` |

**Impact**:
- Import confusion (which one to use?)
- Violates Single Source of Truth principle

**Recommendation**:
- **Keep**: `packages/domain/common/validation.ts` (primary validation module)
- **Remove**: `packages/domain/toolkit/validation/helpers.ts` export
- **Alternative**: Re-export from common/validation if toolkit needs it

---

### 3. **Email Validation Duplication** (Priority: HIGH)

**Problem**: Two different implementations of `isValidEmail()`:

| Location | Implementation | Validation Quality |
|----------|---------------|-------------------|
| `packages/domain/common/validation.ts:69` | Uses Zod `.email()` | ✅ Robust (RFC 5322) |
| `packages/domain/toolkit/validation/email.ts:10` | Manual regex `/^[^\s@]+@[^\s@]+\.[^\s@]+$/` | ⚠️ Basic (misses edge cases) |

**Impact**:
- Zod's `.email()` is more comprehensive and battle-tested
- Manual regex misses many RFC 5322 edge cases
- Risk of accepting invalid emails or rejecting valid ones

**Recommendation**:
```typescript
// ❌ Remove basic regex version from toolkit/validation/email.ts
export function isValidEmail(email: string): boolean {
  const emailRegex = /^[^\s@]+@[^\s@]+\.[^\s@]+$/;
  return emailRegex.test(email);
}

// ✅ Keep Zod-based version in common/validation.ts
export function isValidEmail(email: string): boolean {
  return z.string().email().safeParse(email).success;
}
```

---

### 4. **URL Validation Duplication** (Priority: MEDIUM)

**Problem**: Three overlapping implementations:

| Location | Functions | Notes |
|----------|-----------|-------|
| `packages/domain/common/validation.ts` | `urlSchema`, `validateUrlOrThrow`, `validateUrl`, `isValidUrl` | ✅ Comprehensive (Zod + helpers) |
| `packages/domain/toolkit/validation/url.ts` | `isValidUrl`, `ensureProtocol`, `getHostname` | Mixed (validation + utilities) |
| `packages/domain/toolkit/validation/helpers.ts` | `validateUrl`, `isValidHttpUrl` | Partial duplication |

**Specific Duplications**:
- `isValidUrl` exists in both `common/validation.ts:119` and `toolkit/validation/url.ts:10`
- `validateUrl` exists in both `common/validation.ts:107` and `toolkit/validation/helpers.ts:169`

**Impact**:
- Import confusion
- Inconsistent validation (URL constructor vs Zod)

**Recommendation**:
```typescript
// ✅ Keep in common/validation.ts (schema + validation)
export const urlSchema = z.string().url();
export function isValidUrl(url: string): boolean { /* ... */ }
export function validateUrlOrThrow(url: string): string { /* ... */ }

// ✅ Keep in toolkit/validation/url.ts (utility functions only)
export function ensureProtocol(url: string): string { /* ... */ }
export function getHostname(url: string): string | null { /* ... */ }

// ❌ Remove duplicates from toolkit/validation/helpers.ts
```

---

### 5. **Pagination Schema Duplication** (Priority: LOW)

**Problem**: Two pagination implementations:

| Location | Export | Usage |
|----------|--------|-------|
| `packages/domain/common/schema.ts:99` | `paginationParamsSchema` | ✅ Used in domain schemas |
| `packages/domain/toolkit/validation/helpers.ts:207` | `createPaginationSchema()` | ❓ Utility function with options |

**Analysis**:
- `paginationParamsSchema` has fixed defaults (page=1, limit=20, max=100)
- `createPaginationSchema()` allows customizable min/max/default limits

**Recommendation**:
- **Keep both** but clarify their purposes:
  - `paginationParamsSchema` → Standard pagination (most use cases)
  - `createPaginationSchema()` → Custom pagination (special cases)
- **Document** when to use each in `VALIDATION_PATTERNS.md`

---

## 🟡 Architectural Concerns

### 6. **Business Logic in BFF Service** (Priority: MEDIUM)

**File**: `apps/bff/src/modules/subscriptions/sim-management/services/sim-validation.service.ts`

**Problem**: Contains hardcoded business rules that should be in domain:

```typescript:58:62
// Hardcoded fallback SIM number
if (!account) {
  account = "02000331144508";
  this.logger.warn(`No SIM account identifier found, using test SIM: ${account}`);
}
```

**Issues**:
- Magic numbers in BFF layer (should be in domain or config)
- Business logic for extracting SIM account from custom fields (lines 110-195)
- Hardcoded field name mappings for WHMCS

**Impact**:
- Difficult to test in isolation
- Cannot reuse logic if needed elsewhere
- Violates domain-driven design principles

**Recommendation**:
1. Move SIM account extraction logic to domain:
   ```
   packages/domain/sim/validation.ts
   - extractSimAccountFromSubscription()
   - getSimAccountFromCustomFields()
   ```

2. Move field name mappings to domain contracts:
   ```
   packages/domain/sim/contract.ts
   - SIM_ACCOUNT_FIELD_NAMES constant
   ```

3. Keep only infrastructure concerns in BFF:
   - Database calls
   - HTTP requests
   - Logging

---

## 🟢 Good Practices Observed

### ✅ Proper Validation Patterns

1. **Controllers use ZodValidationPipe** (e.g., `orders.controller.ts:21`, `auth.controller.ts:125`)
   ```typescript
   @UsePipes(new ZodValidationPipe(createOrderRequestSchema))
   ```

2. **Domain schemas are reused** (no duplication in controllers)
   ```typescript
   import { createOrderRequestSchema } from "@customer-portal/domain/orders";
   ```

3. **Business validation separated from format validation**:
   - Format: `orderBusinessValidationSchema` (domain)
   - Business: `OrderValidator.validateBusinessRules()` (BFF service)

4. **Sanitization functions don't perform validation** (e.g., `mappings/validation.ts:106-112`)

---

## 📋 Summary of Recommendations

### Immediate Actions (High Priority)

1. **Remove duplicate `isValidUuid` from toolkit/validation/helpers.ts**
   - Keep only the Zod-based version in common/validation.ts

2. **Remove duplicate `uuidSchema` export from toolkit/validation/helpers.ts**
   - Use common/validation.ts as single source

3. **Remove duplicate `isValidEmail` from toolkit/validation/email.ts**
   - Keep only the Zod-based version in common/validation.ts

4. **Consolidate URL validation**:
   - Remove `isValidUrl` and `validateUrl` from toolkit/validation/helpers.ts
   - Keep validation in common/validation.ts
   - Keep only utility functions in toolkit/validation/url.ts

### Medium Priority

5. **Move SIM business logic to domain**
   - Extract SIM account extraction to `packages/domain/sim/validation.ts`
   - Move field mappings to domain constants

6. **Document pagination schema usage**
   - Clarify when to use `paginationParamsSchema` vs `createPaginationSchema()`

### Low Priority

7. **Review toolkit/validation organization**
   - Consider if toolkit should only contain utilities (not validation)
   - Maybe rename to `toolkit/helpers` or `toolkit/utils`

---

## 📁 Validation File Organization

### Current Structure (Good)
```
packages/domain/
├── common/
│   ├── schema.ts         ✅ Shared Zod schemas
│   └── validation.ts     ✅ Shared validation functions
├── orders/
│   ├── schema.ts         ✅ Order-specific schemas
│   └── validation.ts     ✅ Order business validation
├── auth/
│   └── schema.ts         ✅ Auth schemas
├── mappings/
│   ├── schema.ts         ✅ Mapping schemas
│   └── validation.ts     ✅ Mapping business validation
└── toolkit/
    └── validation/       ⚠️ Overlaps with common/validation
        ├── helpers.ts    ⚠️ Duplicates from common/validation
        ├── email.ts      ⚠️ Duplicates isValidEmail
        ├── url.ts        ⚠️ Duplicates URL validation
        └── string.ts     ✅ String utilities (OK)
```

### Recommended Structure
```
packages/domain/
├── common/
│   ├── schema.ts         → All shared Zod schemas
│   └── validation.ts     → All shared validation functions
├── [domain]/
│   ├── schema.ts         → Domain-specific schemas
│   └── validation.ts     → Domain business validation
└── toolkit/
    ├── formatting/       → Date, currency, text formatting
    ├── typing/          → Type guards and assertions
    └── utilities/       → Pure utility functions (no validation)
        ├── string.ts    → String manipulation (not validation)
        ├── url.ts       → URL utilities (ensureProtocol, getHostname)
        └── email.ts     → Email utilities (getEmailDomain, normalizeEmail)
```

**Key Principle**: 
- **Validation** (anything that returns `boolean` or throws errors) → `common/validation.ts` or domain-specific `validation.ts`
- **Utilities** (transformations, formatting, parsing) → `toolkit/utilities/`

---

## 🎯 Implementation Checklist

- [ ] Remove `isValidUuid` from `toolkit/validation/helpers.ts:23`
- [ ] Remove `uuidSchema` export from `toolkit/validation/helpers.ts:202`
- [ ] Remove `isValidEmail` from `toolkit/validation/email.ts:10`
- [ ] Remove `validateUrl` from `toolkit/validation/helpers.ts:169`
- [ ] Remove `isValidHttpUrl` from `toolkit/validation/helpers.ts:181` (or move to common)
- [ ] Remove `isValidUrl` from `toolkit/validation/url.ts:10`
- [ ] Move SIM account extraction to `packages/domain/sim/validation.ts`
- [ ] Document pagination schema usage patterns
- [ ] Update imports across codebase to use common/validation
- [ ] Add tests to verify validation consistency

---

## 📝 Notes

### Why This Matters

1. **Security**: Inconsistent validation can lead to security vulnerabilities
2. **Maintainability**: Multiple implementations mean multiple places to update
3. **Testing**: Easier to test validation when it's in one place
4. **Developer Experience**: Clear where to find and how to use validation

### Previous Cleanup Efforts

The codebase has already undergone significant validation cleanup:
- Password validation duplication removed
- SKU validation consolidated
- Sanitization functions simplified
- ZodValidationPipe adopted in controllers

This audit builds on that work to eliminate remaining overlaps.

---

## 🔍 Testing Recommendations

After implementing changes, verify:

1. **All imports still resolve**:
   ```bash
   pnpm build
   ```

2. **No validation regressions**:
   ```bash
   pnpm test:unit
   pnpm test:e2e
   ```

3. **Consistent validation behavior**:
   - UUID validation rejects malformed UUIDs
   - Email validation handles edge cases
   - URL validation handles relative URLs correctly

---

## 📚 References

- [VALIDATION_PATTERNS.md](docs/validation/VALIDATION_PATTERNS.md) - Existing validation guidelines
- [VALIDATION_CLEANUP_SUMMARY.md](docs/validation/VALIDATION_CLEANUP_SUMMARY.md) - Previous cleanup work
- Domain README: [packages/domain/README.md](packages/domain/README.md)