Fix number precision for Newtonsoft.Json

[sungam3r]

First commit demonstrates issue with Newtonsoft.

Inspired by changes in tests from #2841.

[Shane32]

I do not understand. Can you explain what this PR is fixing?

[sungam3r]

Newtonsoft.Json always adds .0 when serializes float, double or decimal values. First commit shows it. I changed tests to always execute both serializers. Before changes tests used only STJ serializer.

[Shane32]

Newtonsoft.Json always adds .0 when serializes float, double or decimal values. First commit shows it. I changed tests to always execute both serializers. Before changes tests used only STJ serializer.

And why is that an issue? It’s not wrong. There looks to be a lot of code that now has to execute for every single floating point number serialized. Seems like a complete waste of CPU power. There’s no reason why the serializers cannot perform differently so long as they meet spec.

We could add the converter to execute only during tests if we need to unify the testing. But it would be a shame not to test the default environment.

I just don’t see that anyone would request this change. Except us, and only because of our tests that cover two serialization engines. (User code uses one serializer or the other.) So, it does not seem an appropriate change to make.

Now personally this doesn’t affect me because I don’t use Newtonsoft.Json. But if I did I wouldn’t want this extra code added for no reason (as seen from the users perspective).

This code hurt anything except performance. So, we could add this code. But why injure performance for the sake of a few tests?

[sungam3r]

And why is that an issue?
I just don’t see that anyone would request this change

JamesNK/Newtonsoft.Json#1726 reasoning and reactions here

[Shane32]

I read through that. It seems people have issues with large integer decimal values. Let’s at least restrict this converter to the decimal data type and not float or double. Since the default deserialization mode for numbers with a period is double, none of these issues would affect them.

[Shane32]

Suggested change

	public override bool CanConvert(Type objType) => objType == typeof(decimal) || objType == typeof(float) || objType == typeof(double);
	public override bool CanConvert(Type objType) => objType == typeof(decimal);

[sungam3r]

Ready for review.

[codecov-commenter]

Codecov Report

Merging #2843 (6e8dc57) into master (951753a) will increase coverage by 0.11%.
The diff coverage is 83.33%.

@@            Coverage Diff             @@
##           master    #2843      +/-   ##
==========================================
+ Coverage   83.67%   83.78%   +0.11%     
==========================================
  Files         363      364       +1     
  Lines       14924    14978      +54     
  Branches     2361     2369       +8     
==========================================
+ Hits        12487    12549      +62     
+ Misses       1808     1798      -10     
- Partials      629      631       +2     

Impacted Files	Coverage Δ
...rc/GraphQL.NewtonsoftJson/FixPrecisionConverter.cs	83.33% <83.33%> (ø)
src/GraphQL.NewtonsoftJson/DocumentWriter.cs	62.12% <0.00%> (-1.52%)	⬇️
src/GraphQL/Types/Schema.cs	79.72% <0.00%> (-0.91%)	⬇️
...hQL.NewtonsoftJson/ExecutionResultJsonConverter.cs	88.28% <0.00%> (+15.62%)	⬆️

---

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 951753a...6e8dc57. Read the comment docs.

[Shane32]

Nice