Repository navigation
Conversation
Transports need to add their own keys to the context a user-provided extractor built, for instance the decoded Mcp-Param-* headers: - the provided keys take precedence over the user-extracted ones - neither the user's context nor the map is modified - custom McpTransportContext implementations keep working unchanged Contexts created via McpTransportContext.create are merged into a single map. Other implementations are wrapped once, and further merges combine into a single wrapper rather than adding layers. Signed-off-by: Dariusz Jedrzejczyk <2554306+chemicL@users.noreply.github.com>
There was a problem hiding this comment.
I like the simpler shape of this. However, this raises the question of context equality.
If the keys and values are the same, the contexts should probably be the same, shouldn't they?
The DefaultMcpTransportContext equality we have is dubious too, but it was the only implementation we provided, and now we're compounding problems.
| if (map.isEmpty()) { | ||
| return this; | ||
| } | ||
| return new OverlayMcpTransportContext(this, map); |
There was a problem hiding this comment.
we should not create an overlay when the context is empty
| if (map.isEmpty()) { | |
| return this; | |
| } | |
| return new OverlayMcpTransportContext(this, map); | |
| if (map.isEmpty()) { | |
| return this; | |
| } | |
| if (this == McpTransportContext.EMPTY) { | |
| return McpTransportContext.create(map); | |
| } | |
| return new OverlayMcpTransportContext(this, map); |
There was a problem hiding this comment.
Actually, this wouldn't have any effect. McpTransportContext.EMPTY is a DefaultMcpTransportContext so it means that the overridden method would be called. I'll add a check there for this.metadata.isEmpty().
|
|
||
| OverlayMcpTransportContext that = (OverlayMcpTransportContext) o; | ||
| return this.base.equals(that.base) && this.overlay.equals(that.overlay); | ||
| } |
There was a problem hiding this comment.
This is probably incorrect.
With this:
var one = McpTransportContext.EMPTY.putAll(Map.of("foo", "one", "bar", "two"));
var two = McpTransportContext.create(Map.of("foo", "one")).putAll(Map.of("bar", "two"));
one.equals(two); // -> falsebut both context have the same values.
There was a problem hiding this comment.
We never claimed anything about context equality. As discussed, the equality implementations are now gone. The McpTransportContext contract is to extract values for known keys and that is how users can interact with the context. So the tests are also modified now.
Comparing transport contexts was never a goal, and with putAll a context can be layered over the one a user's extractor returns. Tests now check the entries a context holds rather than context equality. Signed-off-by: Dariusz Jedrzejczyk <2554306+chemicL@users.noreply.github.com>
Signed-off-by: Dariusz Jedrzejczyk <2554306+chemicL@users.noreply.github.com>
Transports need to add their own keys to the context a user-provided extractor built, for instance the decoded
Mcp-Param-*headers:McpTransportContextimplementations keep working unchangedContexts created via
McpTransportContext.createare merged into a single map. Other implementations are wrapped once, and further merges combine into a single wrapper rather than adding layers.This is an alternative implementation to #1285