-
Notifications
You must be signed in to change notification settings - Fork 537
implementing SEP-2549 TTL for List Results #889
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: main
Are you sure you want to change the base?
Changes from all commits
f8edae7
c012157
1103cbe
169b43d
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -1142,6 +1142,34 @@ pub type ProgressNotification = Notification<ProgressNotificationMethod, Progres | |
|
|
||
| pub type Cursor = String; | ||
|
|
||
| /// Scope describing who may cache cacheable list/read results (SEP-2549). | ||
| /// | ||
| /// Defaults to [`CacheScope::Public`] when absent from the wire. | ||
| #[derive(Debug, Default, Clone, Copy, PartialEq, Eq, Serialize, Deserialize)] | ||
| #[cfg_attr(feature = "schemars", derive(schemars::JsonSchema))] | ||
| #[serde(rename_all = "lowercase")] | ||
| #[non_exhaustive] | ||
| pub enum CacheScope { | ||
| /// Any client or intermediary may cache and serve the response to any user. | ||
| #[default] | ||
| Public, | ||
| /// Only the requesting user's client may cache the response. | ||
| Private, | ||
| } | ||
|
|
||
| /// Normalize a `ttlMs` value during deserialization. | ||
| /// | ||
| /// Per SEP-2549, `ttlMs` MUST be `>= 0`; if a server returns a negative value, | ||
| /// clients SHOULD treat it as `0` (immediately stale). This tolerates that case | ||
| /// rather than erroring, while still accepting an absent field as `None`. | ||
| fn deserialize_ttl_ms<'de, D>(deserializer: D) -> Result<Option<u64>, D::Error> | ||
| where | ||
| D: serde::Deserializer<'de>, | ||
| { | ||
| let value = Option::<i64>::deserialize(deserializer)?; | ||
| Ok(value.map(|ttl_ms| ttl_ms.max(0) as u64)) | ||
| } | ||
|
|
||
| macro_rules! paginated_result { | ||
| ($t:ident { | ||
| $i_item: ident: $t_item: ty | ||
|
|
@@ -1151,23 +1179,45 @@ macro_rules! paginated_result { | |
| #[cfg_attr(feature = "schemars", derive(schemars::JsonSchema))] | ||
| #[expect(clippy::exhaustive_structs, reason = "intentionally exhaustive")] | ||
| pub struct $t { | ||
| #[serde(rename = "_meta", skip_serializing_if = "Option::is_none")] | ||
| #[serde(rename = "_meta", default, skip_serializing_if = "Option::is_none")] | ||
| pub meta: Option<Meta>, | ||
| #[serde(skip_serializing_if = "Option::is_none")] | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Why not add as new fields here and avoid the custom implementations of serialization/schema?
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. @jamadeo if we do this then rust api/bin compatibility breaks with consumers of it - the semver rules in ci here don't seem to allow that? basically this would make it a breaking change I believe.
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. You're right, and this has me wondering if we should do new spec support as a breaking change (major version bump). Let me open a discussion for this.
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. @michaelneale @jamadeo I think a major bump is inevitable here, not just for this change, but for the other breaking changes the new spec brings as well. 2026-07-28 is breaking by definition. So for changes targeting v2.0 like this, breaking API changes should be acceptable, and the Given that, could we drop the
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. done |
||
| #[serde(default, skip_serializing_if = "Option::is_none")] | ||
| pub next_cursor: Option<Cursor>, | ||
| /// Time, in milliseconds, that this result may be treated as fresh (SEP-2549). | ||
| #[serde( | ||
| default, | ||
| deserialize_with = "deserialize_ttl_ms", | ||
| skip_serializing_if = "Option::is_none" | ||
| )] | ||
| pub ttl_ms: Option<u64>, | ||
| /// Scope describing who may cache this result (SEP-2549). | ||
| #[serde(default, skip_serializing_if = "Option::is_none")] | ||
| pub cache_scope: Option<CacheScope>, | ||
| pub $i_item: $t_item, | ||
| } | ||
|
|
||
| impl $t { | ||
| pub fn with_all_items( | ||
| items: $t_item, | ||
| ) -> Self { | ||
| pub fn with_all_items(items: $t_item) -> Self { | ||
| Self { | ||
| meta: None, | ||
| next_cursor: None, | ||
| ttl_ms: None, | ||
| cache_scope: None, | ||
| $i_item: items, | ||
| } | ||
| } | ||
|
|
||
| /// Set the time, in milliseconds, that this result may be treated as fresh. | ||
| pub fn with_ttl_ms(mut self, ttl_ms: u64) -> Self { | ||
| self.ttl_ms = Some(ttl_ms); | ||
| self | ||
| } | ||
|
|
||
| /// Set the cache scope for this result. | ||
| pub fn with_cache_scope(mut self, cache_scope: CacheScope) -> Self { | ||
| self.cache_scope = Some(cache_scope); | ||
| self | ||
| } | ||
| } | ||
| }; | ||
| } | ||
|
|
@@ -1239,17 +1289,44 @@ pub type ReadResourceRequestParam = ReadResourceRequestParams; | |
|
|
||
| /// Result containing the contents of a read resource | ||
| #[derive(Debug, Serialize, Deserialize, Clone, PartialEq)] | ||
| #[serde(rename_all = "camelCase")] | ||
| #[cfg_attr(feature = "schemars", derive(schemars::JsonSchema))] | ||
| #[non_exhaustive] | ||
| pub struct ReadResourceResult { | ||
| /// Time, in milliseconds, that this result may be treated as fresh (SEP-2549). | ||
| #[serde( | ||
| default, | ||
| deserialize_with = "deserialize_ttl_ms", | ||
| skip_serializing_if = "Option::is_none" | ||
| )] | ||
| pub ttl_ms: Option<u64>, | ||
| /// Scope describing who may cache this result (SEP-2549). | ||
| #[serde(default, skip_serializing_if = "Option::is_none")] | ||
| pub cache_scope: Option<CacheScope>, | ||
| /// The actual content of the resource | ||
| pub contents: Vec<ResourceContents>, | ||
| } | ||
|
|
||
| impl ReadResourceResult { | ||
| /// Create a new ReadResourceResult with the given contents. | ||
| pub fn new(contents: Vec<ResourceContents>) -> Self { | ||
| Self { contents } | ||
| Self { | ||
| ttl_ms: None, | ||
| cache_scope: None, | ||
| contents, | ||
| } | ||
| } | ||
|
|
||
| /// Set the time, in milliseconds, that this result may be treated as fresh. | ||
| pub fn with_ttl_ms(mut self, ttl_ms: u64) -> Self { | ||
| self.ttl_ms = Some(ttl_ms); | ||
| self | ||
| } | ||
|
|
||
| /// Set the cache scope for this result. | ||
| pub fn with_cache_scope(mut self, cache_scope: CacheScope) -> Self { | ||
| self.cache_scope = Some(cache_scope); | ||
| self | ||
| } | ||
| } | ||
|
|
||
|
|
||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,78 @@ | ||
| use rmcp::model::{CacheScope, ListToolsResult, ReadResourceResult, ResourceContents}; | ||
| use serde_json::json; | ||
|
|
||
| #[test] | ||
| fn paginated_results_serialize_cache_hints_as_top_level_fields() { | ||
| let result = ListToolsResult::with_all_items(Vec::new()) | ||
| .with_ttl_ms(5_000) | ||
| .with_cache_scope(CacheScope::Private); | ||
|
|
||
| let actual = serde_json::to_value(result).expect("serialize list tools result"); | ||
|
|
||
| assert_eq!( | ||
| actual, | ||
| json!({ | ||
| "ttlMs": 5000, | ||
| "cacheScope": "private", | ||
| "tools": [] | ||
| }) | ||
| ); | ||
| assert!(actual.get("_meta").is_none()); | ||
| } | ||
|
|
||
| #[test] | ||
| fn read_resource_results_serialize_cache_hints_as_top_level_fields() { | ||
| let result = | ||
| ReadResourceResult::new(vec![ResourceContents::text("hello", "file:///example.txt")]) | ||
| .with_ttl_ms(10_000) | ||
| .with_cache_scope(CacheScope::Public); | ||
|
|
||
| let actual = serde_json::to_value(result).expect("serialize read resource result"); | ||
|
|
||
| assert_eq!(actual["ttlMs"], 10000); | ||
| assert_eq!(actual["cacheScope"], "public"); | ||
| assert!(actual["contents"][0].get("_meta").is_none()); | ||
| } | ||
|
|
||
| #[test] | ||
| fn cache_hints_are_omitted_when_absent() { | ||
| let result = ListToolsResult::with_all_items(Vec::new()); | ||
| let actual = serde_json::to_value(result).expect("serialize list tools result"); | ||
|
|
||
| assert_eq!(actual, json!({ "tools": [] })); | ||
| } | ||
|
|
||
| #[test] | ||
| fn cache_hints_default_to_none_and_negative_ttl_is_normalized_to_zero() { | ||
| let absent: ListToolsResult = serde_json::from_value(json!({ | ||
| "tools": [] | ||
| })) | ||
| .expect("deserialize result without ttlMs"); | ||
| assert_eq!(absent.ttl_ms, None); | ||
| assert_eq!(absent.cache_scope, None); | ||
|
|
||
| let negative: ReadResourceResult = serde_json::from_value(json!({ | ||
| "ttlMs": -42, | ||
| "cacheScope": "private", | ||
| "contents": [] | ||
| })) | ||
| .expect("deserialize result with negative ttlMs"); | ||
| assert_eq!(negative.ttl_ms, Some(0)); | ||
| assert_eq!(negative.cache_scope, Some(CacheScope::Private)); | ||
| } | ||
|
|
||
| #[test] | ||
| fn cache_scope_round_trips() { | ||
| assert_eq!( | ||
| serde_json::to_value(CacheScope::Public).unwrap(), | ||
| json!("public") | ||
| ); | ||
| assert_eq!( | ||
| serde_json::to_value(CacheScope::Private).unwrap(), | ||
| json!("private") | ||
| ); | ||
| assert_eq!( | ||
| serde_json::from_value::<CacheScope>(json!("private")).unwrap(), | ||
| CacheScope::Private | ||
| ); | ||
| } |
Uh oh!
There was an error while loading. Please reload this page.