Merge pull request #150 from tomhoule/fix-operation-name

Fix wrong operationName when no operation is selected (fixes #149)

Author: tomhoule

graphql_client_cli/src/generate.rs

@@ -21,7 +21,7 @@ pub fn generate_code(
     };
 
     let options = GraphQLClientDeriveOptions {
-        selected_operation,
+        struct_name: selected_operation,
         additional_derives,
         deprecation_strategy,
     };

graphql_client_codegen/src/codegen.rs

@@ -8,10 +8,31 @@ use query::QueryContext;
 use schema;
 use selection::Selection;
 
+/// Selects the first operation matching `struct_name` or the first one. Returns `None` when the query document defines no operation.
+pub(crate) fn select_operation(query: &query::Document, struct_name: &str) -> Option<Operation> {
+    let mut operations: Vec<Operation> = Vec::new();
+
+    for definition in query.definitions.iter() {
+        match definition {
+            query::Definition::Operation(op) => {
+                operations.push(op.into());
+            }
+            _ => (),
+        }
+    }
+
+    operations
+        .iter()
+        .find(|op| op.name == struct_name)
+        .map(|i| i.to_owned())
+        .or_else(|| operations.iter().next().map(|i| i.to_owned()))
+}
+
+/// The main code generation function.
 pub fn response_for_query(
     schema: schema::Schema,
     query: query::Document,
-    selected_operation: String,
+    operation: &Operation,
     additional_derives: Option<String>,
     deprecation_strategy: deprecation::DeprecationStrategy,
 ) -> Result<TokenStream, failure::Error> {
@@ -22,13 +43,10 @@ pub fn response_for_query(
     }
 
     let mut definitions = Vec::new();
-    let mut operations: Vec<Operation> = Vec::new();
 
     for definition in query.definitions {
         match definition {
-            query::Definition::Operation(op) => {
-                operations.push(op.into());
-            }
+            query::Definition::Operation(_op) => (),
             query::Definition::Fragment(fragment) => {
                 let query::TypeCondition::On(on) = fragment.type_condition;
                 context.fragments.insert(
@@ -44,21 +62,6 @@ pub fn response_for_query(
         }
     }
 
-    context.selected_operation = operations
-        .iter()
-        .find(|op| op.name == selected_operation)
-        .map(|i| i.to_owned());
-
-    let opt_operation = context
-        .selected_operation
-        .clone()
-        .or_else(|| operations.iter().next().map(|i| i.to_owned()));
-    let operation = if let Some(operation) = opt_operation {
-        operation
-    } else {
-        panic!("no operation '{}' in query document", selected_operation);
-    };
-
     let response_data_fields = {
         let opt_root_name = operation.root_name(&context.schema);
         let root_name: String = if let Some(root_name) = opt_root_name {

graphql_client_codegen/src/lib.rs

@@ -56,7 +56,7 @@ lazy_static! {
 }
 
 pub struct GraphQLClientDeriveOptions {
-    pub selected_operation: String,
+    pub struct_name: String,
     pub additional_derives: Option<String>,
     pub deprecation_strategy: Option<deprecation::DeprecationStrategy>,
 }
@@ -93,6 +93,16 @@ pub fn generate_module_token_stream(
         }
     };
 
+    // Determine which operation we are generating code for. This will be used in operationName.
+
+    let operation = if let Some(op) = codegen::select_operation(&query, &options.struct_name) {
+        op
+    } else {
+        panic!("Query document defines no operation.")
+    };
+
+    let operation_name_literal = &operation.name;
+
     // Check the schema cache.
     let schema = {
         let mut lock = SCHEMA_CACHE.lock().expect("schema cache is poisoned");
@@ -125,16 +135,15 @@ pub fn generate_module_token_stream(
         }
     };
 
-    let operation_string = options.selected_operation.clone();
     let module_name = Ident::new(
-        options.selected_operation.to_snake_case().as_str(),
+        options.struct_name.to_snake_case().as_str(),
         Span::call_site(),
     );
-    let struct_name = Ident::new(options.selected_operation.as_str(), Span::call_site());
+    let struct_name = Ident::new(options.struct_name.as_str(), Span::call_site());
     let schema_output = codegen::response_for_query(
         schema,
         query,
-        options.selected_operation.clone(),
+        &operation,
         response_derives,
         deprecation_strategy,
     )?;
@@ -148,7 +157,7 @@ pub fn generate_module_token_stream(
             use serde;
 
             pub const QUERY: &'static str = #query_string;
-            pub const OPERATION_NAME: &'static str = #operation_string;
+            pub const OPERATION_NAME: &'static str = #operation_name_literal;
 
             #schema_output
         }

graphql_client_codegen/src/operations.rs

@@ -8,14 +8,14 @@ use syn::Ident;
 use variables::Variable;
 
 #[derive(Debug, Clone)]
-pub(crate) enum OperationType {
+pub enum OperationType {
     Query,
     Mutation,
     Subscription,
 }
 
 #[derive(Debug, Clone)]
-pub(crate) struct Operation {
+pub struct Operation {
     pub name: String,
     pub operation_type: OperationType,
     pub variables: Vec<Variable>,
@@ -113,3 +113,41 @@ impl ::std::convert::From<OperationDefinition> for Operation {
         }
     }
 }
+
+impl<'a> ::std::convert::From<&'a OperationDefinition> for Operation {
+    fn from(definition: &OperationDefinition) -> Operation {
+        match *definition {
+            OperationDefinition::Query(ref q) => Operation {
+                name: q.name.clone().expect("unnamed operation"),
+                operation_type: OperationType::Query,
+                variables: q
+                    .variable_definitions
+                    .iter()
+                    .map(|v| v.clone().into())
+                    .collect(),
+                selection: (&q.selection_set).into(),
+            },
+            OperationDefinition::Mutation(ref m) => Operation {
+                name: m.name.clone().expect("unnamed operation"),
+                operation_type: OperationType::Mutation,
+                variables: m
+                    .variable_definitions
+                    .iter()
+                    .map(|v| v.clone().into())
+                    .collect(),
+                selection: (&m.selection_set).into(),
+            },
+            OperationDefinition::Subscription(ref s) => Operation {
+                name: s.name.clone().expect("unnamed operation"),
+                operation_type: OperationType::Subscription,
+                variables: s
+                    .variable_definitions
+                    .iter()
+                    .map(|v| v.clone().into())
+                    .collect(),
+                selection: (&s.selection_set).into(),
+            },
+            OperationDefinition::SelectionSet(_) => panic!(SELECTION_SET_AT_ROOT),
+        }
+    }
+}

graphql_client_codegen/src/query.rs

@@ -2,7 +2,6 @@ use deprecation::DeprecationStrategy;
 use failure;
 use fragments::GqlFragment;
 use itertools::Itertools;
-use operations::Operation;
 use proc_macro2::Span;
 use proc_macro2::TokenStream;
 use schema::Schema;
@@ -14,7 +13,6 @@ use syn::Ident;
 pub(crate) struct QueryContext {
     pub fragments: BTreeMap<String, GqlFragment>,
     pub schema: Schema,
-    pub selected_operation: Option<Operation>,
     pub deprecation_strategy: DeprecationStrategy,
     variables_derives: Vec<Ident>,
     response_derives: Vec<Ident>,
@@ -26,7 +24,6 @@ impl QueryContext {
         QueryContext {
             fragments: BTreeMap::new(),
             schema,
-            selected_operation: None,
             deprecation_strategy,
             variables_derives: vec![Ident::new("Serialize", Span::call_site())],
             response_derives: vec![Ident::new("Deserialize", Span::call_site())],
@@ -45,7 +42,6 @@ impl QueryContext {
         QueryContext {
             fragments: BTreeMap::new(),
             schema: Schema::new(),
-            selected_operation: None,
             deprecation_strategy: DeprecationStrategy::Allow,
             variables_derives: vec![Ident::new("Serialize", Span::call_site())],
             response_derives: vec![Ident::new("Deserialize", Span::call_site())],

graphql_query_derive/src/lib.rs

@@ -37,7 +37,7 @@ fn build_graphql_client_derive_options(input: &syn::DeriveInput) -> GraphQLClien
         .unwrap_or(deprecation::DeprecationStrategy::default());
 
     GraphQLClientDeriveOptions {
-        selected_operation: input.ident.to_string(),
+        struct_name: input.ident.to_string(),
         additional_derives: response_derives,
         deprecation_strategy: Some(deprecation_strategy),
     }

tests/operation_selection.rs

@@ -5,29 +5,42 @@ extern crate serde_derive;
 extern crate serde;
 extern crate serde_json;
 
+use graphql_client::GraphQLQuery;
+
 #[derive(GraphQLQuery)]
 #[graphql(
     query_path = "tests/operation_selection/queries.graphql",
     schema_path = "tests/operation_selection/schema.graphql",
-    response_derives = "Debug",
+    response_derives = "Debug,PartialEq",
 )]
 pub struct Heights;
 
 #[derive(GraphQLQuery)]
 #[graphql(
     query_path = "tests/operation_selection/queries.graphql",
     schema_path = "tests/operation_selection/schema.graphql",
-    response_derives = "Debug",
+    response_derives = "Debug,PartialEq",
 )]
 pub struct Echo;
 
+// The default is the first operation so this should be the same as Heights
+#[derive(GraphQLQuery)]
+#[graphql(
+    query_path = "tests/operation_selection/queries.graphql",
+    schema_path = "tests/operation_selection/schema.graphql",
+    response_derives = "Debug,PartialEq",
+)]
+pub struct Unrelated;
+
 const HEIGHTS_RESPONSE: &'static str = r##"{"mountainHeight": 224, "buildingHeight": 12}"##;
 const ECHO_RESPONSE: &'static str = r##"{"echo": "tiramisù"}"##;
 
 #[test]
 fn operation_selection_works() {
     let heights_response_data: heights::ResponseData =
         serde_json::from_str(HEIGHTS_RESPONSE).unwrap();
+    let heights_unrelated_response_data: unrelated::ResponseData =
+        serde_json::from_str(HEIGHTS_RESPONSE).unwrap();
     let echo_response_data: echo::ResponseData = serde_json::from_str(ECHO_RESPONSE).unwrap();
 
     let _echo_variables = echo::Variables {
@@ -38,11 +51,51 @@ fn operation_selection_works() {
         building_id: "12".to_string(),
         mountain_name: Some("canigou".to_string()),
     };
+    let _unrelated_variables = unrelated::Variables {
+        building_id: "12".to_string(),
+        mountain_name: Some("canigou".to_string()),
+    };
+
+    let expected_echo = echo::ResponseData {
+        echo: Some("tiramisù".to_string()),
+    };
+    let expected_heights = heights::ResponseData {
+        mountain_height: Some(224),
+        building_height: Some(12),
+    };
 
-    let expected_echo = r##"ResponseData { echo: Some("tiramisù") }"##;
-    let expected_heights =
-        r##"ResponseData { mountain_height: Some(224), building_height: Some(12) }"##;
+    let expected_heights_unrelated = unrelated::ResponseData {
+        mountain_height: Some(224),
+        building_height: Some(12),
+    };
+
+    assert_eq!(expected_echo, echo_response_data);
+    assert_eq!(expected_heights, heights_response_data);
+    assert_eq!(expected_heights_unrelated, heights_unrelated_response_data);
+}
+
+#[test]
+fn operation_name_is_correct() {
+    let echo_variables = echo::Variables {
+        msg: Some("hi".to_string()),
+    };
+
+    let height_variables = heights::Variables {
+        building_id: "12".to_string(),
+        mountain_name: Some("canigou".to_string()),
+    };
+    let unrelated_variables = unrelated::Variables {
+        building_id: "12".to_string(),
+        mountain_name: Some("canigou".to_string()),
+    };
 
-    assert_eq!(expected_echo, format!("{:?}", echo_response_data));
-    assert_eq!(expected_heights, format!("{:?}", heights_response_data));
+    assert_eq!(Echo::build_query(echo_variables).operation_name, "Echo");
+    assert_eq!(
+        Heights::build_query(height_variables).operation_name,
+        "Heights"
+    );
+    assert_eq!(
+        Unrelated::build_query(unrelated_variables).operation_name,
+        "Heights"
+    );
 }