From 5d43532d739097a8beed7bcc15adbd8da7842735 Mon Sep 17 00:00:00 2001 From: Christian Legnitto Date: Thu, 24 Aug 2017 00:44:48 -0700 Subject: [PATCH] Preserve the order of requested fields Fixes https://github.com/graphql-rust/juniper/issues/82 --- CHANGELOG.md | 8 ++- juniper/Cargo.toml | 3 +- juniper/src/ast.rs | 9 ++-- juniper/src/executor_tests/directives.rs | 6 +-- juniper/src/executor_tests/enums.rs | 6 +-- juniper/src/executor_tests/executor.rs | 8 +-- juniper/src/executor_tests/variables.rs | 6 +-- juniper/src/integrations/serde.rs | 10 ++-- juniper/src/lib.rs | 1 + juniper/src/macros/tests/enums.rs | 4 +- juniper/src/macros/tests/field.rs | 4 +- juniper/src/macros/tests/input_object.rs | 4 +- juniper/src/macros/tests/interface.rs | 4 +- juniper/src/macros/tests/object.rs | 4 +- juniper/src/macros/tests/scalar.rs | 4 +- juniper/src/macros/tests/union.rs | 4 +- juniper/src/parser/tests/value.rs | 5 +- juniper/src/tests/query_tests.rs | 68 +++++++++++++++++++++++- juniper/src/tests/type_info_tests.rs | 6 +-- juniper/src/types/base.rs | 20 +++---- juniper/src/value.rs | 10 ++-- 21 files changed, 134 insertions(+), 60 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index ae564712..b8b294cd 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -7,11 +7,17 @@ The repository was restructured to a multi crate workspace to enable several new ### New features -* New juniper_codegen crate which provides custom derives: +* New juniper_codegen crate which provides custom derives: * `#[derive(GraphQLInputObject)]` * `#[derive(GraphQLEnum)]` * `#[derive(GraphQLObject)]` +## Breaking changes + +* To better comply with the specification, order of requested fields is + now preserved. + ([#82](https://github.com/graphql-rust/juniper/issues/82) + ## [0.8.1] – 2017-06-15 Tiny release to fix broken crate metadata on crates.io. diff --git a/juniper/Cargo.toml b/juniper/Cargo.toml index cb92caa5..caa834a9 100644 --- a/juniper/Cargo.toml +++ b/juniper/Cargo.toml @@ -25,6 +25,7 @@ expose-test-schema = [] default = ["uuid"] [dependencies] +ordermap = { version = "^0.2.11", features = ["serde-1"] } serde = { version = "^1.0.8" } serde_derive = {version="^1.0.8" } serde_json = { version="^1.0.2", optional = true } @@ -32,4 +33,4 @@ uuid = { version = "0.5.1", optional = true } [dev-dependencies] bencher = "^0.1.2" -serde_json = { version = "^1.0.2" } \ No newline at end of file +serde_json = { version = "^1.0.2" } diff --git a/juniper/src/ast.rs b/juniper/src/ast.rs index 64675fc1..f1f13c5f 100644 --- a/juniper/src/ast.rs +++ b/juniper/src/ast.rs @@ -1,10 +1,11 @@ use std::fmt; use std::borrow::Cow; -use std::collections::HashMap; use std::hash::Hash; use std::vec; use std::slice; +use ordermap::OrderMap; + use executor::Variables; use parser::Spanning; @@ -258,7 +259,7 @@ impl InputValue { /// /// Similar to `InputValue::list`, it makes each key and value in the given /// hash map not contain any location information. - pub fn object(o: HashMap) -> InputValue + pub fn object(o: OrderMap) -> InputValue where K: AsRef + Eq + Hash, { @@ -347,9 +348,9 @@ impl InputValue { /// Convert the input value to an unlocated object value. /// - /// This constructs a new hashmap that contain references to the keys + /// This constructs a new OrderMap that contain references to the keys /// and values in `self`. - pub fn to_object_value(&self) -> Option> { + pub fn to_object_value(&self) -> Option> { match *self { InputValue::Object(ref o) => Some( o.iter() diff --git a/juniper/src/executor_tests/directives.rs b/juniper/src/executor_tests/directives.rs index ed059892..3d20926c 100644 --- a/juniper/src/executor_tests/directives.rs +++ b/juniper/src/executor_tests/directives.rs @@ -1,4 +1,4 @@ -use std::collections::HashMap; +use ordermap::OrderMap; use value::Value; use executor::Variables; @@ -19,7 +19,7 @@ graphql_object!(TestType: () |&self| { fn run_variable_query(query: &str, vars: Variables, f: F) where - F: Fn(&HashMap) -> (), + F: Fn(&OrderMap) -> (), { let schema = RootNode::new(TestType, EmptyMutation::<()>::new()); @@ -36,7 +36,7 @@ where fn run_query(query: &str, f: F) where - F: Fn(&HashMap) -> (), + F: Fn(&OrderMap) -> (), { run_variable_query(query, Variables::new(), f); } diff --git a/juniper/src/executor_tests/enums.rs b/juniper/src/executor_tests/enums.rs index 9d0d09ac..5081bf48 100644 --- a/juniper/src/executor_tests/enums.rs +++ b/juniper/src/executor_tests/enums.rs @@ -1,4 +1,4 @@ -use std::collections::HashMap; +use ordermap::OrderMap; use value::Value; use ast::InputValue; @@ -35,7 +35,7 @@ graphql_object!(TestType: () |&self| { fn run_variable_query(query: &str, vars: Variables, f: F) where - F: Fn(&HashMap) -> (), + F: Fn(&OrderMap) -> (), { let schema = RootNode::new(TestType, EmptyMutation::<()>::new()); @@ -52,7 +52,7 @@ where fn run_query(query: &str, f: F) where - F: Fn(&HashMap) -> (), + F: Fn(&OrderMap) -> (), { run_variable_query(query, Variables::new(), f); } diff --git a/juniper/src/executor_tests/executor.rs b/juniper/src/executor_tests/executor.rs index 45a4925f..66e58422 100644 --- a/juniper/src/executor_tests/executor.rs +++ b/juniper/src/executor_tests/executor.rs @@ -148,15 +148,15 @@ mod merge_parallel_fragments { Value::object(vec![ ("a", Value::string("Apple")), ("b", Value::string("Banana")), - ("c", Value::string("Cherry")), ("deep", Value::object(vec![ ("b", Value::string("Banana")), - ("c", Value::string("Cherry")), ("deeper", Value::object(vec![ ("b", Value::string("Banana")), ("c", Value::string("Cherry")), ].into_iter().collect())), + ("c", Value::string("Cherry")), ].into_iter().collect())), + ("c", Value::string("Cherry")), ].into_iter().collect())); } } @@ -209,7 +209,7 @@ mod threads_context_correctly { } mod dynamic_context_switching { - use std::collections::HashMap; + use ordermap::OrderMap; use value::Value; use types::scalars::EmptyMutation; @@ -224,7 +224,7 @@ mod dynamic_context_switching { } struct OuterContext { - items: HashMap, + items: OrderMap, } impl Context for OuterContext {} diff --git a/juniper/src/executor_tests/variables.rs b/juniper/src/executor_tests/variables.rs index 1f6bafd6..900d9eda 100644 --- a/juniper/src/executor_tests/variables.rs +++ b/juniper/src/executor_tests/variables.rs @@ -1,4 +1,4 @@ -use std::collections::HashMap; +use ordermap::OrderMap; use value::Value; use ast::InputValue; @@ -120,7 +120,7 @@ graphql_object!(TestType: () |&self| { fn run_variable_query(query: &str, vars: Variables, f: F) where - F: Fn(&HashMap) -> (), + F: Fn(&OrderMap) -> (), { let schema = RootNode::new(TestType, EmptyMutation::<()>::new()); @@ -137,7 +137,7 @@ where fn run_query(query: &str, f: F) where - F: Fn(&HashMap) -> (), + F: Fn(&OrderMap) -> (), { run_variable_query(query, Variables::new(), f); } diff --git a/juniper/src/integrations/serde.rs b/juniper/src/integrations/serde.rs index aabd68d3..6221fc94 100644 --- a/juniper/src/integrations/serde.rs +++ b/juniper/src/integrations/serde.rs @@ -1,7 +1,8 @@ +use ordermap::OrderMap; use serde::{de, ser}; use serde::ser::SerializeMap; + use std::fmt; -use std::collections::HashMap; use {GraphQLError, Value}; use ast::InputValue; @@ -9,7 +10,6 @@ use executor::ExecutionError; use parser::{ParseError, SourcePosition, Spanning}; use validation::RuleError; - impl ser::Serialize for ExecutionError { fn serialize(&self, serializer: S) -> Result where @@ -130,7 +130,7 @@ impl<'de> de::Deserialize<'de> for InputValue { where V: de::MapAccess<'de>, { - let mut values: HashMap = HashMap::new(); + let mut values: OrderMap = OrderMap::new(); while let Some((key, value)) = try!(visitor.next_entry()) { values.insert(key, value); @@ -161,7 +161,7 @@ impl ser::Serialize for InputValue { .serialize(serializer), InputValue::Object(ref v) => v.iter() .map(|&(ref k, ref v)| (k.item.clone(), v.item.clone())) - .collect::>() + .collect::>() .serialize(serializer), } } @@ -214,7 +214,7 @@ impl<'a> ser::Serialize for Spanning> { try!(map.serialize_key("message")); try!(map.serialize_value(&message)); - let mut location = HashMap::new(); + let mut location = OrderMap::new(); location.insert("line".to_owned(), self.start.line() + 1); location.insert("column".to_owned(), self.start.column() + 1); diff --git a/juniper/src/lib.rs b/juniper/src/lib.rs index e43d88e2..9f9daa19 100644 --- a/juniper/src/lib.rs +++ b/juniper/src/lib.rs @@ -121,6 +121,7 @@ extern crate serde_derive; #[cfg(any(test, feature = "expose-test-schema"))] extern crate serde_json; +extern crate ordermap; #[cfg(any(test, feature = "uuid"))] extern crate uuid; diff --git a/juniper/src/macros/tests/enums.rs b/juniper/src/macros/tests/enums.rs index 67cb5906..f0fcec22 100644 --- a/juniper/src/macros/tests/enums.rs +++ b/juniper/src/macros/tests/enums.rs @@ -1,4 +1,4 @@ -use std::collections::HashMap; +use ordermap::OrderMap; use executor::Variables; use value::Value; @@ -88,7 +88,7 @@ graphql_object!(Root: () |&self| { fn run_type_info_query(doc: &str, f: F) where - F: Fn((&HashMap, &Vec)) -> (), + F: Fn((&OrderMap, &Vec)) -> (), { let schema = RootNode::new(Root {}, EmptyMutation::<()>::new()); diff --git a/juniper/src/macros/tests/field.rs b/juniper/src/macros/tests/field.rs index 19a82e94..d4c8dd44 100644 --- a/juniper/src/macros/tests/field.rs +++ b/juniper/src/macros/tests/field.rs @@ -1,4 +1,4 @@ -use std::collections::HashMap; +use ordermap::OrderMap; use value::Value; use ast::InputValue; @@ -59,7 +59,7 @@ graphql_interface!(Interface: () |&self| { fn run_field_info_query(type_name: &str, field_name: &str, f: F) where - F: Fn(&HashMap) -> (), + F: Fn(&OrderMap) -> (), { let doc = r#" query ($typeName: String!) { diff --git a/juniper/src/macros/tests/input_object.rs b/juniper/src/macros/tests/input_object.rs index df0d0fa6..a4219be6 100644 --- a/juniper/src/macros/tests/input_object.rs +++ b/juniper/src/macros/tests/input_object.rs @@ -1,4 +1,4 @@ -use std::collections::HashMap; +use ordermap::OrderMap; use ast::{FromInputValue, InputValue}; use executor::Variables; @@ -105,7 +105,7 @@ graphql_object!(Root: () |&self| { fn run_type_info_query(doc: &str, f: F) where - F: Fn(&HashMap, &Vec) -> (), + F: Fn(&OrderMap, &Vec) -> (), { let schema = RootNode::new(Root {}, EmptyMutation::<()>::new()); diff --git a/juniper/src/macros/tests/interface.rs b/juniper/src/macros/tests/interface.rs index 1120b893..27fd37e2 100644 --- a/juniper/src/macros/tests/interface.rs +++ b/juniper/src/macros/tests/interface.rs @@ -1,4 +1,4 @@ -use std::collections::HashMap; +use ordermap::OrderMap; use std::marker::PhantomData; use ast::InputValue; @@ -135,7 +135,7 @@ graphql_object!(<'a> Root: () as "Root" |&self| { fn run_type_info_query(type_name: &str, f: F) where - F: Fn(&HashMap, &Vec) -> (), + F: Fn(&OrderMap, &Vec) -> (), { let doc = r#" query ($typeName: String!) { diff --git a/juniper/src/macros/tests/object.rs b/juniper/src/macros/tests/object.rs index 0cffeba5..34ebc8e0 100644 --- a/juniper/src/macros/tests/object.rs +++ b/juniper/src/macros/tests/object.rs @@ -1,4 +1,4 @@ -use std::collections::HashMap; +use ordermap::OrderMap; use std::marker::PhantomData; use ast::InputValue; @@ -119,7 +119,7 @@ graphql_object!(<'a> Root: () as "Root" |&self| { fn run_type_info_query(type_name: &str, f: F) where - F: Fn(&HashMap, &Vec) -> (), + F: Fn(&OrderMap, &Vec) -> (), { let doc = r#" query ($typeName: String!) { diff --git a/juniper/src/macros/tests/scalar.rs b/juniper/src/macros/tests/scalar.rs index c8f9c92e..764467c0 100644 --- a/juniper/src/macros/tests/scalar.rs +++ b/juniper/src/macros/tests/scalar.rs @@ -1,4 +1,4 @@ -use std::collections::HashMap; +use ordermap::OrderMap; use executor::Variables; use value::Value; @@ -72,7 +72,7 @@ graphql_object!(Root: () |&self| { fn run_type_info_query(doc: &str, f: F) where - F: Fn(&HashMap) -> (), + F: Fn(&OrderMap) -> (), { let schema = RootNode::new(Root {}, EmptyMutation::<()>::new()); diff --git a/juniper/src/macros/tests/union.rs b/juniper/src/macros/tests/union.rs index 4cc06328..52acab2c 100644 --- a/juniper/src/macros/tests/union.rs +++ b/juniper/src/macros/tests/union.rs @@ -1,4 +1,4 @@ -use std::collections::HashMap; +use ordermap::OrderMap; use std::marker::PhantomData; use ast::InputValue; @@ -112,7 +112,7 @@ graphql_object!(<'a> Root: () as "Root" |&self| { fn run_type_info_query(type_name: &str, f: F) where - F: Fn(&HashMap, &Vec) -> (), + F: Fn(&OrderMap, &Vec) -> (), { let doc = r#" query ($typeName: String!) { diff --git a/juniper/src/parser/tests/value.rs b/juniper/src/parser/tests/value.rs index cce964e3..83b869fe 100644 --- a/juniper/src/parser/tests/value.rs +++ b/juniper/src/parser/tests/value.rs @@ -1,9 +1,10 @@ -use std::collections::HashMap; +use ordermap::OrderMap; use ast::InputValue; use parser::{Lexer, Parser, SourcePosition, Spanning}; use parser::value::parse_value_literal; + fn parse_value(s: &str) -> Spanning { let mut lexer = Lexer::new(s); let mut parser = Parser::new(&mut lexer).expect(&format!("Lexer error on input {:#?}", s)); @@ -112,7 +113,7 @@ fn input_value_literals() { Spanning::start_end( &SourcePosition::new(0, 0, 0), &SourcePosition::new(2, 0, 2), - InputValue::object(HashMap::::new()) + InputValue::object(OrderMap::::new()) ) ); assert_eq!( diff --git a/juniper/src/tests/query_tests.rs b/juniper/src/tests/query_tests.rs index be63a429..92f56d8d 100644 --- a/juniper/src/tests/query_tests.rs +++ b/juniper/src/tests/query_tests.rs @@ -33,6 +33,70 @@ fn test_hero_name() { ); } +#[test] +fn test_hero_field_order() { + let database = Database::new(); + let schema = RootNode::new(&database, EmptyMutation::::new()); + + let doc = r#" + { + hero { + id + name + } + }"#; + assert_eq!( + ::execute(doc, None, &schema, &Variables::new(), &database), + Ok(( + Value::object( + vec![ + ( + "hero", + Value::object( + vec![ + ("id", Value::string("2001")), + ("name", Value::string("R2-D2")), + ].into_iter() + .collect(), + ), + ), + ].into_iter() + .collect() + ), + vec![] + )) + ); + + let doc_reversed = r#" + { + hero { + name + id + } + }"#; + assert_eq!( + ::execute(doc_reversed, None, &schema, &Variables::new(), &database), + Ok(( + Value::object( + vec![ + ( + "hero", + Value::object( + vec![ + ("name", Value::string("R2-D2")), + ("id", Value::string("2001")), + ].into_iter() + .collect(), + ), + ), + ].into_iter() + .collect() + ), + vec![] + )) + ); +} + #[test] fn test_hero_name_and_friends() { let doc = r#" @@ -537,8 +601,8 @@ fn test_query_inline_fragments_droid() { "hero", Value::object( vec![ - ("__typename", Value::string("Droid")), ("name", Value::string("R2-D2")), + ("__typename", Value::string("Droid")), ("primaryFunction", Value::string("Astromech")), ].into_iter() .collect(), @@ -574,8 +638,8 @@ fn test_query_inline_fragments_human() { "hero", Value::object( vec![ - ("__typename", Value::string("Human")), ("name", Value::string("Luke Skywalker")), + ("__typename", Value::string("Human")), ].into_iter() .collect(), ), diff --git a/juniper/src/tests/type_info_tests.rs b/juniper/src/tests/type_info_tests.rs index 1ddf6918..9073cb24 100644 --- a/juniper/src/tests/type_info_tests.rs +++ b/juniper/src/tests/type_info_tests.rs @@ -1,4 +1,4 @@ -use std::collections::HashMap; +use ordermap::OrderMap; use executor::{ExecutionResult, Executor, Registry, Variables}; use value::Value; @@ -13,7 +13,7 @@ pub struct NodeTypeInfo { } pub struct Node { - attributes: HashMap, + attributes: OrderMap, } impl GraphQLType for Node { @@ -59,7 +59,7 @@ fn test_node() { attribute_names: vec!["foo".to_string(), "bar".to_string(), "baz".to_string()], }; let mut node = Node { - attributes: HashMap::new(), + attributes: OrderMap::new(), }; node.attributes.insert("foo".to_string(), "1".to_string()); node.attributes.insert("bar".to_string(), "2".to_string()); diff --git a/juniper/src/types/base.rs b/juniper/src/types/base.rs index 415dbfbe..47c51d72 100644 --- a/juniper/src/types/base.rs +++ b/juniper/src/types/base.rs @@ -1,5 +1,5 @@ -use std::collections::HashMap; -use std::collections::hash_map::Entry; +use ordermap::OrderMap; +use ordermap::Entry; use ast::{Directive, FromInputValue, InputValue, Selection}; use executor::Variables; @@ -66,17 +66,17 @@ pub enum TypeKind { /// Field argument container pub struct Arguments<'a> { - args: Option>, + args: Option>, } impl<'a> Arguments<'a> { #[doc(hidden)] pub fn new( - mut args: Option>, + mut args: Option>, meta_args: &'a Option>, ) -> Arguments<'a> { if meta_args.is_some() && args.is_none() { - args = Some(HashMap::new()); + args = Some(OrderMap::new()); } if let (&mut Some(ref mut args), &Some(ref meta_args)) = (&mut args, meta_args) { @@ -306,7 +306,7 @@ pub trait GraphQLType: Sized { executor: &Executor, ) -> Value { if let Some(selection_set) = selection_set { - let mut result = HashMap::new(); + let mut result = OrderMap::new(); resolve_selection_set_into(self, info, selection_set, executor, &mut result); Value::object(result) } else { @@ -320,7 +320,7 @@ fn resolve_selection_set_into( info: &T::TypeInfo, selection_set: &[Selection], executor: &Executor, - result: &mut HashMap, + result: &mut OrderMap, ) where T: GraphQLType, { @@ -435,7 +435,7 @@ fn resolve_selection_set_into( ); if let Ok(Value::Object(mut hash_map)) = sub_result { - for (k, v) in hash_map.drain() { + for (k, v) in hash_map.drain(..) { result.insert(k, v); } } else if let Err(e) = sub_result { @@ -480,7 +480,7 @@ fn is_excluded(directives: &Option>>, vars: &Variables) false } -fn merge_key_into(result: &mut HashMap, response_name: &str, value: Value) { +fn merge_key_into(result: &mut OrderMap, response_name: &str, value: Value) { match result.entry(response_name.to_owned()) { Entry::Occupied(mut e) => match (e.get_mut().as_mut_object_value(), value) { (Some(dest_obj), Value::Object(src_obj)) => { @@ -494,7 +494,7 @@ fn merge_key_into(result: &mut HashMap, response_name: &str, valu } } -fn merge_maps(dest: &mut HashMap, src: HashMap) { +fn merge_maps(dest: &mut OrderMap, src: OrderMap) { for (key, value) in src { if dest.contains_key(&key) { merge_key_into(dest, &key, value); diff --git a/juniper/src/value.rs b/juniper/src/value.rs index eaac620e..b8c65e5a 100644 --- a/juniper/src/value.rs +++ b/juniper/src/value.rs @@ -1,4 +1,4 @@ -use std::collections::HashMap; +use ordermap::OrderMap; use std::hash::Hash; use parser::Spanning; @@ -22,7 +22,7 @@ pub enum Value { String(String), Boolean(bool), List(Vec), - Object(HashMap), + Object(OrderMap), } impl Value { @@ -59,7 +59,7 @@ impl Value { } /// Construct an object value. - pub fn object(o: HashMap) -> Value + pub fn object(o: OrderMap) -> Value where K: Into + Eq + Hash, { @@ -77,7 +77,7 @@ impl Value { } /// View the underlying object value, if present. - pub fn as_object_value(&self) -> Option<&HashMap> { + pub fn as_object_value(&self) -> Option<&OrderMap> { match *self { Value::Object(ref o) => Some(o), _ => None, @@ -85,7 +85,7 @@ impl Value { } /// Mutable view into the underlying object value, if present. - pub fn as_mut_object_value(&mut self) -> Option<&mut HashMap> { + pub fn as_mut_object_value(&mut self) -> Option<&mut OrderMap> { match *self { Value::Object(ref mut o) => Some(o), _ => None,