Skip to content

Swap public/private method naming in Provider#2902

Merged
jlowin merged 13 commits intomainfrom
swap-methods
Jan 17, 2026
Merged

Swap public/private method naming in Provider#2902
jlowin merged 13 commits intomainfrom
swap-methods

Conversation

@jlowin
Copy link
Copy Markdown
Member

@jlowin jlowin commented Jan 17, 2026

This reverses the public/private naming convention for Provider methods to match user expectations:

Before: Public methods (list_tools, get_tool) were overridden by providers; private methods (_list_tools, _get_tool) applied transforms.

After: Private methods are overridden by providers to supply raw components; public methods apply transforms and are what users call.

This makes far more sense ergonomically—users call provider.list_tools() and get transforms applied automatically. Provider implementers override _list_tools() to supply raw data.

class MyProvider(Provider):
    async def _list_tools(self) -> list[Tool]:
        # Override this to provide raw components
        return [Tool(...)]

# Users call public methods - transforms applied automatically
tools = await provider.list_tools()

The call chain is now:

  1. Provider.list_tools() → builds transform chain → _list_tools()
  2. FastMCP._list_tools() → aggregates via p.list_tools() on sub-providers
  3. Each sub-provider's list_tools() → transforms → _list_tools()

@marvin-context-protocol marvin-context-protocol Bot added enhancement Improvement to existing functionality. For issues and smaller PR improvements. breaking change Breaks backward compatibility. Requires minor version bump. Critical for maintainer attention. provider Related to the FastMCP Provider class labels Jan 17, 2026
@coderabbitai
Copy link
Copy Markdown
Contributor

coderabbitai Bot commented Jan 17, 2026

Walkthrough

This PR renames and rebinds provider and server list/get methods between public and underscore-prefixed forms across the provider stack (base, aggregate, fastmcp_provider, filesystem, local, openapi, proxy, server). Authorization middleware was updated to call the FastMCP public getters (get_tool, get_resource, get_resource_template, get_prompt) instead of underscored helpers. Call sites and delegations were adjusted accordingly; implementations and control flow remain functionally unchanged.

Possibly related PRs

🚥 Pre-merge checks | ✅ 2 | ❌ 1
❌ Failed checks (1 inconclusive)
Check name Status Explanation Resolution
Description check ❓ Inconclusive The PR description provides clear context on the before/after naming conventions, includes a code example showing the new pattern, and explains the call chain. However, it lacks explicit coverage of all required template sections including a linked issue number, self-review confirmation, and testing details. Add the missing required checklist items from the template: link the issue number, confirm testing was performed, and verify all documentation updates. Ensure all checkbox items are explicitly addressed.
✅ Passed checks (2 passed)
Check name Status Explanation
Title check ✅ Passed The title 'Swap public/private method naming in Provider' clearly and concisely describes the main change in the PR, which involves reversing the public/private naming convention for Provider methods.
Docstring Coverage ✅ Passed Docstring coverage is 88.64% which is sufficient. The required threshold is 80.00%.

✏️ Tip: You can configure your own custom pre-merge checks in the settings.

✨ Finishing touches
  • 📝 Generate docstrings

Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out.

❤️ Share

Comment @coderabbitai help to get the list of available commands and usage tips.

Copy link
Copy Markdown
Contributor

@coderabbitai coderabbitai Bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actionable comments posted: 0

Caution

Some comments are outside the diff and can’t be posted inline due to platform limitations.

⚠️ Outside diff range comments (4)
src/fastmcp/server/providers/base.py (1)

12-26: Docstring example uses old naming convention.

The example shows overriding list_tools and get_tool (public methods), but after this PR, providers should override _list_tools and _get_tool (private methods). The public methods in the base class now apply transforms, so overriding them would bypass the transform chain.

📝 Suggested fix
     class DatabaseProvider(Provider):
         def __init__(self, db_url: str):
             super().__init__()
             self.db = Database(db_url)

-        async def list_tools(self) -> list[Tool]:
+        async def _list_tools(self) -> list[Tool]:
             rows = await self.db.fetch("SELECT * FROM tools")
             return [self._make_tool(row) for row in rows]

-        async def get_tool(self, name: str) -> Tool | None:
+        async def _get_tool(self, name: str) -> Tool | None:
             row = await self.db.fetchone("SELECT * FROM tools WHERE name = ?", name)
             return self._make_tool(row) if row else None
src/fastmcp/server/server.py (3)

1226-1277: Minor: Comment at line 1277 appears reversed.

The comment says _get_resource is inherited from Provider - wraps get_resource() with transforms, but the relationship is the opposite: get_resource() (public) wraps _get_resource() (private) with transforms.

📝 Suggested fix
-    # _get_resource is inherited from Provider - wraps get_resource() with transforms
+    # get_resource is inherited from Provider - wraps _get_resource() with transforms

1327-1378: Minor: Comment at line 1378 appears reversed.

Same issue as line 1277 - the public method wraps the private method, not vice versa.

📝 Suggested fix
-    # _get_resource_template is inherited from Provider - wraps get_resource_template() with transforms
+    # get_resource_template is inherited from Provider - wraps _get_resource_template() with transforms

1424-1475: Minor: Comment at line 1475 appears reversed.

Same issue - the public method wraps the private method.

📝 Suggested fix
-    # _get_prompt is inherited from Provider - wraps get_prompt() with transforms
+    # get_prompt is inherited from Provider - wraps _get_prompt() with transforms

Copy link
Copy Markdown
Contributor

@coderabbitai coderabbitai Bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actionable comments posted: 0

Caution

Some comments are outside the diff and can’t be posted inline due to platform limitations.

⚠️ Outside diff range comments (3)
src/fastmcp/server/server.py (3)

1557-1564: Update stale comment to reference get_tool.

The inline note still mentions _get_tool, but get_tool is now the transform-aware entry point.

✏️ Proposed comment fix
-            # Use _get_tool to apply transforms (including visibility)
+            # Use get_tool to apply transforms (including visibility)

1668-1691: Update comment to match get_resource usage.

The comment references _get_resource, but the code now calls get_resource.

✏️ Proposed comment fix
-                # Try concrete resources first (transforms + auth via _get_resource)
+                # Try concrete resources first (transforms + auth via get_resource)

1789-1796: Update comment to reference get_prompt.

The note still points to _get_prompt after the API swap.

✏️ Proposed comment fix
-            # Use _get_prompt to apply transforms (including visibility)
+            # Use get_prompt to apply transforms (including visibility)

@jlowin jlowin merged commit 96782c0 into main Jan 17, 2026
11 checks passed
@jlowin jlowin deleted the swap-methods branch January 17, 2026 20:25
@marvin-context-protocol
Copy link
Copy Markdown
Contributor

Test Failure Analysis

Summary: The test test_rate_limiting_with_different_operations is failing because the authorization middleware changes in this PR are causing extra list_tools calls that exceed the rate limit budget before the test expects it to.

Root Cause: The authorization middleware was changed to call the public get_tool() method instead of the private _get_tool() method. The public get_tool() applies transforms, which internally calls _get_tool(), which in turn calls _list_tools() to fetch all tools. This causes an additional list_tools request to go through the middleware chain.

Impact:

  • Before the PR: Authorization middleware called _get_tool() directly → no extra middleware requests
  • After the PR: Authorization middleware calls get_tool() → transform chain → _get_tool()_list_tools() → extra middleware request

The test test_rate_limiting_with_different_operations expects:

  1. list_tools (client init)
  2. call_tool quick_action
  3. call_tool heavy_computation
  4. call_tool batch_process ← Should fail (4th request, burst_capacity=5)

But now it sees:

  1. list_tools (client init)
  2. call_tool quick_action
  3. list_tools (from authorization middleware → get_tool → _get_tool → _list_tools)
  4. call_tool heavy_computation
  5. call_tool batch_process ← Uses last token, succeeds instead of failing!

Suggested Solution: Revert the authorization middleware changes to call the private methods (_get_tool, _get_resource, _get_resource_template, _get_prompt) instead of the public methods. Authorization checks should happen BEFORE transforms are applied, so it makes sense to use the private methods that bypass the transform chain.

Update src/fastmcp/server/middleware/authorization.py:140 to use _get_tool instead of get_tool (and similarly for resources and prompts at lines 205-206 and 298).

Detailed Analysis

Test Failure Log

FAILED tests/server/middleware/test_rate_limiting.py::TestRateLimitingMiddlewareIntegration::test_rate_limiting_with_different_operations - Failed: DID NOT RAISE <class 'fastmcp.exceptions.ToolError'>

The test configured rate limiting with burst_capacity=5 and expected the 3rd tool call to fail, but it succeeded because the burst capacity was consumed by an unexpected extra list_tools call.

Code Evidence

From src/fastmcp/server/providers/base.py:267:

async def _get_tool(
    self, name: str, version: VersionSpec | None = None
) -> Tool | None:
    tools = await self._list_tools()  # ← This triggers list_tools through middleware!
    matching = [t for t in tools if t.name == name]
    ...

When authorization middleware calls the public get_tool(), it goes through the transform chain which eventually calls _get_tool(), which calls _list_tools(), which goes back through the middleware chain (including rate limiting).

Other Tests Show Awareness of list_tools Calls

Line 400 comment in the same test file:

burst_capacity=4,  # init + list_tools + call + list_tools = 4, so 2nd call fails

This shows developers were already accounting for extra list_tools calls, but the failing test wasn't updated to account for the new behavior introduced by this PR.

Related Files
  • src/fastmcp/server/middleware/authorization.py:140 - Authorization middleware calling get_tool() (should use _get_tool())
  • src/fastmcp/server/middleware/authorization.py:205-206 - Similar issue for resources
  • src/fastmcp/server/middleware/authorization.py:298 - Similar issue for prompts
  • src/fastmcp/server/providers/base.py:267 - _get_tool calls _list_tools() internally
  • tests/server/middleware/test_rate_limiting.py:375-388 - The failing test

@jlowin jlowin removed the breaking change Breaks backward compatibility. Requires minor version bump. Critical for maintainer attention. label Jan 20, 2026
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

enhancement Improvement to existing functionality. For issues and smaller PR improvements. provider Related to the FastMCP Provider class

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant