-
Notifications
You must be signed in to change notification settings - Fork 167
Description
I believe having Quarkus-specific subclasses that add additional functionality is generally a bad thing from a maintenance burden perspective. Even worse if it's supposed to be public API because then application developers using Quarkus will be, in some cases, forced to use different classes (than they would have used in vanilla langchain4j) for no good reason.
QuarkusMcpToolProvider and QuarkusToolProviderRequest are IMO good examples where they could be replaced by vanilla langchain4j classes with proper composition.
QuarkusMcpToolProvider has two purposes:
- Wrap tool executions by a tool wrapper for OpenTelemetry tracing. This can be replaced by simply passing the relevant tool wrapper to the builder of upstream
McpToolProvider, no need to have a separate tool provider class for this. - It adds a filter for tool provider requests that specifies a set of MCP client keys that should be taken into account (
QuarkusToolProviderRequestholds the actual set of keys). This could be replaced in two steps:- Basically move the
QuarkusToolProviderRequest(which serves for holding the key set) into upstream lc4j and name itMcpToolProviderRequestinstead (MCP should have been in its name from the beginning IMO because it is clearly specific to MCP). Probably we would also directly switch it to hold aBiPredicate<McpClient, ToolSpecification>instead of aSet<String>to make its usage more generic. - Change the McpToolProvider.provideTools method to take into account that the argument may be a
McpToolProviderRequestand if so, use the filter from that instance instead of the default filter associated with thisMcpToolProvider.
- Basically move the
This would only be a breaking change if anybody is actually referencing QuarkusToolProviderRequest directly in a Quarkus application's code, which is possible, but hopefully not common. QuarkusMcpToolProvider is package-private, so I would assume it's not used by anybody directly.
I hope nobody's strongly against this, I think the API will be cleaner after this refactoring.