fix: treat only deleted servers as reusable for remote URL validation#1204
fix: treat only deleted servers as reusable for remote URL validation#1204stationeros wants to merge 5 commits intomodelcontextprotocol:mainfrom
Conversation
|
@stationeros Thank you raising for the PR 👍 . Few points as per the design of status behaviour:
|
| // Check if any conflicting server has a different name | ||
| // Only active servers reserve remote URLs. | ||
| for _, conflictingServer := range conflictingServers { | ||
| if conflictingServer.Meta.Official != nil && conflictingServer.Meta.Official.Status != model.StatusActive { |
There was a problem hiding this comment.
if all conflictingServers are deleted then also we will allow this action to happen in case of publish server action.
In case of validating remote url from moving delete to active or updating a server or during publishing a server, we need to check if there is any active or deprecated server with that url, we can add another filter variable to the query of ListServers of include_deleted as false, we will not need another if condition in that case. Filter is designed to add filter condition for ListServer, what we want is to get servers list with same remote url which are not deleted so we can just pass include_deleted
There was a problem hiding this comment.
Thanks for the detailed clarification — agree to this . Have updated the commit and tests
Treat only deleted servers as reusable for remote URL validation.
This updates remote URL validation to check for conflicts only against non-deleted servers. In practice, that means active and deprecated servers continue to reserve their remote URLs, while deleted servers do not.
The validation remains centralized in
validateNoDuplicateRemoteURLsby queryingListServerswithIncludeDeleted: false, rather than adding additional post-query status checks.Motivation and Context
Remote URLs attached to deprecated servers are still valid for downstream clients, so they should continue to participate in conflict checks. Only deleted servers should release their remote URLs for reuse.
This change aligns the implementation with the intended status behavior:
This addresses the behavior discussed in #1193.
How Has This Been Tested?
Added targeted service-layer test coverage for the main scenarios:
I have not validated this in a deployed registry instance; testing was focused on the service behavior and regression coverage around the affected code paths.
Breaking Changes
No breaking changes intended.
This change narrows remote URL reuse to deleted servers only and keeps deprecated servers in the conflict set.
Types of changes
Checklist
Additional context
The implementation is intentionally narrow:
validateNoDuplicateRemoteURLsusesListServerswithIncludeDeleted: falseUpdateServerStatus(..., active)continues to revalidate when restoring fromdeletedUpdateAllVersionsStatus(..., active)does the same for deleted versions in the bulk restore flowThis keeps the behavior consistent across publish, update, and restore flows while preserving the intended lifecycle semantics for deprecated versus deleted servers.