Skip to content

Conversation

@vl-kp
Copy link
Contributor

@vl-kp vl-kp commented Jul 20, 2025

In order to resolve issue #1193

@github-actions github-actions bot added documentation Updates to docs, examples, or guides. Primary change is documentation-related. tests labels Jul 20, 2025
@vl-kp vl-kp marked this pull request as ready for review July 20, 2025 15:01
@vl-kp vl-kp force-pushed the chore/add-dereference_json_schema branch from 907fa6d to 672ba7b Compare July 21, 2025 00:55
@richardkmichael
Copy link
Contributor

richardkmichael commented Jul 22, 2025

This would also solve the same problem with Claude Code, which I asked about on #1159.

This seems like a pragmatic solution.

@vl-kp Thank you for this! I'm wondering why leave $defs in the schema? Isn't it unused, so it seems unexpected and hence potentially confusing (implying a $ref ought to be present elsewhere). I'm not an expert on JSON Schema.

Although, it seems unfortunate to modify core to accommodate offending clients and lead to lot of configuration options.

So I wondered if it was possible using the new middleware feature. It's not at present, because initialization requests are not exposed to middleware. Also, client identification occurs only in the initialize request, not subsequent requests, so storage is needed on the session, and middleware needs the session. I've hacked that up for comment on #1215

@vl-kp
Copy link
Contributor Author

vl-kp commented Jul 22, 2025

I'm wondering why leave $defs in the schema? Isn't it unused, so it seems unexpected and hence potentially confusing (implying a $ref ought to be present elsewhere). I'm not an expert on JSON Schema.

There are some corner cases i am not sure it will happen or not:

  1. for self referencing, current code just directly return
  2. if circular referencing found, it will return the $ref without further processing
  3. for referencing more than the depth(default is 5), the last one we just return the $ref as it is
    so i decided to return $def as well to keep them compatible

@vl-kp vl-kp force-pushed the chore/add-dereference_json_schema branch from d995567 to d9c31a8 Compare July 22, 2025 06:29
@richardkmichael
Copy link
Contributor

I'm wondering why leave $defs in the schema? Isn't it unused, so it seems unexpected and hence potentially confusing (implying a $ref ought to be present elsewhere). I'm not an expert on JSON Schema.

There are some corner cases i am not sure it will happen or not:

1. self referencing, current code just directly return

Ok, if I understand, this is when $defs contains a $ref?

2. for referencing more than the depth(default is 5), the last one we just return the $ref as it is
   so i decided to return $def as well to keep them compatible

I see, thanks! I guess I would add $def only if max_depth is exceeded.

I forgot to ask about max_depth, why not process the entire schema? With multiple recursions, I guess performance? But it's hard to imagine, processing JSON is fast, and I would expect tool schemas to be small (enough). Do you have a schema is so large that with the multiple recursions, it is slow?

@jlowin
Copy link
Owner

jlowin commented Jul 22, 2025

Hi @vl-kp, I have some hesitation about introducing compromises because of client failures; obviously that's not a sustainable way for us to build great features, and folks opening subsequent issues will only deepen the rabbit hole. I do recognize the problem however and I'm intrigued by @richardkmichael's suggestion that MCP Middleware is an excellent way to solve this. I think it is a fantastic motivation for the work in #1215. Then instead of exposing this as a global configuration it could be bundled as e.g. DereferencingMiddleware (making that name up) and added to any server to perform this transformation on-demand.

@vl-kp
Copy link
Contributor Author

vl-kp commented Jul 22, 2025

I guess I would add $def only if max_depth is exceeded.

I agree, i updated to only add $def when corner case happens

I forgot to ask about max_depth, why not process the entire schema? With multiple recursions, I guess performance?

i did a perf test under my Apple M4 Pro(24GB), looks like it doesn't impact too much performance, so i updated to depth to 50, it should meet most of the use cases

Note: max_depth is set higher than schema depth to ensure full resolution
Depth Max Depth  Time (ms)    Fully Resolved  Schema Size 
--------------------------------------------------------------------------------
5            15         0.044        1               487         
25           35         0.096        1               2231        
50           60         0.173        1               4431        
100          110        0.384        1               8835        
200          210        0.692        1               18035       
300          310        0.971        1               27235       
400          410        1.485        1               36435       
500          510        1.546        1               45635       
--------------------------------------------------------------------------------
Summary: Tested depths from 5 to 500 with full resolution
All schemas fully resolved: ✅
Fastest: 0.044ms at depth 5
Slowest: 1.546ms at depth 500 (Python recursion limit)
Performance ratio: 35.5x slower for 100x deeper schema

@vl-kp vl-kp force-pushed the chore/add-dereference_json_schema branch from bf98669 to 906c7b6 Compare July 22, 2025 13:53
@vl-kp
Copy link
Contributor Author

vl-kp commented Jul 22, 2025

Hi @vl-kp, I have some hesitation about introducing compromises because of client failures; obviously that's not a sustainable way for us to build great features, and folks opening subsequent issues will only deepen the rabbit hole. I do recognize the problem however and I'm intrigued by @richardkmichael's suggestion that MCP Middleware is an excellent way to solve this. I think it is a fantastic motivation for the work in #1215. Then instead of exposing this as a global configuration it could be bundled as e.g. DereferencingMiddleware (making that name up) and added to any server to perform this transformation on-demand.

sure, we could wait for middleware feature implemented

@strawgate
Copy link
Collaborator

When I ran into this with Gemini I implemented https://pypi.org/project/jsonref/ with some luck

Might be worth considering vs hand rolling the dereferencing

https://github.com/strawgate/fastmcp-agents/blob/d1ea3d547efd4dd4e056a60d2605659322e95cab/fastmcp-agents-core/src/fastmcp_agents/core/completions/gemini.py#L477

@richardkmichael
Copy link
Contributor

@strawgate Thank you! (FastMCP Agents looks quite handy, glad you mentioned it.)

jsonref has a few perhaps notable issues, although none are my use-case. However, their existence speaks to occasional difficulties with specific schema, which I think makes more of a case for externalizing this from FastMCP. As opposed to an internal implementation, jsonref or hand-rolled, needing to be bug fixed in FastMCP. For example, I'm unsure if a wrapped function used by FastMCP could generate a circular schema.

@vl-kp Thanks for the benchmark. They are mostly < 1 ms, so it's not a concern for me. Note also, jsonref has no max-depth.

@jlowin
Copy link
Owner

jlowin commented Jul 29, 2025

I'm going to close this PR for now since I don't think this is strategically an approach we want to take, but can open a new PR for an alternative like middleware that would more user control

@vl-kp
Copy link
Contributor Author

vl-kp commented Aug 26, 2025

follow up PR #1641

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

Labels

documentation Updates to docs, examples, or guides. Primary change is documentation-related.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants