Clean up publish and edit handler and service logic (#319)
Thanks Camila for being a great Go consultant and advising on how to
tidy this up!
(I'm still a lil grumpy about some parts, but it's in much better shape
😄)
- Consolidated publisher extension validation into a single function.
- Updated the `Publish` and `EditServer` methods to directly use
`XPublisher` from the request.
- Removed outdated tests for publisher extension extraction and
validation.
- Introduced new validation functions for server details and remote URL
matching.
- Enhanced error handling for server name format and remote URL
validation.
- Updated existing tests to reflect changes in validation logic and
structure.
## Motivation and Context
<!-- Why is this change needed? What problem does it solve? -->
Things were starting to become a bit spaghetti-y after lots of small PRs
and contributions from many people. Camila and I went through and
figured out how we could make this a lot cleaner.
## How Has This Been Tested?
<!-- Have you tested this in a real application? Which scenarios were
tested? -->
Tested locally, plus all the integration and unit tests still pass.
## Breaking Changes
<!-- Will users need to update their code or configurations? -->
None
---------
Co-authored-by: Claude <noreply@anthropic.com>
This commit is contained in:
adam jones
GitHub
parent
c0a94cca8c
commit
654cad4aa7
1 changed files with 232 additions and 570 deletions
|
|
@ -1,20 +1,14 @@
|
|||
package validators_test
|
||||
|
||||
import (
|
||||
"encoding/json"
|
||||
"testing"
|
||||
"time"
|
||||
|
||||
"github.com/modelcontextprotocol/registry/internal/validators"
|
||||
apiv0 "github.com/modelcontextprotocol/registry/pkg/api/v0"
|
||||
"github.com/modelcontextprotocol/registry/pkg/model"
|
||||
"github.com/stretchr/testify/assert"
|
||||
"github.com/stretchr/testify/require"
|
||||
)
|
||||
|
||||
func TestObjectValidator_Validate(t *testing.T) {
|
||||
validator := validators.NewObjectValidator()
|
||||
|
||||
func TestValidate(t *testing.T) {
|
||||
tests := []struct {
|
||||
name string
|
||||
serverDetail model.ServerJSON
|
||||
|
|
@ -23,7 +17,7 @@ func TestObjectValidator_Validate(t *testing.T) {
|
|||
{
|
||||
name: "valid server detail with all fields",
|
||||
serverDetail: model.ServerJSON{
|
||||
Name: "test-server",
|
||||
Name: "com.example/test-server",
|
||||
Description: "A test server",
|
||||
Repository: model.Repository{
|
||||
URL: "https://github.com/owner/repo",
|
||||
|
|
@ -51,7 +45,7 @@ func TestObjectValidator_Validate(t *testing.T) {
|
|||
{
|
||||
name: "server with invalid repository source",
|
||||
serverDetail: model.ServerJSON{
|
||||
Name: "test-server",
|
||||
Name: "com.example/test-server",
|
||||
Description: "A test server",
|
||||
Repository: model.Repository{
|
||||
URL: "https://bitbucket.org/owner/repo",
|
||||
|
|
@ -66,7 +60,7 @@ func TestObjectValidator_Validate(t *testing.T) {
|
|||
{
|
||||
name: "server with invalid GitHub URL format",
|
||||
serverDetail: model.ServerJSON{
|
||||
Name: "test-server",
|
||||
Name: "com.example/test-server",
|
||||
Description: "A test server",
|
||||
Repository: model.Repository{
|
||||
URL: "https://github.com/owner", // Missing repo name
|
||||
|
|
@ -81,7 +75,7 @@ func TestObjectValidator_Validate(t *testing.T) {
|
|||
{
|
||||
name: "server with invalid GitLab URL format",
|
||||
serverDetail: model.ServerJSON{
|
||||
Name: "test-server",
|
||||
Name: "com.example/test-server",
|
||||
Description: "A test server",
|
||||
Repository: model.Repository{
|
||||
URL: "https://gitlab.com", // Missing owner and repo
|
||||
|
|
@ -96,7 +90,7 @@ func TestObjectValidator_Validate(t *testing.T) {
|
|||
{
|
||||
name: "package with spaces in name",
|
||||
serverDetail: model.ServerJSON{
|
||||
Name: "test-server",
|
||||
Name: "com.example/test-server",
|
||||
Description: "A test server",
|
||||
Repository: model.Repository{
|
||||
URL: "https://github.com/owner/repo",
|
||||
|
|
@ -118,7 +112,7 @@ func TestObjectValidator_Validate(t *testing.T) {
|
|||
{
|
||||
name: "multiple packages with one invalid",
|
||||
serverDetail: model.ServerJSON{
|
||||
Name: "test-server",
|
||||
Name: "com.example/test-server",
|
||||
Description: "A test server",
|
||||
Repository: model.Repository{
|
||||
URL: "https://github.com/owner/repo",
|
||||
|
|
@ -145,7 +139,7 @@ func TestObjectValidator_Validate(t *testing.T) {
|
|||
{
|
||||
name: "remote with invalid URL",
|
||||
serverDetail: model.ServerJSON{
|
||||
Name: "test-server",
|
||||
Name: "com.example/test-server",
|
||||
Description: "A test server",
|
||||
Repository: model.Repository{
|
||||
URL: "https://github.com/owner/repo",
|
||||
|
|
@ -165,7 +159,7 @@ func TestObjectValidator_Validate(t *testing.T) {
|
|||
{
|
||||
name: "remote with missing scheme",
|
||||
serverDetail: model.ServerJSON{
|
||||
Name: "test-server",
|
||||
Name: "com.example/test-server",
|
||||
Description: "A test server",
|
||||
Repository: model.Repository{
|
||||
URL: "https://github.com/owner/repo",
|
||||
|
|
@ -185,7 +179,7 @@ func TestObjectValidator_Validate(t *testing.T) {
|
|||
{
|
||||
name: "multiple remotes with one invalid",
|
||||
serverDetail: model.ServerJSON{
|
||||
Name: "test-server",
|
||||
Name: "com.example/test-server",
|
||||
Description: "A test server",
|
||||
Repository: model.Repository{
|
||||
URL: "https://github.com/owner/repo",
|
||||
|
|
@ -208,7 +202,7 @@ func TestObjectValidator_Validate(t *testing.T) {
|
|||
{
|
||||
name: "server detail with nil packages and remotes",
|
||||
serverDetail: model.ServerJSON{
|
||||
Name: "test-server",
|
||||
Name: "com.example/test-server",
|
||||
Description: "A test server",
|
||||
Repository: model.Repository{
|
||||
URL: "https://github.com/owner/repo",
|
||||
|
|
@ -225,7 +219,7 @@ func TestObjectValidator_Validate(t *testing.T) {
|
|||
{
|
||||
name: "server detail with empty packages and remotes slices",
|
||||
serverDetail: model.ServerJSON{
|
||||
Name: "test-server",
|
||||
Name: "com.example/test-server",
|
||||
Description: "A test server",
|
||||
Repository: model.Repository{
|
||||
URL: "https://github.com/owner/repo",
|
||||
|
|
@ -243,7 +237,7 @@ func TestObjectValidator_Validate(t *testing.T) {
|
|||
|
||||
for _, tt := range tests {
|
||||
t.Run(tt.name, func(t *testing.T) {
|
||||
err := validator.Validate(&tt.serverDetail)
|
||||
err := validators.ValidateServerJSON(&tt.serverDetail)
|
||||
|
||||
if tt.expectedError == "" {
|
||||
assert.NoError(t, err)
|
||||
|
|
@ -255,234 +249,7 @@ func TestObjectValidator_Validate(t *testing.T) {
|
|||
}
|
||||
}
|
||||
|
||||
func TestExtractPublisherExtensions(t *testing.T) {
|
||||
tests := []struct {
|
||||
name string
|
||||
request apiv0.PublishRequest
|
||||
expected map[string]interface{}
|
||||
}{
|
||||
{
|
||||
name: "nil publisher extensions",
|
||||
request: apiv0.PublishRequest{
|
||||
Server: model.ServerJSON{Name: "test"},
|
||||
},
|
||||
expected: map[string]interface{}{},
|
||||
},
|
||||
{
|
||||
name: "empty publisher extensions",
|
||||
request: apiv0.PublishRequest{
|
||||
Server: model.ServerJSON{Name: "test"},
|
||||
XPublisher: map[string]interface{}{},
|
||||
},
|
||||
expected: map[string]interface{}{},
|
||||
},
|
||||
{
|
||||
name: "simple publisher extensions",
|
||||
request: apiv0.PublishRequest{
|
||||
Server: model.ServerJSON{Name: "test"},
|
||||
XPublisher: map[string]interface{}{
|
||||
"build_info": map[string]interface{}{
|
||||
"version": "1.2.3",
|
||||
"commit": "abc123",
|
||||
},
|
||||
"publisher_metadata": "test-publisher",
|
||||
},
|
||||
},
|
||||
expected: map[string]interface{}{
|
||||
"build_info": map[string]interface{}{
|
||||
"version": "1.2.3",
|
||||
"commit": "abc123",
|
||||
},
|
||||
"publisher_metadata": "test-publisher",
|
||||
},
|
||||
},
|
||||
{
|
||||
name: "nested publisher extensions",
|
||||
request: apiv0.PublishRequest{
|
||||
Server: model.ServerJSON{Name: "test"},
|
||||
XPublisher: map[string]interface{}{
|
||||
"ci": map[string]interface{}{
|
||||
"pipeline": map[string]interface{}{
|
||||
"id": "12345",
|
||||
"branch": "main",
|
||||
},
|
||||
"artifacts": []string{"binary", "docs"},
|
||||
},
|
||||
},
|
||||
},
|
||||
expected: map[string]interface{}{
|
||||
"ci": map[string]interface{}{
|
||||
"pipeline": map[string]interface{}{
|
||||
"id": "12345",
|
||||
"branch": "main",
|
||||
},
|
||||
"artifacts": []string{"binary", "docs"},
|
||||
},
|
||||
},
|
||||
},
|
||||
{
|
||||
name: "nil publisher extensions (should be ignored)",
|
||||
request: apiv0.PublishRequest{
|
||||
Server: model.ServerJSON{Name: "test"},
|
||||
XPublisher: nil,
|
||||
},
|
||||
expected: map[string]interface{}{},
|
||||
},
|
||||
}
|
||||
|
||||
for _, tt := range tests {
|
||||
t.Run(tt.name, func(t *testing.T) {
|
||||
result := validators.ExtractPublisherExtensions(tt.request)
|
||||
assert.Equal(t, tt.expected, result)
|
||||
})
|
||||
}
|
||||
}
|
||||
|
||||
func TestExtractPublisherExtensions_DoesNotDoubleNest(t *testing.T) {
|
||||
// This test specifically verifies the bug we fixed - no double nesting
|
||||
request := apiv0.PublishRequest{
|
||||
Server: model.ServerJSON{Name: "test"},
|
||||
XPublisher: map[string]interface{}{
|
||||
"tool": "publisher-cli",
|
||||
"version": "1.0.0",
|
||||
},
|
||||
}
|
||||
|
||||
result := validators.ExtractPublisherExtensions(request)
|
||||
|
||||
// Verify we get the data directly, not wrapped in another "x-publisher" key
|
||||
assert.Equal(t, "publisher-cli", result["tool"])
|
||||
assert.Equal(t, "1.0.0", result["version"])
|
||||
|
||||
// Verify we don't have double nesting
|
||||
assert.NotContains(t, result, "x-publisher")
|
||||
}
|
||||
|
||||
func TestServerResponse_JSONSerialization(t *testing.T) {
|
||||
// Test that ServerResponse properly serializes to extension wrapper format
|
||||
publishedTime := time.Date(2023, 12, 1, 10, 30, 0, 0, time.UTC)
|
||||
|
||||
response := apiv0.ServerRecord{
|
||||
Server: model.ServerJSON{
|
||||
Name: "test-server",
|
||||
Description: "A test server",
|
||||
Repository: model.Repository{
|
||||
URL: "https://github.com/test/server",
|
||||
Source: "github",
|
||||
ID: "test/server",
|
||||
},
|
||||
VersionDetail: model.VersionDetail{
|
||||
Version: "1.0.0",
|
||||
},
|
||||
},
|
||||
XIOModelContextProtocolRegistry: apiv0.RegistryExtensions{
|
||||
ID: "registry-id-123",
|
||||
PublishedAt: publishedTime,
|
||||
IsLatest: true,
|
||||
ReleaseDate: publishedTime.Format(time.RFC3339),
|
||||
},
|
||||
XPublisher: map[string]interface{}{
|
||||
"build_tool": "ci-pipeline",
|
||||
"commit": "abc123def",
|
||||
},
|
||||
}
|
||||
|
||||
// Serialize to JSON
|
||||
jsonData, err := json.Marshal(response)
|
||||
require.NoError(t, err)
|
||||
|
||||
// Parse back to verify structure
|
||||
var parsed map[string]interface{}
|
||||
err = json.Unmarshal(jsonData, &parsed)
|
||||
require.NoError(t, err)
|
||||
|
||||
// Verify three-layer structure exists
|
||||
assert.Contains(t, parsed, "server")
|
||||
assert.Contains(t, parsed, "x-io.modelcontextprotocol.registry")
|
||||
assert.Contains(t, parsed, "x-publisher")
|
||||
|
||||
// Verify server data
|
||||
serverData := parsed["server"].(map[string]interface{})
|
||||
assert.Equal(t, "test-server", serverData["name"])
|
||||
|
||||
// Verify registry metadata
|
||||
registryData := parsed["x-io.modelcontextprotocol.registry"].(map[string]interface{})
|
||||
assert.Equal(t, "registry-id-123", registryData["id"])
|
||||
|
||||
// Verify publisher extensions
|
||||
publisherData := parsed["x-publisher"].(map[string]interface{})
|
||||
assert.Equal(t, "ci-pipeline", publisherData["build_tool"])
|
||||
assert.Equal(t, "abc123def", publisherData["commit"])
|
||||
}
|
||||
|
||||
func TestPublishRequest_WithPublisherExtensions(t *testing.T) {
|
||||
// Test that PublishRequest properly deserializes publisher extensions
|
||||
jsonData := `{
|
||||
"server": {
|
||||
"name": "test-server",
|
||||
"description": "Test server",
|
||||
"version_detail": {
|
||||
"version": "1.0.0"
|
||||
}
|
||||
},
|
||||
"x-publisher": {
|
||||
"tool": "publisher-cli",
|
||||
"metadata": {
|
||||
"build_date": "2023-12-01",
|
||||
"commit": "abc123"
|
||||
}
|
||||
}
|
||||
}`
|
||||
|
||||
var request apiv0.PublishRequest
|
||||
err := json.Unmarshal([]byte(jsonData), &request)
|
||||
require.NoError(t, err)
|
||||
|
||||
// Verify server data
|
||||
assert.Equal(t, "test-server", request.Server.Name)
|
||||
assert.Equal(t, "Test server", request.Server.Description)
|
||||
|
||||
// Verify publisher extensions
|
||||
require.NotNil(t, request.XPublisher)
|
||||
assert.Equal(t, "publisher-cli", request.XPublisher["tool"])
|
||||
|
||||
metadata := request.XPublisher["metadata"].(map[string]interface{})
|
||||
assert.Equal(t, "2023-12-01", metadata["build_date"])
|
||||
assert.Equal(t, "abc123", metadata["commit"])
|
||||
}
|
||||
|
||||
func TestServerResponse_EmptyExtensions(t *testing.T) {
|
||||
// Test behavior with empty/nil extensions
|
||||
response := apiv0.ServerRecord{
|
||||
Server: model.ServerJSON{
|
||||
Name: "minimal-server",
|
||||
Description: "Minimal test",
|
||||
},
|
||||
XIOModelContextProtocolRegistry: apiv0.RegistryExtensions{
|
||||
ID: "min-id",
|
||||
},
|
||||
XPublisher: nil,
|
||||
}
|
||||
|
||||
jsonData, err := json.Marshal(response)
|
||||
require.NoError(t, err)
|
||||
|
||||
var parsed map[string]interface{}
|
||||
err = json.Unmarshal(jsonData, &parsed)
|
||||
require.NoError(t, err)
|
||||
|
||||
// Should still have the structure, even with nil publisher extensions
|
||||
assert.Contains(t, parsed, "server")
|
||||
assert.Contains(t, parsed, "x-io.modelcontextprotocol.registry")
|
||||
|
||||
// x-publisher should be null/nil in JSON when empty
|
||||
publisherValue, exists := parsed["x-publisher"]
|
||||
if exists {
|
||||
assert.Nil(t, publisherValue)
|
||||
}
|
||||
}
|
||||
|
||||
func TestValidateRemoteNamespaceMatch(t *testing.T) {
|
||||
func TestValidate_RemoteNamespaceMatch(t *testing.T) {
|
||||
tests := []struct {
|
||||
name string
|
||||
serverDetail model.ServerJSON
|
||||
|
|
@ -560,7 +327,7 @@ func TestValidateRemoteNamespaceMatch(t *testing.T) {
|
|||
},
|
||||
},
|
||||
expectError: true,
|
||||
errorMsg: "URL must have a valid hostname",
|
||||
errorMsg: "invalid remote URL",
|
||||
},
|
||||
{
|
||||
name: "empty remotes array",
|
||||
|
|
@ -597,7 +364,7 @@ func TestValidateRemoteNamespaceMatch(t *testing.T) {
|
|||
|
||||
for _, tt := range tests {
|
||||
t.Run(tt.name, func(t *testing.T) {
|
||||
err := validators.ValidateRemoteNamespaceMatch(tt.serverDetail)
|
||||
err := validators.ValidateServerJSON(&tt.serverDetail)
|
||||
|
||||
if tt.expectError {
|
||||
assert.Error(t, err)
|
||||
|
|
@ -609,7 +376,7 @@ func TestValidateRemoteNamespaceMatch(t *testing.T) {
|
|||
}
|
||||
}
|
||||
|
||||
func TestParseServerName(t *testing.T) {
|
||||
func TestValidate_ServerNameFormat(t *testing.T) {
|
||||
tests := []struct {
|
||||
name string
|
||||
serverDetail model.ServerJSON
|
||||
|
|
@ -673,7 +440,7 @@ func TestParseServerName(t *testing.T) {
|
|||
|
||||
for _, tt := range tests {
|
||||
t.Run(tt.name, func(t *testing.T) {
|
||||
_, err := validators.ParseServerName(tt.serverDetail)
|
||||
err := validators.ValidateServerJSON(&tt.serverDetail)
|
||||
|
||||
if tt.expectError {
|
||||
assert.Error(t, err)
|
||||
|
|
|
|||
Loading…
Reference in a new issue