-
Notifications
You must be signed in to change notification settings - Fork 344
change fields related to server json struct to follow camelCase instead of snake_case #488
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
base: tadasant/fix-server-id-inconsistency
Are you sure you want to change the base?
Conversation
Heads up we should also open a PR with the new schema here and bump the version number on it to today's date: https://github.com/modelcontextprotocol/static/tree/main/schemas |
@claude can you do a PR review please |
Claude encountered an error —— View job
I'll analyze this and get back to you. |
@pree-dew I'm not sure the migration commit is totally necessary -- we should be able to support multiple versions of server.json schemas, and in general I'd like to prioritize maintaining our "server.json is immutable" invariant |
Oops (re: Claude^^) Review from Claude Code on localCode Review: PR #488 - Change fields to camelCase instead of snake_case🎯 OverviewThis PR systematically converts JSON field names from ✅ Strengths
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looking great! Main feedback is:
- We shouldn't migrate the old data; we should be able to support multiple schema versions in the table and server.json should remain immutable
- Let's make sure we have a PR on the
static
repository ready to go with a new2025-09-16
version - As such, we should also update everything across this codebase to use the new version instead of
2025-07-09
- Let's start a changelog of sorts for the server.json schema somewhere in the docs. Make it clear what the changes are as we bump the version here
@tadasant It was mentioned in todo #428 (comment) to have migration for historical data. If it is not required, will remove it. I will create a PR for static repo. |
Oh fair - let's see what @domdomegg thinks - but I still think we should pull back on that and simply allow for the possibility that our database isn't only serving the latest schema version of server.json. In fact I could see a future in GA where we would want to allow both publishing and reading of different schema versions. Probably something we should cut an Issue for.
At the very least I think we should add a warning with a link pointing to the changelog so folks can update it themselves. I think the ideal here would be some sort of |
Raised a PR for static modelcontextprotocol/static#5
I agree with the point that we should be able to support multiple version of schema and also agree with changelog idea, it's important for consumers to understand what exactly has changed in schema. I can create a new issue for that if we are looking to cover it in another issue. |
I think we should do the changelog in this PR so that as soon as it's live there are docs indicating the changelog (which folks can use as a defacto migration guide). Doesn't need to be anything fancy -- I expect a single prompt to Claude Code (or equivalent) would make a good file for us here. Separate issue on supporting multiple versions would be helpful 🙏 |
On it @tadasant for changelog. |
Pushed:
Pending:
|
Thanks @pree-dew ! Looking good. I pushed some tweaks to how we manage the new CHANGELOG (I made one for the API and one for the server.json) I think one more request besides what you laid out -- we should add a logic branch in the mcp-publisher that if we see someone trying to push a 2025-07-09 schema, we can error out and point them to the GitHub link with the server.json changelog (and the migration steps section within it). |
I think this got resolved so what's left is to be addressed 👍
Not for this PR: |
…ontextprotocol-registry into refactor-all-server-json
@tadasant changes for camelCase of Are we going ahead with migration? I will make changes for Regarding this:
Do you want to cover this as part of this PR? |
I think we should, because when we land this PR, the breaking changes will be live. So I think it's easier to bake it in here rather than try to coordinate a fast follow while folks might be using the updated schemas.
I'll ping Adam to see if we can get to alignment (I know he's busy with a number of things atm). If we don't hear from him by tomorrow AM let's remove the migration; guessing his list was meant to be comprehensive/helpful but not necessarily prescriptive. |
@tadasant Changes for redirecting users to changelog on using deprecated schema is pushed. Noticed that seed.json was failing because of missing ServerId and VersionId field which was breaking the local setup to test, pushed that also. |
docker-compose.yml
Outdated
@@ -42,5 +43,5 @@ services: | |||
interval: 1s | |||
retries: 30 | |||
ports: | |||
- "5432:5432" | |||
- "5433:5432" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@pree-dew intentional change here?
cmd/publisher/commands/publish.go
Outdated
if strings.Contains(serverJSON.Schema, "2025-07-09") { | ||
return fmt.Errorf(`deprecated schema detected. | ||
|
||
While the 2025-07-09 schema is still supported, we strongly recommend migrating to the current schema format for new servers. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@pree-dew I think we should not support publishing under older versions at this time. My earlier reference to "I think we should support multiple versions" would be for the consumption side of the API - sorry, I was not clear earlier on the difference.
So we should support storing old schema versions that are already published, but we should require that new data being published use the latest schema version.
Maybe we can soften this in the future, but I think in Preview launch here it's fair to force publishers to adhere to the latest standard.
Thank you @pree-dew for continuing to drive this! Sorry for this long tail of nits. I do think we should remove the migration (haven't heard from Adam yet), but almost there now. |
@tadasant No problem, I will address PR comments. I will remove the migration and will test it once. CC @rdimitrov |
Heya, sorry for the late reply! I think my preference would be to migrate the old data, because otherwise consumers will have the pain of mapping both models into one. I think this will be very frustrating for clients. I appreciate we could offer utils or SDKs for consuming registry data, but then we have to do this for every language, and these need to be bundled by a bunch of people (and getting packages approved in enterprises is always a pain 😅). Appreciate this is a little weird with the idea of server.json to be immutable. But conceptually I think the data itself is immutable, not necessarily the structure it is presented in, if that makes sense? Perhaps I might be a little more cautious to make breaking changes / do data format migrations if we were GA, but I think it still being relatively recently in preview is another reason I'm okay with migrating all this data over. |
@tadasant @rdimitrov Pushed the PR changes and removed the migration file. @domdomegg Sorry, pushed the changes before reading the message, can add it once we finalise our approach of dealing with mutability of server.json. |
Motivation and Context
Aligns with issue #428
To bring consistency across naming convention
How Has This Been Tested?
Breaking Changes
Types of changes
Checklist