-
Notifications
You must be signed in to change notification settings - Fork 396
Add [DataContract] and [DataMember] annotations #972
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
Merged
angularsen
merged 3 commits into
angularsen:master
from
lipchev:data-contract-annotations
Oct 17, 2021
Merged
Changes from all commits
Commits
File filter
Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
There are no files selected for viewing
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
218 changes: 218 additions & 0 deletions
218
UnitsNet.Tests/Serialization/Json/DefaultDataContractJsonSerializerTests.cs
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,218 @@ | ||
using System.Globalization; | ||
using System.IO; | ||
using System.Runtime.Serialization; | ||
using System.Runtime.Serialization.Json; | ||
using UnitsNet.Units; | ||
using Xunit; | ||
|
||
namespace UnitsNet.Tests.Serialization.Json | ||
{ | ||
/// <summary> | ||
/// These tests demonstrate the default behavior of the DataContractJsonSerializer when dealing with quantities | ||
/// <remarks> | ||
/// <para>Note that the produced schema is different from the one generated using the UnitsNet.Json package</para> | ||
/// <para> | ||
/// The default schema can easily be modified using a converter, a.k.a. DataContractSurrogate (.NET Framework) | ||
/// </para> | ||
/// </remarks> | ||
/// </summary> | ||
public class DefaultDataContractJsonSerializerTests : SerializationTestsBase<string> | ||
{ | ||
protected override string SerializeObject(object obj) | ||
{ | ||
var serializer = new DataContractJsonSerializer(obj.GetType()); | ||
using var stream = new MemoryStream(); | ||
serializer.WriteObject(stream, obj); | ||
stream.Position = 0; | ||
using var streamReader = new StreamReader(stream); | ||
return streamReader.ReadToEnd(); | ||
} | ||
|
||
protected override T DeserializeObject<T>(string xml) | ||
{ | ||
var serializer = new DataContractJsonSerializer(typeof(T)); | ||
using var stream = new MemoryStream(); | ||
using var writer = new StreamWriter(stream); | ||
writer.Write(xml); | ||
writer.Flush(); | ||
stream.Position = 0; | ||
return (T)serializer.ReadObject(stream); | ||
} | ||
|
||
[Fact] | ||
public void DoubleQuantity_SerializedWithDoubleValueAndIntegerUnit() | ||
{ | ||
var quantity = new Mass(1.20, MassUnit.Milligram); | ||
var expectedJson = "{\"Value\":1.2,\"Unit\":16}"; | ||
|
||
var json = SerializeObject(quantity); | ||
|
||
Assert.Equal(expectedJson, json); | ||
} | ||
|
||
[Fact] | ||
public void DecimalQuantity_SerializedWithDecimalValueValueAndIntegerUnit() | ||
{ | ||
var quantity = new Information(1.20m, InformationUnit.Exabyte); | ||
var expectedJson = "{\"Value\":1.20,\"Unit\":4}"; | ||
|
||
var json = SerializeObject(quantity); | ||
|
||
Assert.Equal(expectedJson, json); | ||
} | ||
|
||
[Fact] | ||
public void DoubleQuantity_InScientificNotation_SerializedWithExpandedValueAndIntegerUnit() | ||
{ | ||
var quantity = new Mass(1E+9, MassUnit.Milligram); | ||
var expectedJson = "{\"Value\":1000000000,\"Unit\":16}"; | ||
|
||
var json = SerializeObject(quantity); | ||
|
||
Assert.Equal(expectedJson, json); | ||
} | ||
|
||
[Fact] | ||
public void DecimalQuantity_InScientificNotation_SerializedWithExpandedValueAndIntegerUnit() | ||
{ | ||
var quantity = new Information(1E+9m, InformationUnit.Exabyte); | ||
var expectedJson = "{\"Value\":1000000000,\"Unit\":4}"; | ||
|
||
var json = SerializeObject(quantity); | ||
|
||
Assert.Equal(expectedJson, json); | ||
} | ||
|
||
[Fact] | ||
public void DoubleQuantity_DeserializedFromDoubleValueAndIntegerUnit() | ||
{ | ||
var json = "{\"Value\":1.2,\"Unit\":16}"; | ||
|
||
var quantity = DeserializeObject<Mass>(json); | ||
|
||
Assert.Equal(1.2, quantity.Value); | ||
Assert.Equal(MassUnit.Milligram, quantity.Unit); | ||
} | ||
|
||
[Fact] | ||
public void DoubleQuantity_DeserializedFromQuotedDoubleValueAndIntegerUnit() | ||
{ | ||
var json = "{\"Value\":\"1.2\",\"Unit\":16}"; | ||
|
||
var quantity = DeserializeObject<Mass>(json); | ||
|
||
Assert.Equal(1.2, quantity.Value); | ||
Assert.Equal(MassUnit.Milligram, quantity.Unit); | ||
} | ||
|
||
[Fact] | ||
public void DoubleZeroQuantity_DeserializedFromIntegerUnitAndNoValue() | ||
{ | ||
var json = "{\"Unit\":16}"; | ||
|
||
var quantity = DeserializeObject<Mass>(json); | ||
|
||
Assert.Equal(0, quantity.Value); | ||
Assert.Equal(MassUnit.Milligram, quantity.Unit); | ||
} | ||
|
||
[Fact] | ||
public void DoubleBaseUnitQuantity_DeserializedFromValueAndNoUnit() | ||
{ | ||
var json = "{\"Value\":1.2}"; | ||
|
||
var quantity = DeserializeObject<Mass>(json); | ||
|
||
Assert.Equal(1.2, quantity.Value); | ||
Assert.Equal(Mass.BaseUnit, quantity.Unit); | ||
} | ||
|
||
[Fact] | ||
public void DoubleZeroBaseQuantity_DeserializedFromEmptyInput() | ||
{ | ||
var json = "{}"; | ||
|
||
var quantity = DeserializeObject<Mass>(json); | ||
|
||
Assert.Equal(0, quantity.Value); | ||
Assert.Equal(Mass.BaseUnit, quantity.Unit); | ||
} | ||
|
||
[Fact] | ||
public void DecimalQuantity_DeserializedFromDecimalValueAndIntegerUnit() | ||
{ | ||
var json = "{\"Value\":1.200,\"Unit\":4}"; | ||
|
||
var quantity = DeserializeObject<Information>(json); | ||
|
||
Assert.Equal(1.200m, quantity.Value); | ||
Assert.Equal("1.200", quantity.Value.ToString(CultureInfo.InvariantCulture)); | ||
Assert.Equal(InformationUnit.Exabyte, quantity.Unit); | ||
} | ||
|
||
[Fact] | ||
public void DecimalQuantity_DeserializedFromQuotedDecimalValueAndIntegerUnit() | ||
{ | ||
var json = "{\"Value\":\"1.200\",\"Unit\":4}"; | ||
|
||
var quantity = DeserializeObject<Information>(json); | ||
|
||
Assert.Equal(1.200m, quantity.Value); | ||
Assert.Equal("1.200", quantity.Value.ToString(CultureInfo.InvariantCulture)); | ||
angularsen marked this conversation as resolved.
Show resolved
Hide resolved
|
||
Assert.Equal(InformationUnit.Exabyte, quantity.Unit); | ||
} | ||
|
||
[Fact] | ||
public void DecimalZeroQuantity_DeserializedFromIntegerUnitAndNoValue() | ||
{ | ||
var json = "{\"Unit\":4}"; | ||
|
||
var quantity = DeserializeObject<Information>(json); | ||
|
||
Assert.Equal(0, quantity.Value); | ||
Assert.Equal(InformationUnit.Exabyte, quantity.Unit); | ||
} | ||
|
||
[Fact] | ||
public void DecimalBaseUnitQuantity_DeserializedFromDecimalValueAndNoUnit() | ||
{ | ||
var json = "{\"Value\":1.200}"; | ||
|
||
var quantity = DeserializeObject<Information>(json); | ||
|
||
Assert.Equal(1.200m, quantity.Value); | ||
Assert.Equal("1.200", quantity.Value.ToString(CultureInfo.InvariantCulture)); | ||
Assert.Equal(Information.BaseUnit, quantity.Unit); | ||
} | ||
|
||
[Fact] | ||
public void DecimalZeroBaseQuantity_DeserializedFromEmptyInput() | ||
{ | ||
var json = "{}"; | ||
|
||
var quantity = DeserializeObject<Information>(json); | ||
|
||
Assert.Equal(0, quantity.Value); | ||
Assert.Equal(Information.BaseUnit, quantity.Unit); | ||
} | ||
|
||
[Fact] | ||
public void InterfaceObject_IncludesTypeInformation() | ||
{ | ||
var testObject = new TestInterfaceObject { Quantity = new Information(1.20m, InformationUnit.Exabyte) }; | ||
var expectedJson = "{\"Quantity\":{\"__type\":\"Information:#UnitsNet\",\"Value\":1.20,\"Unit\":4}}"; | ||
|
||
var json = SerializeObject(testObject); | ||
|
||
Assert.Equal(expectedJson, json); | ||
} | ||
|
||
[Fact] | ||
public void InterfaceObject_WithMissingKnownTypeInformation_ThrowsSerializationException() | ||
{ | ||
var testObject = new TestInterfaceObject { Quantity = new Volume(1.2, VolumeUnit.Microliter) }; | ||
|
||
Assert.Throws<SerializationException>(() => SerializeObject(testObject)); | ||
} | ||
} | ||
} |
Oops, something went wrong.
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I keep forgetting, but wasn't there some good reason to use quoted numbers when serializing?
At first I thought json could not represent double.MaxValue or decimal.MaxValue, but it seems just fine representing both of these in a round-trip.
I tried a quick example in linqpad:
Result:

Similarly, if I instead of MaxValue chose the smallest number of each, it still roundtrips just fine for both Newtonsoft and System.Text.Json.
So. I can't really remember why quoted strings were used to begin with? Is it maybe not necessary after all? Do you remember?
This is the one that introduced it, but I don't really see a description of the problem it solved:
#868
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think this relates to the scale being lost while de-serializing using Newtonsoft - you could see it in the debugger (or in you example of ****0335.0 - that's a double right there). At the same time - this doesn't happen during serialization - the exact scale is preserved.
After doing the initial round of tests- I actually stumbled upon an option that might be able to resolve this issue- but haven't had time to actually figure out what it does: FloatParseHandling.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There is also an option for the special floating point numbers, e.g. NaN, PositiveInfinity and NegativeInfinity - however given that none of those are really supported by UnitsNet (the Guard aggressively protecting against it) I don't think this would have been a reason to use the string representation for all numbers.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm just trying to understand better, so here is another example, now using our json.net serializer and schema:
You wrote:
I think you are right, but only if we were actually trying to deserialize back into the wrong number type:
double.MaxValue
serialized to JSON and deserialized to a decimal property would probably result inOverflowException
decimal.MaxValue
serialized to JSON and deserialized to a double property, would lose precision.However, this shouldn't be a problem when serializing decimal and double quantities if our converters do their job.
What I am beginning to realize is that the problem is not JSON itself, nor is it SystemTextJson or NewtonsoftJson. It is simply that when deserializing, we need to know whether to deserialize to
double
or todecimal
. And we do have this information encoded in our JSON schema. So that leads me to think that theValueString
property in our schema is really redundant.ValueType
should be sufficient, shouldn't it?If I am not entirely missing something, then bringing this line of thought back to this PR; should the data contract have a
ValueType
property to support deserializing decimal values without losing precision?And, should we deprecate the
ValueString
property in our json.net schema and bump the major version with a breaking change to remove that redundant property?There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
In my opinion- the
ValueType
is redundant - it can easily be determined from the Unit (I'm checking the corresponding QuantityInfo.Zero).Unfortunately, in the case of Newtonsoft- knowing the target doesn't help. Here's a fiddle for you to consider:
JToken.Load(new JsonTextReader(new StringReader("{\"DecimalValue\":1.20}"))).Value<decimal>("DecimalValue").Dump()
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
All our converters use
JToken.Load
, but that might be because everybody was copying over from the previous implementation..The documentation isn't much help either- the first example uses
JToken
for theWriteJson
(without implementing a correspondingReadJson
!?).The second one is just as useless (I'm not even sure what it is supposed to demonstrate) .
After browsing through the source code - it seems like the implementation of
DeserializeObject
avoids calling JToken/JObject.Load: it's going through a while(reader.Read()) loop, parsing each token it encounters (e.g. TokenType.StartObject -> TokenType.PropertyName ... -> TokenType.EndObject).Following that logic, I managed to deserialize my
DecimalValue
- it isn't pretty, but I think we can manage..Uh oh!
There was an error while loading. Please reload this page.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ok, so to recap.
ValueType
andValueString
in our existing json.net schema, the converters should be able to do without since we can look up the number type fromQuantityInfo
.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This PR doesn't affect the existing code in any way... It's just about the DataContract and DataMember annotations- and the tests for the corresponding DataContract serializers (xml and json - but without any of the surrogate definitions)..
I sensed you were still in the spirit of our discussion in #907 but the point of starting over was to take it one step at a time. I was of course, going to propose an implementation for a JsonConverter but in another PR.
Other than that - yes you are correct, I think we can manage without the extra properties - and yes, my current implementation (not part of this PR) only supported decimals using the "quoted" numbers schema- and needs to be re-written (once again).
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
You should however, take a close look at the tests- as they would probably be shared between all serializers (DC, Newtonsoft, System.Text and whatever else comes after..).
Uh oh!
There was an error while loading. Please reload this page.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Sorry if I am being slow to grasp all the things, I keep forgetting details as time passes between each time I visit this topic. I think I'm once again getting the mental map decent and this PR looks ready to merge to me.
Just for my own summary:
The tests are excellent, really thorough 👏