Skip to content

Feature implementation from commits d32b528..fa159e3#5

Open
codeOwlAI wants to merge 15 commits intofeature-base-branch-5from
feature-head-branch-5
Open

Feature implementation from commits d32b528..fa159e3#5
codeOwlAI wants to merge 15 commits intofeature-base-branch-5from
feature-head-branch-5

Conversation

@codeOwlAI
Copy link
Owner

@codeOwlAI codeOwlAI commented Jun 30, 2025

PR Summary

Data Model Refactoring: Entry to Server Type Conversion

Overview

This PR refactors the data model structure by changing the primary model type from Entry to Server across multiple components and restructures the ServerDetail object to use a nested Server sub-structure.

Change Types

Type Description
Refactor Changed data model from Entry to Server type
Breaking Modified return types of public API methods

Affected Modules

Module / File Change Description
database/memory.go Restructured ServerDetail to use nested Server sub-structure
database/mongo.go Changed MongoDB.List return type from []*model.Entry to []*model.Server
service/fake_service.go Modified result slice type from []model.Entry to []model.Server
service/registry_service.go Changed GetAll method return type from []model.Entry to []model.Server

Breaking Changes

  • Changed return type of MongoDB.List function from []*model.Entry to []*model.Server
  • Changed return type of Registry Service's GetAll method from []model.Entry to []model.Server
  • Restructured ServerDetail object with nested fields in a Server sub-structure

Notes for Reviewers

  • Verify that all consumers of these APIs are updated to handle the new return types
  • Check for any type mismatch errors in function signatures as flagged in fake_service.go

toby and others added 15 commits May 7, 2025 22:27
First (incomplete) cut of OpenAPI spec for registry API. To be continued in separate issues.
Refs modelcontextprotocol#28. After some thinking, this tries for a 'best of both worlds'
approach. Simple arguments can be present essentially as the original
spec recommended. As discussed, we don't specify the args for `npx` and
friends directly, so a simple package is merely...

```yaml
- name: my-api-mcp
  # ...
  packages:
    - type: npm
      name: "@c4312/my-api-mcp"
      version: "0.0.1"
      arguments:
        # Named inputs in 'arguments' generate a `--name=<value>`
        - type: named
          name: "api-key"
          is_required: true
          is_secret: true
```

However, I think we need to be able to specify runtime args in some
cases, otherwise the registry eventually mirrors all docker configs and
options which is not a great situation to be in. So, I suggest a
`runtime_args` in the same format. Say I wanted that for npm, I could:

```yaml
- name: my-api-mcp
  # ...
  packages:
    - type: npm
      name: "@c4312/my-api-mcp"
      version: "0.0.1"
      runtime_arguments:
        # Arguments with a pre-specified `value` don't get prompted for input
        - type: positional
          value: "--experimental-eventsource"
```

But Docker mounts are... complex, and ultimately we may deal with cases
that need multiple inputs in an argument like that. So for that I suggest
the third kind of "templated" input that does basic replacements and has
`properties` that follow the same general input schema:

```yaml
- name: server-filesystem
  # ...
  packages:
    - type: docker
      name: "mcp/filesystem"
      version: "0.0.1"
      runtime_arguments:
        - type: template
          description: Mount paths to access.
          required: true
          repated: true
          template: "--mount=type=bind,src={host_path},dst={container_path}"
          values:
            host_path:
              description: Path to access on the local machine
              required: true
              format: filepath
            container_path:
              description: Path to mount in the container. It should be rooted in `/project` directory.
              required: true
              format: string
              default: "/project"

      arguments:
        - type: positional
          value: "/project"
```

And then client config ends up being pretty simple, for the above case
for example a client could have this config:

```
{
  "filesystem": {
    "server": "@modelcontextprotocol/servers/src/filesystem@1.0.2",
    "package": "docker",
    "settings": {
      "mount_config": [
        { "source_path": "/Users/username/Desktop", "target_path": "/project/desktop" },
        { "source_path": "/path/to/other/allowed/dir", "target_path": "//projects/other/allowed/dir,ro" },
      ]
    }
  }
}
```

Which might generate a CLI:

```
docker run \
  --mount=type=bind,src=/Users/username/Desktop,dst=/project/desktop \
  --mount=type=bind,src=/path/to/other/allowed/dir,dst=/project/other/allowed/dir,ro \
  mcp/filesystem:1.0.2 \
  /project
```

I think this does a good job of accomplishing the goals we care about:

- We get nicely defined inputs that are easy to extend, easy to process,
  and easy for clients to configure.
- Versioning is enforced as we don't require the package to specify the
  identifier to npx/uvx/etc.
- We _usually_ can break away from a dependency on a specific runtime.
  This promise is broken a bit with `runtime_arguments`, most packages
  should not need this and those that do are those inherently more
  tightly coupled to a specific runtime like Docker (e.g. they would be
  pretty uncommon for Node or Python.)
- We don't have a suple complex template system with loops and fallbacks
  and conditionals, like we would if we tried to have a single template
  string for a command.
* Update the model to handle new changes to schema based on modelcontextprotocol#33

* Made the import script a little simpler.
* Add a new updated seed file
* update template mcp.json file for publisher
Description: entry.Description,
VersionDetail: entry.VersionDetail,
Repository: entry.Repository,
Server: model.Server{
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🐛 Correctness Issue

Breaking API Change: ServerDetail Structure Modification.

The ServerDetail structure has been changed to nest fields within a Server sub-structure, which will break all code that accesses these fields directly.

🔄 Dependencies Affected

internal/memory.go

Function: MemoryDB.Publish

Issue: Method accesses ServerDetail fields directly that are now nested in Server sub-structure

Suggestion: Update all direct field accesses to use the new Server sub-structure

Current Code (Diff):

// Publish adds a new ServerDetail to the database
func (db *MemoryDB) Publish(ctx context.Context, serverDetail *model.ServerDetail) error {
    if ctx.Err() != nil {
        return ctx.Err()
    }

    db.mu.Lock()
    defer db.mu.Unlock()

    // check for name
-   if serverDetail.Name == "" {
+   if serverDetail.Server.Name == "" {
        return ErrInvalidInput
    }

    // check that the name and the version are unique
    for _, entry := range db.entries {
-       if entry.Name == serverDetail.Name && entry.VersionDetail.Version == serverDetail.VersionDetail.Version {
+       if entry.Name == serverDetail.Server.Name && entry.VersionDetail.Version == serverDetail.Server.VersionDetail.Version {
            return ErrAlreadyExists
        }
    }

-   if serverDetail.Repository.URL == "" {
+   if serverDetail.Server.Repository.URL == "" {
        return ErrInvalidInput
    }

    db.entries[serverDetail.ID] = &model.Server{
-       ID:            serverDetail.ID,
-       Name:          serverDetail.Name,
-       Description:   serverDetail.Description,
-       VersionDetail: serverDetail.VersionDetail,
-       Repository:    serverDetail.Repository,
+       ID:            serverDetail.Server.ID,
+       Name:          serverDetail.Server.Name,
+       Description:   serverDetail.Server.Description,
+       VersionDetail: serverDetail.Server.VersionDetail,
+       Repository:    serverDetail.Server.Repository,
    }

    return nil
}


// List retrieves MCPRegistry entries with optional filtering and pagination
func (db *MongoDB) List(ctx context.Context, filter map[string]interface{}, cursor string, limit int) ([]*model.Entry, string, error) {
func (db *MongoDB) List(ctx context.Context, filter map[string]interface{}, cursor string, limit int) ([]*model.Server, string, error) {
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🐛 Correctness Issue

Breaking API Change: Return Type Modified.

The List method's return type has changed from []*model.Entry to []*model.Server, which will cause compilation failures or runtime crashes in any code expecting Entry objects.

Current Code (Diff):

- func (db *MongoDB) List(ctx context.Context, filter map[string]interface{}, cursor string, limit int) ([]*model.Server, string, error) {
+ func (db *MongoDB) List(ctx context.Context, filter map[string]interface{}, cursor string, limit int) ([]*model.Entry, string, error) {
📝 Committable suggestion

‼️ IMPORTANT
Trust, but verify! 🕵️ Please review this suggestion with the care of a code archaeologist - check that it perfectly replaces the highlighted code, preserves all lines, maintains proper indentation, and won't break anything in production. Your future self will thank you! 🚀

Suggested change
func (db *MongoDB) List(ctx context.Context, filter map[string]interface{}, cursor string, limit int) ([]*model.Server, string, error) {
func (db *MongoDB) List(ctx context.Context, filter map[string]interface{}, cursor string, limit int) ([]*model.Entry, string, error) {

// Convert from []*model.Entry to []model.Entry
result := make([]model.Entry, len(entries))
// Convert from []*model.Server to []model.Server
result := make([]model.Server, len(entries))
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🐛 Correctness Issue

Type mismatch between function signature and implementation.

The function signature returns []model.Entry but the implementation now creates a []model.Server slice, which will cause compilation errors.

Current Code (Diff):

-	result := make([]model.Server, len(entries))
+	result := make([]model.Entry, len(entries))
📝 Committable suggestion

‼️ IMPORTANT
Trust, but verify! 🕵️ Please review this suggestion with the care of a code archaeologist - check that it perfectly replaces the highlighted code, preserves all lines, maintains proper indentation, and won't break anything in production. Your future self will thank you! 🚀

Suggested change
result := make([]model.Server, len(entries))
result := make([]model.Entry, len(entries))


// Convert from []*model.Entry to []model.Entry
result := make([]model.Entry, len(entries))
// Convert from []*model.Server to []model.Server
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🐛 Correctness Issue

Incorrect comment for type conversion.

The comment incorrectly states converting from []*model.Server to []model.Server when the function signature requires []model.Entry.

Current Code (Diff):

-	// Convert from []*model.Server to []model.Server
+	// Convert from []*model.Entry to []model.Entry
📝 Committable suggestion

‼️ IMPORTANT
Trust, but verify! 🕵️ Please review this suggestion with the care of a code archaeologist - check that it perfectly replaces the highlighted code, preserves all lines, maintains proper indentation, and won't break anything in production. Your future self will thank you! 🚀

Suggested change
// Convert from []*model.Server to []model.Server
// Convert from []*model.Entry to []model.Entry


// GetAll returns all registry entries
func (s *registryServiceImpl) GetAll() ([]model.Entry, error) {
func (s *registryServiceImpl) GetAll() ([]model.Server, error) {
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🐛 Correctness Issue

Breaking API Change: Return Type Modified.

The return type of GetAll() was changed from []model.Entry to []model.Server, which will break all code that calls this method.

Current Code (Diff):

- func (s *registryServiceImpl) GetAll() ([]model.Server, error) {
+ func (s *registryServiceImpl) GetAll() ([]model.Entry, error) {
📝 Committable suggestion

‼️ IMPORTANT
Trust, but verify! 🕵️ Please review this suggestion with the care of a code archaeologist - check that it perfectly replaces the highlighted code, preserves all lines, maintains proper indentation, and won't break anything in production. Your future self will thank you! 🚀

Suggested change
func (s *registryServiceImpl) GetAll() ([]model.Server, error) {
func (s *registryServiceImpl) GetAll() ([]model.Entry, error) {

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

6 participants