JsonDicomConverter allows serializing DS/IS dicom item with invalid values

[pengchen0692]

Fixes #1355 .

Checklist

• The pull request branch is in sync with latest commit on the fo-dicom/development branch
• I have updated API documentation
• I have included unit tests
• I have updated the change log
• I am listed in the CONTRIBUTORS file

Changes proposed in this pull request:

• When autoValidate is false, write DS or IS value as string instead of number

[codecov]

Codecov Report

Merging #1354 (21a9a76) into development (f386528) will decrease coverage by 0.03%.
The diff coverage is 100.00%.

@@               Coverage Diff               @@
##           development    #1354      +/-   ##
===============================================
- Coverage        79.49%   79.46%   -0.04%     
===============================================
  Files              276      276              
  Lines            24409    24427      +18     
===============================================
+ Hits             19405    19411       +6     
- Misses            5004     5016      +12     

Impacted Files	Coverage Δ
FO-DICOM.Core/Serialization/DicomJson.cs	100.00% <100.00%> (ø)
FO-DICOM.Core/Serialization/JsonDicomConverter.cs	94.23% <100.00%> (+0.11%)	⬆️
Serialization/FO-DICOM.Json/JsonDicomConverter.cs	96.52% <100.00%> (+0.07%)	⬆️
FO-DICOM.Core/ServiceProviderHost.cs	0.00% <0.00%> (-100.00%)	⬇️
FO-DICOM.Core/Setup.cs	73.33% <0.00%> (-20.00%)	⬇️

---

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 f386528...21a9a76. Read the comment docs.

[mrbean-bremen]

Thanks, looks good to me!

[wsugarman]

Should we move this line to the IS and DS cases below instead of doing it upfront? Perhaps we simply use WriteJsonElement<string> for IS, but update WriteJsonDecimalString to accept the autoValidate flag

[pengchen0692]

maybe have method WriteJsonIntegerString to handle IS individually?

[wsugarman]

How does this auto-validation work with DicomValidation.PerformValidation? This is isolated to simply DicomDataset or should that also define this behavior too?

[pengchen0692]

I think the structure is a bit confusing, guess there is some historical reason.

[gofal]

There are 2 steps to turn validation off. By default fo-dicom should do validation. but you can turn the validation off explicitly for some DicomDatasets (that was requested by some users). And then the worst thing is, to turn validation off globally (was also requested). This property to turn validation off globally is marked obsolete, so that you at least get some warning that you should not do that :-)

[gofal]

So i am pretty fine, if the global PerformValidation is not implemented here. Lets wait, until it is requested, fine?

[pvrobays]

@pengchen0692 Thanks for the nice PR. We stumbled upon the same issue and I was just about to fix it myself until I found this PR.
Sorry to comment on a closed PR, but I have a concern with the current implementation though.

Once you set your JsonDicomConverter to autoValidate: false, by default we'll handle all DS and IS values as JSON strings instead of JSON numbers.

So say we have the following valid dataset:

var dataset = new DicomDataset();
dataset.Add(new DicomDecimalString(DicomTag.PatientSize, 3.5m));
dataset.Add(new DicomIntegerString(DicomTag.ReferencedFrameNumber, 76));

then setting autoValidate: true will return JSON numbers:

var json = JsonConvert.SerializeObject(dataset, new JsonDicomConverter(autoValidate: false));
Assert.Equal("{\"00081160\":{\"vr\":\"IS\",\"Value\":[76]},\"00101020\":{\"vr\":\"DS\",\"Value\":[3.5]}}", json);
// note: there are no quotes around the numbers

and setting autoValidate: false will return JSON strings:

var json = JsonConvert.SerializeObject(dataset, new JsonDicomConverter(autoValidate: false));
Assert.Equal("{\"00081160\":{\"vr\":\"IS\",\"Value\":[\"76\"]},\"00101020\":{\"vr\":\"DS\",\"Value\":[\"3.5\"]}}", json);
// note: the numbers are surrounded with quotes

So my proposed solution would be to first try and parse the values as numbers, if that fails and autoValidate = false then fallback to parse it as strings.

[gofal]

I see, that there is a difference. Throuh both are valid, according to standard: https://dicom.nema.org/medical/dicom/current/output/chtml/part18/sect_F.2.3.html
"IS" is actually "IntegerString", so you can savely represent them as integer as well as string. So every parser has to deal with both json-types.

Do you think, storing the value as number has some advantages for some reason than storing as string? The option autovalidate actually just forwards the string content (because IS is a string-type in DICOM), instead of parsing it as integer.
Maybe the naming could be different, instead of "autovalidate" you could claim that the parameter should be "writeNumbersAsString" ?

[pvrobays]

Hi @gofal! I like your idea of making the parameter more explicit. Come to think of it, an autoValidate parameter for JSON serialization seems a bit off. For deserialization it makes sense.

Meanwhile I've had an internal discussion with @amoerie and would propose the following:
As the standard states that for some VRs (IS, DS, SV and UV) it's allowed to serialize them both as number and as string, I would suggest to make this an (enum) option in the JsonDicomConvert. For example:

JsonConvert.SerializeObject(dataset, new JsonDicomConverter(numberSerializationMode: NumberSerializationMode.AsString));

The possible values for NumberSerializationMode could be:

• AsString (default?)
• AsNumber

Then the next question is: what should we do with an invalid number in case NumberSerialzationMode = AsNumber? Should we just throw an exception in that case, or then fall back to serialize it as a string?
Therefor the NumberSerializationMode may have a third option: PreferablyAsNumber.

I'd like to hear your feedback on this @gofal. What would be a sensible default?

FYI, we also considered to look at the DicomDataSet.AutoValidate to automatically choose between the NumberSerializationMode. However, that property is obsolete and probably shouldn't be used.

[amoerie]

Yeah, a nice addition to this story could have been to default to AsNumber for validated DICOM datasets and AsString for unvalidated DICOM datasets, but AFAIK there is no public property that indicates whether a DICOM dataset is already validated.

I also found it confusing that "autoValidate" suddenly also applies to serialisation. My assumption was that this parameter only applied to deserialisation. Therefore, I think a new parameter of type NumberSerializationMode with default value NumberSerializationMode.AsString makes the most sense.

[gofal]

@pvrobays I also like this idea of having a serialization parameter. I'd be fine with both, if AsString or AsNumber would be the default. The AsNumber would be backwardscompatible (and maybe prevent some unittests to get red suddenly) and should normally not be any problem. The AsString would be the savest choice. So the PreferablyAsNumber would be the best out of the two worlds!

[gofal]

The DicomDataset.AutoValidate property is something that I had to add due to the preasure of the community. I strongly think that a library has the responsibility to only create valid data. Therefore the validation was introduced. If the user wants to overrule this for some reason, then he takes responsibility and has to set the autovalidation property of a dataset to "false" before adding messy data to the dataset.
But so many applications out there seem to work with invalid data (because they already receive the invalid data from other sources) and therefore had to turn off validation for nearly every single dataset. So many users demanded to turn Validation off globaly. The compromise was to add this property, but to mark it as obsolete, so that at least they get a warning all the time, or to prevent new users from using this property.

[gofal]

Ok, then I guess it's time to create a new issue and a new PR with that changes?

[pvrobays]

Thanks for the background information @gofal.

I'll open an issue and PR one of the following days. So to conclude:

• Remove the autoValidate from serialization
• Add numberSerializationMode to the serialization with 3 modes:

1. AsNumber (default, to be backwards compatible) - will throw an exception in case it tries to serialize an invalid number
2. AsString - always serializes as a string,
3. PreferablyAsNumber - tries to serialize as a number, if that fails will serialize it as a string

FYI @pengchen0692 @amoerie