Feat: deserialize numbers as strings

[earloc]

Synopsis

Enables opt-in support for deserializing "numbers as strings", by providing a configured KiotaJsonSerializationContext, in accordance to JsonSerializerOptions.NumberHandling.

This is to provide a more streamlined approach when dealing with APIs, which may return numbers as strings, even when their openAPI specifies a more specific type, like

"type": "number",
"format": "double"

where the current behavior is to fail silently, while populating such properties with null.

Closes #550

via JsonSerializerOptions.Web

var context = new KiotaJsonSerializationContext(new JsonSerializerOptions(JsonSerializerOptions.Web)); // can handle numbers-as-strings per default

var apiClient = new ApiClient(new HttpClientRequestAdapter
(
    parseNodeFactory: new JsonParseNodeFactory(context),
    httpClient: client
));

via customized JsonSerializerOptions

var context = new KiotaJsonSerializationContext(new JsonSerializerOptions()
{
    NumberHandling = JsonNumberHandling.AllowReadingFromString
};

var apiClient = new ApiClient(new HttpClientRequestAdapter
(
    parseNodeFactory: new JsonParseNodeFactory(context),
    httpClient: client
));

acceptance criterias

1. ☑️ GetDouble returns a value with an underlying type number, and no configuration from the application of the serialization context
2. ☑️ GetDouble does not throw with an underlying type string, and no configuration from the application of the serialization context
3. ☑️ GetDouble returns a value with an underlying type string, and a configuration of the serialization context.
4. ☑️ GetDouble returns null with an underlying type string, and a configuration of the serialization context, but the runtime-value does not contain a valid number. GetDouble throws a JsonException with an underlying type string, and a configuration of the serialization context but the runtime-value does not contain a valid number.

[earloc]

@baywet here's my take on enabling this as a first-class citizen in the lib.
What I´m currently still wondering about is: how to handle non-numbers when users opted-in to allow for reading "numbers as strings", f.e. when an expected Integer contains the value "not-a-number" or alike.

☑️ Default behavior (not opted-in) would be, to fail silently and populate null, so no breaking change, here .
❌ In opt-in scenarios, however, this would now fail with an exception (see ab78f8e), as the underlying Deserialize would throw then .

I guess general philosophy here is, to not throw at all and populate nulls instead. So in order to be in line with this principle, we might need to either

1. accept throwing in this case
2. wrap the deserialization and catch (specific?) exceptions, falling back to null

what do you think?

[earloc]

@microsoft-github-policy-service agree

[baywet]

Thanks for the contribution!

[baywet]

@earloc thanks for the very detailed analysis here in addition to this great pull request!
I think that as a general design principal, we're not afraid of throwing when things go completely off the rails, instead of silently failing, which can be much harder to investigate for people running into the scenario.

I believe this means the tests need to be updated with a couple of assert.throw for this case + formatting needs to be fixed.

[earloc]

@baywet okidokey, I refactored the tests to reflect this behavior (throwing, when opted-in for reading "numbers-as-strings" and encountering a non-deserializable value.

Also ran a dotnet format... anything else regarding formatting I still missed?

[baywet]

Thank you for making the changes!

[baywet]

@earloc the CI has failed, can you have a look when you have a minute please? (messaging since contributors don't always receive notifications)

[earloc]

Looks like some difference in runtime-behavior for .net462. The positive-tests for parsing "numbers-as-string" seem to fail, there. Need to switch to a windows-machine to reproduce this, as my current setup doesn't allow the targeting pack to be installed. 🤔

[earloc]

@baywet could it be, that for net462 / netstandard2.0 we need to somehow register a custom JsonConverter?
Didn't found a strong evidence, yet, that there is a major difference around JsonNumberHandling.AllowReadAsString, other than this blog-post or this one.

GH-Copilot stated, that the netstandard2.0-implementation is incomplete in this regards (without giving me any helpful links in a short session ^^).

If this is true, I'd rather modify the tests to expect such exceptions when running against net462 / netstandard2.0, even when opted-in - as this behavior would only be fixable, by providing the respective JsonConverters from user-code. (Or is there some magic bullet in Kiota, which could help here and I just don't know about?)

[baywet]

Thank you for the additional information.

Yes, the information on this is sparse...

Just to confirm, the exceptions are ONLY going to be thrown on netfx when the JsonNumberHandling.AllowReadingFromString is used but no additional converter is passed?
It's not going to throw when no options are passed (vast majority of cases)
It's not going to throw when an option is passed, and while running on net5+

If that's an accurate depiction of the situation, I believe adding a documentation comment (remark or equivalent) for the Kiota serialization context to detail that scenario should be sufficient.
Please who really want the number as string parsing to work can bring additional converters by deriving from that type. If this is a common scenario, we can think of adding those with conditional compilation as well.

What do you think?

[earloc]

@baywet yes, wonderful wrapup of the current situation. Here's a differrent take to showcase this:

runtime	AllowReadingFromString	runtime value	result	behavior	comment
> net5	no	42	42	unchanged
> net5	no	"42"	null	unchanged
> net5	no	"not-a-number"	null	unchanged
> net5	yes	42	42	unchanged
> net5	yes	"42"	42	new	this is what this change is all about ;)
> net5	yes	"not-a-number"	throws	new	would work for me, at least
netfx	no	42	42	unchanged
netfx	no	"42"	null	unchanged
netfx	no	"not-a-number"	null	unchanged
netfx	yes	42	42	unchanged
netfx	yes	"42"	throws	new	different from >net5, needs additional JsonConverters to work
netfx	yes	"not-a-number"	throws	new	different then before, but users need to first opt-in for this to happen

In general, we could bring this w/o any differrence in runtime-behavior for netfx, if we just conditionally compile the underlying Get*Value-methods to behave as before this change on any < net5 fx.

But I rather chose to accept this behavior (and cover/document it throug the testcases) and NOT do conditional compilation for this, as it:

• adds complexitiy to the code-base
• may help someone still stuck on netfx to deal with "numbers-as-strings", even if there might be additional work needed (by adding respective JsonConverters)

I pushed again, hopefully letting CI pass (worked on my machine ;) )

[earloc]

If you might be so kind and point my nose to where this could be documented, I might jump in there as well.
And again thx for the tight feedback loop 👍.

[baywet]

Thank you for making the changes!