From b7573f18cf4bd3749485d62fb633cdf795d864b0 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Tom=20Houl=C3=A9?= Date: Thu, 21 Jun 2018 09:00:58 +0200 Subject: [PATCH 1/2] Fix nullability bug on From impls of FieldType --- graphql_query_derive/src/field_type.rs | 142 ++++++++++++++++++------- graphql_query_derive/src/schema.rs | 22 +--- tests/union_query.rs | 7 +- 3 files changed, 112 insertions(+), 59 deletions(-) diff --git a/graphql_query_derive/src/field_type.rs b/graphql_query_derive/src/field_type.rs index b20b53350..293bfbf45 100644 --- a/graphql_query_derive/src/field_type.rs +++ b/graphql_query_derive/src/field_type.rs @@ -1,5 +1,5 @@ use enums::ENUMS_PREFIX; -use graphql_parser::schema; +use graphql_parser; use introspection_response; use proc_macro2::{Ident, Span, TokenStream}; use query::QueryContext; @@ -56,36 +56,34 @@ impl FieldType { FieldType::Vector(inner) => inner.inner_name_string(), } } - - pub fn to_string(&self) -> String { - match &self { - FieldType::Named(name) => name.to_string(), - FieldType::Optional(inner) => format!("Option<{}>", inner.to_string()), - FieldType::Vector(inner) => format!("Vec<{}>", inner.to_string()), - } - } } -impl ::std::convert::From for FieldType { - fn from(schema_type: schema::Type) -> FieldType { +impl ::std::convert::From for FieldType { + fn from(schema_type: graphql_parser::schema::Type) -> FieldType { from_schema_type_inner(schema_type, false) } } -fn from_schema_type_inner(inner: schema::Type, non_null: bool) -> FieldType { - let inner_field_type = match inner { - schema::Type::ListType(inner) => { +fn from_schema_type_inner(inner: graphql_parser::schema::Type, non_null: bool) -> FieldType { + match inner { + graphql_parser::schema::Type::ListType(inner) => { let inner = from_schema_type_inner(*inner, false); - FieldType::Vector(Box::new(inner)) + let f = FieldType::Vector(Box::new(inner)); + if non_null { + f + } else { + FieldType::Optional(Box::new(f)) + } } - schema::Type::NamedType(name) => FieldType::Named(Ident::new(&name, Span::call_site())), - schema::Type::NonNullType(inner) => from_schema_type_inner(*inner, true), - }; - - if non_null { - inner_field_type - } else { - FieldType::Optional(Box::new(inner_field_type)) + graphql_parser::schema::Type::NamedType(name) => { + let f = FieldType::Named(Ident::new(&name, Span::call_site())); + if non_null { + f + } else { + FieldType::Optional(Box::new(f)) + } + } + graphql_parser::schema::Type::NonNullType(inner) => from_schema_type_inner(*inner, true), } } @@ -93,25 +91,33 @@ fn from_json_type_inner(inner: &introspection_response::TypeRef, non_null: bool) use introspection_response::*; let inner = inner.clone(); - let inner_field_type = match inner.kind { + match inner.kind { Some(__TypeKind::NON_NULL) => { from_json_type_inner(&inner.of_type.expect("inner type is missing"), true) } - Some(__TypeKind::LIST) => FieldType::Vector(Box::new(from_json_type_inner( - &inner.of_type.expect("inner type is missing"), - false, - ))), - Some(_) => FieldType::Named(Ident::new( - &inner.name.expect("type name"), - Span::call_site(), - )), + Some(__TypeKind::LIST) => { + let f = FieldType::Vector(Box::new(from_json_type_inner( + &inner.of_type.expect("inner type is missing"), + false, + ))); + if non_null { + f + } else { + FieldType::Optional(Box::new(f)) + } + } + Some(_) => { + let f = FieldType::Named(Ident::new( + &inner.name.expect("type name"), + Span::call_site(), + )); + if non_null { + f + } else { + FieldType::Optional(Box::new(f)) + } + } None => unreachable!("non-convertible type"), - }; - - if non_null { - inner_field_type - } else { - FieldType::Optional(Box::new(inner_field_type)) } } @@ -120,3 +126,63 @@ impl ::std::convert::From for FieldT from_json_type_inner(&schema_type.type_ref, false) } } + +#[cfg(test)] +mod tests { + use super::*; + use graphql_parser::schema::Type as GqlParserType; + use introspection_response::{FullTypeFieldsType, TypeRef, __TypeKind}; + + #[test] + fn field_type_from_graphql_parser_schema_type_works() { + let ty = GqlParserType::NamedType("Cat".to_string()); + assert_eq!( + FieldType::from(ty), + FieldType::Optional(Box::new(FieldType::Named(Ident::new( + "Cat", + Span::call_site() + )))) + ); + + let ty = GqlParserType::NonNullType(Box::new(GqlParserType::NamedType("Cat".to_string()))); + + assert_eq!( + FieldType::from(ty), + FieldType::Named(Ident::new("Cat", Span::call_site())) + ); + } + + #[test] + fn field_type_from_introspection_response_works() { + let ty = FullTypeFieldsType { + type_ref: TypeRef { + kind: Some(__TypeKind::OBJECT), + name: Some("Cat".to_string()), + of_type: None, + }, + }; + assert_eq!( + FieldType::from(ty), + FieldType::Optional(Box::new(FieldType::Named(Ident::new( + "Cat", + Span::call_site() + )))) + ); + + let ty = FullTypeFieldsType { + type_ref: TypeRef { + kind: Some(__TypeKind::NON_NULL), + name: None, + of_type: Some(Box::new(TypeRef { + kind: Some(__TypeKind::OBJECT), + name: Some("Cat".to_string()), + of_type: None, + })), + }, + }; + assert_eq!( + FieldType::from(ty), + FieldType::Named(Ident::new("Cat", Span::call_site())) + ); + } +} diff --git a/graphql_query_derive/src/schema.rs b/graphql_query_derive/src/schema.rs index 50dd77cd3..e159ef463 100644 --- a/graphql_query_derive/src/schema.rs +++ b/graphql_query_derive/src/schema.rs @@ -369,17 +369,11 @@ mod tests { }, GqlObjectField { name: "id".to_string(), - type_: FieldType::Optional(Box::new(FieldType::Named(Ident::new( - "ID", - Span::call_site(), - )))), + type_: FieldType::Named(Ident::new("ID", Span::call_site())), }, GqlObjectField { name: "name".to_string(), - type_: FieldType::Optional(Box::new(FieldType::Named(Ident::new( - "String", - Span::call_site(), - )))), + type_: FieldType::Named(Ident::new("String", Span::call_site())), }, GqlObjectField { name: "friends".to_string(), @@ -392,18 +386,12 @@ mod tests { }, GqlObjectField { name: "friendsConnection".to_string(), - type_: FieldType::Optional(Box::new(FieldType::Named(Ident::new( - "FriendsConnection", - Span::call_site(), - )))), + type_: FieldType::Named(Ident::new("FriendsConnection", Span::call_site())), }, GqlObjectField { name: "appearsIn".to_string(), - type_: FieldType::Optional(Box::new(FieldType::Vector(Box::new( - FieldType::Optional(Box::new(FieldType::Named(Ident::new( - "Episode", - Span::call_site(), - )))), + type_: FieldType::Vector(Box::new(FieldType::Optional(Box::new( + FieldType::Named(Ident::new("Episode", Span::call_site())), )))), }, GqlObjectField { diff --git a/tests/union_query.rs b/tests/union_query.rs index 62e1fdd24..ac31b9f33 100644 --- a/tests/union_query.rs +++ b/tests/union_query.rs @@ -9,9 +9,9 @@ const RESPONSE: &'static str = include_str!("union_query_response.json"); #[derive(GraphQLQuery)] #[GraphQLQuery( - query_path = "tests/union_query.graphql", - schema_path = "tests/union_schema.graphql", + query_path = "tests/union_query.graphql", schema_path = "tests/union_schema.graphql" )] +#[allow(dead_code)] struct UnionQuery; #[test] @@ -20,10 +20,9 @@ fn union_query_deserialization() { println!("{:?}", response_data); - let expected = r##"ResponseData { names: Some([Some(Person(RustMyQueryNamesOnPerson { firstName: Some("Audrey"), lastName: Some("Lorde") })), Some(Dog(RustMyQueryNamesOnDog { name: Some("Laïka") })), Some(Organization(RustMyQueryNamesOnOrganization { title: Some("Mozilla") })), Some(Dog(RustMyQueryNamesOnDog { name: Some("Norbert") }))]) }"##; + let expected = r##"ResponseData { names: Some([Person(RustMyQueryNamesOnPerson { firstName: "Audrey", lastName: Some("Lorde") }), Dog(RustMyQueryNamesOnDog { name: "Laïka" }), Organization(RustMyQueryNamesOnOrganization { title: "Mozilla" }), Dog(RustMyQueryNamesOnDog { name: "Norbert" })]) }"##; assert_eq!(format!("{:?}", response_data), expected); assert_eq!(response_data.names.map(|names| names.len()), Some(4)); - } From 851f3564e228c166df6583512af610bc8c96ec89 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Tom=20Houl=C3=A9?= Date: Thu, 21 Jun 2018 18:28:57 +0200 Subject: [PATCH 2/2] Fix warnings --- graphql_query_derive/src/unions.rs | 40 +++++++++++++++--------------- 1 file changed, 20 insertions(+), 20 deletions(-) diff --git a/graphql_query_derive/src/unions.rs b/graphql_query_derive/src/unions.rs index c44e354c1..4082289e5 100644 --- a/graphql_query_derive/src/unions.rs +++ b/graphql_query_derive/src/unions.rs @@ -1,6 +1,5 @@ use constants::*; use failure; -use heck::SnakeCase; use proc_macro2::{Ident, Span, TokenStream}; use query::QueryContext; use selection::{Selection, SelectionItem}; @@ -15,7 +14,7 @@ enum UnionError { #[fail(display = "Unknown type: {}", ty)] UnknownType { ty: String }, #[fail(display = "Missing __typename in selection for {}", union_name)] - MissingTypename { union_name: String } + MissingTypename { union_name: String }, } impl GqlUnion { @@ -37,7 +36,9 @@ impl GqlUnion { }); if typename_field.is_none() { - Err(UnionError::MissingTypename { union_name: prefix.into() })?; + Err(UnionError::MissingTypename { + union_name: prefix.into(), + })?; } let variants: Result, failure::Error> = selection @@ -53,7 +54,7 @@ impl GqlUnion { }) .map(|item| { match item { - SelectionItem::Field(f) => panic!("field selection on union"), + SelectionItem::Field(_) => panic!("field selection on union"), SelectionItem::FragmentSpread(_) => panic!("fragment spread on union"), SelectionItem::InlineFragment(frag) => { let variant_name = Ident::new(&frag.on, Span::call_site()); @@ -63,7 +64,7 @@ impl GqlUnion { let variant_type = Ident::new(&new_prefix, Span::call_site()); let field_object_type = - query_context.schema.objects.get(&frag.on).map(|f| { + query_context.schema.objects.get(&frag.on).map(|_f| { query_context.maybe_expand_field( &frag.on, &frag.fields, @@ -71,7 +72,7 @@ impl GqlUnion { ) }); let field_interface = - query_context.schema.interfaces.get(&frag.on).map(|f| { + query_context.schema.interfaces.get(&frag.on).map(|_f| { query_context.maybe_expand_field( &frag.on, &frag.fields, @@ -79,7 +80,7 @@ impl GqlUnion { ) }); // nested unions, is that even a thing? - let field_union_type = query_context.schema.unions.get(&frag.on).map(|f| { + let field_union_type = query_context.schema.unions.get(&frag.on).map(|_f| { query_context.maybe_expand_field(&frag.on, &frag.fields, &new_prefix) }); @@ -184,7 +185,10 @@ mod tests { assert!(result.is_err()); - assert_eq!(format!("{}", result.unwrap_err()), "Missing __typename in selection for Meow"); + assert_eq!( + format!("{}", result.unwrap_err()), + "Missing __typename in selection for Meow" + ); } #[test] @@ -196,21 +200,17 @@ mod tests { }), SelectionItem::InlineFragment(SelectionInlineFragment { on: "User".to_string(), - fields: Selection(vec![ - SelectionItem::Field(SelectionField { - name: "first_name".to_string(), - fields: Selection(vec![]), - }), - ]), + fields: Selection(vec![SelectionItem::Field(SelectionField { + name: "first_name".to_string(), + fields: Selection(vec![]), + })]), }), SelectionItem::InlineFragment(SelectionInlineFragment { on: "Organization".to_string(), - fields: Selection(vec![ - SelectionItem::Field(SelectionField { - name: "title".to_string(), - fields: Selection(vec![]), - }), - ]), + fields: Selection(vec![SelectionItem::Field(SelectionField { + name: "title".to_string(), + fields: Selection(vec![]), + })]), }), ]; let mut context = QueryContext::new_empty();