fix(model): preserve elicitation property order - #1150
Conversation
bd02a04 to
f85271d
Compare
|
@nightcityblade Thanks for addressing my comment. The no-local test fix looks good. Sorry to bring this up late, but I noticed the Could we fix the ordering within 3.x instead? I think we could keep pub struct ElicitationSchema {
// ...
pub property_order: Option<Vec<String>>,
}Let me know if this would work as a fix. |
f85271d to
ce1e18e
Compare
|
Thanks for the suggestion — the metadata approach works without changing the public I updated the PR so that:
Validation passed with |
a3663f4 to
c240e6f
Compare
|
Follow-up on the CI failures: the current head ( Local validation passed:
The refreshed fork workflows are currently awaiting GitHub Actions approval. |
| /// .build(); | ||
| /// ``` | ||
| #[derive(Debug, Clone, PartialEq, Serialize, Deserialize)] | ||
| #[derive(Debug, Clone, PartialEq, Serialize)] |
There was a problem hiding this comment.
Serialize is still derived for ElicitationSchema, so properties: BTreeMap<...> gets serialized alphabetically. That means the round-trip and proxy/relay parts of the issue are still unresolved. When property_order is available, serialization should follow it and then append any properties that are not listed.
There was a problem hiding this comment.
Thanks, you're right — deserialization captured the order, but derived serialization still sorted the BTreeMap.
The current head (297deb9) now converts through the private wire representation on serialization, rebuilding its IndexMap from property_order (and appending any newly added properties). The regression test asserts that a deserialize/serialize round trip preserves the exact property order.
Local validation passed:
cargo +nightly fmt --all -- --checkcargo test -p rmcp --all-features --lib model::elicitation_schema::tests— 27 passedcargo test -p rmcp --all-features --test test_elicitation— 56 passedcargo clippy -p rmcp --all-targets --all-features -- -D warnings
c240e6f to
297deb9
Compare
What Problem This Solves
ElicitationSchema.propertiesuses aBTreeMap, so deserializing a server-declared form sorts its fields alphabetically. Clients could not recover the presentation order from the typed model.Why This Change Was Made
The public
propertiesfield, constructor, and builder remainBTreeMap-based to avoid a breaking API change. Deserialization now uses an internalIndexMapwire representation to capture encountered property names in a new serde-skippedproperty_order: Option<Vec<String>>field.IndexMapis therefore not exposed in the public API.User Impact
Clients can render deserialized elicitation fields in server-declared order by consulting
property_order, while existing code usingpropertiesremains source-compatible within 3.x.Evidence
cargo test -p rmcp --all-features --lib test_legacy_enum_schema_roundtrip_preserves_enum_names— passedcargo +nightly fmt --all -- --check— passedFixes #1109