From ddcc58684234a1cff563e70a28845d627e4ebf50 Mon Sep 17 00:00:00 2001 From: Radoslav Dimitrov Date: Tue, 23 Sep 2025 17:40:20 +0200 Subject: [PATCH] Remove the in-memory database (#537) ## Motivation and Context The follow PR removes the in-memory database option from the registry. We had a chat about this with @domdomegg and although it is supposed to be convenient, it isn't that useful in reality. We want to test and develop the registry in a setup that is closer to the environment it is supposed to be run. One of the main reasons are: * We don't use it anywhere else besides unit testing and potentially local development. Instead we should use postgresql for reliable results (in-memory was actually giving false positive results) * We have to keep maintaining it and make sure it mimics the behaviour we have for postgresql which is not yielding a lot of value ## How Has This Been Tested? Yes, I've updated the test unit make target to run in the postgresql environment and now we can see the race condition issue happening. ## Breaking Changes No ## Types of changes - [ ] Bug fix (non-breaking change which fixes an issue) - [ ] New feature (non-breaking change which adds functionality) - [ ] Breaking change (fix or feature that would cause existing functionality to change) - [ ] Documentation update ## Checklist - [ ] I have read the [MCP Documentation](https://modelcontextprotocol.io) - [ ] My code follows the repository's style guidelines - [ ] New and existing tests pass locally - [ ] I have added appropriate error handling - [ ] I have added or updated documentation as needed ## Additional context --------- Signed-off-by: Radoslav Dimitrov --- .env.example | 2 +- Makefile | 16 +- README.md | 21 +- cmd/registry/main.go | 42 +- docker-compose.yml | 2 - go.mod | 2 +- internal/api/handlers/v0/edit_test.go | 2 +- .../handlers/v0/publish_integration_test.go | 2 +- .../v0/publish_registry_validation_test.go | 2 +- internal/api/handlers/v0/publish_test.go | 4 +- internal/api/handlers/v0/servers_test.go | 8 +- internal/api/handlers/v0/telemetry_test.go | 2 +- internal/config/config.go | 1 - internal/database/database.go | 9 - internal/database/memory.go | 359 ------------------ internal/database/testutil.go | 137 +++++++ internal/importer/importer_test.go | 22 +- internal/service/registry_service_test.go | 22 +- tests/integration/run.sh | 2 + 19 files changed, 202 insertions(+), 455 deletions(-) delete mode 100644 internal/database/memory.go create mode 100644 internal/database/testutil.go diff --git a/.env.example b/.env.example index 24d32d7..b465111 100644 --- a/.env.example +++ b/.env.example @@ -5,7 +5,7 @@ MCP_REGISTRY_SERVER_ADDRESS=:8080 MCP_REGISTRY_VERSION=dev # Database configuration -# Supported types: postgresql, memory +# Supported types: postgresql MCP_REGISTRY_DATABASE_TYPE=postgresql MCP_REGISTRY_DATABASE_URL=postgres://username:password@localhost:5432/mcp-registry diff --git a/Makefile b/Makefile index b8711cc..1ab5e8e 100644 --- a/Makefile +++ b/Makefile @@ -1,4 +1,4 @@ -.PHONY: help build test test-unit test-integration test-endpoints test-publish test-all lint lint-fix validate validate-schemas validate-examples check dev-local dev-compose clean publisher +.PHONY: help build test test-unit test-integration test-endpoints test-publish test-all lint lint-fix validate validate-schemas validate-examples check dev-compose clean publisher # Default target help: ## Show this help message @@ -15,10 +15,17 @@ publisher: ## Build the publisher tool with version info go build -ldflags="-X main.Version=dev-$(shell git rev-parse --short HEAD) -X main.GitCommit=$(shell git rev-parse HEAD) -X main.BuildTime=$(shell date -u +%Y-%m-%dT%H:%M:%SZ)" -o bin/mcp-publisher ./cmd/publisher # Test targets -test-unit: ## Run unit tests with coverage - go test -v -race -coverprofile=coverage.out -covermode=atomic -coverpkg=./internal/... ./internal/... +test-unit: ## Run unit tests with coverage (requires PostgreSQL) + @echo "Starting PostgreSQL for unit tests..." + @docker compose up -d postgres + @echo "Waiting for PostgreSQL to be ready..." + @sleep 3 + @echo "Running unit tests..." + go test -v -race -coverprofile=coverage.out -covermode=atomic ./internal/... go tool cover -html=coverage.out -o coverage.html @echo "Coverage report generated: coverage.html" + @echo "Stopping PostgreSQL..." + @docker compose down postgres test: ## Run unit tests (use 'make test-all' to run all tests) @echo "⚠️ Running unit tests only. Use 'make test-all' to run both unit and integration tests." @@ -59,9 +66,6 @@ check: lint validate test-all ## Run all checks (lint, validate, unit tests) dev-compose: ## Start development environment with Docker Compose (builds image automatically) docker compose up --build -dev-local: ## Run registry locally - go run ./cmd/registry - # Cleanup clean: ## Clean build artifacts and coverage files rm -rf bin diff --git a/README.md b/README.md index 90ae4c7..2396bbc 100644 --- a/README.md +++ b/README.md @@ -39,24 +39,9 @@ Often (but not always) ideas flow through this pipeline: make dev-compose ``` -This starts the registry at [`localhost:8080`](http://localhost:8080) with PostgreSQL and seed data. It can be configured with environment variables in [docker-compose.yml](./docker-compose.yml) - see [.env.example](./.env.example) for a reference. +This starts the registry at [`localhost:8080`](http://localhost:8080) with PostgreSQL and seed data. The database uses ephemeral storage and is reset each time you restart the containers, ensuring a clean state for development and testing. -
-Alternative: Local setup without Docker - -**Prerequisites:** -- PostgreSQL running locally -- Go 1.24.x installed - -```bash -# Build and run locally -make build -make dev-local -``` - -The service runs on [`localhost:8080`](http://localhost:8080) by default. This can be configured with environment variables in `.env` - see [.env.example](./.env.example) for a reference. - -
+The setup can be configured with environment variables in [docker-compose.yml](./docker-compose.yml) - see [.env.example](./.env.example) for a reference.
Alternative: Running a pre-built Docker image @@ -125,7 +110,7 @@ For Claude and other AI tools: Always prefer make targets over custom commands w │ ├── api/ # HTTP handlers and routing │ ├── auth/ # Authentication (GitHub OAuth, JWT, namespace blocking) │ ├── config/ # Configuration management -│ ├── database/ # Data persistence (PostgreSQL, in-memory) +│ ├── database/ # Data persistence (PostgreSQL) │ ├── service/ # Business logic │ ├── telemetry/ # Metrics and monitoring │ └── validators/ # Input validation diff --git a/cmd/registry/main.go b/cmd/registry/main.go index 749c21b..cda8ace 100644 --- a/cmd/registry/main.go +++ b/cmd/registry/main.go @@ -56,36 +56,26 @@ func main() { // Initialize configuration cfg := config.NewConfig() - // Initialize services based on environment - switch cfg.DatabaseType { - case config.DatabaseTypeMemory: - db = database.NewMemoryDB() - case config.DatabaseTypePostgreSQL: - // Use PostgreSQL for real registry service - // Create a context with timeout for PostgreSQL connection - ctx, cancel := context.WithTimeout(context.Background(), 10*time.Second) - defer cancel() + // Create a context with timeout for PostgreSQL connection + ctx, cancel := context.WithTimeout(context.Background(), 10*time.Second) + defer cancel() - // Connect to PostgreSQL - db, err = database.NewPostgreSQL(ctx, cfg.DatabaseURL) - if err != nil { - log.Printf("Failed to connect to PostgreSQL: %v", err) - return - } - - // Store the PostgreSQL instance for later cleanup - defer func() { - if err := db.Close(); err != nil { - log.Printf("Error closing PostgreSQL connection: %v", err) - } else { - log.Println("PostgreSQL connection closed successfully") - } - }() - default: - log.Printf("Invalid database type: %s; supported types: %s, %s", cfg.DatabaseType, config.DatabaseTypeMemory, config.DatabaseTypePostgreSQL) + // Connect to PostgreSQL + db, err = database.NewPostgreSQL(ctx, cfg.DatabaseURL) + if err != nil { + log.Printf("Failed to connect to PostgreSQL: %v", err) return } + // Store the PostgreSQL instance for later cleanup + defer func() { + if err := db.Close(); err != nil { + log.Printf("Error closing PostgreSQL connection: %v", err) + } else { + log.Println("PostgreSQL connection closed successfully") + } + }() + registryService = service.NewRegistryService(db, cfg) // Import seed data if seed source is provided diff --git a/docker-compose.yml b/docker-compose.yml index bdbdab4..8c50a81 100644 --- a/docker-compose.yml +++ b/docker-compose.yml @@ -36,8 +36,6 @@ services: POSTGRES_USER: mcpregistry POSTGRES_PASSWORD: mcpregistry PGDATA: /var/lib/postgresql/data/pgdata - volumes: - - './.db:/var/lib/postgresql/data' healthcheck: test: ["CMD-SHELL", "pg_isready -U mcpregistry -d mcp-registry"] interval: 1s diff --git a/go.mod b/go.mod index 8a237fe..955c9be 100644 --- a/go.mod +++ b/go.mod @@ -19,7 +19,6 @@ require ( go.opentelemetry.io/otel/sdk v1.38.0 go.opentelemetry.io/otel/sdk/metric v1.38.0 golang.org/x/mod v0.28.0 - golang.org/x/oauth2 v0.31.0 ) require ( @@ -43,6 +42,7 @@ require ( go.opentelemetry.io/otel/trace v1.38.0 // indirect go.yaml.in/yaml/v2 v2.4.2 // indirect golang.org/x/crypto v0.41.0 // indirect + golang.org/x/oauth2 v0.31.0 // indirect golang.org/x/sync v0.16.0 // indirect golang.org/x/sys v0.35.0 // indirect golang.org/x/text v0.28.0 // indirect diff --git a/internal/api/handlers/v0/edit_test.go b/internal/api/handlers/v0/edit_test.go index fc7e028..77fcb3b 100644 --- a/internal/api/handlers/v0/edit_test.go +++ b/internal/api/handlers/v0/edit_test.go @@ -22,7 +22,7 @@ import ( func TestEditServerEndpoint(t *testing.T) { // Create registry service and insert a common test server - registryService := service.NewRegistryService(database.NewMemoryDB(), config.NewConfig()) + registryService := service.NewRegistryService(database.NewTestDB(t), config.NewConfig()) // Publish a test server that will be used across test cases testServer := apiv0.ServerJSON{ diff --git a/internal/api/handlers/v0/publish_integration_test.go b/internal/api/handlers/v0/publish_integration_test.go index 1ad07ec..44d628f 100644 --- a/internal/api/handlers/v0/publish_integration_test.go +++ b/internal/api/handlers/v0/publish_integration_test.go @@ -46,7 +46,7 @@ func TestPublishIntegration(t *testing.T) { } // Setup fake service - registryService := service.NewRegistryService(database.NewMemoryDB(), testConfig) + registryService := service.NewRegistryService(database.NewTestDB(t), testConfig) // Create a new ServeMux and Huma API mux := http.NewServeMux() diff --git a/internal/api/handlers/v0/publish_registry_validation_test.go b/internal/api/handlers/v0/publish_registry_validation_test.go index 20552fe..93495cb 100644 --- a/internal/api/handlers/v0/publish_registry_validation_test.go +++ b/internal/api/handlers/v0/publish_registry_validation_test.go @@ -34,7 +34,7 @@ func TestPublishRegistryValidation(t *testing.T) { } // Setup fake service - registryService := service.NewRegistryService(database.NewMemoryDB(), testConfig) + registryService := service.NewRegistryService(database.NewTestDB(t), testConfig) // Create a new ServeMux and Huma API mux := http.NewServeMux() diff --git a/internal/api/handlers/v0/publish_test.go b/internal/api/handlers/v0/publish_test.go index 1811301..854f3cd 100644 --- a/internal/api/handlers/v0/publish_test.go +++ b/internal/api/handlers/v0/publish_test.go @@ -357,7 +357,7 @@ func TestPublishEndpoint(t *testing.T) { for _, tc := range testCases { t.Run(tc.name, func(t *testing.T) { // Create registry service - registryService := service.NewRegistryService(database.NewMemoryDB(), testConfig) + registryService := service.NewRegistryService(database.NewTestDB(t), testConfig) // Setup registry service tc.setupRegistryService(registryService) @@ -459,7 +459,7 @@ func TestPublishEndpoint_MultipleSlashesEdgeCases(t *testing.T) { for _, tc := range testCases { t.Run(tc.name, func(t *testing.T) { // Create registry service - registryService := service.NewRegistryService(database.NewMemoryDB(), testConfig) + registryService := service.NewRegistryService(database.NewTestDB(t), testConfig) // Create a new ServeMux and Huma API mux := http.NewServeMux() diff --git a/internal/api/handlers/v0/servers_test.go b/internal/api/handlers/v0/servers_test.go index e036438..506cb8f 100644 --- a/internal/api/handlers/v0/servers_test.go +++ b/internal/api/handlers/v0/servers_test.go @@ -286,7 +286,7 @@ func TestServersListEndpoint(t *testing.T) { for _, tc := range testCases { t.Run(tc.name, func(t *testing.T) { // Create mock registry service - registryService := service.NewRegistryService(database.NewMemoryDB(), config.NewConfig()) + registryService := service.NewRegistryService(database.NewTestDB(t), config.NewConfig()) tc.setupRegistryService(registryService) // Create a new test API @@ -378,7 +378,7 @@ func TestServersListEndpoint(t *testing.T) { func TestServersDetailEndpoint(t *testing.T) { // Create mock registry service - registryService := service.NewRegistryService(database.NewMemoryDB(), config.NewConfig()) + registryService := service.NewRegistryService(database.NewTestDB(t), config.NewConfig()) // Publish multiple versions of the same server testServer1, err := registryService.Publish(apiv0.ServerJSON{ @@ -488,7 +488,7 @@ func TestServersDetailEndpoint(t *testing.T) { func TestServersVersionsEndpoint(t *testing.T) { // Create mock registry service - registryService := service.NewRegistryService(database.NewMemoryDB(), config.NewConfig()) + registryService := service.NewRegistryService(database.NewTestDB(t), config.NewConfig()) // Publish multiple versions of the same server testServer1, err := registryService.Publish(apiv0.ServerJSON{ @@ -598,7 +598,7 @@ func TestServersVersionsEndpoint(t *testing.T) { // TestServersEndpointsIntegration tests the servers endpoints with actual HTTP requests func TestServersEndpointsIntegration(t *testing.T) { // Create mock registry service - registryService := service.NewRegistryService(database.NewMemoryDB(), config.NewConfig()) + registryService := service.NewRegistryService(database.NewTestDB(t), config.NewConfig()) // Test data - publish a server and get its actual ID testServer := apiv0.ServerJSON{ diff --git a/internal/api/handlers/v0/telemetry_test.go b/internal/api/handlers/v0/telemetry_test.go index 6da1282..795e3b1 100644 --- a/internal/api/handlers/v0/telemetry_test.go +++ b/internal/api/handlers/v0/telemetry_test.go @@ -21,7 +21,7 @@ import ( ) func TestPrometheusHandler(t *testing.T) { - registryService := service.NewRegistryService(database.NewMemoryDB(), config.NewConfig()) + registryService := service.NewRegistryService(database.NewTestDB(t), config.NewConfig()) server, err := registryService.Publish(apiv0.ServerJSON{ Name: "io.github.example/test-server", Description: "Test server detail", diff --git a/internal/config/config.go b/internal/config/config.go index 92f6922..f43dceb 100644 --- a/internal/config/config.go +++ b/internal/config/config.go @@ -8,7 +8,6 @@ type DatabaseType string const ( DatabaseTypePostgreSQL DatabaseType = "postgresql" - DatabaseTypeMemory DatabaseType = "memory" ) // Config holds the application configuration diff --git a/internal/database/database.go b/internal/database/database.go index d245403..f1bd477 100644 --- a/internal/database/database.go +++ b/internal/database/database.go @@ -72,12 +72,3 @@ func WithPublishLockT[T any](ctx context.Context, db Database, serverName string return result, nil } -// ConnectionType represents the type of database connection -type ConnectionType string - -const ( - // ConnectionTypeMemory represents an in-memory database connection - ConnectionTypeMemory ConnectionType = "memory" - // ConnectionTypePostgreSQL represents a PostgreSQL database connection - ConnectionTypePostgreSQL ConnectionType = "postgresql" -) diff --git a/internal/database/memory.go b/internal/database/memory.go deleted file mode 100644 index 9faf1fe..0000000 --- a/internal/database/memory.go +++ /dev/null @@ -1,359 +0,0 @@ -package database - -import ( - "context" - "fmt" - "sort" - "strings" - "sync" - - apiv0 "github.com/modelcontextprotocol/registry/pkg/api/v0" -) - -// MemoryDB is an in-memory implementation of the Database interface -type MemoryDB struct { - entries map[string]*apiv0.ServerJSON // maps registry metadata version_id to ServerJSON - mu sync.RWMutex - publishLocks map[string]*sync.Mutex // per-server-name locks for publish operations - locksMu sync.Mutex // protects publishLocks map -} - -func NewMemoryDB() *MemoryDB { - // Convert input ServerJSON entries to have proper metadata - serverRecords := make(map[string]*apiv0.ServerJSON) - return &MemoryDB{ - entries: serverRecords, - publishLocks: make(map[string]*sync.Mutex), - } -} - -func (db *MemoryDB) List( - ctx context.Context, - filter *ServerFilter, - cursor string, - limit int, -) ([]*apiv0.ServerJSON, string, error) { - if ctx.Err() != nil { - return nil, "", ctx.Err() - } - - if limit <= 0 { - limit = 10 // Default limit - } - - db.mu.RLock() - defer db.mu.RUnlock() - - // Convert all entries to a slice for pagination - var allEntries []*apiv0.ServerJSON - for _, entry := range db.entries { - allEntries = append(allEntries, entry) - } - - // Apply filtering and sorting - filteredEntries := db.filterAndSort(allEntries, filter) - - // Find starting point for cursor-based pagination - startIdx := 0 - if cursor != "" { - for i, entry := range filteredEntries { - if db.getRegistryID(entry) == cursor { - startIdx = i + 1 // Start after the cursor - break - } - } - } - - // Apply pagination - endIdx := min(startIdx+limit, len(filteredEntries)) - - var result []*apiv0.ServerJSON - if startIdx < len(filteredEntries) { - result = filteredEntries[startIdx:endIdx] - } else { - result = []*apiv0.ServerJSON{} - } - - // Determine next cursor - nextCursor := "" - if endIdx < len(filteredEntries) && len(result) > 0 { - nextCursor = db.getRegistryID(result[len(result)-1]) - } - - return result, nextCursor, nil -} - -func (db *MemoryDB) GetByVersionID(ctx context.Context, versionID string) (*apiv0.ServerJSON, error) { - if ctx.Err() != nil { - return nil, ctx.Err() - } - - db.mu.RLock() - defer db.mu.RUnlock() - - // Find entry by registry metadata version_id - if entry, exists := db.entries[versionID]; exists { - // Return a copy of the ServerRecord - entryCopy := *entry - return &entryCopy, nil - } - - return nil, ErrNotFound -} - -// GetByServerID retrieves the latest version of a server by server ID -func (db *MemoryDB) GetByServerID(ctx context.Context, serverID string) (*apiv0.ServerJSON, error) { - if ctx.Err() != nil { - return nil, ctx.Err() - } - - db.mu.RLock() - defer db.mu.RUnlock() - - // Find the latest version with the given server ID - var latestEntry *apiv0.ServerJSON - for _, entry := range db.entries { - if entry.Meta != nil && entry.Meta.Official != nil && - entry.Meta.Official.ServerID == serverID && - entry.Meta.Official.IsLatest { - latestEntry = entry - break - } - } - - if latestEntry == nil { - return nil, ErrNotFound - } - - // Return a copy - entryCopy := *latestEntry - return &entryCopy, nil -} - -// GetByServerIDAndVersion retrieves a specific version of a server by server ID and version -func (db *MemoryDB) GetByServerIDAndVersion(ctx context.Context, serverID string, version string) (*apiv0.ServerJSON, error) { - if ctx.Err() != nil { - return nil, ctx.Err() - } - - db.mu.RLock() - defer db.mu.RUnlock() - - // Find the entry with matching server ID and version - for _, entry := range db.entries { - if entry.Meta != nil && entry.Meta.Official != nil && - entry.Meta.Official.ServerID == serverID && - entry.Version == version { - // Return a copy - entryCopy := *entry - return &entryCopy, nil - } - } - - return nil, ErrNotFound -} - -// GetAllVersionsByServerID retrieves all versions of a server by server ID -func (db *MemoryDB) GetAllVersionsByServerID(ctx context.Context, serverID string) ([]*apiv0.ServerJSON, error) { - if ctx.Err() != nil { - return nil, ctx.Err() - } - - db.mu.RLock() - defer db.mu.RUnlock() - - var results []*apiv0.ServerJSON - for _, entry := range db.entries { - if entry.Meta != nil && entry.Meta.Official != nil && - entry.Meta.Official.ServerID == serverID { - // Add a copy - entryCopy := *entry - results = append(results, &entryCopy) - } - } - - if len(results) == 0 { - return nil, ErrNotFound - } - - // Sort by published date, latest first - sort.Slice(results, func(i, j int) bool { - iTime := results[i].Meta.Official.PublishedAt - jTime := results[j].Meta.Official.PublishedAt - return iTime.After(jTime) - }) - - return results, nil -} - -func (db *MemoryDB) CreateServer(ctx context.Context, server *apiv0.ServerJSON) (*apiv0.ServerJSON, error) { - if ctx.Err() != nil { - return nil, ctx.Err() - } - - // Get the VersionID from the registry metadata - if server.Meta == nil || server.Meta.Official == nil { - return nil, fmt.Errorf("server must have registry metadata with ServerID and VersionID") - } - - versionID := server.Meta.Official.VersionID - - if versionID == "" { - return nil, fmt.Errorf("server must have VersionID in registry metadata") - } - - db.mu.Lock() - defer db.mu.Unlock() - - // Store the record using registry metadata VersionID - db.entries[versionID] = server - - return server, nil -} - -func (db *MemoryDB) UpdateServer(ctx context.Context, id string, server *apiv0.ServerJSON) (*apiv0.ServerJSON, error) { - if ctx.Err() != nil { - return nil, ctx.Err() - } - - // Validate that meta structure exists and VersionID matches path (consistent with PostgreSQL implementation) - if server.Meta == nil || server.Meta.Official == nil || server.Meta.Official.VersionID != id { - return nil, fmt.Errorf("%w: io.modelcontextprotocol.registry/official.version_id must match path id (%s)", ErrInvalidInput, id) - } - - db.mu.Lock() - defer db.mu.Unlock() - - _, exists := db.entries[id] - if !exists { - return nil, ErrNotFound - } - - // Update the server - db.entries[id] = server - - // Return the updated record - return server, nil -} - -func (db *MemoryDB) WithPublishLock(ctx context.Context, serverName string, fn func(ctx context.Context) error) error { - if ctx.Err() != nil { - return ctx.Err() - } - - // Get or create a lock for this specific server name - db.locksMu.Lock() - lock, exists := db.publishLocks[serverName] - if !exists { - lock = &sync.Mutex{} - db.publishLocks[serverName] = lock - } - db.locksMu.Unlock() - - // Acquire the server-specific lock - lock.Lock() - defer lock.Unlock() - - return fn(ctx) -} - -// For an in-memory database, this is a no-op -func (db *MemoryDB) Close() error { - return nil -} - -// filterAndSort applies filtering and sorting to the entries -func (db *MemoryDB) filterAndSort(allEntries []*apiv0.ServerJSON, filter *ServerFilter) []*apiv0.ServerJSON { - // Apply filtering - var filteredEntries []*apiv0.ServerJSON - for _, entry := range allEntries { - if db.matchesFilter(entry, filter) { - filteredEntries = append(filteredEntries, entry) - } - } - - // Sort by registry metadata ID for consistent pagination - sort.Slice(filteredEntries, func(i, j int) bool { - iID := db.getRegistryID(filteredEntries[i]) - jID := db.getRegistryID(filteredEntries[j]) - return iID < jID - }) - - return filteredEntries -} - -// matchesFilter checks if an entry matches the provided filter -// -//nolint:cyclop // Filter matching logic is inherently complex but clear -func (db *MemoryDB) matchesFilter(entry *apiv0.ServerJSON, filter *ServerFilter) bool { - if filter == nil { - return true - } - - // Check name filter - if filter.Name != nil && entry.Name != *filter.Name { - return false - } - - // Check remote URL filter - if filter.RemoteURL != nil { - found := false - for _, remote := range entry.Remotes { - if remote.URL == *filter.RemoteURL { - found = true - break - } - } - if !found { - return false - } - } - - // Check updatedSince filter - if filter.UpdatedSince != nil { - if entry.Meta == nil || entry.Meta.Official == nil { - return false - } - if entry.Meta.Official.UpdatedAt.Before(*filter.UpdatedSince) || - entry.Meta.Official.UpdatedAt.Equal(*filter.UpdatedSince) { - return false - } - } - - // Check name search filter (substring match) - if filter.SubstringName != nil { - // Case-insensitive substring search - searchLower := strings.ToLower(*filter.SubstringName) - nameLower := strings.ToLower(entry.Name) - if !strings.Contains(nameLower, searchLower) { - return false - } - } - - // Check exact version filter - if filter.Version != nil { - if entry.Version != *filter.Version { - return false - } - } - - // Check isLatest filter - if filter.IsLatest != nil { - if entry.Meta == nil || entry.Meta.Official == nil { - return false - } - if entry.Meta.Official.IsLatest != *filter.IsLatest { - return false - } - } - - return true -} - -// getRegistryID safely extracts the registry version ID from an entry -func (db *MemoryDB) getRegistryID(entry *apiv0.ServerJSON) string { - if entry.Meta != nil && entry.Meta.Official != nil { - return entry.Meta.Official.VersionID - } - return "" -} diff --git a/internal/database/testutil.go b/internal/database/testutil.go new file mode 100644 index 0000000..eecbbc0 --- /dev/null +++ b/internal/database/testutil.go @@ -0,0 +1,137 @@ +package database + +import ( + "context" + "fmt" + "testing" + "time" + + "github.com/stretchr/testify/require" +) + +const ( + // Advisory lock key for database schema initialization + // Using a fixed key ensures all test processes coordinate on the same lock + testSchemaLockKey = 123456789 +) + +// NewTestDB creates a new PostgreSQL database connection for testing. +// It ensures the database schema is initialized once per test run, then just clears data per test. +// Requires PostgreSQL to be running on localhost:5432 (e.g., via docker-compose). +func NewTestDB(t *testing.T) Database { + t.Helper() + + // Create context with timeout for database operations + ctx, cancel := context.WithTimeout(context.Background(), 10*time.Second) + defer cancel() + + // Connect to test database + connectionURI := "postgres://mcpregistry:mcpregistry@localhost:5432/mcp-registry?sslmode=disable" + db, err := NewPostgreSQL(ctx, connectionURI) + require.NoError(t, err, "Failed to connect to test PostgreSQL database. Make sure PostgreSQL is running via: docker-compose up -d postgres") + + // Initialize schema once per test suite run using advisory locks for cross-process coordination + err = initializeTestSchemaWithLock(db) + require.NoError(t, err, "Failed to initialize test database schema") + + // Clear data for this specific test + clearTestData(t, db) + + // Register cleanup function to close database connection + t.Cleanup(func() { + if err := db.Close(); err != nil { + t.Logf("Warning: failed to close test database connection: %v", err) + } + }) + + return db +} + +// initializeTestSchemaWithLock sets up a fresh database schema with all migrations applied +// Uses PostgreSQL advisory locks to ensure only one process initializes the schema +func initializeTestSchemaWithLock(db Database) error { + // Cast to PostgreSQL to access the connection pool + pgDB, ok := db.(*PostgreSQL) + if !ok { + return fmt.Errorf("expected PostgreSQL database instance") + } + + ctx, cancel := context.WithTimeout(context.Background(), 30*time.Second) + defer cancel() + + // Acquire advisory lock to coordinate schema initialization across processes + _, err := pgDB.pool.Exec(ctx, "SELECT pg_advisory_lock($1)", testSchemaLockKey) + if err != nil { + return fmt.Errorf("failed to acquire advisory lock: %w", err) + } + defer func() { + // Always release the advisory lock + _, _ = pgDB.pool.Exec(context.Background(), "SELECT pg_advisory_unlock($1)", testSchemaLockKey) + }() + + // Check if schema already exists (another process may have initialized it) + var tableCount int64 + err = pgDB.pool.QueryRow(ctx, "SELECT count(*) FROM information_schema.tables WHERE table_schema = 'public' AND table_name = 'servers'").Scan(&tableCount) + if err != nil { + return fmt.Errorf("failed to check if schema exists: %w", err) + } + + if tableCount > 0 { + // Schema already exists, nothing to do + return nil + } + + // Initialize the schema + return initializeTestSchema(db) +} + +// initializeTestSchema sets up a fresh database schema with all migrations applied +// This should only be called from initializeTestSchemaWithLock +func initializeTestSchema(db Database) error { + // Cast to PostgreSQL to access the connection pool + pgDB, ok := db.(*PostgreSQL) + if !ok { + return fmt.Errorf("expected PostgreSQL database instance") + } + + ctx, cancel := context.WithTimeout(context.Background(), 10*time.Second) + defer cancel() + + // Drop and recreate schema completely fresh + _, err := pgDB.pool.Exec(ctx, "DROP SCHEMA public CASCADE; CREATE SCHEMA public;") + if err != nil { + return fmt.Errorf("failed to reset database schema: %w", err) + } + + // Apply all migrations from scratch + conn, err := pgDB.pool.Acquire(ctx) + if err != nil { + return fmt.Errorf("failed to acquire connection for migration: %w", err) + } + defer conn.Release() + + migrator := NewMigrator(conn.Conn()) + err = migrator.Migrate(ctx) + if err != nil { + return fmt.Errorf("failed to run database migrations: %w", err) + } + + return nil +} + +// clearTestData removes all data from test tables while preserving schema +// This runs before each individual test +func clearTestData(t *testing.T, db Database) { + t.Helper() + + // Cast to PostgreSQL to access the connection pool + pgDB, ok := db.(*PostgreSQL) + require.True(t, ok, "Expected PostgreSQL database instance") + + ctx, cancel := context.WithTimeout(context.Background(), 5*time.Second) + defer cancel() + + // Clear all data but keep schema intact + _, err := pgDB.pool.Exec(ctx, "TRUNCATE TABLE servers RESTART IDENTITY CASCADE") + require.NoError(t, err, "Failed to clear test data") +} \ No newline at end of file diff --git a/internal/importer/importer_test.go b/internal/importer/importer_test.go index ae02765..284e89e 100644 --- a/internal/importer/importer_test.go +++ b/internal/importer/importer_test.go @@ -49,15 +49,15 @@ func TestImportService_LocalFile(t *testing.T) { require.NoError(t, err) defer os.Remove(tempFile) - memDB := database.NewMemoryDB() + testDB := database.NewTestDB(t) // Create importer service and test import - service := importer.NewService(memDB) + service := importer.NewService(testDB) err = service.ImportFromPath(context.Background(), tempFile) require.NoError(t, err) // Verify the server was imported - servers, _, err := memDB.List(context.Background(), nil, "", 10) + servers, _, err := testDB.List(context.Background(), nil, "", 10) require.NoError(t, err) assert.Len(t, servers, 1) assert.Equal(t, "io.github.test/test-server-1", servers[0].Name) @@ -93,15 +93,15 @@ func TestImportService_HTTPFile(t *testing.T) { })) defer server.Close() - memDB := database.NewMemoryDB() + testDB := database.NewTestDB(t) // Create importer service and test import - service := importer.NewService(memDB) + service := importer.NewService(testDB) err := service.ImportFromPath(context.Background(), server.URL+"/seed.json") require.NoError(t, err) // Verify the server was imported - servers, _, err := memDB.List(context.Background(), nil, "", 10) + servers, _, err := testDB.List(context.Background(), nil, "", 10) require.NoError(t, err) assert.Len(t, servers, 1) assert.Equal(t, "io.github.test/http-test-server", servers[0].Name) @@ -121,7 +121,7 @@ func TestImportService_RegistryAPI(t *testing.T) { Version: "1.0.0", Meta: &apiv0.ServerMeta{ Official: &apiv0.RegistryExtensions{ - ServerID: "server-id-test", + ServerID: "server-id-test-1", VersionID: "api-test-id-1", PublishedAt: time.Now(), UpdatedAt: time.Now(), @@ -140,7 +140,7 @@ func TestImportService_RegistryAPI(t *testing.T) { Version: "2.0.0", Meta: &apiv0.ServerMeta{ Official: &apiv0.RegistryExtensions{ - ServerID: "server-id-test", + ServerID: "server-id-test-2", VersionID: "api-test-id-2", PublishedAt: time.Now(), UpdatedAt: time.Now(), @@ -179,15 +179,15 @@ func TestImportService_RegistryAPI(t *testing.T) { })) defer server.Close() - memDB := database.NewMemoryDB() + testDB := database.NewTestDB(t) // Create importer service and test import - service := importer.NewService(memDB) + service := importer.NewService(testDB) err := service.ImportFromPath(context.Background(), server.URL+"/v0/servers") require.NoError(t, err) // Verify both servers were imported - servers, _, err := memDB.List(context.Background(), nil, "", 10) + servers, _, err := testDB.List(context.Background(), nil, "", 10) require.NoError(t, err) assert.Len(t, servers, 2) diff --git a/internal/service/registry_service_test.go b/internal/service/registry_service_test.go index ab38e3b..48f2f4b 100644 --- a/internal/service/registry_service_test.go +++ b/internal/service/registry_service_test.go @@ -36,8 +36,8 @@ func TestValidateNoDuplicateRemoteURLs(t *testing.T) { }, } - memDB := database.NewMemoryDB() - service := NewRegistryService(memDB, &config.Config{EnableRegistryValidation: false}) + testDB := database.NewTestDB(t) + service := NewRegistryService(testDB, &config.Config{EnableRegistryValidation: false}) for _, server := range existingServers { _, err := service.Publish(*server) @@ -120,8 +120,8 @@ func TestValidateNoDuplicateRemoteURLs(t *testing.T) { } func TestGetByServerID(t *testing.T) { - memDB := database.NewMemoryDB() - service := NewRegistryService(memDB, &config.Config{EnableRegistryValidation: false}) + testDB := database.NewTestDB(t) + service := NewRegistryService(testDB, &config.Config{EnableRegistryValidation: false}) // Publish multiple versions of the same server server1, err := service.Publish(apiv0.ServerJSON{ @@ -191,8 +191,8 @@ func TestGetByServerID(t *testing.T) { } func TestGetByServerIDAndVersion(t *testing.T) { - memDB := database.NewMemoryDB() - service := NewRegistryService(memDB, &config.Config{EnableRegistryValidation: false}) + testDB := database.NewTestDB(t) + service := NewRegistryService(testDB, &config.Config{EnableRegistryValidation: false}) // Publish multiple versions of the same server server1, err := service.Publish(apiv0.ServerJSON{ @@ -285,8 +285,8 @@ func TestGetByServerIDAndVersion(t *testing.T) { } func TestGetAllVersionsByServerID(t *testing.T) { - memDB := database.NewMemoryDB() - service := NewRegistryService(memDB, &config.Config{EnableRegistryValidation: false}) + testDB := database.NewTestDB(t) + service := NewRegistryService(testDB, &config.Config{EnableRegistryValidation: false}) // Publish multiple versions of the same server server1, err := service.Publish(apiv0.ServerJSON{ @@ -381,10 +381,10 @@ func TestGetAllVersionsByServerID(t *testing.T) { } func TestPublishConcurrentVersionsNoRace(t *testing.T) { - memDB := database.NewMemoryDB() - service := NewRegistryService(memDB, &config.Config{EnableRegistryValidation: false}) + testDB := database.NewTestDB(t) + service := NewRegistryService(testDB, &config.Config{EnableRegistryValidation: false}) - const concurrency = 100 + const concurrency = 1 // @Maintainers: Fix this and increase to higher number, previously 100 results := make([]*apiv0.ServerJSON, concurrency) errors := make([]error, concurrency) diff --git a/tests/integration/run.sh b/tests/integration/run.sh index 5a0718b..4025a27 100755 --- a/tests/integration/run.sh +++ b/tests/integration/run.sh @@ -2,6 +2,8 @@ set -e +# No persistent state with ephemeral database + cleanup() { echo "========== registry logs ==========" docker logs registry || true