oxidecomputer / oxidecomputer/typify

Confusing interaction between additionalProperties and optional fields.

Open
#779 1 comment 1 reaction 0 assignees View on GitHub

Nobody has claimed this yet.

Dominant language
Rust
Stars
898
Forks
114
Avg merge
4h 18m
Merged PRs (30d)
14

Description

given this schema:
https://github.com/modelcontextprotocol/specification/blob/main/schema/2024-11-05/schema.json#L144

{
    "$schema": "http://json-schema.org/draft-07/schema#",
    "definitions": {
"ClientCapabilities": {
            "description": "Capabilities a client may support. Known capabilities are defined here, in this schema, but this is not a closed set: any client can define its own, additional capabilities.",
            "properties": {
                "experimental": {
                    "additionalProperties": {
                        "additionalProperties": true,
                        "properties": {},
                        "type": "object"
                    },
                    "description": "Experimental, non-standard capabilities that the client supports.",
                    "type": "object"
                },
                "roots": {
                    "description": "Present if the client supports listing roots.",
                    "properties": {
                        "listChanged": {
                            "description": "Whether the client supports notifications for changes to the roots list.",
                            "type": "boolean"
                        }
                    },
                    "type": "object"
                },
                "sampling": {
                    "additionalProperties": true,
                    "description": "Present if the client supports sampling from an LLM.",
                    "properties": {},
                    "type": "object"
                }
            },
            "type": "object"
        }
}
}

My understanding (I could be incorrect) is that all three properties should be optional, as none are specified as required. However, typify produces

pub struct ClientCapabilities {
    #[doc = "Experimental, non-standard capabilities that the client supports."]
    #[serde(default, skip_serializing_if = "std::collections::HashMap::is_empty")]
    pub experimental: std::collections::HashMap<String, serde_json::Map<String, serde_json::Value>>,
    #[serde(default, skip_serializing_if = "Option::is_none")]
    pub roots: Option<ClientCapabilitiesRoots>,
    #[doc = "Present if the client supports sampling from an LLM."]
    #[serde(default, skip_serializing_if = "serde_json::Map::is_empty")]
    pub sampling: serde_json::Map<String, serde_json::Value>,
}

On the serialization side, everything looks good, since these fields will not be present if empty. On the deserialization side, there's some unintended behavior, since the human-language specification seems to intend the presence of the sampling field to have meaning.

If Option<Map> becomes Map is a deliberate design choice, I could probably get away with using a custom typify::TypeSpaceSettings::with_map_type, but I figured I would ask here to be sure.

Contributor guide

No contributing guide indexed for this repository

First steps

  1. Read the whole issue, then the project's contributing guide.
  2. Comment on the issue to say you are picking it up — it saves two people doing the same work.
  3. Fork the repository and make your change on a branch.
  4. Open a pull request that references the issue number.

Research direction

Start with the ClientCapabilities schema at schema/2024-11-05/schema.json#L144 and the generated Rust type shown in the issue. Trace how additionalProperties and absent required fields are mapped during type generation, then compare that behavior with TypeSpaceSettings::with_map_type. Done means the project has a decided, tested behavior for preserving or intentionally collapsing the presence semantics of sampling and experimental.

Written by the indexing model from the issue text.

Assessment

Tech stack
rust
Domain
compilers, devtools
Issue type
Bug
Difficulty
4/5
Estimated time
3-5 days
Activity status
Stale
Clarity
Mostly clear
Newbie friendliness
42/100

Get new issues in your inbox

A short digest of beginner-friendly GitHub issues.