diff --git a/docs/VALIDATION_CENTRALIZATION.md b/docs/VALIDATION_CENTRALIZATION.md new file mode 100644 index 00000000..c646fc0a --- /dev/null +++ b/docs/VALIDATION_CENTRALIZATION.md @@ -0,0 +1,194 @@ +# Validation Centralization - Critical Issue #3 Fixed + +## Overview + +This document describes the changes made to centralize all business logic validation in the edge function, removing duplicate validation from the React frontend. + +## Problem Statement + +Previously, validation was duplicated in two places: + +1. **React Frontend** (`useModerationActions.ts`): Performed full business logic validation using Zod schemas before calling the edge function +2. **Edge Function** (`process-selective-approval`): Also performed full business logic validation + +This created several issues: +- **Duplicate Code**: Same validation logic maintained in two places +- **Inconsistency Risk**: Frontend and backend could have different validation rules +- **Performance**: Unnecessary network round-trips for validation data fetching +- **Single Source of Truth Violation**: No clear authority on what's valid + +## Solution: Edge Function as Single Source of Truth + +### Architecture Changes + +``` +┌─────────────────────────────────────────────────────────────────┐ +│ BEFORE (Duplicate) │ +├─────────────────────────────────────────────────────────────────┤ +│ │ +│ React Frontend Edge Function │ +│ ┌──────────────┐ ┌──────────────┐ │ +│ │ UX Validation│ │ Business │ │ +│ │ + │──────────────▶│ Validation │ │ +│ │ Business │ If valid │ │ │ +│ │ Validation │ call edge │ (Duplicate) │ │ +│ └──────────────┘ └──────────────┘ │ +│ ❌ Duplicate validation logic │ +└─────────────────────────────────────────────────────────────────┘ + +┌─────────────────────────────────────────────────────────────────┐ +│ AFTER (Centralized) ✅ │ +├─────────────────────────────────────────────────────────────────┤ +│ │ +│ React Frontend Edge Function │ +│ ┌──────────────┐ ┌──────────────┐ │ +│ │ UX Validation│ │ Business │ │ +│ │ Only │──────────────▶│ Validation │ │ +│ │ (non-empty, │ Always │ (Authority) │ │ +│ │ format) │ call edge │ │ │ +│ └──────────────┘ └──────────────┘ │ +│ ✅ Single source of truth │ +└─────────────────────────────────────────────────────────────────┘ +``` + +### Changes Made + +#### 1. React Frontend (`src/hooks/moderation/useModerationActions.ts`) + +**Removed:** +- Import of `validateMultipleItems` from `entityValidationSchemas` +- 200+ lines of validation code that: + - Fetched full item data with relational joins + - Ran Zod validation on all items + - Blocked approval if validation failed + - Logged validation errors + +**Added:** +- Clear comment explaining validation happens server-side only +- Enhanced error handling to detect validation errors from edge function + +**What Remains:** +- Basic error handling for edge function responses +- Toast notifications for validation failures +- Proper error logging with validation flag + +#### 2. Validation Schemas (`src/lib/entityValidationSchemas.ts`) + +**Updated:** +- Added comprehensive documentation header +- Marked schemas as "documentation only" for React app +- Clarified that edge function is the authority +- Noted these schemas should mirror edge function validation + +**Status:** +- File retained for documentation and future reference +- Not imported anywhere in production React code +- Can be used for basic client-side UX validation if needed + +#### 3. Edge Function (`supabase/functions/process-selective-approval/index.ts`) + +**No Changes Required:** +- Already has comprehensive validation via `validateEntityDataStrict()` +- Already returns proper 400 errors for validation failures +- Already includes detailed error messages + +## Validation Responsibilities + +### Client-Side (React Forms) + +**Allowed:** +- ✅ Non-empty field validation (required fields) +- ✅ Basic format validation (email, URL format) +- ✅ Character length limits +- ✅ Input masking and formatting +- ✅ Immediate user feedback for UX + +**Not Allowed:** +- ❌ Business rule validation (e.g., closing date after opening date) +- ❌ Cross-field validation +- ❌ Database constraint validation +- ❌ Entity relationship validation +- ❌ Status/state validation + +### Server-Side (Edge Function) + +**Authoritative For:** +- ✅ All business logic validation +- ✅ Cross-field validation +- ✅ Database constraint validation +- ✅ Entity relationship validation +- ✅ Status/state validation +- ✅ Security validation +- ✅ Data integrity checks + +## Error Handling Flow + +```typescript +// 1. User clicks "Approve" in UI +// 2. React calls edge function immediately (no validation) +const { data, error } = await invokeWithTracking('process-selective-approval', { + itemIds: [...], + submissionId: '...' +}); + +// 3. Edge function validates and returns error if invalid +if (error) { + // Error contains validation details from edge function + // React displays the error message + toast({ + title: 'Validation Failed', + description: error.message // e.g., "Park name is required" + }); +} +``` + +## Benefits + +1. **Single Source of Truth**: Edge function is the authority +2. **Consistency**: No risk of frontend/backend validation diverging +3. **Performance**: No pre-validation data fetching in frontend +4. **Maintainability**: Update validation in one place +5. **Security**: Can't bypass validation by manipulating frontend +6. **Simplicity**: Frontend code is simpler and cleaner + +## Testing Validation + +To test that validation works: + +1. Submit a park without required fields +2. Submit a park with invalid dates (closing before opening) +3. Submit a ride without a park_id +4. Submit a company with invalid email format + +Expected: Edge function should return 400 error with detailed message, React should display error toast. + +## Migration Guide + +If you need to add new validation rules: + +1. ✅ **Add to edge function** (`process-selective-approval/index.ts`) + - Update `validateEntityDataStrict()` function + - Add to appropriate entity type case + +2. ✅ **Update documentation schemas** (`entityValidationSchemas.ts`) + - Keep schemas in sync for reference + - Update comments if rules change + +3. ❌ **DO NOT add to React validation** + - React should only do basic UX validation + - Business logic belongs in edge function + +## Related Issues + +This fix addresses: +- ✅ Critical Issue #3: Validation centralization +- ✅ Removes ~200 lines of duplicate code +- ✅ Eliminates validation timing gap +- ✅ Simplifies frontend logic +- ✅ Improves maintainability + +## Files Changed + +- `src/hooks/moderation/useModerationActions.ts` - Removed validation logic +- `src/lib/entityValidationSchemas.ts` - Updated documentation +- `docs/VALIDATION_CENTRALIZATION.md` - This document diff --git a/src/hooks/moderation/useModerationActions.ts b/src/hooks/moderation/useModerationActions.ts index 9d7da83f..a8989c1c 100644 --- a/src/hooks/moderation/useModerationActions.ts +++ b/src/hooks/moderation/useModerationActions.ts @@ -4,7 +4,7 @@ import { supabase } from '@/lib/supabaseClient'; import { useToast } from '@/hooks/use-toast'; import { logger } from '@/lib/logger'; import { getErrorMessage, handleError, isSupabaseConnectionError } from '@/lib/errorHandler'; -import { validateMultipleItems } from '@/lib/entityValidationSchemas'; +// Validation removed from client - edge function is single source of truth import { invokeWithTracking } from '@/lib/edgeFunctionTracking'; import type { User } from '@supabase/supabase-js'; import type { ModerationItem } from '@/types/moderation'; @@ -133,206 +133,10 @@ export function useModerationActions(config: ModerationActionsConfig): Moderatio if (submissionItems && submissionItems.length > 0) { if (action === 'approved') { - // Fetch full item data for validation with relational joins - const { data: fullItems, error: itemError } = await supabase - .from('submission_items') - .select(` - id, - item_type, - park_submission:park_submissions!submission_items_park_submission_id_fkey(*), - ride_submission:ride_submissions!submission_items_ride_submission_id_fkey(*), - company_submission:company_submissions!submission_items_company_submission_id_fkey(*), - ride_model_submission:ride_model_submissions!submission_items_ride_model_submission_id_fkey(*), - timeline_event_submission:timeline_event_submissions!submission_items_timeline_event_submission_id_fkey(*), - photo_submission:photo_submissions!submission_items_photo_submission_id_fkey(*) - `) - .eq('submission_id', item.id) - .in('status', ['pending', 'rejected']); - - if (itemError) { - throw new Error(`Failed to fetch submission items: ${itemError.message}`); - } - - if (fullItems && fullItems.length > 0) { - console.info('[Submission Flow] Preparing items for validation', { - submissionId: item.id, - itemCount: fullItems.length, - itemTypes: fullItems.map(i => i.item_type), - timestamp: new Date().toISOString() - }); - - // Transform to include item_data - const itemsWithData = await Promise.all(fullItems.map(async item => { - let itemData = {}; - switch (item.item_type) { - case 'park': { - const parkSub = (item.park_submission as any) || {}; - let locationData: any = null; - if (parkSub?.id) { - const { data } = await supabase - .from('park_submission_locations') - .select('*') - .eq('park_submission_id', parkSub.id) - .maybeSingle(); - locationData = data; - } - itemData = { - ...parkSub, - location: locationData || undefined - }; - break; - } - case 'ride': - itemData = item.ride_submission || {}; - break; - case 'operator': - case 'manufacturer': - case 'designer': - case 'property_owner': - itemData = { - ...(item.company_submission || {}), - company_id: item.company_submission?.id // Use company_submission ID for validation - }; - break; - case 'ride_model': - itemData = { - ...(item.ride_model_submission || {}), - ride_model_id: item.ride_model_submission?.id // Use ride_model_submission ID for validation - }; - break; - case 'milestone': - case 'timeline_event': - itemData = item.timeline_event_submission || {}; - break; - case 'photo': - case 'photo_edit': - case 'photo_delete': - itemData = item.photo_submission || {}; - break; - default: - logger.warn(`Unknown item_type in validation: ${item.item_type}`); - itemData = {}; - } - return { - id: item.id, - item_type: item.item_type, - item_data: itemData - }; - })); - - // Run validation on all items - try { - console.info('[Submission Flow] Starting validation', { - submissionId: item.id, - itemCount: itemsWithData.length, - itemTypes: itemsWithData.map(i => i.item_type), - timestamp: new Date().toISOString() - }); - - const validationResults = await validateMultipleItems(itemsWithData); - - console.info('[Submission Flow] Validation completed', { - submissionId: item.id, - resultsCount: validationResults.size, - timestamp: new Date().toISOString() - }); - - // Check for blocking errors - const itemsWithBlockingErrors = itemsWithData.filter(item => { - const result = validationResults.get(item.id); - return result && result.blockingErrors.length > 0; - }); - - // CRITICAL: Block approval if any item has blocking errors - if (itemsWithBlockingErrors.length > 0) { - console.warn('[Submission Flow] Validation found blocking errors', { - submissionId: item.id, - itemsWithErrors: itemsWithBlockingErrors.map(i => ({ - itemId: i.id, - itemType: i.item_type, - errors: validationResults.get(i.id)?.blockingErrors - })), - timestamp: new Date().toISOString() - }); - - // Log detailed blocking errors - itemsWithBlockingErrors.forEach(item => { - const result = validationResults.get(item.id); - logger.error('Validation blocking approval', { - submissionId: item.id, - itemId: item.id, - itemType: item.item_type, - blockingErrors: result?.blockingErrors - }); - }); - - const errorDetails = itemsWithBlockingErrors.map(item => { - const result = validationResults.get(item.id); - const itemName = (item.item_data as any)?.name || item.item_type; - const errors = result?.blockingErrors.map(e => `${e.field}: ${e.message}`).join(', '); - return `${itemName} - ${errors}`; - }).join('; '); - - throw new Error(`Validation failed: ${errorDetails}`); - } - - // Check for warnings (optional - can proceed but inform user) - const itemsWithWarnings = itemsWithData.filter(item => { - const result = validationResults.get(item.id); - return result && result.warnings.length > 0; - }); - - if (itemsWithWarnings.length > 0) { - logger.info('Approval proceeding with warnings', { - submissionId: item.id, - warningCount: itemsWithWarnings.length - }); - } - } catch (error) { - // Check if this is a validation error or system error - if (getErrorMessage(error).includes('Validation failed:')) { - // This is expected - validation rules preventing approval - handleError(error, { - action: 'Validation Blocked Approval', - userId: user?.id, - metadata: { - submissionId: item.id, - submissionType: item.submission_type, - selectedItemCount: itemsWithData.length - } - }); - - toast({ - title: 'Cannot Approve - Validation Errors', - description: getErrorMessage(error), - variant: 'destructive', - }); - - // Return early - do NOT proceed with approval - return; - } else { - // Unexpected validation system error - const errorId = handleError(error, { - action: 'Validation System Failure', - userId: user?.id, - metadata: { - submissionId: item.id, - submissionType: item.submission_type, - phase: 'validation' - } - }); - - toast({ - title: 'Validation System Error', - description: `Unable to validate submission (ref: ${errorId.slice(0, 8)})`, - variant: 'destructive', - }); - - // Return early - do NOT proceed with approval - return; - } - } - } + // ⚠️ VALIDATION CENTRALIZED IN EDGE FUNCTION + // All business logic validation happens in process-selective-approval edge function. + // Client-side only performs basic UX validation (non-empty, format) in forms. + // If server-side validation fails, the edge function returns detailed 400/500 errors. const { data, error, requestId, attempts } = await invokeWithTracking( 'process-selective-approval', @@ -465,8 +269,14 @@ export function useModerationActions(config: ModerationActionsConfig): Moderatio const isNetworkError = isSupabaseConnectionError(error); const errorMessage = getErrorMessage(error) || `Failed to ${variables.action} content`; + // Check if this is a validation error from edge function + const isValidationError = errorMessage.includes('Validation failed') || + errorMessage.includes('blocking errors') || + errorMessage.includes('blockingErrors'); + toast({ - title: isNetworkError ? 'Connection Error' : 'Action Failed', + title: isNetworkError ? 'Connection Error' : + isValidationError ? 'Validation Failed' : 'Action Failed', description: errorMessage, variant: 'destructive', }); @@ -477,6 +287,7 @@ export function useModerationActions(config: ModerationActionsConfig): Moderatio error: errorMessage, errorId: error.errorId, isNetworkError, + isValidationError, }); }, onSuccess: (data) => { diff --git a/src/lib/entityValidationSchemas.ts b/src/lib/entityValidationSchemas.ts index b3588c29..8f4bcb8a 100644 --- a/src/lib/entityValidationSchemas.ts +++ b/src/lib/entityValidationSchemas.ts @@ -3,9 +3,25 @@ import { supabase } from '@/lib/supabaseClient'; import { handleNonCriticalError, getErrorMessage } from '@/lib/errorHandler'; import { logger } from '@/lib/logger'; +// ============================================ +// VALIDATION SCHEMAS - DOCUMENTATION ONLY +// ============================================ +// ⚠️ NOTE: These schemas are currently NOT used in the React application. +// All business logic validation happens server-side in the edge function. +// These schemas are kept for: +// 1. Documentation of validation rules +// 2. Potential future use for client-side UX validation (basic checks only) +// 3. Reference when updating edge function validation logic +// +// DO NOT import these in production code for business logic validation. +// ============================================ + // ============================================ // CENTRALIZED VALIDATION SCHEMAS -// Single source of truth for all entity validation +// ⚠️ CRITICAL: These schemas represent the validation rules +// They should mirror the validation in process-selective-approval edge function +// Client-side should NOT perform business logic validation +// Client-side only does basic UX validation (non-empty, format checks) in forms // ============================================ const currentYear = new Date().getFullYear();