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

Add support for JsonSerializerOptions.NumberHandling #494

Merged
merged 3 commits into from
Oct 20, 2023

Conversation

viceroypenguin
Copy link
Contributor

This PR adds support for JsonSerializerOptions.NumberHandling on net5+, while retaining old behavior on other frameworks.

When supported (net5+) and Conversions.TreatNumberAsStringInSystemTextJson is not set, then the converter will read JsonSerializerOptions.NumberHandling to determine whether to allow parsing numeric values from string tokens, similar to the effect JsonSerializerOptions.NumberHandling has on deserializing numeric primitives.

@SteveDunn I was not able to get the verify files updated correctly. I ran the test.ps1, and it failed due to TZ issues, and would not generate any of the verify files.

I have verified that this change works correctly for my situation; if this change is not desired, feel free to close.

Fixes #493

@viceroypenguin
Copy link
Contributor Author

@SteveDunn Would you be opposed to the migration of the models to Scriban or something? This might make some of these changes easier if we can pass in a tfm from the Compilation and affect which part of the model is used (only using the property name converters on tfm6+, etc.). Obviously I'd offer that in a separate PR if you were amenable.

@SteveDunn
Copy link
Owner

@SteveDunn Would you be opposed to the migration of the models to Scriban or something? This might make some of these changes easier if we can pass in a tfm from the Compilation and affect which part of the model is used (only using the property name converters on tfm6+, etc.). Obviously I'd offer that in a separate PR if you were amenable.

Thanks for the PR! Yes, it is painful when making a change to the generated code. There is a flag that can be set in .build.ps1. I've documented it in the readme, but I'll also add it to the wiki. It is:
.\Build.ps1 -v "Minimal" -resetSnapshots $true

This deletes all of the snapshots and treats all of the generated code as the correct code from now on.

I've never heard of Scriban, but I'll take a look. Thanks for the recommendation.

@SteveDunn
Copy link
Owner

Also, could you tell me what tests are failing due to time-zone differences? I thought (hoped!) that I'd fixed all of those.

@viceroypenguin
Copy link
Contributor Author

viceroypenguin commented Oct 10, 2023

Also, could you tell me what tests are failing due to time-zone differences? I thought (hoped!) that I'd fixed all of those.

ConsumerTests.Instances.InstanceTests+DateTimeTests.DateTimeOffsets appears to be the offending test. I get four failures for this test (note, I am in CST, so -06:00 in the winter):

############################################
****  Running end to end tests with the local version of the NuGet package:999.9.30960716
############################################

Test run for C:\Users\<username>\source\repos\viceroypenguin\Vogen\tests\ConsumerTests\bin\Debug\net461\ConsumerTests.dll (.NETFramework,Version=v4.6.1)
Microsoft (R) Test Execution Command Line Tool Version 17.7.1 (x64)
Copyright (c) Microsoft Corporation.  All rights reserved.

Starting test execution, please wait...
A total of 1 test files matched the specified pattern.
[xUnit.net 00:00:14.79]     ConsumerTests.Instances.InstanceTests+DateTimeTests.DateTimeOffsets [FAIL]
  Failed ConsumerTests.Instances.InstanceTests+DateTimeTests.DateTimeOffsets [492 ms]
  Error Message:
   Expected DateTimeOffsetInstance.iso8601_1.Value.UtcDateTime to be <2022-12-13>, but found <2022-12-13 06:00:00>.

  Stack Trace:
     at FluentAssertions.Execution.XUnit2TestFramework.Throw(String message)
   at FluentAssertions.Execution.CollectingAssertionStrategy.ThrowIfAny(IDictionary`2 context)
   at ConsumerTests.Instances.InstanceTests.DateTimeTests.DateTimeOffsets() in C:\Users\<username>\source\repos\viceroypenguin\Vogen\tests\ConsumerTests\Instances\InstanceTests.cs:line 93

Failed!  - Failed:     1, Passed:   995, Skipped:     0, Total:   996, Duration: 10 s - ConsumerTests.dll (net461)
Test run for C:\Users\<username>\source\repos\viceroypenguin\Vogen\tests\ConsumerTests\bin\Debug\netcoreapp3.1\ConsumerTests.dll (.NETCoreApp,Version=v3.1)
Microsoft (R) Test Execution Command Line Tool Version 17.7.1 (x64)
Copyright (c) Microsoft Corporation.  All rights reserved.

Starting test execution, please wait...
A total of 1 test files matched the specified pattern.
[xUnit.net 00:00:03.02]     ConsumerTests.Instances.InstanceTests+DateTimeTests.DateTimeOffsets [FAIL]
  Failed ConsumerTests.Instances.InstanceTests+DateTimeTests.DateTimeOffsets [181 ms]
  Error Message:
   Expected DateTimeOffsetInstance.iso8601_1.Value.UtcDateTime to be <2022-12-13>, but found <2022-12-13 06:00:00>.

  Stack Trace:
     at FluentAssertions.Execution.XUnit2TestFramework.Throw(String message)
   at FluentAssertions.Execution.TestFrameworkProvider.Throw(String message)
   at FluentAssertions.Execution.CollectingAssertionStrategy.ThrowIfAny(IDictionary`2 context)
   at ConsumerTests.Instances.InstanceTests.DateTimeTests.DateTimeOffsets() in C:\Users\<username>\source\repos\viceroypenguin\Vogen\tests\ConsumerTests\Instances\InstanceTests.cs:line 93

Failed!  - Failed:     1, Passed:   995, Skipped:     0, Total:   996, Duration: 882 ms - ConsumerTests.dll (netcoreapp3.1)
Test run for C:\Users\<username>\source\repos\viceroypenguin\Vogen\tests\ConsumerTests\bin\Debug\net5.0\ConsumerTests.dll (.NETCoreApp,Version=v5.0)
Microsoft (R) Test Execution Command Line Tool Version 17.7.1 (x64)
Copyright (c) Microsoft Corporation.  All rights reserved.

Starting test execution, please wait...
A total of 1 test files matched the specified pattern.
[xUnit.net 00:00:03.09]     ConsumerTests.Instances.InstanceTests+DateTimeTests.DateTimeOffsets [FAIL]
  Failed ConsumerTests.Instances.InstanceTests+DateTimeTests.DateTimeOffsets [220 ms]
  Error Message:
   Expected DateTimeOffsetInstance.iso8601_1.Value.UtcDateTime to be <2022-12-13>, but found <2022-12-13 06:00:00>.

  Stack Trace:
     at FluentAssertions.Execution.XUnit2TestFramework.Throw(String message)
   at FluentAssertions.Execution.TestFrameworkProvider.Throw(String message)
   at FluentAssertions.Execution.CollectingAssertionStrategy.ThrowIfAny(IDictionary`2 context)
   at ConsumerTests.Instances.InstanceTests.DateTimeTests.DateTimeOffsets() in C:\Users\<username>\source\repos\viceroypenguin\Vogen\tests\ConsumerTests\Instances\InstanceTests.cs:line 93

Failed!  - Failed:     1, Passed:   995, Skipped:     0, Total:   996, Duration: 1 s - ConsumerTests.dll (net5.0)
Test run for C:\Users\<username>\source\repos\viceroypenguin\Vogen\tests\ConsumerTests\bin\Debug\net6.0\ConsumerTests.dll (.NETCoreApp,Version=v6.0)
Microsoft (R) Test Execution Command Line Tool Version 17.7.1 (x64)
Copyright (c) Microsoft Corporation.  All rights reserved.

Starting test execution, please wait...
A total of 1 test files matched the specified pattern.
[xUnit.net 00:00:08.00]     ConsumerTests.Instances.InstanceTests+DateTimeTests.DateTimeOffsets [FAIL]
  Failed ConsumerTests.Instances.InstanceTests+DateTimeTests.DateTimeOffsets [147 ms]
  Error Message:
   Expected DateTimeOffsetInstance.iso8601_1.Value.UtcDateTime to be <2022-12-13>, but found <2022-12-13 06:00:00>.

  Stack Trace:
     at FluentAssertions.Execution.XUnit2TestFramework.Throw(String message)
   at FluentAssertions.Execution.TestFrameworkProvider.Throw(String message)
   at FluentAssertions.Execution.CollectingAssertionStrategy.ThrowIfAny(IDictionary`2 context)
   at ConsumerTests.Instances.InstanceTests.DateTimeTests.DateTimeOffsets() in C:\Users\<username>\source\repos\viceroypenguin\Vogen\tests\ConsumerTests\Instances\InstanceTests.cs:line 93

Failed!  - Failed:     1, Passed:  1023, Skipped:     0, Total:  1024, Duration: 2 s - ConsumerTests.dll (net6.0)
Test run for C:\Users\<username>\source\repos\viceroypenguin\Vogen\tests\ConsumerTests\bin\Debug\net7.0\ConsumerTests.dll (.NETCoreApp,Version=v7.0)
Microsoft (R) Test Execution Command Line Tool Version 17.7.1 (x64)
Copyright (c) Microsoft Corporation.  All rights reserved.

Starting test execution, please wait...
A total of 1 test files matched the specified pattern.
[xUnit.net 00:00:02.11]     ConsumerTests.Instances.InstanceTests+DateTimeTests.DateTimeOffsets [FAIL]
  Failed ConsumerTests.Instances.InstanceTests+DateTimeTests.DateTimeOffsets [157 ms]
  Error Message:
   Expected DateTimeOffsetInstance.iso8601_1.Value.UtcDateTime to be <2022-12-13>, but found <2022-12-13 06:00:00>.

  Stack Trace:
     at FluentAssertions.Execution.XUnit2TestFramework.Throw(String message)
   at FluentAssertions.Execution.TestFrameworkProvider.Throw(String message)
   at FluentAssertions.Execution.CollectingAssertionStrategy.ThrowIfAny(IDictionary`2 context)
   at FluentAssertions.Execution.AssertionScope.Dispose()
   at ConsumerTests.Instances.InstanceTests.DateTimeTests.DateTimeOffsets() in C:\Users\<username>\source\repos\viceroypenguin\Vogen\tests\ConsumerTests\Instances\InstanceTests.cs:line 93
   at System.RuntimeMethodHandle.InvokeMethod(Object target, Void** arguments, Signature sig, Boolean isConstructor)
   at System.Reflection.MethodInvoker.Invoke(Object obj, IntPtr* args, BindingFlags invokeAttr)

Failed!  - Failed:     1, Passed:  1073, Skipped:     0, Total:  1074, Duration: 1 s - ConsumerTests.dll (net7.0)
Exec:
At C:\Users\<username>\source\repos\viceroypenguin\Vogen\Build.ps1:50 char:9
+         throw ("Exec: " + $errorMessage)
+         ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
    + CategoryInfo          : OperationStopped: (Exec: :String) [], RuntimeException
    + FullyQualifiedErrorId : Exec:

@viceroypenguin
Copy link
Contributor Author

@SteveDunn any chance this PR can be merged and a new release dropped soon?

@SteveDunn
Copy link
Owner

Hi @viceroypenguin - sorry for slow responses lately, it's been busy in my day job and I'm trying to finish up #479 which has taken much longer than expected!

I'll hopefully be finished within a day or so, I'll release both at the weekend, if that's OK with you?

@SteveDunn SteveDunn self-requested a review October 20, 2023 05:31
@SteveDunn SteveDunn merged commit 1453c3f into SteveDunn:main Oct 20, 2023
5 checks passed
@SteveDunn
Copy link
Owner

Merged just now - thank you again @viceroypenguin ! I'll fix those tests soon too - I think I know what's wrong.

@SteveDunn
Copy link
Owner

Should be in nuget soon v. 3.0.22

@viceroypenguin viceroypenguin deleted the numberhandling branch October 23, 2023 22:07
@viceroypenguin
Copy link
Contributor Author

Thank you @SteveDunn!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Support JsonSerializerOptions.NumberHandling
2 participants