Skip to content

Conversation

jalateras
Copy link

Summary

This PR fixes an issue where remote-only servers (servers with no repository) were still including an empty repository object with blank URL and source fields in their JSON output. This adds unnecessary noise to the API responses.

What Changed

  1. Added omitempty tags to Repository struct fields: The URL and Source fields in the Repository struct now have omitempty tags to prevent empty string values from being serialized.

  2. Implemented custom MarshalJSON for ServerJSON: A custom marshaling method was added to properly handle the case where a Repository struct is initialized but contains only empty values. This ensures the entire repository field is omitted from the JSON output when all its fields are empty.

  3. Added comprehensive tests: Test coverage was added for both the Repository struct marshaling and the ServerJSON marshaling to verify the behavior works correctly.

Technical Details

The fix uses a two-pronged approach:

  • First, the Repository struct fields get omitempty tags so individual empty fields are omitted
  • Second, a custom MarshalJSON method for ServerJSON checks if all Repository fields are empty and omits the entire repository object in that case

This ensures backward compatibility while fixing the issue for remote-only servers.

Testing

  • Added unit tests for Repository JSON marshaling/unmarshaling
  • Added integration tests for ServerJSON to verify empty repositories are omitted
  • All existing tests pass

Fixes #463

- Add omitempty tags to Repository struct fields (URL and Source)
- Implement custom MarshalJSON for ServerJSON to handle empty repositories
- Add comprehensive tests for JSON marshaling behavior
- Ensure backward compatibility with existing data

This prevents empty repository objects with blank URL and source fields
from being included in the JSON output for remote-only servers, reducing
unnecessary noise in the API responses.

Fixes modelcontextprotocol#463
Copy link
Collaborator

@rdimitrov rdimitrov left a comment

Choose a reason for hiding this comment

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

@jalateras - Thanks for finding and addressing this issue! 💯 The solution works well, added a small suggestion to make it even simpler 👍

)

// MarshalJSON implements custom JSON marshaling for ServerJSON to properly handle empty repositories
func (s ServerJSON) MarshalJSON() ([]byte, error) {
Copy link
Collaborator

Choose a reason for hiding this comment

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

Instead of custom marshalling could we handle this in the database retrieval methods? After unmarshalling ServerJSON in ./internal/database/postgres.go and ./internal/database/memory.go have something like:

// Clean empty repository to make omitempty work
if serverJSON.Repository.URL == "" && serverJSON.Repository.Source == "" &&
   serverJSON.Repository.ID == "" && serverJSON.Repository.Subfolder == "" {
    serverJSON.Repository = model.Repository{}
}

Your current approach works fine too though! My only reasoning is this would be simpler to maintain and leverage the existing:

Repository    model.Repository    `json:"repository,omitempty"`

rather than a custom marshalling logic. What do you think?

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.

Blank repository get inserted in published server
2 participants