Move ValidateName calls after NameConverter is applied (graphql-dotnet#1961)

Shane32 · web-flow · commit 28b9a1eb4a93 · 2020-10-28T13:06:54.000-04:00
diff --git a/src/GraphQL.Tests/Types/ComplexGraphTypeTests.cs b/src/GraphQL.Tests/Types/ComplexGraphTypeTests.cs
@@ -4,6 +4,7 @@
 using System.ComponentModel;
 using System.ComponentModel.DataAnnotations;
 using System.Linq;
+using GraphQL.Conversion;
 using GraphQL.StarWars.Types;
 using GraphQL.Types;
 using GraphQL.Utilities;
@@ -302,13 +303,27 @@ public void throws_informative_exception_when_no_types_defined_on_more_generic_t
             exception.Message.ShouldStartWith("The declared field 'genericname' on 'ListOfDroid' requires a field 'Type' when no 'ResolvedType' is provided.");
         }
 
+        private Exception test_field_name(string fieldName)
+        {
+            // test failure
+            return Should.Throw<ArgumentOutOfRangeException>(() =>
+            {
+                var type = new ObjectGraphType();
+                type.Field<StringGraphType>(fieldName);
+                var schema = new Schema
+                {
+                    Query = type
+                };
+                schema.Initialize();
+            });
+        }
+
         [Theory]
         [InlineData("__id")]
         [InlineData("___id")]
         public void throws_when_field_name_prefix_with_reserved_underscores(string fieldName)
         {
-            var type = new ComplexType<TestObject>();
-            var exception = Should.Throw<ArgumentOutOfRangeException>(() => type.Field<StringGraphType>(fieldName));
+            var exception = test_field_name(fieldName);
 
             exception.Message.ShouldStartWith($"A field name: {fieldName} must not begin with \"__\", which is reserved by GraphQL introspection.");
         }
@@ -319,12 +334,64 @@ public void throws_when_field_name_prefix_with_reserved_underscores(string field
         [InlineData("id$")]
         public void throws_when_field_name_doesnot_follow_spec(string fieldName)
         {
-            var type = new ComplexType<TestObject>();
-            var exception = Should.Throw<ArgumentOutOfRangeException>(() => type.Field<StringGraphType>(fieldName));
+            var exception = test_field_name(fieldName);
 
             exception.Message.ShouldStartWith($"A field name must match /^[_a-zA-Z][_a-zA-Z0-9]*$/ but {fieldName} does not.");
         }
 
+        [Theory]
+        [InlineData("__id")]
+        [InlineData("___id")]
+        [InlineData("i#d")]
+        [InlineData("i$d")]
+        [InlineData("id$")]
+        public void does_not_throw_with_filtering_nameconverter(string fieldName)
+        {
+            var type = new ObjectGraphType();
+            type.Field<StringGraphType>(fieldName);
+            var schema = new Schema
+            {
+                Query = type,
+                NameConverter = new TestNameConverter(fieldName, "pass")
+            };
+            schema.Initialize();
+        }
+
+        [Fact]
+        public void throws_with_bad_namevalidator()
+        {
+            var exception = Should.Throw<ArgumentOutOfRangeException>(() =>
+            {
+                var type = new ObjectGraphType();
+                type.Field<StringGraphType>("hello");
+                var schema = new Schema
+                {
+                    Query = type,
+                    NameConverter = new TestNameConverter("hello", "hello$")
+                };
+                schema.Initialize();
+            });
+
+            exception.Message.ShouldStartWith($"A field name must match /^[_a-zA-Z][_a-zA-Z0-9]*$/ but hello$ does not.");
+        }
+
+        private class TestNameConverter : INameConverter
+        {
+            private readonly string _from;
+            private readonly string _to;
+            public TestNameConverter(string from, string to)
+            {
+                _from = from;
+                _to = to;
+            }
+
+            public string NameForArgument(string argumentName, IComplexGraphType parentGraphType, FieldType field)
+                => argumentName == _from ? _to : argumentName;
+
+            public string NameForField(string fieldName, IComplexGraphType parentGraphType)
+                => fieldName == _from ? _to : fieldName;
+        }
+
         [Theory]
         [InlineData("")]
         [InlineData(null)]
diff --git a/src/GraphQL/Builders/ConnectionBuilder.cs b/src/GraphQL/Builders/ConnectionBuilder.cs
@@ -107,8 +107,6 @@ public ConnectionBuilder<TSourceType> Bidirectional()
 
         public ConnectionBuilder<TSourceType> Name(string name)
         {
-            NameValidator.ValidateName(name);
-
             FieldType.Name = name;
             return this;
         }
diff --git a/src/GraphQL/Builders/FieldBuilder.cs b/src/GraphQL/Builders/FieldBuilder.cs
@@ -55,8 +55,6 @@ public virtual FieldBuilder<TSourceType, TReturnType> Type(IGraphType type)
 
         public virtual FieldBuilder<TSourceType, TReturnType> Name(string name)
         {
-            NameValidator.ValidateName(name);
-
             FieldType.Name = name;
             return this;
         }
diff --git a/src/GraphQL/Types/Composite/ComplexGraphType.cs b/src/GraphQL/Types/Composite/ComplexGraphType.cs
@@ -75,7 +75,8 @@ public virtual FieldType AddField(FieldType fieldType)
                 }
             }
 
-            NameValidator.ValidateName(fieldType.Name);
+            //check if fieldType.Name has never been set
+            NameValidator.ValidateNameNotNull(fieldType.Name);
 
             if (HasField(fieldType.Name))
             {
diff --git a/src/GraphQL/Types/Fields/FieldType.cs b/src/GraphQL/Types/Fields/FieldType.cs
@@ -12,7 +12,16 @@ public class FieldType : MetadataProvider, IFieldType, IProvideResolvedType
         private object _defaultValue;
         private IValue _defaultValueAST;
 
-        public string Name { get; set; }
+        private string _name;
+        public string Name
+        {
+            get => _name;
+            set
+            {
+                NameValidator.ValidateNameNotNull(value);
+                _name = value;
+            }
+        }
 
         public string Description { get; set; }
 
diff --git a/src/GraphQL/Types/GraphTypesLookup.cs b/src/GraphQL/Types/GraphTypesLookup.cs
@@ -5,6 +5,7 @@
 using GraphQL.Conversion;
 using GraphQL.Introspection;
 using GraphQL.Types.Relay;
+using GraphQL.Utilities;
 
 namespace GraphQL.Types
 {
@@ -362,6 +363,7 @@ private void HandleField(IComplexGraphType parentType, FieldType field, TypeColl
             if (applyNameConverter)
             {
                 field.Name = NameConverter.NameForField(field.Name, parentType);
+                NameValidator.ValidateName(field.Name);
             }
 
             if (field.ResolvedType == null)
@@ -382,6 +384,7 @@ private void HandleField(IComplexGraphType parentType, FieldType field, TypeColl
                 if (applyNameConverter)
                 {
                     arg.Name = NameConverter.NameForArgument(arg.Name, parentType, field);
+                    NameValidator.ValidateName(arg.Name, "argument");
                 }
 
                 if (arg.ResolvedType != null)
diff --git a/src/GraphQL/Types/QueryArgument.cs b/src/GraphQL/Types/QueryArgument.cs
@@ -37,7 +37,16 @@ public QueryArgument(Type type)
             Type = type;
         }
 
-        public string Name { get; set; }
+        private string _name;
+        public string Name
+        {
+            get => _name;
+            set
+            {
+                NameValidator.ValidateNameNotNull(value, "argument");
+                _name = value;
+            }
+        }
 
         public string Description { get; set; }
 
diff --git a/src/GraphQL/Types/QueryArguments.cs b/src/GraphQL/Types/QueryArguments.cs
@@ -28,7 +28,7 @@ public QueryArgument this[int index]
             get => ArgumentsList != null ? ArgumentsList[index] : throw new IndexOutOfRangeException();
             set
             {
-                if (value != null)
+                if (value != null && string.IsNullOrEmpty(value.Name))
                 {
                     NameValidator.ValidateName(value.Name, "argument");
                 }
@@ -49,7 +49,8 @@ public void Add(QueryArgument argument)
             if (argument == null)
                 throw new ArgumentNullException(nameof(argument));
 
-            NameValidator.ValidateName(argument.Name, "argument");
+            if (string.IsNullOrEmpty(argument.Name))
+                NameValidator.ValidateName(argument.Name, "argument");
 
             if (ArgumentsList == null)
                 ArgumentsList = new List<QueryArgument>();
diff --git a/src/GraphQL/Utilities/NameValidator.cs b/src/GraphQL/Utilities/NameValidator.cs
@@ -10,11 +10,7 @@ public static class NameValidator
 
         public static void ValidateName(string name, string type = "field")
         {
-            if (string.IsNullOrWhiteSpace(name))
-            {
-                throw new ArgumentOutOfRangeException(nameof(name),
-                    $"A {type} name can not be null or empty.");
-            }
+            ValidateNameNotNull(name, type);
 
             if (name.Length > 1 && name.StartsWith(RESERVED_PREFIX, StringComparison.InvariantCulture))
             {
@@ -27,5 +23,14 @@ public static void ValidateName(string name, string type = "field")
                     $"A {type} name must match /^[_a-zA-Z][_a-zA-Z0-9]*$/ but {name} does not.");
             }
         }
+
+        internal static void ValidateNameNotNull(string name, string type = "field")
+        {
+            if (string.IsNullOrWhiteSpace(name))
+            {
+                throw new ArgumentOutOfRangeException(nameof(name),
+                    $"A {type} name can not be null or empty.");
+            }
+        }
     }
 }

Original file line number	Diff line number	Diff line change
`@@ -107,8 +107,6 @@ public ConnectionBuilder<TSourceType> Bidirectional()`
`107`	`107`
`108`	`108`	`public ConnectionBuilder<TSourceType> Name(string name)`
`109`	`109`	`{`
`110`		`- NameValidator.ValidateName(name);`
`111`		`-`
`112`	`110`	`FieldType.Name = name;`
`113`	`111`	`return this;`
`114`	`112`	`}`
Original file line number	Diff line number	Diff line change
`@@ -55,8 +55,6 @@ public virtual FieldBuilder<TSourceType, TReturnType> Type(IGraphType type)`
`55`	`55`
`56`	`56`	`public virtual FieldBuilder<TSourceType, TReturnType> Name(string name)`
`57`	`57`	`{`
`58`		`- NameValidator.ValidateName(name);`
`59`		`-`
`60`	`58`	`FieldType.Name = name;`
`61`	`59`	`return this;`
`62`	`60`	`}`
Original file line number	Diff line number	Diff line change
`@@ -75,7 +75,8 @@ public virtual FieldType AddField(FieldType fieldType)`
`75`	`75`	`}`
`76`	`76`	`}`
`77`	`77`
`78`		`- NameValidator.ValidateName(fieldType.Name);`
	`78`	`+ //check if fieldType.Name has never been set`
	`79`	`+ NameValidator.ValidateNameNotNull(fieldType.Name);`
`79`	`80`
`80`	`81`	`if (HasField(fieldType.Name))`
`81`	`82`	`{`
Original file line number	Diff line number	Diff line change
`@@ -5,6 +5,7 @@`
`5`	`5`	`using GraphQL.Conversion;`
`6`	`6`	`using GraphQL.Introspection;`
`7`	`7`	`using GraphQL.Types.Relay;`
	`8`	`+using GraphQL.Utilities;`
`8`	`9`
`9`	`10`	`namespace GraphQL.Types`
`10`	`11`	`{`
`@@ -362,6 +363,7 @@ private void HandleField(IComplexGraphType parentType, FieldType field, TypeColl`
`362`	`363`	`if (applyNameConverter)`
`363`	`364`	`{`
`364`	`365`	`field.Name = NameConverter.NameForField(field.Name, parentType);`
	`366`	`+ NameValidator.ValidateName(field.Name);`
`365`	`367`	`}`
`366`	`368`
`367`	`369`	`if (field.ResolvedType == null)`
`@@ -382,6 +384,7 @@ private void HandleField(IComplexGraphType parentType, FieldType field, TypeColl`
`382`	`384`	`if (applyNameConverter)`
`383`	`385`	`{`
`384`	`386`	`arg.Name = NameConverter.NameForArgument(arg.Name, parentType, field);`
	`387`	`+ NameValidator.ValidateName(arg.Name, "argument");`
`385`	`388`	`}`
`386`	`389`
`387`	`390`	`if (arg.ResolvedType != null)`
Original file line number	Diff line number	Diff line change
`@@ -28,7 +28,7 @@ public QueryArgument this[int index]`
`28`	`28`	`get => ArgumentsList != null ? ArgumentsList[index] : throw new IndexOutOfRangeException();`
`29`	`29`	`set`
`30`	`30`	`{`
`31`		`- if (value != null)`
	`31`	`+ if (value != null && string.IsNullOrEmpty(value.Name))`
`32`	`32`	`{`
`33`	`33`	`NameValidator.ValidateName(value.Name, "argument");`
`34`	`34`	`}`
`@@ -49,7 +49,8 @@ public void Add(QueryArgument argument)`
`49`	`49`	`if (argument == null)`
`50`	`50`	`throw new ArgumentNullException(nameof(argument));`
`51`	`51`
`52`		`- NameValidator.ValidateName(argument.Name, "argument");`
	`52`	`+ if (string.IsNullOrEmpty(argument.Name))`
	`53`	`+ NameValidator.ValidateName(argument.Name, "argument");`
`53`	`54`
`54`	`55`	`if (ArgumentsList == null)`
`55`	`56`	`ArgumentsList = new List<QueryArgument>();`
Original file line number	Diff line number	Diff line change
`@@ -10,11 +10,7 @@ public static class NameValidator`
`10`	`10`
`11`	`11`	`public static void ValidateName(string name, string type = "field")`
`12`	`12`	`{`
`13`		`- if (string.IsNullOrWhiteSpace(name))`
`14`		`- {`
`15`		`- throw new ArgumentOutOfRangeException(nameof(name),`
`16`		`- $"A {type} name can not be null or empty.");`
`17`		`- }`
	`13`	`+ ValidateNameNotNull(name, type);`
`18`	`14`
`19`	`15`	`if (name.Length > 1 && name.StartsWith(RESERVED_PREFIX, StringComparison.InvariantCulture))`
`20`	`16`	`{`
`@@ -27,5 +23,14 @@ public static void ValidateName(string name, string type = "field")`
`27`	`23`	`$"A {type} name must match /^[_a-zA-Z][_a-zA-Z0-9]*$/ but {name} does not.");`
`28`	`24`	`}`
`29`	`25`	`}`
	`26`	`+`
	`27`	`+ internal static void ValidateNameNotNull(string name, string type = "field")`
	`28`	`+ {`
	`29`	`+ if (string.IsNullOrWhiteSpace(name))`
	`30`	`+ {`
	`31`	`+ throw new ArgumentOutOfRangeException(nameof(name),`
	`32`	`+ $"A {type} name can not be null or empty.");`
	`33`	`+ }`
	`34`	`+ }`
`30`	`35`	`}`
`31`	`36`	`}`