From 41b4059c110a60aee86a912dda2987fecc5a3fcb Mon Sep 17 00:00:00 2001 From: Ryan Gribble Date: Fri, 16 Feb 2018 20:12:44 +1000 Subject: [PATCH] Deserializer should handle nullable StringEnum (#1760) * add deserializer tests for nullable enums, StringEnum and nullable StringEnum properties * Fix deserializing nullable structs by using the underlying type when nullable * StringEnum 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 into helper function * serializer needs to treat StringEnum 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 | 29 +++--- Octokit.Tests/SimpleJsonSerializerTests.cs | 102 ++++++++++++++++++++- Octokit/Http/SimpleJsonSerializer.cs | 39 ++++---- Octokit/Models/Response/StringEnum.cs | 10 +- Octokit/SimpleJson.cs | 12 +++ 5 files changed, 151 insertions(+), 41 deletions(-) diff --git a/Octokit.Tests/Models/StringEnumTests.cs b/Octokit.Tests/Models/StringEnumTests.cs index c4bfa9502a..64259b050f 100644 --- a/Octokit.Tests/Models/StringEnumTests.cs +++ b/Octokit.Tests/Models/StringEnumTests.cs @@ -7,6 +7,14 @@ public class StringEnumTests { public class TheCtor { + [Fact] + public void ShouldThrowForNullOrEmptyStringValues() + { + Assert.Throws(() => new StringEnum(null)); + + Assert.Throws(() => new StringEnum("")); + } + [Fact] public void ShouldSetValue() { @@ -42,16 +50,12 @@ public void ShouldThrowForInvalidEnumValue() } } - 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(value); - + var stringEnum = new StringEnum("Cow"); Assert.Throws(() => stringEnum.Value); } @@ -96,13 +100,10 @@ public void ShouldReturnTrueForValidValue() Assert.True(result); } - [Theory] - [InlineData("")] - [InlineData(null)] - [InlineData("Cow")] - public void ShouldReturnFalseForInvalidValue(string value) + [Fact] + public void ShouldReturnFalseForInvalidValue() { - var stringEnum = new StringEnum(value); + var stringEnum = new StringEnum("Cow"); AccountType type; var result = stringEnum.TryParse(out type); diff --git a/Octokit.Tests/SimpleJsonSerializerTests.cs b/Octokit.Tests/SimpleJsonSerializerTests.cs index 7194cd352e..0d7be5098d 100644 --- a/Octokit.Tests/SimpleJsonSerializerTests.cs +++ b/Octokit.Tests/SimpleJsonSerializerTests.cs @@ -116,12 +116,30 @@ public void HandlesEnum() 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 @@ public void DefaultsMissingParameters() } [Fact] - public void DeserializesEnumWithParameterAttribute() + public void DeserializesEnums() { const string json1 = @"{""some_enum"":""+1""}"; const string json2 = @"{""some_enum"":""utf-8""}"; @@ -322,6 +340,78 @@ public void DeserializesEnumWithParameterAttribute() 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(json1); + var sample2 = new SimpleJsonSerializer().Deserialize(json2); + var sample3 = new SimpleJsonSerializer().Deserialize(json3); + var sample4 = new SimpleJsonSerializer().Deserialize(json4); + var sample5 = new SimpleJsonSerializer().Deserialize(json5); + var sample6 = new SimpleJsonSerializer().Deserialize(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(json1); + var sample2 = new SimpleJsonSerializer().Deserialize(json2); + var sample3 = new SimpleJsonSerializer().Deserialize(json3); + var sample4 = new SimpleJsonSerializer().Deserialize(json4); + var sample5 = new SimpleJsonSerializer().Deserialize(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(json1); + var sample2 = new SimpleJsonSerializer().Deserialize(json2); + var sample3 = new SimpleJsonSerializer().Deserialize(json3); + var sample4 = new SimpleJsonSerializer().Deserialize(json4); + var sample5 = new SimpleJsonSerializer().Deserialize(json5); + var sample6 = new SimpleJsonSerializer().Deserialize(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 @@ public class ObjectWithEnumProperty public string Name { get; set; } public SomeEnum SomeEnum { get; set; } + + public SomeEnum? SomeEnumNullable { get; set; } + + public StringEnum StringEnum { get; set; } + + public StringEnum? StringEnumNullable { get; set; } } public enum SomeEnum diff --git a/Octokit/Http/SimpleJsonSerializer.cs b/Octokit/Http/SimpleJsonSerializer.cs index ba4fc52e3e..6067177b41 100644 --- a/Octokit/Http/SimpleJsonSerializer.cs +++ b/Octokit/Http/SimpleJsonSerializer.cs @@ -66,8 +66,21 @@ protected override bool TrySerializeUnknownTypes(object input, out object output Ensure.ArgumentNotNull(input, "input"); var type = input.GetType(); - var jsonObject = new JsonObject(); var getters = GetCache[type]; + + if (ReflectionUtils.IsStringEnumWrapper(type)) + { + // Handle StringEnum by getting the underlying enum value, then using the enum serializer + // Note this will throw if the StringEnum 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 @@ public override object DeserializeObject(object value, Type type) 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 @@ public override object DeserializeObject(object value, Type type) 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 @@ public override object DeserializeObject(object value, Type type) } } - 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) diff --git a/Octokit/Models/Response/StringEnum.cs b/Octokit/Models/Response/StringEnum.cs index 521fab7b5f..86680c80a3 100644 --- a/Octokit/Models/Response/StringEnum.cs +++ b/Octokit/Models/Response/StringEnum.cs @@ -29,7 +29,9 @@ public StringEnum(TEnum parsedValue) public StringEnum(string stringValue) { - _stringValue = stringValue ?? string.Empty; + Ensure.ArgumentNotNullOrEmptyString(stringValue, nameof(stringValue)); + + _stringValue = stringValue; _parsedValue = null; } @@ -66,12 +68,6 @@ public bool TryParse(out TEnum value) 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 diff --git a/Octokit/SimpleJson.cs b/Octokit/SimpleJson.cs index 016e9aab4c..87653eb92e 100644 --- a/Octokit/SimpleJson.cs +++ b/Octokit/SimpleJson.cs @@ -1772,6 +1772,18 @@ public static bool IsAssignableFrom(Type type1, Type type2) 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 GetInterfaces(Type type) { #if SIMPLE_JSON_TYPEINFO