fixed a bug in can_merge function

Also changed the comparison algorithm to O(n + m) from O(nm)

Author: imor

src/builder.rs

@@ -5,6 +5,7 @@ use crate::sql_types::*;
 use graphql_parser::query::*;
 use serde::Serialize;
 use std::collections::HashMap;
+use std::hash::Hash;
 use std::ops::Deref;
 use std::str::FromStr;
 use std::sync::Arc;
@@ -264,6 +265,7 @@ pub fn to_insert_builder<'a, T>(
 ) -> Result<InsertBuilder, String>
 where
     T: Text<'a> + Eq + AsRef<str> + Clone,
+    T::Value: Hash,
 {
     let type_ = field.type_().unmodified_type();
     let type_name = type_
@@ -429,6 +431,7 @@ pub fn to_update_builder<'a, T>(
 ) -> Result<UpdateBuilder, String>
 where
     T: Text<'a> + Eq + AsRef<str> + Clone,
+    T::Value: Hash,
 {
     let type_ = field.type_().unmodified_type();
     let type_name = type_
@@ -534,6 +537,7 @@ pub fn to_delete_builder<'a, T>(
 ) -> Result<DeleteBuilder, String>
 where
     T: Text<'a> + Eq + AsRef<str> + Clone,
+    T::Value: Hash,
 {
     let type_ = field.type_().unmodified_type();
     let type_name = type_
@@ -643,6 +647,7 @@ pub fn to_function_call_builder<'a, T>(
 ) -> Result<FunctionCallBuilder, String>
 where
     T: Text<'a> + Eq + AsRef<str> + Clone,
+    T::Value: Hash,
 {
     let type_ = field.type_().unmodified_type();
     let alias = alias_or_name(query_field);
@@ -1337,6 +1342,7 @@ pub fn to_connection_builder<'a, T>(
 ) -> Result<ConnectionBuilder, String>
 where
     T: Text<'a> + Eq + AsRef<str> + Clone,
+    T::Value: Hash,
 {
     let type_ = field.type_().unmodified_type();
     let type_ = type_.return_type();
@@ -1510,6 +1516,7 @@ fn to_page_info_builder<'a, T>(
 ) -> Result<PageInfoBuilder, String>
 where
     T: Text<'a> + Eq + AsRef<str> + Clone,
+    T::Value: Hash,
 {
     let type_ = field.type_().unmodified_type();
     let type_name = type_.name().ok_or(format!(
@@ -1572,6 +1579,7 @@ fn to_edge_builder<'a, T>(
 ) -> Result<EdgeBuilder, String>
 where
     T: Text<'a> + Eq + AsRef<str> + Clone,
+    T::Value: Hash,
 {
     let type_ = field.type_().unmodified_type();
     let type_name = type_.name().ok_or(format!(
@@ -1639,6 +1647,7 @@ pub fn to_node_builder<'a, T>(
 ) -> Result<NodeBuilder, String>
 where
     T: Text<'a> + Eq + AsRef<str> + Clone,
+    T::Value: Hash,
 {
     let type_ = field.type_().unmodified_type();
 
@@ -1982,6 +1991,7 @@ impl __Schema {
     ) -> Result<__EnumValueBuilder, String>
     where
         T: Text<'a> + Eq + AsRef<str> + Clone,
+        T::Value: Hash,
     {
         let selection_fields = normalize_selection_set(
             &query_field.selection_set,
@@ -2034,6 +2044,7 @@ impl __Schema {
     ) -> Result<__InputValueBuilder, String>
     where
         T: Text<'a> + Eq + AsRef<str> + Clone,
+        T::Value: Hash,
     {
         let selection_fields = normalize_selection_set(
             &query_field.selection_set,
@@ -2099,6 +2110,7 @@ impl __Schema {
     ) -> Result<__FieldBuilder, String>
     where
         T: Text<'a> + Eq + AsRef<str> + Clone,
+        T::Value: Hash,
     {
         let selection_fields = normalize_selection_set(
             &query_field.selection_set,
@@ -2175,6 +2187,7 @@ impl __Schema {
     ) -> Result<Option<__TypeBuilder>, String>
     where
         T: Text<'a> + Eq + AsRef<str> + Clone,
+        T::Value: Hash,
     {
         if field.type_.unmodified_type() != __Type::__Type(__TypeType {}) {
             return Err("can not build query for non-__type type".to_string());
@@ -2227,6 +2240,7 @@ impl __Schema {
     ) -> Result<__TypeBuilder, String>
     where
         T: Text<'a> + Eq + AsRef<str> + Clone,
+        T::Value: Hash,
     {
         let field_map = field_map(&__Type::__Type(__TypeType {}));
 
@@ -2421,6 +2435,7 @@ impl __Schema {
     ) -> Result<__DirectiveBuilder, String>
     where
         T: Text<'a> + Eq + AsRef<str> + Clone,
+        T::Value: Hash,
     {
         let selection_fields = normalize_selection_set(
             &query_field.selection_set,
@@ -2490,6 +2505,7 @@ impl __Schema {
     ) -> Result<__SchemaBuilder, String>
     where
         T: Text<'a> + Eq + AsRef<str> + Clone,
+        T::Value: Hash,
     {
         let type_ = field.type_.unmodified_type();
         let type_name = type_

src/merge.rs

@@ -1,18 +1,21 @@
+use std::{collections::HashMap, hash::Hash};
+
 use graphql_parser::query::{Field, Text, Value};
 use indexmap::IndexMap;
 
 use crate::parser_util::alias_or_name;
 
 /// Merges duplicates in a vector of fields. The fields in the vector are added to a
 /// map from field name to field. If a field with the same name already exists in the
-/// map, the existing and new fields' children are combined into the existing field's
+/// map, the existing and new field's children are combined into the existing field's
 /// children. These children will be merged later when they are normalized.
 ///
 /// The map is an `IndexMap` to ensure iteration order of the fields is preserved.
 /// This prevents tests from being flaky due to field order changing between test runs.
 pub fn merge<'a, 'b, T>(fields: Vec<Field<'a, T>>) -> Result<Vec<Field<'a, T>>, String>
 where
     T: Text<'a> + Eq + AsRef<str>,
+    T::Value: Hash,
 {
     let mut merged: IndexMap<String, Field<'a, T>> = IndexMap::new();
 
@@ -41,6 +44,7 @@ where
 fn can_merge<'a, T>(field_a: &Field<'a, T>, field_b: &Field<'a, T>) -> Result<bool, String>
 where
     T: Text<'a> + Eq + AsRef<str>,
+    T::Value: Hash,
 {
     if field_a.name != field_b.name {
         return Err(format!(
@@ -59,24 +63,81 @@ where
     Ok(true)
 }
 
+/// Compares two sets of arguments and returns true only if
+/// both are the same. `arguments_a` should not have two
+/// arguments with the same name. Similarly `arguments_b`
+/// should not have duplicates. It is assumed that [Argument
+/// Uniqueness] validation has already been run by the time
+/// this function is called.
+///
+/// [Argument Uniqueness]: https://spec.graphql.org/October2021/#sec-Argument-Uniqueness
 fn same_arguments<'a, 'b, T>(
     arguments_a: &[(T::Value, Value<'a, T>)],
     arguments_b: &[(T::Value, Value<'a, T>)],
 ) -> bool
 where
     T: Text<'a> + Eq + AsRef<str>,
+    T::Value: Hash,
 {
     if arguments_a.len() != arguments_b.len() {
         return false;
     }
 
+    let mut arguments_a_map = HashMap::new();
     for (arg_a_name, arg_a_val) in arguments_a {
-        for (arg_b_name, arg_b_val) in arguments_b {
-            if arg_a_name == arg_b_name && arg_a_val != arg_b_val {
-                return false;
+        arguments_a_map.insert(arg_a_name, arg_a_val);
+    }
+
+    for (arg_b_name, arg_b_val) in arguments_b {
+        match arguments_a_map.get(arg_b_name) {
+            Some(arg_a_val) => {
+                if *arg_a_val != arg_b_val {
+                    return false;
+                }
             }
+            None => return false,
         }
     }
 
     true
 }
+
+#[cfg(test)]
+mod tests {
+
+    use super::same_arguments;
+    use graphql_parser::query::Value;
+
+    #[test]
+    fn same_args_test() {
+        let arguments_a = vec![("a", Value::Int(1.into()))];
+        let arguments_b = vec![("a", Value::Int(1.into()))];
+        let result = same_arguments::<&str>(&arguments_a, &arguments_b);
+        assert!(result);
+
+        let arguments_a = vec![("a", Value::Int(1.into())), ("b", Value::Int(2.into()))];
+        let arguments_b = vec![("a", Value::Int(1.into())), ("b", Value::Int(2.into()))];
+        let result = same_arguments::<&str>(&arguments_a, &arguments_b);
+        assert!(result);
+
+        let arguments_a = vec![("a", Value::Int(1.into())), ("b", Value::Int(2.into()))];
+        let arguments_b = vec![("b", Value::Int(2.into())), ("a", Value::Int(1.into()))];
+        let result = same_arguments::<&str>(&arguments_a, &arguments_b);
+        assert!(result);
+
+        let arguments_a = vec![("a", Value::Int(1.into()))];
+        let arguments_b = vec![("a", Value::Int(2.into()))];
+        let result = same_arguments::<&str>(&arguments_a, &arguments_b);
+        assert!(!result);
+
+        let arguments_a = vec![("a", Value::Int(1.into())), ("b", Value::Int(1.into()))];
+        let arguments_b = vec![("a", Value::Int(1.into()))];
+        let result = same_arguments::<&str>(&arguments_a, &arguments_b);
+        assert!(!result);
+
+        let arguments_a = vec![("a", Value::Int(1.into()))];
+        let arguments_b = vec![("b", Value::Int(1.into()))];
+        let result = same_arguments::<&str>(&arguments_a, &arguments_b);
+        assert!(!result);
+    }
+}

src/parser_util.rs

@@ -2,6 +2,7 @@ use crate::graphql::{EnumSource, __InputValue, __Type, ___Type};
 use crate::{gson, merge::merge};
 use graphql_parser::query::*;
 use std::collections::HashMap;
+use std::hash::Hash;
 
 pub fn alias_or_name<'a, T>(query_field: &graphql_parser::query::Field<'a, T>) -> String
 where
@@ -22,6 +23,7 @@ pub fn normalize_selection_set<'a, 'b, T>(
 ) -> Result<Vec<Field<'a, T>>, String>
 where
     T: Text<'a> + Eq + AsRef<str> + Clone,
+    T::Value: Hash,
 {
     let mut selections: Vec<Field<'a, T>> = vec![];
 
@@ -139,6 +141,7 @@ pub fn normalize_selection<'a, 'b, T>(
 ) -> Result<Vec<Field<'a, T>>, String>
 where
     T: Text<'a> + Eq + AsRef<str> + Clone,
+    T::Value: Hash,
 {
     let mut selections: Vec<Field<'a, T>> = vec![];
 

src/resolve.rs

@@ -1,4 +1,5 @@
 use std::collections::HashSet;
+use std::hash::Hash;
 
 use crate::builder::*;
 use crate::graphql::*;
@@ -23,6 +24,7 @@ pub fn resolve_inner<'a, T>(
 ) -> GraphQLResponse
 where
     T: Text<'a> + Eq + AsRef<str> + Clone,
+    T::Value: Hash,
 {
     match variables {
         serde_json::Value::Object(_) => (),
@@ -131,6 +133,7 @@ fn resolve_query<'a, 'b, T>(
 ) -> GraphQLResponse
 where
     T: Text<'a> + Eq + AsRef<str> + Clone,
+    T::Value: Hash,
 {
     let variable_definitions = &query.variable_definitions;
     resolve_selection_set(
@@ -151,6 +154,7 @@ fn resolve_selection_set<'a, 'b, T>(
 ) -> GraphQLResponse
 where
     T: Text<'a> + Eq + AsRef<str> + Clone,
+    T::Value: Hash,
 {
     use crate::graphql::*;
 
@@ -338,6 +342,7 @@ fn resolve_mutation<'a, 'b, T>(
 ) -> GraphQLResponse
 where
     T: Text<'a> + Eq + AsRef<str> + Clone,
+    T::Value: Hash,
 {
     let variable_definitions = &query.variable_definitions;
     resolve_mutation_selection_set(
@@ -358,6 +363,7 @@ fn resolve_mutation_selection_set<'a, 'b, T>(
 ) -> GraphQLResponse
 where
     T: Text<'a> + Eq + AsRef<str> + Clone,
+    T::Value: Hash,
 {
     use crate::graphql::*;