diff --git a/.changesets/fix_openai_incompatibility.md b/.changesets/fix_openai_incompatibility.md new file mode 100644 index 00000000..d5e37298 --- /dev/null +++ b/.changesets/fix_openai_incompatibility.md @@ -0,0 +1,5 @@ +### fix: generate openAI-compatible json schemas for list types - @DaleSeo PR #272 + +The MCP server is generating JSON schemas that don't match OpenAI's function calling specification. It puts `oneOf` at the array level instead of using `items` to define the JSON schemas for the GraphQL list types. While some other LLMs are more flexible about this, it technically violates the [JSON Schema specification](https://json-schema.org/understanding-json-schema/reference/array) that OpenAI strictly follows. + +This PR updates the list type handling logic to move `oneOf` inside `items` for GraphQL list types. diff --git a/crates/apollo-mcp-server/src/operations.rs b/crates/apollo-mcp-server/src/operations.rs index 736e372f..7cec37e0 100644 --- a/crates/apollo-mcp-server/src/operations.rs +++ b/crates/apollo-mcp-server/src/operations.rs @@ -1044,25 +1044,40 @@ fn type_to_schema( custom_scalar_map, definitions, ); + let items_schema = if list_type.is_non_null() { + inner_type_schema + } else { + schema_factory( + None, + None, + None, + None, + Some(SubschemaValidation { + one_of: Some(vec![ + inner_type_schema, + Schema::Object(SchemaObject { + instance_type: Some(SingleOrVec::Single(Box::new( + InstanceType::Null, + ))), + ..Default::default() + }), + ]), + ..Default::default() + }), + None, + ) + }; + schema_factory( None, Some(InstanceType::Array), None, - list_type.is_non_null().then(|| ArrayValidation { - items: Some(SingleOrVec::Single(Box::new(inner_type_schema.clone()))), - ..Default::default() - }), - (!list_type.is_non_null()).then(|| SubschemaValidation { - one_of: Some(vec![ - inner_type_schema, - Schema::Object(SchemaObject { - instance_type: Some(SingleOrVec::Single(Box::new(InstanceType::Null))), - ..Default::default() - }), - ]), + Some(ArrayValidation { + items: Some(SingleOrVec::Single(Box::new(items_schema))), ..Default::default() }), None, + None, ) } } @@ -1272,7 +1287,7 @@ mod tests { .unwrap() .unwrap(); - insta::assert_debug_snapshot!(operation, @r###" + insta::assert_debug_snapshot!(operation, @r#" Operation { tool: Tool { name: "MutationName", @@ -1304,7 +1319,7 @@ mod tests { }, operation_name: "MutationName", } - "###); + "#); } #[test] @@ -1326,7 +1341,7 @@ mod tests { .unwrap() .unwrap(); - insta::assert_debug_snapshot!(operation, @r###" + insta::assert_debug_snapshot!(operation, @r#" Operation { tool: Tool { name: "MutationName", @@ -1358,7 +1373,7 @@ mod tests { }, operation_name: "MutationName", } - "###); + "#); } #[test] @@ -1381,7 +1396,7 @@ mod tests { .unwrap(); let tool = Tool::from(operation); - insta::assert_debug_snapshot!(tool, @r###" + insta::assert_debug_snapshot!(tool, @r#" Tool { name: "QueryName", description: Some( @@ -1403,7 +1418,7 @@ mod tests { }, ), } - "###); + "#); insta::assert_snapshot!(serde_json::to_string_pretty(&serde_json::json!(tool.input_schema)).unwrap(), @r#" { "type": "object", @@ -1432,7 +1447,7 @@ mod tests { .unwrap(); let tool = Tool::from(operation); - insta::assert_debug_snapshot!(tool, @r###" + insta::assert_debug_snapshot!(tool, @r#" Tool { name: "QueryName", description: Some( @@ -1458,8 +1473,8 @@ mod tests { }, ), } - "###); - insta::assert_snapshot!(serde_json::to_string_pretty(&serde_json::json!(tool.input_schema)).unwrap(), @r###" + "#); + insta::assert_snapshot!(serde_json::to_string_pretty(&serde_json::json!(tool.input_schema)).unwrap(), @r#" { "type": "object", "properties": { @@ -1468,7 +1483,7 @@ mod tests { } } } - "###); + "#); } #[test] @@ -1491,7 +1506,7 @@ mod tests { .unwrap(); let tool = Tool::from(operation); - insta::assert_debug_snapshot!(tool, @r###" + insta::assert_debug_snapshot!(tool, @r#" Tool { name: "QueryName", description: Some( @@ -1520,8 +1535,8 @@ mod tests { }, ), } - "###); - insta::assert_snapshot!(serde_json::to_string_pretty(&serde_json::json!(tool.input_schema)).unwrap(), @r###" + "#); + insta::assert_snapshot!(serde_json::to_string_pretty(&serde_json::json!(tool.input_schema)).unwrap(), @r#" { "type": "object", "required": [ @@ -1533,7 +1548,7 @@ mod tests { } } } - "###); + "#); } #[test] @@ -1556,7 +1571,7 @@ mod tests { .unwrap(); let tool = Tool::from(operation); - insta::assert_debug_snapshot!(tool, @r###" + insta::assert_debug_snapshot!(tool, @r#" Tool { name: "QueryName", description: Some( @@ -1570,14 +1585,16 @@ mod tests { "properties": Object { "id": Object { "type": String("array"), - "oneOf": Array [ - Object { - "type": String("string"), - }, - Object { - "type": String("null"), - }, - ], + "items": Object { + "oneOf": Array [ + Object { + "type": String("string"), + }, + Object { + "type": String("null"), + }, + ], + }, }, }, }, @@ -1593,8 +1610,8 @@ mod tests { }, ), } - "###); - insta::assert_snapshot!(serde_json::to_string_pretty(&serde_json::json!(tool.input_schema)).unwrap(), @r###" + "#); + insta::assert_snapshot!(serde_json::to_string_pretty(&serde_json::json!(tool.input_schema)).unwrap(), @r#" { "type": "object", "required": [ @@ -1603,18 +1620,20 @@ mod tests { "properties": { "id": { "type": "array", - "oneOf": [ - { - "type": "string" - }, - { - "type": "null" - } - ] + "items": { + "oneOf": [ + { + "type": "string" + }, + { + "type": "null" + } + ] + } } } } - "###); + "#); } #[test] @@ -1637,7 +1656,7 @@ mod tests { .unwrap(); let tool = Tool::from(operation); - insta::assert_debug_snapshot!(tool, @r###" + insta::assert_debug_snapshot!(tool, @r#" Tool { name: "QueryName", description: Some( @@ -1669,8 +1688,8 @@ mod tests { }, ), } - "###); - insta::assert_snapshot!(serde_json::to_string_pretty(&serde_json::json!(tool.input_schema)).unwrap(), @r###" + "#); + insta::assert_snapshot!(serde_json::to_string_pretty(&serde_json::json!(tool.input_schema)).unwrap(), @r#" { "type": "object", "required": [ @@ -1685,7 +1704,7 @@ mod tests { } } } - "###); + "#); } #[test] @@ -1708,7 +1727,7 @@ mod tests { .unwrap(); let tool = Tool::from(operation); - insta::assert_debug_snapshot!(tool, @r###" + insta::assert_debug_snapshot!(tool, @r#" Tool { name: "QueryName", description: Some( @@ -1719,14 +1738,16 @@ mod tests { "properties": Object { "id": Object { "type": String("array"), - "oneOf": Array [ - Object { - "type": String("string"), - }, - Object { - "type": String("null"), - }, - ], + "items": Object { + "oneOf": Array [ + Object { + "type": String("string"), + }, + Object { + "type": String("null"), + }, + ], + }, }, }, }, @@ -1742,25 +1763,27 @@ mod tests { }, ), } - "###); - insta::assert_snapshot!(serde_json::to_string_pretty(&serde_json::json!(tool.input_schema)).unwrap(), @r###" + "#); + insta::assert_snapshot!(serde_json::to_string_pretty(&serde_json::json!(tool.input_schema)).unwrap(), @r#" { "type": "object", "properties": { "id": { "type": "array", - "oneOf": [ - { - "type": "string" - }, - { - "type": "null" - } - ] + "items": { + "oneOf": [ + { + "type": "string" + }, + { + "type": "null" + } + ] + } } } } - "###); + "#); } #[test] @@ -1783,7 +1806,7 @@ mod tests { .unwrap(); let tool = Tool::from(operation); - insta::assert_debug_snapshot!(tool, @r###" + insta::assert_debug_snapshot!(tool, @r#" Tool { name: "QueryName", description: Some( @@ -1812,8 +1835,8 @@ mod tests { }, ), } - "###); - insta::assert_snapshot!(serde_json::to_string_pretty(&serde_json::json!(tool.input_schema)).unwrap(), @r###" + "#); + insta::assert_snapshot!(serde_json::to_string_pretty(&serde_json::json!(tool.input_schema)).unwrap(), @r#" { "type": "object", "properties": { @@ -1825,7 +1848,7 @@ mod tests { } } } - "###); + "#); } #[test] @@ -1848,7 +1871,7 @@ mod tests { .unwrap(); let tool = Tool::from(operation); - insta::assert_debug_snapshot!(tool, @r###" + insta::assert_debug_snapshot!(tool, @r#" Tool { name: "QueryName", description: Some( @@ -1859,22 +1882,26 @@ mod tests { "properties": Object { "id": Object { "type": String("array"), - "oneOf": Array [ - Object { - "type": String("array"), - "oneOf": Array [ - Object { - "type": String("string"), - }, - Object { - "type": String("null"), + "items": Object { + "oneOf": Array [ + Object { + "type": String("array"), + "items": Object { + "oneOf": Array [ + Object { + "type": String("string"), + }, + Object { + "type": String("null"), + }, + ], }, - ], - }, - Object { - "type": String("null"), - }, - ], + }, + Object { + "type": String("null"), + }, + ], + }, }, }, }, @@ -1890,33 +1917,37 @@ mod tests { }, ), } - "###); - insta::assert_snapshot!(serde_json::to_string_pretty(&serde_json::json!(tool.input_schema)).unwrap(), @r###" + "#); + insta::assert_snapshot!(serde_json::to_string_pretty(&serde_json::json!(tool.input_schema)).unwrap(), @r#" { "type": "object", "properties": { "id": { "type": "array", - "oneOf": [ - { - "type": "array", - "oneOf": [ - { - "type": "string" - }, - { - "type": "null" + "items": { + "oneOf": [ + { + "type": "array", + "items": { + "oneOf": [ + { + "type": "string" + }, + { + "type": "null" + } + ] } - ] - }, - { - "type": "null" - } - ] + }, + { + "type": "null" + } + ] + } } } } - "###); + "#); } #[test] @@ -1939,7 +1970,7 @@ mod tests { .unwrap(); let tool = Tool::from(operation); - insta::assert_debug_snapshot!(tool, @r###" + insta::assert_debug_snapshot!(tool, @r##" Tool { name: "QueryName", description: Some( @@ -1983,7 +2014,7 @@ mod tests { }, ), } - "###); + "##); } #[test] @@ -2006,7 +2037,7 @@ mod tests { .unwrap(); let tool = Tool::from(operation); - insta::assert_debug_snapshot!(tool, @r###" + insta::assert_debug_snapshot!(tool, @r##" Tool { name: "QueryName", description: Some( @@ -2045,7 +2076,7 @@ mod tests { }, ), } - "###); + "##); } #[test] @@ -2064,7 +2095,7 @@ mod tests { false, false, ); - insta::assert_debug_snapshot!(operation, @r###" + insta::assert_debug_snapshot!(operation, @r#" Err( TooManyOperations { source_path: Some( @@ -2073,7 +2104,7 @@ mod tests { count: 2, }, ) - "###); + "#); } #[test] @@ -2123,7 +2154,7 @@ mod tests { false, false, ); - insta::assert_debug_snapshot!(operation, @r###" + insta::assert_debug_snapshot!(operation, @r#" Err( NoOperations { source_path: Some( @@ -2131,7 +2162,7 @@ mod tests { ), }, ) - "###); + "#); } #[test] @@ -2150,13 +2181,13 @@ mod tests { false, false, ); - insta::assert_debug_snapshot!(operation, @r###" + insta::assert_debug_snapshot!(operation, @r" Err( NoOperations { source_path: None, }, ) - "###); + "); } #[test] @@ -2190,7 +2221,7 @@ mod tests { .ok_or("Expected warning about unknown type in logs".to_string()) }); - insta::assert_debug_snapshot!(tool, @r###" + insta::assert_debug_snapshot!(tool, @r#" Tool { name: "QueryName", description: Some( @@ -2214,7 +2245,7 @@ mod tests { }, ), } - "###); + "#); } #[test] @@ -2248,7 +2279,7 @@ mod tests { .ok_or("Expected warning about custom scalar without map in logs".to_string()) }); - insta::assert_debug_snapshot!(tool, @r###" + insta::assert_debug_snapshot!(tool, @r##" Tool { name: "QueryName", description: Some( @@ -2279,7 +2310,7 @@ mod tests { }, ), } - "###); + "##); } #[test] @@ -2317,7 +2348,7 @@ mod tests { .ok_or("Expected warning about custom scalar missing in logs".to_string()) }); - insta::assert_debug_snapshot!(tool, @r###" + insta::assert_debug_snapshot!(tool, @r##" Tool { name: "QueryName", description: Some( @@ -2348,7 +2379,7 @@ mod tests { }, ), } - "###); + "##); } #[test] @@ -2374,7 +2405,7 @@ mod tests { .unwrap(); let tool = Tool::from(operation); - insta::assert_debug_snapshot!(tool, @r###" + insta::assert_debug_snapshot!(tool, @r##" Tool { name: "QueryName", description: Some( @@ -2406,7 +2437,7 @@ mod tests { }, ), } - "###); + "##); } #[test] @@ -2557,7 +2588,7 @@ mod tests { insta::assert_snapshot!( operation.tool.description.unwrap(), - @r###" + @r#" Get a list of A The returned value is an array of type `A` --- @@ -2609,7 +2640,7 @@ mod tests { type Z { zzz: Int } - "### + "# ); } @@ -2644,7 +2675,7 @@ mod tests { insta::assert_snapshot!( operation.tool.description.unwrap(), - @r###"Overridden tool #description"### + @"Overridden tool #description" ); } @@ -2677,7 +2708,7 @@ mod tests { insta::assert_snapshot!( operation.tool.description.unwrap(), - @r###"The returned value is optional and has type `String`"### + @"The returned value is optional and has type `String`" ); } @@ -2702,11 +2733,11 @@ mod tests { insta::assert_snapshot!( operation.tool.description.unwrap(), - @r###" - The returned value is optional and has type `String` - --- - The returned value is optional and has type `RealEnum` - "### + @r" + The returned value is optional and has type `String` + --- + The returned value is optional and has type `RealEnum` + " ); } @@ -2731,15 +2762,16 @@ mod tests { insta::assert_snapshot!( operation.tool.description.unwrap(), - @r###" - """the description for the enum""" - enum RealEnum { - """ENUM_VALUE_1 is a value""" - ENUM_VALUE_1 - """ENUM_VALUE_2 is a value""" - ENUM_VALUE_2 - } - "### + @r#" + --- + """the description for the enum""" + enum RealEnum { + """ENUM_VALUE_1 is a value""" + ENUM_VALUE_1 + """ENUM_VALUE_2 is a value""" + ENUM_VALUE_2 + } + "# ); } @@ -2764,7 +2796,7 @@ mod tests { insta::assert_snapshot!( operation.tool.description.unwrap(), - @r###""### + @"" ); } @@ -2811,7 +2843,7 @@ mod tests { .unwrap() .unwrap(); - insta::assert_debug_snapshot!(operation.tool, @r###" + insta::assert_debug_snapshot!(operation.tool, @r##" Tool { name: "Test", description: Some( @@ -2854,7 +2886,7 @@ mod tests { }, ), } - "###); + "##); } #[test] @@ -2880,7 +2912,7 @@ mod tests { .unwrap(); let tool = Tool::from(operation); - insta::assert_debug_snapshot!(tool, @r###" + insta::assert_debug_snapshot!(tool, @r#" Tool { name: "QueryName", description: Some( @@ -2906,7 +2938,7 @@ mod tests { }, ), } - "###); + "#); } #[test] @@ -2930,7 +2962,7 @@ mod tests { .unwrap(); let tool = Tool::from(operation); - insta::assert_snapshot!(serde_json::to_string_pretty(&serde_json::json!(tool.input_schema)).unwrap(), @r###" + insta::assert_snapshot!(serde_json::to_string_pretty(&serde_json::json!(tool.input_schema)).unwrap(), @r#" { "type": "object", "properties": { @@ -2940,7 +2972,7 @@ mod tests { } } } - "###); + "#); } #[test] @@ -3000,7 +3032,7 @@ mod tests { .unwrap(); let tool = Tool::from(operation); - insta::assert_snapshot!(serde_json::to_string_pretty(&serde_json::json!(tool.input_schema)).unwrap(), @r###" + insta::assert_snapshot!(serde_json::to_string_pretty(&serde_json::json!(tool.input_schema)).unwrap(), @r#" { "type": "object", "properties": { @@ -3014,7 +3046,7 @@ mod tests { } } } - "###); + "#); } #[test] @@ -3076,7 +3108,7 @@ mod tests { .unwrap(); let tool = Tool::from(operation); - insta::assert_snapshot!(serde_json::to_string_pretty(&serde_json::json!(tool.input_schema)).unwrap(), @r###" + insta::assert_snapshot!(serde_json::to_string_pretty(&serde_json::json!(tool.input_schema)).unwrap(), @r#" { "type": "object", "properties": { @@ -3086,7 +3118,7 @@ mod tests { } } } - "###); + "#); } #[test] @@ -3109,7 +3141,7 @@ mod tests { .unwrap(); let tool = Tool::from(operation); - insta::assert_snapshot!(serde_json::to_string_pretty(&serde_json::json!(tool.input_schema)).unwrap(), @r###" + insta::assert_snapshot!(serde_json::to_string_pretty(&serde_json::json!(tool.input_schema)).unwrap(), @r#" { "type": "object", "properties": { @@ -3119,7 +3151,7 @@ mod tests { } } } - "###); + "#); } #[test] @@ -3142,7 +3174,7 @@ mod tests { .unwrap(); let tool = Tool::from(operation); - insta::assert_snapshot!(serde_json::to_string_pretty(&serde_json::json!(tool.input_schema)).unwrap(), @r###" + insta::assert_snapshot!(serde_json::to_string_pretty(&serde_json::json!(tool.input_schema)).unwrap(), @r#" { "type": "object", "properties": { @@ -3152,7 +3184,7 @@ mod tests { } } } - "###); + "#); } #[test] @@ -3175,7 +3207,7 @@ mod tests { .unwrap(); let tool = Tool::from(operation); - insta::assert_snapshot!(serde_json::to_string_pretty(&serde_json::json!(tool.input_schema)).unwrap(), @r###" + insta::assert_snapshot!(serde_json::to_string_pretty(&serde_json::json!(tool.input_schema)).unwrap(), @r#" { "type": "object", "properties": { @@ -3189,7 +3221,7 @@ mod tests { } } } - "###); + "#); } #[test] @@ -3240,7 +3272,7 @@ mod tests { .unwrap(); let tool = Tool::from(operation); - insta::assert_snapshot!(serde_json::to_string_pretty(&serde_json::json!(tool.input_schema)).unwrap(), @r###" + insta::assert_snapshot!(serde_json::to_string_pretty(&serde_json::json!(tool.input_schema)).unwrap(), @r#" { "type": "object", "properties": { @@ -3254,7 +3286,7 @@ mod tests { } } } - "###); + "#); } #[test] @@ -3277,11 +3309,233 @@ mod tests { .unwrap(); let tool = Tool::from(operation); - insta::assert_snapshot!(serde_json::to_string_pretty(&serde_json::json!(tool.input_schema)).unwrap(), @r###" + insta::assert_snapshot!(serde_json::to_string_pretty(&serde_json::json!(tool.input_schema)).unwrap(), @r#" { "type": "object", "properties": {} } - "###); + "#); + } + + #[test] + fn nullable_list_of_nullable_input_objects() { + let operation = Operation::from_document( + RawOperation { + source_text: "query QueryName($objects: [RealInputObject]) { id }".to_string(), + persisted_query_id: None, + headers: None, + variables: None, + source_path: None, + }, + &SCHEMA, + None, + MutationMode::None, + false, + false, + ) + .unwrap() + .unwrap(); + let tool = Tool::from(operation); + + insta::assert_debug_snapshot!(tool, @r##" + Tool { + name: "QueryName", + description: Some( + "The returned value is optional and has type `String`", + ), + input_schema: { + "type": String("object"), + "properties": Object { + "objects": Object { + "type": String("array"), + "items": Object { + "oneOf": Array [ + Object { + "$ref": String("#/definitions/RealInputObject"), + }, + Object { + "type": String("null"), + }, + ], + }, + }, + }, + "definitions": Object { + "RealInputObject": Object { + "type": String("object"), + "required": Array [ + String("required"), + ], + "properties": Object { + "optional": Object { + "description": String("optional is a input field that is optional"), + "type": String("string"), + }, + "required": Object { + "description": String("required is a input field that is required"), + "type": String("string"), + }, + }, + }, + }, + }, + annotations: Some( + ToolAnnotations { + title: None, + read_only_hint: Some( + true, + ), + destructive_hint: None, + idempotent_hint: None, + open_world_hint: None, + }, + ), + } + "##); + insta::assert_snapshot!(serde_json::to_string_pretty(&serde_json::json!(tool.input_schema)).unwrap(), @r##" + { + "type": "object", + "properties": { + "objects": { + "type": "array", + "items": { + "oneOf": [ + { + "$ref": "#/definitions/RealInputObject" + }, + { + "type": "null" + } + ] + } + } + }, + "definitions": { + "RealInputObject": { + "type": "object", + "required": [ + "required" + ], + "properties": { + "optional": { + "description": "optional is a input field that is optional", + "type": "string" + }, + "required": { + "description": "required is a input field that is required", + "type": "string" + } + } + } + } + } + "##); + } + + #[test] + fn non_nullable_list_of_non_nullable_input_objects() { + let operation = Operation::from_document( + RawOperation { + source_text: "query QueryName($objects: [RealInputObject!]!) { id }".to_string(), + persisted_query_id: None, + headers: None, + variables: None, + source_path: None, + }, + &SCHEMA, + None, + MutationMode::None, + false, + false, + ) + .unwrap() + .unwrap(); + let tool = Tool::from(operation); + + insta::assert_debug_snapshot!(tool, @r##" + Tool { + name: "QueryName", + description: Some( + "The returned value is optional and has type `String`", + ), + input_schema: { + "type": String("object"), + "required": Array [ + String("objects"), + ], + "properties": Object { + "objects": Object { + "type": String("array"), + "items": Object { + "$ref": String("#/definitions/RealInputObject"), + }, + }, + }, + "definitions": Object { + "RealInputObject": Object { + "type": String("object"), + "required": Array [ + String("required"), + ], + "properties": Object { + "optional": Object { + "description": String("optional is a input field that is optional"), + "type": String("string"), + }, + "required": Object { + "description": String("required is a input field that is required"), + "type": String("string"), + }, + }, + }, + }, + }, + annotations: Some( + ToolAnnotations { + title: None, + read_only_hint: Some( + true, + ), + destructive_hint: None, + idempotent_hint: None, + open_world_hint: None, + }, + ), + } + "##); + insta::assert_snapshot!(serde_json::to_string_pretty(&serde_json::json!(tool.input_schema)).unwrap(), @r##" + { + "type": "object", + "required": [ + "objects" + ], + "properties": { + "objects": { + "type": "array", + "items": { + "$ref": "#/definitions/RealInputObject" + } + } + }, + "definitions": { + "RealInputObject": { + "type": "object", + "required": [ + "required" + ], + "properties": { + "optional": { + "description": "optional is a input field that is optional", + "type": "string" + }, + "required": { + "description": "required is a input field that is required", + "type": "string" + } + } + } + } + } + "##); } }