Skip to content

Remove the last of the panics for custom scalars #26

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.

Sign up for GitHub

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

Merged
merged 7 commits into from
Apr 25, 2025
Merged

Remove the last of the panics for custom scalars #26

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
7 commits
Select commit Hold shift + click to select a range
d7ec759
if a custom scalar isn’t defined, allow any typue
Jephuff Apr 25, 2025
da3863c
remove last panic for unknown type
Jephuff Apr 25, 2025
c674d25
lint panics
Jephuff Apr 25, 2025
67fec2b
run clippy in vscode
Jephuff Apr 25, 2025
9b3d528
use factory for all of them
Jephuff Apr 25, 2025
12499bc
run format on save
Jephuff Apr 25, 2025
d630c55
run format
Jephuff Apr 25, 2025
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
7 changes: 7 additions & 0 deletions .vscode/settings.json
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,7 @@
{
"rust-analyzer.check.command": "clippy",
"[rust]": {
"editor.defaultFormatter": "rust-lang.rust-analyzer",
"editor.formatOnSave": true,
}
}
1 change: 1 addition & 0 deletions Cargo.toml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -19,3 +19,4 @@ exit = "deny"
expect_used = "deny"
indexing_slicing = "deny"
unwrap_used = "deny"
panic = "deny"
128 changes: 94 additions & 34 deletions crates/mcp-apollo-server/src/operations.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -262,14 +262,15 @@ fn get_json_schema(

fn schema_factory(
description: Option<String>,
instance_type: InstanceType,
instance_type: Option<InstanceType>,
object_validation: Option<ObjectValidation>,
array_validation: Option<ArrayValidation>,
subschema_validation: Option<SubschemaValidation>,
enum_values: Option<Vec<Value>>,
) -> Schema {
Schema::Object(SchemaObject {
instance_type: Some(SingleOrVec::Single(Box::new(instance_type))),
instance_type: instance_type
.map(|instance_type| SingleOrVec::Single(Box::new(instance_type))),
object: object_validation.map(Box::new),
array: array_validation.map(Box::new),
subschemas: subschema_validation.map(Box::new),
Expand Down Expand Up @@ -324,13 +325,30 @@ fn type_to_schema(
) -> Schema {
match variable_type {
Type::NonNullNamed(named) | Type::Named(named) => match named.as_str() {
"String" | "ID" => {
schema_factory(description, InstanceType::String, None, None, None, None)
}
"Int" | "Float" => {
schema_factory(description, InstanceType::Number, None, None, None, None)
}
"Boolean" => schema_factory(description, InstanceType::Boolean, None, None, None, None),
"String" | "ID" => schema_factory(
description,
Some(InstanceType::String),
None,
None,
None,
None,
),
"Int" | "Float" => schema_factory(
description,
Some(InstanceType::Number),
None,
None,
None,
None,
),
"Boolean" => schema_factory(
description,
Some(InstanceType::Boolean),
None,
None,
None,
None,
),
_ => {
if let Some(input_type) = graphql_schema.get_input_object(named) {
let mut obj = ObjectValidation::default();
Expand All @@ -354,7 +372,7 @@ fn type_to_schema(

schema_factory(
description,
InstanceType::Object,
Some(InstanceType::Object),
Some(obj),
None,
None,
Expand All @@ -374,17 +392,17 @@ fn type_to_schema(
custom_schema.metadata = Some(Box::new(meta));
Schema::Object(custom_schema)
} else {
panic!("custom scalar missing from custom_scalar_map")
tracing::warn!(name=?named, "custom scalar missing from custom_scalar_map");
schema_factory(description, None, None, None, None, None)
}
} else {
panic!(
"custom scalars aren't currently supported without a custom_scalar_map"
)
tracing::warn!(name=?named, "custom scalars aren't currently supported without a custom_scalar_map");
schema_factory(None, None, None, None, None, None)
}
} else if let Some(enum_type) = graphql_schema.get_enum(named) {
schema_factory(
description,
InstanceType::String,
Some(InstanceType::String),
None,
None,
None,
Expand All @@ -397,8 +415,8 @@ fn type_to_schema(
),
)
} else {
// TODO: Should this be an "any" type or an error?
panic!("Type not found in schema! {named}")
tracing::warn!(name=?named, "Type not found in schema");
schema_factory(None, None, None, None, None, None)
}
}
},
Expand All @@ -407,7 +425,7 @@ fn type_to_schema(
type_to_schema(description, list_type, graphql_schema, custom_scalar_map);
schema_factory(
None,
InstanceType::Array,
Some(InstanceType::Array),
None,
list_type.is_non_null().then(|| ArrayValidation {
items: Some(SingleOrVec::Single(Box::new(inner_type_schema.clone()))),
Expand Down Expand Up @@ -998,36 +1016,78 @@ mod tests {
"###);
}

// TODO: This should not cause a panic
#[test]
#[should_panic(expected = "Type not found in schema! FakeType")]
fn unknown_type_should_error() {
let _operation =
Operation::from_document("query QueryName($id: FakeType) { id }", &SCHEMA, None);
fn unknown_type_should_be_any() {
// TODO: should this test that the warning was logged?
let operation =
Operation::from_document("query QueryName($id: FakeType) { id }", &SCHEMA, None)
.unwrap();
let tool = Tool::from(operation);

insta::assert_debug_snapshot!(tool, @r###"
Tool {
name: "QueryName",
description: "The returned value is optional and has type `String`",
input_schema: {
"properties": Object {
"id": Object {},
},
"type": String("object"),
},
}
"###);
}

// TODO: This should not cause a panic
#[test]
#[should_panic(
expected = "custom scalars aren't currently supported without a custom_scalar_map"
)]
fn custom_scalar_without_map_should_error() {
let _operation = Operation::from_document(
fn custom_scalar_without_map_should_be_any() {
// TODO: should this test that the warning was logged?
let operation = Operation::from_document(
"query QueryName($id: RealCustomScalar) { id }",
&SCHEMA,
None,
);
)
.unwrap();
let tool = Tool::from(operation);

insta::assert_debug_snapshot!(tool, @r###"
Tool {
name: "QueryName",
description: "The returned value is optional and has type `String`",
input_schema: {
"properties": Object {
"id": Object {},
},
"type": String("object"),
},
}
"###);
}

// TODO: This should not cause a panic
#[test]
#[should_panic(expected = "custom scalar missing from custom_scalar_map")]
fn custom_scalar_with_map_but_not_found_should_error() {
let _operation = Operation::from_document(
// TODO: should this test that the warning was logged?
let operation = Operation::from_document(
"query QueryName($id: RealCustomScalar) { id }",
&SCHEMA,
Some(&HashMap::new()),
);
)
.unwrap();
let tool = Tool::from(operation);

insta::assert_debug_snapshot!(tool, @r###"
Tool {
name: "QueryName",
description: "The returned value is optional and has type `String`",
input_schema: {
"properties": Object {
"id": Object {
"description": String("RealCustomScalar exists"),
},
},
"type": String("object"),
},
}
"###);
}

#[test]
Expand Down