Deserialization of unknown enum value

[HaydnDias]

Hello!

Whilst testing we found a regression after switching from nswag -> kiota, we used to get a Newtonsoft.Json.JsonSerializationException when we deserialized an entity with an unknown enum value, it'd give us an error like Error converting value "EXAMPLE_ENUM_STRING_VALUE" to type 'System.Nullable`1[Example.Project.Models.ExampleEnum]'..

With kiota, the JsonParseNode returns null if it can't parse it, I know we can implement our own IParseNode and IParseNodeFactory for this, however it does seem wastefulhaving to have a copy of the entire JsonParseNode class for what is effectively a one line change, and it means that we'll need to keep our implementation up to date with changes made here if we want to keep it as close to the default implementation as possible.

Currently we've made a change to GetEnumValue as below:

{
	var rawValue = _jsonNode.GetString();
	if(string.IsNullOrEmpty(rawValue)) return null;
	
	var type = typeof(T);
	rawValue = ToEnumRawName<T>(rawValue!);
	if(type.GetCustomAttributes<FlagsAttribute>().Any())
	{
		return (T)(object)rawValue!
			.Split(',')
			.Select(x => Enum.TryParse<T>(x, true, out var result) ? result : (T?)null)
			.Where(x => !x.Equals(null))
			.Select(x => (int)(object)x!)
			.Sum();
	}
	else
-		return Enum.TryParse<T>(rawValue, true,out var result) ? result : null;
+		return Enum.TryParse<T>(rawValue, true,out var result) ? result : 
+				throw new ArgumentOutOfRangeException(type.FullName, rawValue, "Unable to parse enum value");
}

Reasoning for this is we'd like to catch and log when we receive a new enum value as sometimes third parties do add new ones without telling us and we need to be aware.

What are your thoughts on potential solutions to adding something official to be able to handle cases like this?

[baywet]

Hi @HaydnDias
Thanks for using kiota and for raising this.
This is something where we have a bit of an implementation drift I believe.
Upon receiving an unknown value, the following languages will return null

• java
• dotnet (as demonstrated above)
• typescript

where-as the following languages will throw:

• python
• php
• go

We could see two different ways: it's better to quietly fail as it doesn't break the application right away, but it might derail its logic. Or no, it's better to fail loudly so people know they need to update their code.

Before we take any action on that (alignment will be required either way), I'd like @darrelmiller opinion on the matter.

[HaydnDias]

@baywet Hey! Yeah we're adopting Kiota for one of our core libraries, we actually found some other concerns, I think it is likely the case that the provided JsonParseNode implentation just doesn't suit our needs.

I agree there are two different ways, could it be that we have two JsonParseNode implementations in this package, or potentially more ways to configure behaviour.

For us we want loud sirens for when something fails to deserialize, we've modified your provided JsonParseNode implementation here https://github.com/CapitalOnTap/marqeta-csharp-core-sdk/blob/9cecf3065ae4649c96ae0b4c25c85d8de50a3436/Marqeta.Core.Sdk/Serialization/Json/CustomJsonParseNode.cs

Some notable changes are, removing safety around JsonNode.ValueKind, as we would rather the deserializer throw an exception, we added a try/catch around the fieldDeserializer.Invoke in the AssignFieldValues method, which then throws a descriptive exception, and the exception throwin for non-parsed Enum, so far this is working for us, and the information provided when something goes wrong is invaluable.

It's not a final, we have more testing to do, but I think it's fair to say we want a stricter parser, with allowances for some things like string -> number.

Appreciate all the work you do on Kiota, I forsee us using it heavily and hope we get time to contribute back with PRs rather than just issues!

[darrelmiller]

a) Yes we need to align across languages
b) One of the reasons we isolated serialization into separate packages is so that Kiota users can create their own serialization libraries if necessary.
c) Having said that, I think if we can relax the initial rules on reading the value to allow nulls and then support a secondary set of rules that can be run after parsing to do additional checks.

Something like this:

Address.Rules = new[]
{
    (address) =>  address.Street == null ? throw new ArgumentException("Street is required") : null,
    (address) => address.Type == null ? throw new ArgumentException("Type is required") : null,
};

[andrueastman]

Transferring issue as part of #238