-
Notifications
You must be signed in to change notification settings - Fork 677
refactor: Improve resource template management and API consistency #576
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
refactor: Improve resource template management and API consistency #576
Conversation
- Convert resource templates from List to Map-based storage for better lookup performance - Add runtime resource template management methods (add/remove/list) - Replace McpError with more appropriate exception types (IllegalArgumentException, IllegalStateException) - Enhance completion request handling with better validation and error messages - Add type-safe constants for reference types (PromptReference.TYPE, ResourceReference.TYPE) - Improve separation between static resources and dynamic resource templates - Add comprehensive resource and resource template listing capabilities - Reorganize imports and improve code structure across server implementations - Update test cases to reflect new API patterns - Add tests for listing, removing, and managing resources - Add tests for resource template operations (add, remove, list) - Include capability validation tests for resource templates - Cover edge cases like removing nonexistent resources/templates - Apply changes to both async and sync server test suites - Reorganize imports for better code organization This refactoring improves type safety, performance, and maintainability while providing a more consistent API for resource template management across all server implementations (Async, Sync, Stateless variants). Signed-off-by: Christian Tzolov <christian.tzolov@broadcom.com>
Signed-off-by: Christian Tzolov <christian.tzolov@broadcom.com>
Signed-off-by: Christian Tzolov <christian.tzolov@broadcom.com>
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.
Looks good to me. Thanks for this fix
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.
listResourceTemplates
appears to be typed incorrectly - looks good beyond that.
.stream() | ||
.map(McpServerFeatures.AsyncResourceSpecification::resource) | ||
.filter(resource -> !resource.uri().contains("{")) | ||
// .filter(resource -> !resource.uri().contains("{")) |
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.
nit: leftover comment should be deleted
public Flux<McpSchema.Resource> listResourceTemplates() { | ||
return Flux.fromIterable(this.resources.values()).map(McpServerFeatures.AsyncResourceSpecification::resource); | ||
} |
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.
Shouldn't this be this.resourceTemplates
and return a Flux<McpSchema.ResourceTemplate>
? Looks like this is correct in McpStatelessAsyncServer
, but not here.
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.
Nice catch! will be fixed.
Though this is a blunder it doesn't affect the MPC spec.
The listResource() and listResourceTemplates() are an extension of the Server's runtime API to compliment the addResource()/removeResource() and addResourceTemplate()/removeResourceTemplate().
In separate PR we will add similar list methods for tools, prompts, completions as well.
* List all registered resource templates. | ||
* @return A list of all registered resource templates | ||
*/ | ||
public List<McpSchema.Resource> listResourceTemplates() { |
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.
List<McpSchema.ResourceTemplate>
- the return type was wrong in McpAsyncServer
as well.
|
||
private Map<String, Object> meta; | ||
|
||
public Builder uri(String uri) { |
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.
uriTemplate
// Note: Based on the current implementation, listResourceTemplates() returns | ||
// List<Resource> | ||
// This appears to be a bug in the implementation that should return | ||
// List<ResourceTemplate> |
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.
// Note: Based on the current implementation, listResourceTemplates() returns | ||
// Flux<Resource> | ||
// This appears to be a bug in the implementation that should return | ||
// Flux<ResourceTemplate> |
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.
// Note: Based on the current implementation, listResourceTemplates() returns | ||
// List<Resource> | ||
// This appears to be a bug in the implementation that should return | ||
// List<ResourceTemplate> |
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.
Signed-off-by: Christian Tzolov <christian.tzolov@broadcom.com>
Thank you @LucaButBoring . Last commit should address the above-mentioned issues |
Signed-off-by: Christian Tzolov <christian.tzolov@broadcom.com>
- Provide separation of concerns between static resources and parameterized resource templates. - Add AsyncResourceTemplateSpecification and SyncResourceTemplateSpecification for both McpServerFeatures and McpStatelessServerFeatures - Change resource template storage from List to Map to acomodate the resource read handler. - Add runtime management methods: addResourceTemplate(), removeResourceTemplate(), listResourceTemplates() - Improve error handling by using IllegalArgumentException/IllegalStateException instead of McpError - Add new interfaces (Meta, Identifier) and reorganize schema hierarchy - Enhance completion request validation with better error messages - Add ResourceTemplate.Builder for easier template construction - Update all server implementations (Async, Sync, Stateless) consistently - Add type-safe constants for reference types (PromptReference.TYPE, ResourceReference.TYPE) - Add tests for new resource template management functionality - Clean up imports and remove unused dependencies Co-authored-by: Pascal Vantrepote <pascal@confluent.io> Signed-off-by: Christian Tzolov <christian.tzolov@broadcom.com>
Rebased, ,squashed and merged at ff98e29 |
Refactor resource template management from List-based to Map-based storage, add new resource template specification classes, and enhance resource/template handling with improved error handling and runtime management capabilities.
New Resource Template Specification Classes:
AsyncResourceTemplateSpecification
andSyncResourceTemplateSpecification
toMcpServerFeatures
AsyncResourceTemplateSpecification
andSyncResourceTemplateSpecification
toMcpStatelessServerFeatures
Resource Template Handling Refactoring:
List<ResourceTemplate>
toMap<String, ResourceTemplateSpecification>
Enhanced Runtime Management:
addResourceTemplate()
andremoveResourceTemplate()
methods to all server classeslistResourceTemplates()
for introspectionError Handling Improvements:
McpError
with appropriateIllegalArgumentException
andIllegalStateException
McpSchema Enhancements:
ResourceTemplate.Builder
class for easier template constructionMeta
,Identifier
, andAnnotated
interfacesMcpError
.Test Coverage: