Deserializer should handle nullable StringEnum<T> (#1760)

* add deserializer tests for nullable enums, StringEnum and nullable StringEnum properties

* Fix deserializing nullable structs by using the underlying type when nullable

* StringEnum<T> should behave like an enum, returning default(T) when it is uninitialised/null/blank

* Don't allow null to be passed into StringEnum ctor - if it needs to be null then it should be declared as nullable

* fix expected json

* move logic to determine if property is a StringEnum<T> into helper function

* serializer needs to treat StringEnum<T> specially by serializing the enum value according to existing serializer strategy (parameter attributes and so on)

* remove fallback to default(T)

* add test to assert ctor throws exception when null passed in

* remove test for default(T) fallback behaviour

* Fix exception in serializer test - StringEnum property must be initialized otherwise an exception is thrown when attempting to serialize

* Dont allow empty strings either

Octokit.Tests/Models/StringEnumTests.cs

@@ -7,6 +7,14 @@ namespace Octokit.Tests.Models
     {
         public class TheCtor
         {
+            [Fact]
+            public void ShouldThrowForNullOrEmptyStringValues()
+            {
+                Assert.Throws<ArgumentNullException>(() => new StringEnum<AccountType>(null));
+
+                Assert.Throws<ArgumentException>(() => new StringEnum<AccountType>(""));
+            }
+
             [Fact]
             public void ShouldSetValue()
             {
@@ -42,16 +50,12 @@ namespace Octokit.Tests.Models
             }
         }
 
-        public class TheParsedValueProperty
+        public class TheValueProperty
         {
-            [Theory]
-            [InlineData("")]
-            [InlineData(null)]
-            [InlineData("Cow")]
-            public void ShouldThrowForInvalidValue(string value)
+            [Fact]
+            public void ShouldThrowForInvalidValue()
             {
-                var stringEnum = new StringEnum<AccountType>(value);
-
+                var stringEnum = new StringEnum<AccountType>("Cow");
                 Assert.Throws<ArgumentException>(() => stringEnum.Value);
             }
 
@@ -96,13 +100,10 @@ namespace Octokit.Tests.Models
                 Assert.True(result);
             }
 
-            [Theory]
-            [InlineData("")]
-            [InlineData(null)]
-            [InlineData("Cow")]
-            public void ShouldReturnFalseForInvalidValue(string value)
+            [Fact]
+            public void ShouldReturnFalseForInvalidValue()
             {
-                var stringEnum = new StringEnum<AccountType>(value);
+                var stringEnum = new StringEnum<AccountType>("Cow");
 
                 AccountType type;
                 var result = stringEnum.TryParse(out type);

Octokit.Tests/SimpleJsonSerializerTests.cs

@@ -116,12 +116,30 @@ namespace Octokit.Tests
                 var item = new ObjectWithEnumProperty
                 {
                     Name = "Ferris Bueller",
-                    SomeEnum = SomeEnum.PlusOne
+                    SomeEnum = SomeEnum.Unicode,
+                    SomeEnumNullable = SomeEnum.Unicode,
+                    StringEnum = SomeEnum.SomethingElse,
+                    StringEnumNullable = SomeEnum.SomethingElse
                 };
 
                 var json = new SimpleJsonSerializer().Serialize(item);
 
-                Assert.Equal("{\"name\":\"Ferris Bueller\",\"some_enum\":\"+1\"}", json);
+                Assert.Equal("{\"name\":\"Ferris Bueller\",\"some_enum\":\"unicode\",\"some_enum_nullable\":\"unicode\",\"string_enum\":\"something else\",\"string_enum_nullable\":\"something else\"}", json);
+            }
+
+            [Fact]
+            public void HandlesEnumDefaults()
+            {
+                var item = new ObjectWithEnumProperty
+                {
+                    Name = "Ferris Bueller",
+                    SomeEnum = SomeEnum.PlusOne,
+                    StringEnum = SomeEnum.PlusOne
+                };
+
+                var json = new SimpleJsonSerializer().Serialize(item);
+
+                Assert.Equal("{\"name\":\"Ferris Bueller\",\"some_enum\":\"+1\",\"string_enum\":\"+1\"}", json);
             }
         }
 
@@ -301,7 +319,7 @@ namespace Octokit.Tests
             }
 
             [Fact]
-            public void DeserializesEnumWithParameterAttribute()
+            public void DeserializesEnums()
             {
                 const string json1 = @"{""some_enum"":""+1""}";
                 const string json2 = @"{""some_enum"":""utf-8""}";
@@ -322,6 +340,78 @@ namespace Octokit.Tests
                 Assert.Equal(SomeEnum.Unicode, sample5.SomeEnum);
             }
 
+            [Fact]
+            public void DeserializesNullableEnums()
+            {
+                const string json1 = @"{""some_enum_nullable"":""+1""}";
+                const string json2 = @"{""some_enum_nullable"":""utf-8""}";
+                const string json3 = @"{""some_enum_nullable"":""something else""}";
+                const string json4 = @"{""some_enum_nullable"":""another_example""}";
+                const string json5 = @"{""some_enum_nullable"":""unicode""}";
+                const string json6 = @"{""some_enum_nullable"":null}";
+
+                var sample1 = new SimpleJsonSerializer().Deserialize<ObjectWithEnumProperty>(json1);
+                var sample2 = new SimpleJsonSerializer().Deserialize<ObjectWithEnumProperty>(json2);
+                var sample3 = new SimpleJsonSerializer().Deserialize<ObjectWithEnumProperty>(json3);
+                var sample4 = new SimpleJsonSerializer().Deserialize<ObjectWithEnumProperty>(json4);
+                var sample5 = new SimpleJsonSerializer().Deserialize<ObjectWithEnumProperty>(json5);
+                var sample6 = new SimpleJsonSerializer().Deserialize<ObjectWithEnumProperty>(json6);
+
+                Assert.Equal(SomeEnum.PlusOne, sample1.SomeEnumNullable);
+                Assert.Equal(SomeEnum.Utf8, sample2.SomeEnumNullable);
+                Assert.Equal(SomeEnum.SomethingElse, sample3.SomeEnumNullable);
+                Assert.Equal(SomeEnum.AnotherExample, sample4.SomeEnumNullable);
+                Assert.Equal(SomeEnum.Unicode, sample5.SomeEnumNullable);
+                Assert.False(sample6.SomeEnumNullable.HasValue);
+            }
+
+            [Fact]
+            public void DeserializesStringEnums()
+            {
+                const string json1 = @"{""string_enum"":""+1""}";
+                const string json2 = @"{""string_enum"":""utf-8""}";
+                const string json3 = @"{""string_enum"":""something else""}";
+                const string json4 = @"{""string_enum"":""another_example""}";
+                const string json5 = @"{""string_enum"":""unicode""}";
+
+                var sample1 = new SimpleJsonSerializer().Deserialize<ObjectWithEnumProperty>(json1);
+                var sample2 = new SimpleJsonSerializer().Deserialize<ObjectWithEnumProperty>(json2);
+                var sample3 = new SimpleJsonSerializer().Deserialize<ObjectWithEnumProperty>(json3);
+                var sample4 = new SimpleJsonSerializer().Deserialize<ObjectWithEnumProperty>(json4);
+                var sample5 = new SimpleJsonSerializer().Deserialize<ObjectWithEnumProperty>(json5);
+
+                Assert.Equal(SomeEnum.PlusOne, sample1.StringEnum);
+                Assert.Equal(SomeEnum.Utf8, sample2.StringEnum);
+                Assert.Equal(SomeEnum.SomethingElse, sample3.StringEnum);
+                Assert.Equal(SomeEnum.AnotherExample, sample4.StringEnum);
+                Assert.Equal(SomeEnum.Unicode, sample5.StringEnum);
+            }
+
+            [Fact]
+            public void DeserializesNullableStringEnums()
+            {
+                const string json1 = @"{""string_enum_nullable"":""+1""}";
+                const string json2 = @"{""string_enum_nullable"":""utf-8""}";
+                const string json3 = @"{""string_enum_nullable"":""something else""}";
+                const string json4 = @"{""string_enum_nullable"":""another_example""}";
+                const string json5 = @"{""string_enum_nullable"":""unicode""}";
+                const string json6 = @"{""string_enum_nullable"":null}";
+
+                var sample1 = new SimpleJsonSerializer().Deserialize<ObjectWithEnumProperty>(json1);
+                var sample2 = new SimpleJsonSerializer().Deserialize<ObjectWithEnumProperty>(json2);
+                var sample3 = new SimpleJsonSerializer().Deserialize<ObjectWithEnumProperty>(json3);
+                var sample4 = new SimpleJsonSerializer().Deserialize<ObjectWithEnumProperty>(json4);
+                var sample5 = new SimpleJsonSerializer().Deserialize<ObjectWithEnumProperty>(json5);
+                var sample6 = new SimpleJsonSerializer().Deserialize<ObjectWithEnumProperty>(json6);
+
+                Assert.Equal(SomeEnum.PlusOne, sample1.StringEnumNullable);
+                Assert.Equal(SomeEnum.Utf8, sample2.StringEnumNullable);
+                Assert.Equal(SomeEnum.SomethingElse, sample3.StringEnumNullable);
+                Assert.Equal(SomeEnum.AnotherExample, sample4.StringEnumNullable);
+                Assert.Equal(SomeEnum.Unicode, sample5.StringEnumNullable);
+                Assert.False(sample6.StringEnumNullable.HasValue);
+            }
+
             [Fact]
             public void ShouldDeserializeMultipleEnumValues()
             {
@@ -369,6 +459,12 @@ namespace Octokit.Tests
         public string Name { get; set; }
 
         public SomeEnum SomeEnum { get; set; }
+
+        public SomeEnum? SomeEnumNullable { get; set; }
+
+        public StringEnum<SomeEnum> StringEnum { get; set; }
+
+        public StringEnum<SomeEnum>? StringEnumNullable { get; set; }
     }
 
     public enum SomeEnum

Octokit/Http/SimpleJsonSerializer.cs

@@ -66,8 +66,21 @@ namespace Octokit.Internal
                 Ensure.ArgumentNotNull(input, "input");
 
                 var type = input.GetType();
-                var jsonObject = new JsonObject();
                 var getters = GetCache[type];
+
+                if (ReflectionUtils.IsStringEnumWrapper(type))
+                {
+                    // Handle StringEnum<T> by getting the underlying enum value, then using the enum serializer
+                    // Note this will throw if the StringEnum<T> was initialised using a string that is not a valid enum member
+                    var inputEnum = (getters["value"](input) as Enum);
+                    if (inputEnum != null)
+                    {
+                        output = SerializeEnum(inputEnum);
+                        return true;
+                    }
+                }
+
+                var jsonObject = new JsonObject();
                 foreach (var getter in getters)
                 {
                     if (getter.Value != null)
@@ -134,6 +147,12 @@ namespace Octokit.Internal
 
                 if (stringValue != null)
                 {
+                    // If it's a nullable type, use the underlying type
+                    if (ReflectionUtils.IsNullableType(type))
+                    {
+                        type = Nullable.GetUnderlyingType(type);
+                    }
+
                     var typeInfo = ReflectionUtils.GetTypeInfo(type);
 
                     if (typeInfo.IsEnum)
@@ -141,15 +160,6 @@ namespace Octokit.Internal
                         return DeserializeEnumHelper(stringValue, type);
                     }
 
-                    if (ReflectionUtils.IsNullableType(type))
-                    {
-                        var underlyingType = Nullable.GetUnderlyingType(type);
-                        if (ReflectionUtils.GetTypeInfo(underlyingType).IsEnum)
-                        {
-                            return DeserializeEnumHelper(stringValue, underlyingType);
-                        }
-                    }
-
                     if (ReflectionUtils.IsTypeGenericeCollectionInterface(type))
                     {
                         // OAuth tokens might be a string of comma-separated values
@@ -161,14 +171,9 @@ namespace Octokit.Internal
                         }
                     }
 
-                    if (typeInfo.IsGenericType)
+                    if (ReflectionUtils.IsStringEnumWrapper(type))
                     {
-                        var typeDefinition = typeInfo.GetGenericTypeDefinition();
-
-                        if (typeof(StringEnum<>).IsAssignableFrom(typeDefinition))
-                        {
-                            return Activator.CreateInstance(type, stringValue);
-                        }
+                        return Activator.CreateInstance(type, stringValue);
                     }
                 }
                 else if (jsonValue != null)

Octokit/Models/Response/StringEnum.cs

@@ -29,7 +29,9 @@ namespace Octokit
 
         public StringEnum(string stringValue)
         {
-            _stringValue = stringValue ?? string.Empty;
+            Ensure.ArgumentNotNullOrEmptyString(stringValue, nameof(stringValue));
+
+            _stringValue = stringValue;
             _parsedValue = null;
         }
 
@@ -66,12 +68,6 @@ namespace Octokit
                 return true;
             }
 
-            if (string.IsNullOrEmpty(StringValue))
-            {
-                value = default(TEnum);
-                return false;
-            }
-
             try
             {
                 // Use the SimpleJsonSerializer to parse the string to Enum according to the GitHub Api strategy

Octokit/SimpleJson.cs

@@ -1772,6 +1772,18 @@ namespace Octokit
                 return GetTypeInfo(type1).IsAssignableFrom(GetTypeInfo(type2));
             }
 
+            public static bool IsStringEnumWrapper(Type type)
+            {
+                var typeInfo = ReflectionUtils.GetTypeInfo(type);
+                if (typeInfo.IsGenericType)
+                {
+                    var typeDefinition = typeInfo.GetGenericTypeDefinition();
+
+                    return typeof(StringEnum<>).IsAssignableFrom(typeDefinition);
+                }
+                return false;
+            }
+
             public static IEnumerable<Type> GetInterfaces(Type type)
             {
 #if SIMPLE_JSON_TYPEINFO