Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Deserializer should handle nullable StringEnum<T> #1760

Merged
merged 12 commits into from
Feb 16, 2018
Merged

Deserializer should handle nullable StringEnum<T> #1760

Show file tree
Hide file tree
Changes from 7 commits
Commits
Show all changes
12 commits
Select commit Hold shift + click to select a range
2af9044
add deserializer tests for nullable enums, StringEnum and nullable St…
ryangribble Feb 11, 2018
5fa9975
Fix deserializing nullable structs by using the underlying type when …
ryangribble Feb 11, 2018
ae3fcb2
StringEnum<T> should behave like an enum, returning default(T) when i…
ryangribble Feb 12, 2018
6ba82d4
Don't allow null to be passed into StringEnum ctor - if it needs to b…
ryangribble Feb 12, 2018
2bfb345
fix expected json
ryangribble Feb 13, 2018
8382cec
move logic to determine if property is a StringEnum<T> into helper fu…
ryangribble Feb 13, 2018
0b6e19d
serializer needs to treat StringEnum<T> specially by serializing the …
ryangribble Feb 13, 2018
fb156d3
remove fallback to default(T)
ryangribble Feb 13, 2018
42d40ea
add test to assert ctor throws exception when null passed in
ryangribble Feb 13, 2018
1f86849
remove test for default(T) fallback behaviour
ryangribble Feb 13, 2018
959a34c
Fix exception in serializer test - StringEnum property must be initia…
ryangribble Feb 13, 2018
60089fe
Dont allow empty strings either
ryangribble Feb 13, 2018
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
15 changes: 12 additions & 3 deletions Octokit.Tests/Models/StringEnumTests.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -46,15 +46,25 @@ public class TheParsedValueProperty
{
[Theory]
[InlineData("")]
[InlineData(null)]
[InlineData("Cow")]
public void ShouldThrowForInvalidValue(string value)
{
var stringEnum = new StringEnum<AccountType>(value);

Assert.Throws<ArgumentException>(() => stringEnum.Value);
}

public class SomeObject
{
public StringEnum<AccountType> SomeEnumProperty { get; set; }
}

[Fact]
public void ShouldReturnDefaultWhenUninitialized()
{
var test = new SomeObject();
Assert.Equal(AccountType.User, test.SomeEnumProperty.Value);
}

[Fact]
public void ShouldHandleUnderscores()
{
Expand Down Expand Up @@ -98,7 +108,6 @@ public void ShouldReturnTrueForValidValue()

[Theory]
[InlineData("")]
[InlineData(null)]
[InlineData("Cow")]
public void ShouldReturnFalseForInvalidValue(string value)
{
Expand Down
100 changes: 97 additions & 3 deletions Octokit.Tests/SimpleJsonSerializerTests.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -116,12 +116,28 @@ 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"
};

var json = new SimpleJsonSerializer().Serialize(item);

Assert.Equal("{\"name\":\"Ferris Bueller\",\"some_enum\":\"+1\",\"string_enum\":\"+1\"}", json);
}
}

Expand Down Expand Up @@ -301,7 +317,7 @@ public void DefaultsMissingParameters()
}

[Fact]
public void DeserializesEnumWithParameterAttribute()
public void DeserializesEnums()
{
const string json1 = @"{""some_enum"":""+1""}";
const string json2 = @"{""some_enum"":""utf-8""}";
Expand All @@ -322,6 +338,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<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()
{
Expand Down Expand Up @@ -369,6 +457,12 @@ public class ObjectWithEnumProperty
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
Expand Down
39 changes: 22 additions & 17 deletions Octokit/Http/SimpleJsonSerializer.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -65,8 +65,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<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)
Expand Down Expand Up @@ -144,22 +157,19 @@ 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)
{
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
Expand All @@ -171,14 +181,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)
Expand Down
11 changes: 9 additions & 2 deletions Octokit/Models/Response/StringEnum.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -29,7 +29,9 @@ public StringEnum(TEnum parsedValue)

public StringEnum(string stringValue)
{
_stringValue = stringValue ?? string.Empty;
Ensure.ArgumentNotNull(stringValue, nameof(stringValue));
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

should we enforce no empty strings here too?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yeah, I guess it wouldn't hurt.


_stringValue = stringValue;
_parsedValue = null;
}

Expand Down Expand Up @@ -66,7 +68,7 @@ public bool TryParse(out TEnum value)
return true;
}

if (string.IsNullOrEmpty(StringValue))
if (StringValue == null)
Copy link
Contributor

@khellang khellang Feb 13, 2018 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

How can StringValue be null at this point?

Copy link
Contributor Author

@ryangribble ryangribble Feb 13, 2018 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It seems to still be null in the case where you have a class that has a member of StringEnum<T> that is unitialized. We want this to return default(T) so it behaves like a normal enum

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I added a test to repro this here: https://github.com/octokit/octokit.net/pull/1760/files#diff-728143ef4b5a14ceed6d74d06f9c9df7R62

this is certainly doing my head in!

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Couldn't this potentially lead to incorrect behavior?

If you have an uninitialized property, it means that the payload is missing the property, which means it's optional, but Octokit has modeled it as required (by it not being nullable).

By just silently falling back to the default value, you could potentially give the user the incorrect default (who decides what the "default value" is?)

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

right... I was losing sight of the fact we only use this StringEnum<T> in response models, because the unit tests are using dummy classes where it's possible to declare classes with uninitialised members and so on.

But from a technical point of view - the same would be true of an enum property on a response class now wouldn't it? If it was not marked nullable, and the API response didnt specify it, then we will be providing the "incorrect" default enum value on the response model instead of a null.

I guess the question is, should an incorrectly not nullable StringEnum<T> behave the same as an incorrectly not nullable regular enum?

Copy link
Contributor

@khellang khellang Feb 13, 2018 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

But from a technical point of view - the same would be true of an enum property on a response class now wouldn't it?

Yes, but that doesn't make it any less wrong 😉 There's a reason you should always declare all value-type properties as nullable when binding to external input. It's the only way to check whether that input actually existed, or if it's just the default (uninitialized) value.

I guess the question is, should an incorrectly not nullable StringEnum<T> behave the same as an incorrectly not nullable regular enum?

I vote "no", simply because it's a nice way to "flush out" mistakes where we've assumed something was required, but was actually optional.

Copy link
Contributor

@khellang khellang Feb 13, 2018 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Let's say you have a model:

public class UpdateAccount
{
    public Guid Id { get; set; }
    public decimal Amount { get; set; }
}

And someone posts

{ "id": "3d216536-0869-4c9d-b2b7-2a735498c634" }

You'd end up with account.Amount == 0, which would be pretty bad.

Instead, you'd want to declare it nullable and be able to check account.Amount.HasValue and issue a validation error.

{
value = default(TEnum);
return false;
Expand Down Expand Up @@ -144,6 +146,11 @@ public override string ToString()

private TEnum ParseValue()
{
if (_stringValue == null)
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

You've already verified in the ctor that _stringValue can't be null?

{
return default(TEnum);
}

TEnum value;
if (TryParse(out value))
{
Expand Down
12 changes: 12 additions & 0 deletions Octokit/SimpleJson.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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<Type> GetInterfaces(Type type)
{
#if SIMPLE_JSON_TYPEINFO
Expand Down