From 2d49e5b68caacdc337b7a67b17fcab05746c6842 Mon Sep 17 00:00:00 2001
From: Tadas Antanavicius <tadas@tadasant.com>
Date: Tue, 30 Sep 2025 06:43:26 -0700
Subject: [PATCH] fix: prevent NULL status values during migration 008 (#587)
MIME-Version: 1.0
Content-Type: text/plain; charset=UTF-8
Content-Transfer-Encoding: 8bit

## Summary
- Fixes migration 008 failing with "column contains null values" error
- Adds COALESCE protection to handle 126 servers without status fields
- Includes test utilities to reproduce and verify fixes with production
data

## Problem

Migration 008 was failing in production because:
- 126 out of 781 servers have no `status` field in their JSON
- When extracting `rec.value->>'status'`, these became NULL values
- The `ALTER TABLE servers ALTER COLUMN status SET NOT NULL` constraint
then failed

## Solution

1. **Prevent NULLs during migration**: Use COALESCE to provide 'active'
default when status is NULL/empty
2. **Enhanced debugging**: Added RAISE NOTICE statements to log
problematic values
3. **Safety check**: Double-check that throws exception if any NULLs
remain
4. **Test utilities**: Created mirror_data tools to test migrations with
real production data

## Testing

Created and used `scripts/mirror_data/` tools to:
- Fetch all 781 servers from production API
- Load them into a test database
- Verify the migration fix works correctly

### Results:
- **Before fix**: 126 NULL status values created during migration
- **After fix**: 0 NULL status values, all servers have valid status
- Migration completes successfully with NOT NULL constraint

## Test plan
- [x] Tested locally with production data using mirror_data tools
- [x] Verified 0 NULL values after migration (vs 126 before)
- [x] Confirmed NOT NULL constraint adds successfully
- [ ] Deploy to staging and verify migration succeeds
- [ ] Monitor production deployment

🤖 Generated with [Claude Code](https://claude.com/claude-code)

Co-authored-by: Claude <noreply@anthropic.com>
---
 .../008_separate_official_metadata.sql        |  38 ++-
 scripts/mirror_data/.gitignore                |   2 +
 scripts/mirror_data/README.md                 | 237 ++++++++++++++++++
 scripts/mirror_data/fetch_production_data.go  |  87 +++++++
 scripts/mirror_data/load_production_data.go   | 144 +++++++++++
 5 files changed, 506 insertions(+), 2 deletions(-)
 create mode 100644 scripts/mirror_data/.gitignore
 create mode 100644 scripts/mirror_data/README.md
 create mode 100644 scripts/mirror_data/fetch_production_data.go
 create mode 100644 scripts/mirror_data/load_production_data.go

diff --git a/internal/database/migrations/008_separate_official_metadata.sql b/internal/database/migrations/008_separate_official_metadata.sql
index a0b4af4..ad01864 100644
--- a/internal/database/migrations/008_separate_official_metadata.sql
+++ b/internal/database/migrations/008_separate_official_metadata.sql
@@ -38,7 +38,10 @@ BEGIN
             -- Note: status is at top level in old format, not in official_meta
             UPDATE servers
             SET
-                status = rec.value->>'status',
+                status = COALESCE(
+                    NULLIF(NULLIF(rec.value->>'status', ''), 'null'),
+                    'active'
+                ),
                 published_at = COALESCE((official_meta->>'publishedAt')::TIMESTAMP WITH TIME ZONE, NOW()),
                 updated_at = COALESCE((official_meta->>'updatedAt')::TIMESTAMP WITH TIME ZONE, NOW()),
                 is_latest = COALESCE((official_meta->>'isLatest')::BOOLEAN, true)
@@ -47,7 +50,10 @@ BEGIN
             -- Handle records without official metadata (set defaults)
             UPDATE servers
             SET
-                status = rec.value->>'status',
+                status = COALESCE(
+                    NULLIF(NULLIF(rec.value->>'status', ''), 'null'),
+                    'active'
+                ),
                 published_at = NOW(),
                 updated_at = NOW(),
                 is_latest = true
@@ -84,6 +90,23 @@ DROP FUNCTION migrate_official_metadata();
 
 -- Safety check: Normalize invalid status values before adding NOT NULL constraints
 -- This handles: NULL (missing field), empty string, string "null", and any invalid values
+-- First, log how many records have problematic status values for debugging
+DO $$
+DECLARE
+    null_count INTEGER;
+    empty_count INTEGER;
+    invalid_count INTEGER;
+BEGIN
+    SELECT COUNT(*) INTO null_count FROM servers WHERE status IS NULL;
+    SELECT COUNT(*) INTO empty_count FROM servers WHERE status = '' OR status = 'null';
+    SELECT COUNT(*) INTO invalid_count FROM servers WHERE status IS NOT NULL AND status NOT IN ('active', 'deprecated', 'deleted', '', 'null');
+
+    IF null_count > 0 OR empty_count > 0 OR invalid_count > 0 THEN
+        RAISE NOTICE 'Status cleanup: NULL=%, empty/null string=%, invalid=%', null_count, empty_count, invalid_count;
+    END IF;
+END $$;
+
+-- Fix all problematic status values
 UPDATE servers
 SET status = 'active'
 WHERE status IS NULL
@@ -91,6 +114,17 @@ WHERE status IS NULL
    OR status = 'null'
    OR status NOT IN ('active', 'deprecated', 'deleted');
 
+-- Double-check that no NULLs remain (this should never fail after the above)
+DO $$
+DECLARE
+    remaining_nulls INTEGER;
+BEGIN
+    SELECT COUNT(*) INTO remaining_nulls FROM servers WHERE status IS NULL;
+    IF remaining_nulls > 0 THEN
+        RAISE EXCEPTION 'Still have % NULL status values after cleanup!', remaining_nulls;
+    END IF;
+END $$;
+
 UPDATE servers SET published_at = NOW() WHERE published_at IS NULL;
 
 -- Safety check: Ensure updated_at is never NULL
diff --git a/scripts/mirror_data/.gitignore b/scripts/mirror_data/.gitignore
new file mode 100644
index 0000000..8768ebf
--- /dev/null
+++ b/scripts/mirror_data/.gitignore
@@ -0,0 +1,2 @@
+# Ignore downloaded production data
+production_servers.json
\ No newline at end of file
diff --git a/scripts/mirror_data/README.md b/scripts/mirror_data/README.md
new file mode 100644
index 0000000..5735c8d
--- /dev/null
+++ b/scripts/mirror_data/README.md
@@ -0,0 +1,237 @@
+# Mirror Production Data
+
+> **Note:** These tools were created by Claude Code as a simple way to kick the tires on data migrations.
+> They are not intended for production use.
+
+Tools to fetch and load production registry data for local testing and migration debugging.
+
+## Overview
+
+These scripts help you:
+1. Fetch all server data from the production registry API
+2. Load it into a local PostgreSQL database
+3. Test migrations against real production data
+
+## Prerequisites
+
+- Go 1.24.x
+- PostgreSQL (via Docker or local installation)
+- Required Go packages:
+  ```bash
+  go get github.com/google/uuid github.com/lib/pq
+  ```
+
+## Usage
+
+### 1. Fetch Production Data
+
+```bash
+# From the project root directory
+go run scripts/mirror_data/fetch_production_data.go
+```
+
+This will:
+- Fetch all servers from https://registry.modelcontextprotocol.io/v0/servers
+- Handle pagination automatically
+- Save data to `scripts/mirror_data/production_servers.json`
+- Be respectful to the API with rate limiting
+
+Output: `production_servers.json` containing all server records
+
+### 2. Set Up Test Database
+
+Start a PostgreSQL container for testing:
+
+```bash
+docker run -d --name test-postgres \
+  -p 5433:5432 \
+  -e POSTGRES_PASSWORD=testpass \
+  -e POSTGRES_DB=registry_test \
+  postgres:16
+```
+
+Or reset an existing database:
+
+```bash
+docker exec test-postgres psql -U postgres postgres -c "DROP DATABASE IF EXISTS registry_test;"
+docker exec test-postgres psql -U postgres postgres -c "CREATE DATABASE registry_test;"
+```
+
+### 3. Load Production Data
+
+```bash
+# From the project root directory
+go run scripts/mirror_data/load_production_data.go
+```
+
+This will:
+1. Connect to PostgreSQL at `localhost:5433`
+2. Apply migrations up to a configurable point (default: migration 007)
+3. Load all production server data
+4. Analyze the data and report statistics
+5. Show sample servers with NULL status values
+
+To test a different migration, edit `maxMigration` in the script (line 24)
+
+### 4. Test Migrations
+
+After loading the data, you can test migrations against real production data.
+
+#### Testing a Single Migration
+
+```bash
+# Test migration 008
+cat internal/database/migrations/008_separate_official_metadata.sql | \
+  docker exec -i test-postgres psql -U postgres -d registry_test
+```
+
+#### Debugging Failed Migrations
+
+1. **Check the error output:**
+```bash
+# Run migration and capture all output
+cat internal/database/migrations/008_separate_official_metadata.sql | \
+  docker exec -i test-postgres psql -U postgres -d registry_test 2>&1 | \
+  grep -E "(ERROR|NOTICE|WARNING)"
+```
+
+2. **Inspect data before migration:**
+```bash
+# Check for NULL values that might cause issues
+docker exec test-postgres psql -U postgres -d registry_test -c \
+  "SELECT COUNT(*) FROM servers WHERE value->>'status' IS NULL;"
+
+# Find sample problematic records
+docker exec test-postgres psql -U postgres -d registry_test -c \
+  "SELECT value->>'name', value->>'version' FROM servers \
+   WHERE value->>'status' IS NULL LIMIT 5;"
+```
+
+3. **Test partial migrations:**
+```bash
+# Extract and run only part of a migration to isolate issues
+# For example, run only up to the NOT NULL constraint:
+docker exec test-postgres psql -U postgres -d registry_test <<EOF
+BEGIN;
+-- Paste migration SQL up to the problematic line
+-- Check intermediate state
+SELECT COUNT(*) FROM servers WHERE status IS NULL;
+ROLLBACK;  -- or COMMIT if testing further
+EOF
+```
+
+#### Testing Migration Rollback
+
+```bash
+# Start a transaction to test and rollback
+docker exec test-postgres psql -U postgres -d registry_test <<EOF
+BEGIN;
+\i /dev/stdin < internal/database/migrations/008_separate_official_metadata.sql
+-- Inspect the results
+\d servers
+SELECT COUNT(*) FROM servers;
+ROLLBACK;  -- Undo all changes
+EOF
+```
+
+#### Iterative Testing Workflow
+
+1. **Reset database to clean state:**
+```bash
+docker exec test-postgres psql -U postgres postgres -c "DROP DATABASE IF EXISTS registry_test;"
+docker exec test-postgres psql -U postgres postgres -c "CREATE DATABASE registry_test;"
+go run scripts/mirror_data/load_production_data.go
+```
+
+2. **Test your migration changes:**
+```bash
+cat internal/database/migrations/008_separate_official_metadata.sql | \
+  docker exec -i test-postgres psql -U postgres -d registry_test
+```
+
+3. **Verify the results:**
+```bash
+# Check final table structure
+docker exec test-postgres psql -U postgres -d registry_test -c "\d servers"
+
+# Verify data integrity
+docker exec test-postgres psql -U postgres -d registry_test -c \
+  "SELECT COUNT(*), COUNT(DISTINCT server_name) FROM servers;"
+```
+
+#### Common Migration Issues to Check
+
+- **NULL values:** Check columns before adding NOT NULL constraints
+- **Unique constraints:** Verify no duplicates exist before adding
+- **Check constraints:** Validate all existing data matches the constraint
+- **Data type changes:** Ensure all values can be cast to new type
+- **Foreign keys:** Verify referential integrity before adding
+
+### 5. Clean Up
+
+```bash
+docker stop test-postgres
+docker rm test-postgres
+```
+
+## Configuration
+
+The `load_production_data.go` script connects to:
+- Host: `localhost`
+- Port: `5433`
+- Database: `registry_test`
+- User: `postgres`
+- Password: `testpass`
+
+Modify line 17 in the script if you need different connection parameters.
+
+## Example Output
+
+When running `load_production_data.go`:
+
+```
+Running migrations 001-007...
+  Applying migration 1: 001_initial_schema.sql
+  Applying migration 2: 002_add_server_extensions.sql
+  ...
+
+Loading production data...
+Loading 779 servers...
+Data loaded successfully!
+
+Total servers in database: 779
+
+Analyzing status field in JSON data...
+  Total servers: 779
+  NULL status: 126
+  Empty status: 0
+  'null' string status: 0
+  'active' status: 652
+  'deprecated' status: 0
+  'deleted' status: 0
+  Other/Invalid: 1
+
+Sample servers with NULL status:
+  - com.biodnd/agent-ip@0.1.2
+  - io.github.jkakar/cookwith-mcp@1.0.0
+  ...
+
+Database is ready for testing migration 008!
+```
+
+## Troubleshooting
+
+### Port already in use
+If port 5433 is already in use, either:
+- Stop the existing service using that port
+- Use a different port in both the Docker command and the Go script
+
+### Connection refused
+Ensure the PostgreSQL container is running and healthy:
+```bash
+docker ps | grep test-postgres
+docker logs test-postgres
+```
+
+### Migration failures
+The script stops at migration 007 intentionally. To test migration 008, run it manually after the data is loaded.
\ No newline at end of file
diff --git a/scripts/mirror_data/fetch_production_data.go b/scripts/mirror_data/fetch_production_data.go
new file mode 100644
index 0000000..ea2d32e
--- /dev/null
+++ b/scripts/mirror_data/fetch_production_data.go
@@ -0,0 +1,87 @@
+// This tool was created by Claude Code as a simple way to kick the tires on data migrations
+// by fetching production data from the public registry API.
+// It is not intended for production use.
+//
+//go:build ignore
+
+package main
+
+import (
+	"encoding/json"
+	"fmt"
+	"io"
+	"net/http"
+	"os"
+	"time"
+)
+
+type ServerResponse struct {
+	Servers  []json.RawMessage `json:"servers"`
+	Metadata struct {
+		NextCursor string `json:"next_cursor,omitempty"`
+		Count      int    `json:"count"`
+	} `json:"metadata"`
+}
+
+func main() {
+	baseURL := "https://registry.modelcontextprotocol.io/v0/servers"
+	var allServers []json.RawMessage
+	cursor := ""
+	pageCount := 0
+
+	for {
+		pageCount++
+		url := baseURL
+		if cursor != "" {
+			url = fmt.Sprintf("%s?cursor=%s", baseURL, cursor)
+		}
+
+		fmt.Printf("Fetching page %d: %s\n", pageCount, url)
+
+		resp, err := http.Get(url)
+		if err != nil {
+			panic(fmt.Sprintf("Failed to fetch: %v", err))
+		}
+		defer resp.Body.Close()
+
+		body, err := io.ReadAll(resp.Body)
+		if err != nil {
+			panic(fmt.Sprintf("Failed to read body: %v", err))
+		}
+
+		var serverResp ServerResponse
+		if err := json.Unmarshal(body, &serverResp); err != nil {
+			panic(fmt.Sprintf("Failed to parse JSON: %v", err))
+		}
+
+		allServers = append(allServers, serverResp.Servers...)
+		fmt.Printf("  Got %d servers (total: %d)\n", len(serverResp.Servers), len(allServers))
+
+		if serverResp.Metadata.NextCursor == "" {
+			break
+		}
+		cursor = serverResp.Metadata.NextCursor
+
+		// Be nice to the API
+		time.Sleep(100 * time.Millisecond)
+	}
+
+	// Save all servers to a file
+	output := map[string]interface{}{
+		"servers": allServers,
+		"count":   len(allServers),
+		"fetched": time.Now().Format(time.RFC3339),
+	}
+
+	data, err := json.MarshalIndent(output, "", "  ")
+	if err != nil {
+		panic(fmt.Sprintf("Failed to marshal output: %v", err))
+	}
+
+	outputFile := "production_servers.json"
+	if err := os.WriteFile(outputFile, data, 0644); err != nil {
+		panic(fmt.Sprintf("Failed to write file: %v", err))
+	}
+
+	fmt.Printf("\nDone! Saved %d servers to %s\n", len(allServers), outputFile)
+}
diff --git a/scripts/mirror_data/load_production_data.go b/scripts/mirror_data/load_production_data.go
new file mode 100644
index 0000000..7109446
--- /dev/null
+++ b/scripts/mirror_data/load_production_data.go
@@ -0,0 +1,144 @@
+// This tool was created by Claude Code as a simple way to kick the tires on data migrations
+// by loading production data into a test database for migration testing.
+// It is not intended for production use.
+//
+//go:build ignore
+
+package main
+
+import (
+	"database/sql"
+	"encoding/json"
+	"fmt"
+	"io/ioutil"
+	"log"
+	"path/filepath"
+
+	"github.com/google/uuid"
+	_ "github.com/lib/pq"
+)
+
+func main() {
+	// Database connection
+	db, err := sql.Open("postgres", "postgres://postgres:testpass@localhost:5433/registry_test?sslmode=disable")
+	if err != nil {
+		log.Fatal("Failed to connect to database:", err)
+	}
+	defer db.Close()
+
+	// Run migrations up to a specific point (configure as needed)
+	maxMigration := 7 // Change this to test different migration states
+	fmt.Printf("Running migrations 001-%03d...\n", maxMigration)
+	migrationsDir := "internal/database/migrations"
+	for i := 1; i <= maxMigration; i++ {
+		migrationFile := filepath.Join(migrationsDir, fmt.Sprintf("%03d_*.sql", i))
+		files, err := filepath.Glob(migrationFile)
+		if err != nil || len(files) == 0 {
+			log.Fatalf("Migration file %d not found", i)
+		}
+
+		content, err := ioutil.ReadFile(files[0])
+		if err != nil {
+			log.Fatalf("Failed to read migration %d: %v", i, err)
+		}
+
+		fmt.Printf("  Applying migration %d: %s\n", i, filepath.Base(files[0]))
+		if _, err := db.Exec(string(content)); err != nil {
+			log.Fatalf("Failed to apply migration %d: %v", i, err)
+		}
+	}
+
+	// Load production data
+	fmt.Println("\nLoading production data...")
+	data, err := ioutil.ReadFile("scripts/mirror_data/production_servers.json")
+	if err != nil {
+		log.Fatal("Failed to read production data:", err)
+	}
+
+	var prodData struct {
+		Servers []json.RawMessage `json:"servers"`
+	}
+	if err := json.Unmarshal(data, &prodData); err != nil {
+		log.Fatal("Failed to parse production data:", err)
+	}
+
+	fmt.Printf("Loading %d servers...\n", len(prodData.Servers))
+
+	// Prepare insert statement
+	stmt, err := db.Prepare("INSERT INTO servers (version_id, value) VALUES ($1, $2)")
+	if err != nil {
+		log.Fatal("Failed to prepare statement:", err)
+	}
+	defer stmt.Close()
+
+	// Insert each server
+	for i, server := range prodData.Servers {
+		// Generate a unique version_id
+		versionID := uuid.New().String()
+
+		// The server data is already JSON, just insert it
+		if _, err := stmt.Exec(versionID, server); err != nil {
+			log.Printf("Failed to insert server %d: %v", i, err)
+			continue
+		}
+	}
+
+	fmt.Println("Data loaded successfully!")
+
+	// Verify the data
+	var count int
+	db.QueryRow("SELECT COUNT(*) FROM servers").Scan(&count)
+	fmt.Printf("\nTotal servers in database: %d\n", count)
+
+	// Check for NULL status values in the JSON
+	fmt.Println("\nAnalyzing status field in JSON data...")
+
+	rows, err := db.Query(`
+		SELECT
+			COUNT(*) as total,
+			COUNT(CASE WHEN value->>'status' IS NULL THEN 1 END) as null_status,
+			COUNT(CASE WHEN value->>'status' = '' THEN 1 END) as empty_status,
+			COUNT(CASE WHEN value->>'status' = 'null' THEN 1 END) as string_null_status,
+			COUNT(CASE WHEN value->>'status' = 'active' THEN 1 END) as active_status,
+			COUNT(CASE WHEN value->>'status' = 'deprecated' THEN 1 END) as deprecated_status,
+			COUNT(CASE WHEN value->>'status' = 'deleted' THEN 1 END) as deleted_status
+		FROM servers
+	`)
+	if err != nil {
+		log.Fatal("Failed to analyze data:", err)
+	}
+	defer rows.Close()
+
+	if rows.Next() {
+		var total, nullStatus, emptyStatus, stringNullStatus, activeStatus, deprecatedStatus, deletedStatus int
+		rows.Scan(&total, &nullStatus, &emptyStatus, &stringNullStatus, &activeStatus, &deprecatedStatus, &deletedStatus)
+
+		fmt.Printf("  Total servers: %d\n", total)
+		fmt.Printf("  NULL status: %d\n", nullStatus)
+		fmt.Printf("  Empty status: %d\n", emptyStatus)
+		fmt.Printf("  'null' string status: %d\n", stringNullStatus)
+		fmt.Printf("  'active' status: %d\n", activeStatus)
+		fmt.Printf("  'deprecated' status: %d\n", deprecatedStatus)
+		fmt.Printf("  'deleted' status: %d\n", deletedStatus)
+		fmt.Printf("  Other/Invalid: %d\n", total-nullStatus-emptyStatus-stringNullStatus-activeStatus-deprecatedStatus-deletedStatus)
+	}
+
+	// Print sample servers with no status
+	fmt.Println("\nSample servers with NULL status:")
+	rows, err = db.Query(`
+		SELECT value->>'name', value->>'version'
+		FROM servers
+		WHERE value->>'status' IS NULL
+		LIMIT 5
+	`)
+	if err == nil {
+		defer rows.Close()
+		for rows.Next() {
+			var name, version string
+			rows.Scan(&name, &version)
+			fmt.Printf("  - %s@%s\n", name, version)
+		}
+	}
+
+	fmt.Printf("\nDatabase is ready for testing migration %03d!\n", maxMigration+1)
+}