microsoft / kiota-dotnet

Abstractions library for the Kiota generated SDKs in dotnet
https://aka.ms/kiota/docs
MIT License
32 stars 29 forks source link

Use invariant culture for (de)serialization #298

Closed MartinM85 closed 1 month ago

MartinM85 commented 1 month ago

fixes #280 fixes https://github.com/microsoft/kiota-dotnet/issues/283

Unit tests discovered several issues:

  1. FormSerializationWriter.WriteAnyValue writes array as string Test WriteAdditionalData_AreWrittenCorrectly() { "prop12", new byte[] { 2, 4, 6 } } is serialized as prop12=System.Byte%5B%5D

  2. Time class has two constructors TextParseNode.GetTimeValue parses DateTime value from string and calls Time(DateTime) ctor. In this case the Date property of DateTime contains current date. If you create a new instance of Time by calling Time(int hour, int minute, int second) ctor as part of unit test, then Date property has year, month and day set to 1. When a unit test is created, Assert.Equal will fail, because it compares also DateTime properties, which can be different. I would suggest to get rid of DateTime property and Time(DateTime) from Time. Seems to me better than overridden Equals method.

  3. Issue JsonSerializer.Serialize inside JsonSerializationWriter.WriteFloatValue and JsonSerializationWriter.WriteDoubleValue serialize for .net462. Unit tests fail Double

Assert.Equal() Failure: Strings differ
              ↓ (pos 3)
Expected: "36.8"
Actual:   "36.799999999999997"
              ↑ (pos 3)

Float

Assert.Equal() Failure: Strings differ
              ↓ (pos 3)
Expected: "36.8"
Actual:   "36.7999992"
              ↑ (pos 3)

I think that net462 is used in tests projects, because xunit runner doesn't support .netstandard2.x

  1. Any reason why are GetEnumValue and GetCollectionOfEnumValues defined explicitly in FormParseNode?
baywet commented 1 month ago

I think that net462 is used in tests projects, because xunit runner doesn't support .netstandard2.x

The tests should run fine for both netfx462 and net8

Any reason why are GetEnumValue and GetCollectionOfEnumValues defined explicitly in FormParseNode?

No, it was probably the auto-suggestion from vscode at the time

MartinM85 commented 1 month ago

Still one opened question: Is it expected that FormSerializationWriter.WriteAnyValue writes array as string? new byte[] { 2, 4, 6 } is serialized as System.Byte%5B%5D

baywet commented 1 month ago

Still one opened question: Is it expected that FormSerializationWriter.WriteAnyValue writes array as string? new byte[] { 2, 4, 6 } is serialized as System.Byte%5B%5D

I think the challenge here is there is no agreed upon and standardized way to represent by arrays for the form of mediatype. This is probably why we don't do anything specific here and it ends up being the dotnet representation of this type. Would it be better to base64 encode it instead?

baywet commented 1 month ago

This probably says it all....

The application/x-www-form-urlencoded format is in many ways an aberrant monstrosity, the result of many years of implementation accidents and compromises leading to a set of requirements necessary for interoperability, but in no way representing good design practices.

source

MartinM85 commented 1 month ago

Still one opened question: Is it expected that FormSerializationWriter.WriteAnyValue writes array as string? new byte[] { 2, 4, 6 } is serialized as System.Byte%5B%5D

I think the challenge here is there is no agreed upon and standardized way to represent by arrays for the form of mediatype. This is probably why we don't do anything specific here and it ends up being the dotnet representation of this type. Would it be better to base64 encode it instead?

I was asking because FormSerializationWriter.WriteAnyValue writes IEnumerable, but not an array

andrueastman commented 1 month ago

I was asking because FormSerializationWriter.WriteAnyValue writes IEnumerable, but not an array

I believe arrays implement IEnumerable.

Is it expected that FormSerializationWriter.WriteAnyValue writes array as string?

One thing to note is that WriteAnyValue is private and not publicly exposed. It is therefore always called from a "nested" perspective (you'll probably be serializing a property of an object) . Given that that Form Serialization doesn't really support nested objects, the method will always write primitives.

https://github.com/microsoft/kiota-dotnet/blob/e06f0868375f666f5d4e647e016e218bd4dd6714/src/serialization/form/FormSerializationWriter.cs#L189

What we could probably do is add a case for the byte [] that will base64 encode it as @baywet suggested.

https://github.com/microsoft/kiota-dotnet/blob/e06f0868375f666f5d4e647e016e218bd4dd6714/src/serialization/form/FormSerializationWriter.cs#L86

MartinM85 commented 1 month ago

I was asking because FormSerializationWriter.WriteAnyValue writes IEnumerable, but not an array

I believe arrays implement IEnumerable.

array implements IEnumerable, but case is for IEnumerable which won't work for arrays

baywet commented 1 month ago

Yes it was probably a matter of the type assertion engine not matching in this case and just adding a new one so it explicitly matches. I can see that you have already done that. Thank you for making the change