Added DataContract annotations to all Quantities {Value, Unit} #907
Conversation
- added tests covering the default DataContractSerializer (xml) & DataContractJsonSerializer (json)
- added DataContractSurrogates (n/a in .netcore) that can be used in the same way as the JsonConverter (i.e. offer an alternative json representation):
a) GenericQuantityDataContractSurrogate: {"1.20", "InformationUnit.Exabyte"} // handles the double/decimals automatically
b) ExtendedQuantityDataContractSurrogate: {"1.2", "InformationUnit.Exabyte", ""1.20", "decimal"} // same format as the one used with the UnitsNetIQuantityJsonConverter
c) BasicQuantityContractSurrogate: {"1.2", "InformationUnit.Exabyte"} // same format as the one used with the UnitsNetJsonConverter (obsolete)
d) QuantityWithAbbreviationContractSurrogate: {"1", "kg", "Mass"} // can be used in conjunction with a much wider range of clients (also handles the double/decimal variations)
- added tests (mostly shared) for each of these
- added two-way compatibilty tests with both UnitsNetJsonConverter & UnitsNetIQuantityJsonConverter (in their respective test-projects- only applicable to net48, which I had to include)
|
Right, so I did a couple of tests and turns out that adding a second framework to any of the tests projects causes the build to freeze (tested on the main repository). |
- moved all Serialization.DataContract.Compatibility.Json tests to another new project (net48) - added missing tests for the BasicQuantityContractSurrogate
Codecov Report
@@ Coverage Diff @@
## master #907 +/- ##
=========================================
+ Coverage 0 82.7% +82.7%
=========================================
Files 0 294 +294
Lines 0 44039 +44039
=========================================
+ Hits 0 36403 +36403
- Misses 0 7636 +7636
Continue to review full report at Codecov.
|
…alization of double/decimal lists by using the IEnumerable interface
|
This issue has been automatically marked as stale because it has not had recent activity. It will be closed if no further activity occurs. Thank you for your contributions. |
|
Hi, As for the Surrogates - I really don't like how they introduced all the conditionals (which I failed to properly configure on the project level) - but I had no idea they were removed in core... Anyway- since they don't add any dependencies - I figured I'd keep them, allowing me to have the json-serializers-compatibility-tests (as a kind of a baseline contract for whatever other serialization library support we add- like System.Text.Json). If you want- I can remove the Surrogates / Compatibility Tests (leaving only the 'default' DataContract xml/json serialization/deserialization tests). Honestly, my goal was to make another PR after this one introducing a json.net Converter using the abbreviation-based schema (which I personally prefer, and regardless of preference, would be forced to use soon):
|
There was a problem hiding this comment.
I think this is solid work 👏
Although WCF data contracts are no longer part of .NET future, there are tons of projects still on .NET 4 and on WCF stack for sure. I'm positive to include this for those targeting net40 of UnitsNet nuget.
Just some minor observations. I only skimmed the implementation and tests, but it looks really good to me. I trust that you who actually will be consuming this will be in a good position to verify it works as intended and it's easy to consume.
One thing, the README serialization section should be updated to reflect the new data contract support and the surrogates we provide out of the box. You might want to link to a wiki page if it gets too detailed to include on the front page.
Update:
Oh, and yeah, did you merge in latest master yet? Test projects have been upgraded to net5.0 recently.
...ract.Tests/DataContractJsonSerializerTests/Surrogates/BasicQuantityContractSurrogateTests.cs
Outdated
Show resolved
Hide resolved
| /// The quantity type identifier. | ||
| /// </summary> | ||
| [DataMember(Order = 3)] | ||
| public TQuantity QuantityType { get; protected set; } |
There was a problem hiding this comment.
Codecov points out some of these data contract members may be lacking test coverage. If that is correct, I think we should add some basic serialization tests for these.
|
This issue has been automatically marked as stale because it has not had recent activity. It will be closed if no further activity occurs. Thank you for your contributions. |
- publishing the generated quantities
|
Ok here's a cheat-sheet of all the serialization schemas currently supported: the last 3 I have stashed locally as I wasn't sure which one(s) to select. Note that both json serializers can handle quoted numbers (so do System.Text), and in fact- this appears to be the only way for handling decimals with JsonNet (and possibly for System.Text as well). No out-of-the-box support in protobuf either :)
|
|
That's one impressive list @lipchev 😅 If decimals must be represented as strings, then I suppose it is simpler and more consistent to serialize all quantity values regardless of number type. Beyond that, let me know when this is getting ready to review or if you are waiting for feedback on anything specific 👍 |
I did a quick test today with the System.Text serializer and it looks like it is correctly handling the unquote decimal precision. That means that JsonNet is the only one that actually requires the use of strings (and the additional properties of the ExtendedContract).
These are fully supported by the For JsonNet users there are two scenarios w.r.t. the value type:
Unless there is some significant difference in performance between the UnitsNetIQuantityJsonConverter and the (not yet implemented version) of the PreciseQuantityJsonConverter, I think I would favor the latter. Finally, for people only interested in the IQuantity (double) interface it is trivial to provide slightly faster (and cleaner) implementations: e.g. So the main question is - HowMany converter-versions do we provide? Bonus question: what do you think about "QuantityType" as property name- maybe we should use something shorter? |
|
First, I need to understand something. Is this PR primarily about supporting WCF xml + json? System.text.json and json.net is discussed here also. There is #966, won't that cover system.text.json?
Yes! The fewest number of options that we absolutely need to provide, please. |
|
We are going about it backwards: from the existing schemas (implementations of JsonNet) that are out there we've got the corresponding DataContracts (1:1 mapping of a particular schema). Those can then be used by any of the serializers/converters (e.g. ValueUnit is the QuantityValueContract. Same for the ExtendedValueUnit and ExtendedQuantityValueContract. #966 is also based on the ExtendedQuantityValue[Contract]. Those can both be refactored using the common (generic) contract- but that is not the point of this PR. The point was to add the [DataContract] annotations and to provide reference schema(s), implemented (as an example) by the DataContractSurrogates. At the same time, I was making a point about having both an internal and external schema (our Delphi application already has a table with standard units and conversions, but knows nothing about MassUnit.Milligram). The problem is then further divided into two parts by the (JsonNet's) existing ExtendedQuantityValue schema- which I argue should be deprecated in favor of the GenericQuantityDataContract schema (with the unquoted number- example implementation with GenericQuantityDataContractSurrogate). While this PR should be good to go as it (not sure I finally managed to make codecov happy- but I think things are otherwise covered), I started work on the JsonNet-based implementations of the QuantityWithAbbreviationSchema (QuantityWithAbbreviationJsonConverter and PreciseQuantityWithAbbreviationJsonConverter)- trying out the capabilities of the different serializers (producing the cheat-sheet). The two implementations above are ready- and mostly covered, but I haven't pushed them as they add to the existing JsonConverter/Tests projects. If you would rather have it all in one- I can push them as well (giving the PR some broader name)... |
That is my question as well- we can easily have both the Generic & Extended schemas implemented- but should we and which one would you recommend? |
| @@ -1,4 +1,4 @@ | |||
| <?xml version="1.0" encoding="utf-8"?> | |||
| <packages> | |||
| <package id="nanoFramework.CoreLibrary" version="1.10.4-preview.11" targetFramework="netnanoframework10" /> | |||
There was a problem hiding this comment.
I don't think this PR should have to touch these nanoframework files?
Make sure you have merged in latest origin/master and then maybe revert these.
There was a problem hiding this comment.
That is my question as well- we can easily have both the Generic & Extended schemas implemented- but should we and which one would you recommend?
I don't like the extended schema. If some quantities need quoted numbers, then I think it would be simpler to just quote all numbers.
I have to admit I am struggling to grasp all the contracts, and surrogates and converters here and in #966.
That table alone is mind-boggling to put it mildly 😅 so bear with me if I am not quite getting it.
How about these two contracts:
- Shape A: Quoted number + full enum name
- Shape B: Quoted number + unit abbreviation
It could then look like this:
- WCF and its
DataContractSerializer(xml) orDataContractJsonSerializer(json)- Shape A:
GenericQuantityDataContractSurrogate - Shape B:
QuantityWithAbbreviationContractSurrogate
- Shape A:
- Json.NET and its
JsonConverter- Shape A:
GenericQuantityJsonConverter(but with quoted numbers) - Shape B:
QuantityWithAbbreviationJsonConverter(but with quoted numbers)
- Shape A:
- System.Text.Json and its
JsonSerializer - Protobuf and other data contract compatible serializers
- I assume similar converters as above must be created for each? That pesky decimal seems to be the main problem.
There was a problem hiding this comment.
Those got auto-incremented on the last commit (after running the build script)- I didn't follow much of the discussion around the nano-framework, so I thought this was intentional.
There was a problem hiding this comment.
Yes, sorry, that was a flaw in the setup earlier. It should have been fixed by now.
There was a problem hiding this comment.
I've updated the summary:
- From what I gather there should be no backwards support for the Basic & Extended schemas (will remove the corresponding Surrogates).
- All values should be quoted: will update the contracts & tests
- This would break the compatibility tests- so those should be removed as well
- Should I add the new JsonNet converters? System.Text?
- If (4) then should I keep the DC <-> JsonNet compatibility tests? Maybe change them something about them? I first wanted to make it work with strings and quantities- but the format is never exactly the same : there is a difference in serializing slashes- like in "mg/l", so I went with the serialize-with-one -deserialize-with-another workflow..
- Nano packages should be reverted
There was a problem hiding this comment.
- I think breaking backwards compatibility for new contracts/shapes is fine.
- Yes
- Yes
- json.net and system.text converters can be added in separate PRs, if it makes sense to scope this PR can to only the data contracts. You decide. Also I don't know PR 966 well enough, if you should reuse that work or not.
- If I understand correctly, Shape A and Shape B should have compatible JSON formats regardless of DC, JsonNet or SystemText being used. If so, then yes, compatibility tests sounds useful to keep.
- Yes
|
I'm going to start this over: this time without the DataContractSurrogates - there is simply too much overhead (with the dependency to .NET Framework) - and I can't imagine anybody is ever going to upgrade to that particular setup. |
|
@lipchev Ok, sounds good to me. It did feel a bit overwhelming with all different ways to serialize, and I believe surrogates and WCF is largely a thing of the past. I can imagine a very small minority would be interested in that. |
..also
a) GenericQuantityDataContractSurrogate: {"1.20", "InformationUnit.Exabyte"} // output should be quoted- numeric input should be handled automatically, quoted input- parsed
b)
ExtendedQuantityDataContractSurrogate: {"1.2", "InformationUnit.Exabyte", ""1.20", "decimal"} // same format as the one used with the UnitsNetIQuantityJsonConverterc)
BasicQuantityContractSurrogate: {"1.2", "InformationUnit.Exabyte"} // same format as the one used with the UnitsNetJsonConverter (obsolete)d) QuantityWithAbbreviationContractSurrogate: {"1", "kg", "Mass"} // output should be quoted- numeric input should be handled automatically, quoted input- parsed
added two-way compatibility tests with both UnitsNetJsonConverter & UnitsNetIQuantityJsonConverter (in their respective test-projects- only applicable to net48, which I had to include)(the DC <-> JsonNet compatibility would be broken- unless I also include their respective converters)Finally here is some background info on how you can setup data contract surrogates as a service behavior (pretty much the same way you fix the nasty date format, only they've removed the property, along with the whole interface after net48).