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

Some decimal values are converted to too long decimal string and fails validation #1454

Closed
magla42 opened this issue Sep 27, 2022 · 2 comments · Fixed by #1457
Closed

Some decimal values are converted to too long decimal string and fails validation #1454

magla42 opened this issue Sep 27, 2022 · 2 comments · Fixed by #1457
Labels

Comments

@magla42
Copy link
Contributor

magla42 commented Sep 27, 2022

Describe the bug
There are corner cases of decimal values that are not converted to a valid VR=DS value, causing fo-dicom to throw a validation exception.

To Reproduce
The code

var dataset = new DicomDataset();
dataset.Add(DicomTag.TableTopLateralPosition, -12345678901234.1m);

causes this exception to be thrown:

FellowOakDicom.DicomValidationException : Content "-1.2345678901E+13" does not validate VR DS: value exceeds maximum length of 16 characters
at FellowOakDicom.DicomValidation.ValidateDS(String content)

The example is of course completely bonkers in practice and I have no use case for when this would actually happen, but the fact that there exists valid decimal values that can not be converted to a valid DS attribute is at least slightly worrying.

Expected behavior
For a decimal to always be converted to a valid Decimal String. In this case, e.g., "-1.23456789E+13"

One solution could for example be to change the line
valueString = value.ToString("G11", CultureInfo.InvariantCulture);
in DicomDecimalString.ToDecimalString to
valueString = value.ToString("G10", CultureInfo.InvariantCulture);

Environment
Fellow Oak DICOM version: 5.0.2.4
OS: Windows 10 x64
Platform: .NET Framework 4.8

@magla42 magla42 added the new label Sep 27, 2022
@gofal
Copy link
Contributor

gofal commented Oct 2, 2022

You got a point. the code already tries to serialize the decimal. And in case the lenth of 16 characters is exceeded, the serialization is redone using the G11 formatter. But this G11 can still be too long.
Your suggestion to use G10 as formatter would be perfect solution.

Would you mind to make a PR containing this change and adding a method to unittests to add the number you posted above?

@gofal gofal added bug and removed new labels Oct 2, 2022
@magla42
Copy link
Contributor Author

magla42 commented Oct 3, 2022

Absolutely, I'll make time for that as soon as possible

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

Successfully merging a pull request may close this issue.

2 participants