Add MCP resource handling #21661
Add MCP resource handling #21661
Conversation
NobodysNightmare
force-pushed
the
mcp-resources
branch
3 times, most recently
from
730a2fa to
46201f4
Compare
| @template_suffix = suffix if suffix.present? | ||
| return nil if @template_suffix.nil? | ||
|
|
||
| UriTemplate.new("#{Setting.protocol}://#{Setting.host_name}#{@template_suffix}") |
There was a problem hiding this comment.
I am open for ideas to improve efficiency here. I find it slightly sad that every call to uri_template creates a new UriTemplate instance (which involves compiling a regex).
On the other hand, we can't properly cache this in a simple instance variable: The setter version of this method is only called once, when the class gets first defined. However, the actual template should change immediately if the Setting.host_name changes (though it doesn't do that very often 🤷).
For now I am happy to live with the fact that this code is not 100% efficient, but it still makes me a tad sad.
NobodysNightmare
force-pushed
the
mcp-resources
branch
2 times, most recently
from
d36e41f to
5919e27
Compare
NobodysNightmare
force-pushed
the
mcp-resources
branch
from
5919e27 to
823c80d
Compare
This is against the schema for definition for links, which already allow the title to be missing, but don't allow it to be null. If it's present it must have a string as a value.
Allows to list resources and resource templates and reading their contents.
NobodysNightmare
force-pushed
the
mcp-resources
branch
from
823c80d to
bd32e00
Compare
| end | ||
|
|
||
| def read_resource(uri) | ||
| resource = enabled.find { |r| r.uri == uri || r.uri_template&.match?(uri) } |
There was a problem hiding this comment.
🟡 use somehow a variable name, that indicates, that this is actually not a resource, but a resource class.
| API::V3::Projects::ProjectRepresenter.create(project, current_user:) | ||
| end | ||
| end | ||
| end |
There was a problem hiding this comment.
🟢 I like how the specific resource in the end looks like. very small, easy to read.
| else | ||
| { | ||
| href: nil, | ||
| title: nil |
|
|
||
| require "spec_helper" | ||
|
|
||
| RSpec.describe McpResources::Project, with_flag: { mcp_server: true } do # rubocop:disable RSpec/SpecFilePathFormat |
There was a problem hiding this comment.
🟡 why having a different namespace structure for the specs, so that you have to disable the rubocop here? any specific reason? I read from the chosen namespace, that you basically only want to test the templates. But why not using the original namespace and just use a nested describe "#resource_templates"?
There was a problem hiding this comment.
Maybe I need to reorganize my specs... I started out with spec/requests/mcp and put tests there in a structure that I thought would be useful. Since none of them had a class name as a description, but rather a scenario (a string), this was no issue.
Now for the resources I added a lot of specs and I put them into the same folder structure, but it was the first time that I had a use case for described_class so I figured that I could replace the scenario name with the class of the resource being tested. This is when Rubocop started complaining.
I think I'll have to simply give up my previous spec ordering scheme and obey the one that Rubocop wishes for...
| end | ||
| end | ||
|
|
||
| describe "#match?" do |
There was a problem hiding this comment.
🟡 maybe add a test case for using bad characters for the variables we have? e.g. a $ in the id variable
Allows to list resources and resource templates and reading their contents.
Ticket
https://community.openproject.org/wp/68689