fastapi_mcp icon indicating copy to clipboard operation
fastapi_mcp copied to clipboard

limit tool name length

Open DougieHauser opened this issue 8 months ago • 12 comments

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

DougieHauser avatar Apr 15 '25 21:04 DougieHauser

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!

codecov[bot] avatar Apr 19 '25 23:04 codecov[bot]

I think syncing tadata-org:main latest changes back to your branch would resolve test coverage issues

shahar4499 avatar Apr 19 '25 23:04 shahar4499

@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.

DougieHauser avatar Apr 20 '25 12:04 DougieHauser

Hey @shira-ayal, just pinging to see if there are any more expected changes. Thanks.

DougieHauser avatar Apr 24 '25 15:04 DougieHauser

Hi @DougieHauser ! Looks great, waiting for @shahar4499 to take a final look before we merge :)

shira-ayal avatar Apr 24 '25 15:04 shira-ayal

Hi @DougieHauser ! Looks great, waiting for @shahar4499 to take a final look before we merge :)

Hey @shahar4499, just a quick ping. Thanks.

DougieHauser avatar Apr 28 '25 07:04 DougieHauser

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

jshcrm avatar May 18 '25 21:05 jshcrm

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

jshcrm avatar May 18 '25 21:05 jshcrm

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

DougieHauser avatar May 20 '25 08:05 DougieHauser

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 avatar May 20 '25 08:05 DougieHauser

@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

jshcrm avatar May 20 '25 15:05 jshcrm

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.

shahar4499 avatar Jul 28 '25 13:07 shahar4499