limit tool name length
Describe your changes
Resolves tadata-org/fastapi_mcp#64
Attempts to filter tools whose operation_id + server name are too long.
Note: The codebase seems to have logic overlap between the functions server.py/_filter_tools and convert.py/def convert_openapi_to_mcp_tools - I didn't want to rock the boat too much on my first attempted contribution, but I would be happy to try my luck at refactoring (perhaps in a separate ticket).
Issue ticket number and link (if applicable)
https://github.com/tadata-org/fastapi_mcp/issues/64
Screenshots of the feature / bugfix
Checklist before requesting a review
- [V] Added relevant tests
- [V] Run ruff & mypy
- [V] All tests pass
Codecov Report
Attention: Patch coverage is 68.75000% with 5 lines in your changes missing coverage. Please review.
| Files with missing lines | Patch % | Lines |
|---|---|---|
| fastapi_mcp/server.py | 78.57% | 3 Missing :warning: |
| fastapi_mcp/openapi/convert.py | 0.00% | 2 Missing :warning: |
:loudspeaker: Thoughts on this report? Let us know!
I think syncing tadata-org:main latest changes back to your branch would resolve test coverage issues
@shahar4499 - Thanks, I rebased my branch with the "upstream" repo but couldn't get the codecov bot to run again.
I then tried to push a "dummy commit" in order to trigger it, but again, no such luck.
Is there anything I'm missing?
Thanks.
Hey @shira-ayal, just pinging to see if there are any more expected changes. Thanks.
Hi @DougieHauser ! Looks great, waiting for @shahar4499 to take a final look before we merge :)
Hi @DougieHauser ! Looks great, waiting for @shahar4499 to take a final look before we merge :)
Hey @shahar4499, just a quick ping. Thanks.
Just want to chime in here as a user that I hit the operation_id max length issue when using claude desktop, and it was not obvious that I needed to add an operation_id to my endpoints with the proper length as a workaround. I did eventually find that referenced in the docs. This PR looks like it will simply exclude endpoints that violate the operation_id length, which all of mine did when I let operation ids be auto generated. I'd love to see some kind of console logging or warning when an endpoint is going to be excluded, or maybe an option to automatically reduce the operation_id to a length that fits instead of excluding
Unless I'm missing something this PR will just remove the tools and not offer any feedback that that happened or why, which would then lead to me digging through code and debugging why fastapi_mcp isn't doing the one thing it's supposed to do, create tools from my endpoints
That's a good point, @jshcrm ..
Personally, it's my 1st PR on this repo, and I tried to stay consistent with what seemed like the convention that was already in place for other filters such as _include_operations, _exclude_operations, _include_tags and _exclude_tags
TBH, I could just go ahead and add logging for all of them.. Just want to make sure this repo is still maintained, because I believe it's been pretty stale for a month now.
@shahar4499 @shira-ayal
On second though, @jshcrm ..
The max limit functionality does seem less "explicit" than the others.. I've added a warning for it alone, at this point. Thanks.
@DougieHauser Awesome I think that warning will go a long way! Thanks for adding that. I'm also hoping this repo is still being maintained - there was a release a few days ago but I'd really love to see the switch from sse to streamable-http
Hey @DougieHauser , sorry for the long wait. We've been heads down on some stuff at Tadata.
Anyway, I agree with @jshcrm that just removing the operations due to them being too long is not ideal.
Thinking about this more broadly, "long tool names" is just a first in a series of validations to be added to the library.
So maybe instead of removing tools, you can throw a validation error and fail the creation of the FastApiMCP instance?
And to enable/disable this behavior, you can accept a validations_behavior param, that can be either "ignore" | "warn" | "raise", with the default being raise.
If that's too aggressive we can change it to warn.