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.

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

Use invariant culture for (de)serialization #298

Merged

Conversation

MartinM85
Copy link
Contributor

@MartinM85 MartinM85 commented Jul 16, 2024

fixes #280
fixes #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?

@MartinM85 MartinM85 requested a review from a team as a code owner July 16, 2024 13:40
Copy link
Member

@baywet baywet left a comment

Choose a reason for hiding this comment

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

This is great work! I left a couple of questions for you.

src/abstractions/Time.cs Show resolved Hide resolved
src/abstractions/Time.cs Outdated Show resolved Hide resolved
src/serialization/form/FormParseNode.cs Outdated Show resolved Hide resolved
src/serialization/form/FormParseNode.cs Show resolved Hide resolved
@baywet
Copy link
Member

baywet commented Jul 16, 2024

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

Copy link
Member

@andrueastman andrueastman left a comment

Choose a reason for hiding this comment

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

@MartinM85 We'll also need a changelog entry/version bump here as well. Otherwise looks good to me.

@MartinM85
Copy link
Contributor Author

MartinM85 commented Jul 17, 2024

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
Copy link
Member

baywet commented Jul 17, 2024

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
Copy link
Member

baywet commented Jul 17, 2024

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
Copy link
Contributor Author

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
Copy link
Member

andrueastman commented Jul 18, 2024

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.

if(depth > 0) throw new InvalidOperationException("Form serialization does not support nested objects.");

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

@MartinM85
Copy link
Contributor Author

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
Copy link
Member

baywet commented Jul 18, 2024

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

Copy link
Member

@baywet baywet left a comment

Choose a reason for hiding this comment

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

Thanks for the great work here!
@andrueastman for final review, merge and publish

@andrueastman andrueastman merged commit 10b879b into microsoft:main Jul 18, 2024
7 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
Archived in project
Development

Successfully merging this pull request may close these issues.

Additional control over DateTime serialization Date/Time serialization uses current culture
3 participants