Skip to content

Draft: Allow for vendor extensions via x-publisher #298

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Open
wants to merge 10 commits into
base: main
Choose a base branch
from

Conversation

rdimitrov
Copy link
Contributor

@rdimitrov rdimitrov commented Aug 22, 2025

This PR serves as a placeholder and brings together all of the changes related to supporting vendor extensions passed through the API. The idea is to have a single place to track and review the work as it comes together (even if it’s still a bit of a work in progress).

Disclaimer:

As I mentioned in Discord, this turned out to be a bit larger than I initially expected. The goal of this PR is to get it opened up early so we can kick off the first round of reviews while I continue refining and adding to it. That way, feedback can start flowing in parallel and help guide the remaining work.

The changes in this PR cover:

  • Updates on the data model
  • Updates on both databases (in-memory and postgresql), added a new table to PSQL for tracking the extensions leaving the server table only for the clean server.json properties
  • Updates the seed.json file (thus the big line change)
  • Adds a seed-migrate tool to help migrating an existing seed file to the new format
  • Updates the seed import mechanism to support this
  • Updates the existing tests - unit, integration and validation tests
  • Adds the publish handler to the OpenAPI spec (it was missing)
  • Fixes linting errors
  • Updates on the API side on (publish, list, get) to support the new format of ServerResponse -
{
  "server": {},
  "x-io.modelcontextprotocol.registry": {}, /*not available for publish since this gets populated by the registry*/
  "x-publisher": {}
}

What's left

  • Test it a bit more since I had it implemented first for mongodb, but then used claude to reimplement it for postgresql
  • Have an initial round of discussions with @domdomegg and @tadasant to make sure the overall direction feels right
  • Talk through what the publishing step should look like, i.e. whether we want a dedicated file to cover the extensions (similar to how we currently use server.json)
  • Ensure we don't degrade the existing test coverage
  • Double-check that everything works as expected and the implementation feels good for all of us
  • Once we’re aligned, decide on the best next step: should we break this down into a few smaller follow-up PRs for easier review, or keep it as one larger PR?

Motivation and Context

Motivated by the discussions in: #284

How Has This Been Tested?

Locally and by running all of the tests

Breaking Changes

Yes, it changes both the API and the format of the seed file

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
  • 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

@codecov-commenter
Copy link

Signed-off-by: Radoslav Dimitrov <radoslav@stacklok.com>
Signed-off-by: Radoslav Dimitrov <radoslav@stacklok.com>
Signed-off-by: Radoslav Dimitrov <radoslav@stacklok.com>
Signed-off-by: Radoslav Dimitrov <radoslav@stacklok.com>
Signed-off-by: Radoslav Dimitrov <radoslav@stacklok.com>
Signed-off-by: Radoslav Dimitrov <radoslav@stacklok.com>
Signed-off-by: Radoslav Dimitrov <radoslav@stacklok.com>
Signed-off-by: Radoslav Dimitrov <radoslav@stacklok.com>
Signed-off-by: Radoslav Dimitrov <radoslav@stacklok.com>
Signed-off-by: Radoslav Dimitrov <radoslav@stacklok.com>
@rdimitrov
Copy link
Contributor Author

Asked Claude to suggest how we can split it eventually -

Based on the code changes, this can be split into smaller, more reviewable PRs. Here's my recommended approach:

  Suggested PR Sequence

  PR 1: Core Data Model Refactor

  Foundation - No API changes
  - internal/model/model.go - New types (ServerDetail, RegistryMetadata, ServerRecord, etc.)
  - Helper functions (ValidatePublisherExtensions, ExtractPublisherExtensions, etc.)
  - Keep old types for backwards compatibility during transition
  - Risk: Low - No external API impact

  PR 2: Database Layer Updates

  Storage architecture changes
  - internal/database/database.go - Interface updates
  - internal/database/postgres.go - Two-table implementation
  - internal/database/memory.go - In-memory equivalent
  - Database migrations (002_add_server_extensions.sql)
  - Risk: Medium - Database schema changes
  - Depends on: PR 1

  PR 3: Service Layer Integration

  Business logic updates
  - internal/service/service.go - Interface changes
  - internal/service/registry_service.go - Implementation updates
  - internal/service/fake_service.go - Test service updates
  - Risk: Medium - Core business logic
  - Depends on: PR 1, 2

  PR 4: API Response Format Changes

  Non-breaking API updates
  - internal/api/handlers/v0/servers.go - List/Get endpoints to return ServerResponse
  - OpenAPI spec updates for response format
  - Update existing tests
  - Risk: High - Breaking API changes
  - Depends on: PR 1, 2, 3

  PR 5: Publish Endpoint Implementation

  New functionality
  - internal/api/handlers/v0/publish.go - Extension validation and handling
  - OpenAPI spec for publish endpoint
  - Publish-specific tests
  - Risk: Low - New endpoint, no breaking changes
  - Depends on: PR 1, 2, 3

  PR 6: Migration & Seed Updates

  Data migration support
  - tools/migrate-seed/ - Seed migration tool
  - Updated seed data format
  - tools/validate-examples/ - Validation updates
  - Risk: Low - Tooling and data
  - Depends on: All previous PRs

  Alternative: Feature Flag Approach

⏺ Update Todos
  ⎿  ☒ Analyze the PR structure and dependencies     
     ☒ Identify logical boundaries for PR splitting
     ☐ Consider migration and backwards compatibility
     ☒ Propose PR sequence with dependencies

⏺ If you want to deploy incrementally:
  - Add a feature flag for "extension wrapper format"
  - Deploy the storage changes first (they can store both formats)
  - Gradually switch API endpoints to new format
  - Remove old code once fully migrated

  Benefits of Splitting

  Easier Review:
  - Each PR has a clear, single responsibility
  - Smaller diffs are easier to understand
  - Can focus review effort on specific concerns

  Reduced Risk:
  - Database changes isolated and testable independently
  - API breaking changes clearly identified in PR 4
  - New functionality (publish) separated from changes to existing endpoints

  Parallel Development:
  - Multiple team members can work on different PRs simultaneously
  - Earlier PRs can be merged while later ones are still in review

  Challenges to Consider

  Dependency Chain:
  - PRs must be merged in sequence
  - Later PRs need rebasing if earlier ones change

  Testing:
  - Integration tests may need to span multiple PRs
  - Consider adding temporary compatibility layers

  Coordination:
  - More PRs = more coordination overhead
  - Need clear communication about the sequence

⏺ Update Todos
  ⎿  ☒ Analyze the PR structure and dependencies     
     ☒ Identify logical boundaries for PR splitting
     ☒ Consider migration and backwards compatibility
     ☒ Propose PR sequence with dependencies

⏺ My Recommendation: Start with the 6-PR sequence above. PR 1-3 are the foundation and can be reviewed/merged relatively safely. 
PR 4 is where the breaking changes happen, so it needs the most careful review. The sequence allows for incremental progress while maintaining system stability.

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.

2 participants