Skip to content

Conversation

codecracker007
Copy link

Add RFC 6570 support for optional parameters as form-style query expansions

Motivation and Context

Adds optional parameter support with RFC 6570 compliance as form-style query expansions. This change addresses issue #378, .

Breaking Changes

None

Types of changes

  • Bug fix (non-breaking change which fixes an issue)
  • New feature (non-breaking change which adds functionality)
  • Breaking change (fix or feature that would cause existing functionality to change)
  • Documentation update

Checklist

  • I have read the MCP Documentation
  • My code follows the repository's style guidelines
  • New and existing tests pass locally
  • I have added appropriate error handling
  • I have added or updated documentation as needed

@ihrpr ihrpr added this to the r-05-25 milestone May 13, 2025
Copy link
Contributor

@ihrpr ihrpr left a comment

Choose a reason for hiding this comment

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

Thank you for working on this!

This PR makes good progress toward RFC 6570 support by adding form-style query expansion.

There are a few suggestions:

  1. Type Conversion for Query Parameters. Query parameters from URLs are always strings, but the code doesn't convert them to match function parameter
    types.

Example of the problem:

@server.resource("resource://items/{category}{?limit}")
def get_items(category: str, limit: int = 10) -> str:
    # When called with resource://items/books?limit=20
    # limit will be string "20" not int 20
    return f"Items in {category}, limit: {limit} (type: {type(limit)})"

Suggested fix: Add type conversion based on function annotations in

  1. Handle Edge Cases in Query Parsing. The current implementation might not handle edge cases properly:
  • Empty query values (?param=)
  • URL-encoded values (?name=John%20Doe)
  1. Add Tests for Type Conversion Scenarios

  2. Consider Documenting Query Parameter Behavior

@codecracker007
Copy link
Author

@ihrpr thanks for the feedback

during the fn call with params result = self.fn(**params)

can i use call_fn_with_arg_validation

async def call_fn_with_arg_validation(
for type validation and conversion and should i fallback to default values for optional parameters if validation fails for any one of them

@codecracker007
Copy link
Author

@ihrpr the pydantic validate_call already takes care of the compatible type conversion based on annontated parameters for query parameters and raises validation error for incompatible types added a decorator which check if the validation error is raised by optional parameters and ignores them so they fall back to default values to keep it compliant

added relevant tests covering these cases

for the cases in query parsing the empty values and url encoded parameters are handled by parse_qs added relevant tests covering these

@codecracker007 codecracker007 requested a review from ihrpr May 31, 2025 12:41
@felixweinberger felixweinberger added the needs more work Not ready to be merged yet, needs additional changes. label Sep 5, 2025
Copy link
Contributor

@felixweinberger felixweinberger left a comment

Choose a reason for hiding this comment

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

Apologies for the delay in coming back to this - there seem to be some failing tests & merge conflicts, would you be able to resolve?

I'll prioritize responding quickly if you have the bandwidth to update the PR here.

@codecracker007 codecracker007 requested review from a team and felixweinberger September 6, 2025 08:23
@felixweinberger felixweinberger added needs sync Needs sync with latest main branch to ensure CI passes needs maintainer action Potentially serious issue - needs proactive fix and maintainer attention and removed needs more work Not ready to be merged yet, needs additional changes. labels Sep 29, 2025
@felixweinberger felixweinberger self-assigned this Sep 30, 2025
@felixweinberger felixweinberger linked an issue Oct 7, 2025 that may be closed by this pull request
Copy link
Contributor

@felixweinberger felixweinberger left a comment

Choose a reason for hiding this comment

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

Hi @codecracker007 thank you for this contribution.

I think this PR is heading in the right direction in terms of adding support for optional params on resources. However, to lower the maintenance burden as well as adhering to a "fail fast" principle, we shouldn't do the automatic retry logic when clients provide invalid optional params and fallback to the default.

If clients pass in invalid optional params, we should just fail like we would have without this retry logic.

return [TextContent(type="text", text=result)]


def use_defaults_on_optional_validation_error(
Copy link
Contributor

Choose a reason for hiding this comment

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

We should fail fast if an optional parameter was provided with a non-matching type.

I think we should completely remove this as well as the call to it in templates.py


if uri_params != func_params:
raise ValueError(
f"Mismatch between URI parameters {uri_params} and function parameters {func_params}"
Copy link
Contributor

Choose a reason for hiding this comment

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

note: removing this validation makes sense because we now support optional params.

validated_fn = validate_call(original_fn)

# Then, apply our decorator to handle default fallback for optional params
final_fn = use_defaults_on_optional_validation_error(validated_fn)
Copy link
Contributor

Choose a reason for hiding this comment

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

remove, we should fail fast and not silently when invalid params are provided by clients.


# self.fn is now multiply-decorated:
# 1. validate_call for coercion/validation
# 2. our new decorator for default fallback on optional param validation err
Copy link
Contributor

Choose a reason for hiding this comment

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

don't use a fallback, just fail if the optional params are supplied incorrectly

with pytest.raises(Exception): # Specific exception type may vary
await session.read_resource(AnyUrl("resource://users/123/invalid")) # Invalid template


Copy link
Contributor

Choose a reason for hiding this comment

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

thank you for adding significant test coverage - we'll want to update these to catch expected validation errors rather than the fallback logic.

) -> ResourceTemplate:
"""Create a template from a function."""
func_name = name or fn.__name__
original_fn = fn
Copy link
Contributor

Choose a reason for hiding this comment

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

original_fn, fn, final_fn - this gets quite confusing.

I think if we remove the use_defaults_on_optional_validation_error we shouldn't need any of these and can just stick to fn

@felixweinberger felixweinberger added needs more work Not ready to be merged yet, needs additional changes. and removed needs maintainer action Potentially serious issue - needs proactive fix and maintainer attention labels Oct 17, 2025
@Kludex
Copy link
Member

Kludex commented Oct 17, 2025

Don't we want something more FastAPI-ish? Like Annotated[type[T], Query()]? 🤔

@felixweinberger
Copy link
Contributor

felixweinberger commented Oct 17, 2025

Don't we want something more FastAPI-ish? Like Annotated[type[T], Query()]? 🤔

Something like this?

from typing import Annotated

@mcp.resource("search://{category}/{id}")
def search(
    category: Annotated[str, Path()],
    id: Annotated[int, Path()],
    format: Annotated[str, Query()] = "json",
    limit: Annotated[int, Query()] = 10
):
    ...

Note: there's also #1439 which is touching the same area so we'd need some coordination cc: @beaterblank

@Kludex
Copy link
Member

Kludex commented Oct 17, 2025

Don't we want something more FastAPI-ish? Like Annotated[type[T], Query()]? 🤔

Something like this?

from typing import Annotated

@mcp.resource("search://{category}/{id}")
def search(
    category: Annotated[str, Path()],
    id: Annotated[int, Path()],
    format: Annotated[str, Query()] = "json",
    limit: Annotated[int, Query()] = 10
):
    ...

Note: there's also #1439 which is touching the same area so we'd need some coordination cc: @beaterblank

For category and id, you wouldn't need it, but you could... Also, I think Query/Path would have the default parameter, so you don't need to add default in the parameters.

@felixweinberger
Copy link
Contributor

Don't we want something more FastAPI-ish? Like Annotated[type[T], Query()]? 🤔

Something like this?

from typing import Annotated

@mcp.resource("search://{category}/{id}")
def search(
    category: Annotated[str, Path()],
    id: Annotated[int, Path()],
    format: Annotated[str, Query()] = "json",
    limit: Annotated[int, Query()] = 10
):
    ...

Note: there's also #1439 which is touching the same area so we'd need some coordination cc: @beaterblank

For category and id, you wouldn't need it, but you could... Also, I think Query/Path would have the default parameter, so you don't need to add default in the parameters.

OK thanks, so more like this?

from typing import Annotated

@mcp.resource("search://{category}/{id}")
def search(
    category: str,
    id: int,
    format: Annotated[str, Query(default="json")],
    limit: Annotated[int, Query(default=10)],
):
    ...

@beaterblank
Copy link

beaterblank commented Oct 17, 2025

Don't we want something more FastAPI-ish? Like Annotated[type[T], Query()]? 🤔

Something like this?

from typing import Annotated

@mcp.resource("search://{category}/{id}")
def search(
    category: Annotated[str, Path()],
    id: Annotated[int, Path()],
    format: Annotated[str, Query()] = "json",
    limit: Annotated[int, Query()] = 10
):
    ...

Note: there's also #1439, which is touching the same area, so we'd need some coordination cc: @beaterblank

Yes, using Annotated sounds like a good approach, but we should ideally support both styles.

Also, a small nitpick, the classes should be named PathParameter and QueryParameter, since I spent a few minutes confused :) or maybe its best stick with how fast api does it.

I also noticed an issue in my PR: I’m currently inferring types only from the URI, but this should be done based on the function signature value's types instead. can reuse the code from this PR for that.

Additionally, since both features modify the same function in different ways, we should refactor the logic into smaller, more focused functions. That will make the codebase cleaner and easier to extend in the future.

At this point, I could either merge this PR into mine and resolve the conflicts properly, or do it the other way around.

merging this into #1439

beaterblank added a commit to beaterblank/python-sdk that referenced this pull request Oct 17, 2025
@beaterblank
Copy link

Can close this PR merged into #1439 .

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

needs more work Not ready to be merged yet, needs additional changes. needs sync Needs sync with latest main branch to ensure CI passes

Projects

None yet

Development

Successfully merging this pull request may close these issues.

FastMCP should support all of RFC 6570

5 participants