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

Exception "File Set ID must not be null or empty" thrown on calling DicomDirectory constructor with no parameters #1176

Closed
rachelyeshurun opened this issue May 9, 2021 · 2 comments

Comments

@rachelyeshurun
Copy link

Describe the bug
DicomDirectory() throws exception

dir.FileSetID' threw an exception of type 'Dicom.DicomDataException

Stack trace:

at Dicom.DicomDataset.GetSingleValue[T](DicomTag tag) in C:\\dev\\GitHub\\fo-dicom\\fo-dicom\\DICOM\\DicomDataset.cs:line 496\r\n   at Dicom.Media.DicomDirectory.get_FileSetID() in C:\\dev\\GitHub\\fo-dicom\\fo-dicom\\DICOM\\Media\\DicomDirectory.cs:line 62

To Reproduce
Steps to reproduce the behavior (code snippet or clear description)
Type the following code and run in debug to see the exception that is thrown

DicomDirectory dir = new DicomDirectory();

Note there is an easy workaround to just set the FileSetID yourself after construction.

            DicomDirectory dir = new DicomDirectory
            {
                FileSetID = "DICOMDIR"
            };

Expected behavior
DicomDirectory() constructor should not throw that exception since the user didn't supply the FileSetId string empty or otherwise. The constructor code itself sets FileSetID to empty string. See DicomDirectory.cs

        /// <summary>
        /// Initializes a new instance of the <see cref="DicomDirectory"/> class.
        /// </summary>
        /// <param name="explicitVr">Indicates whether or not Value Representation of the DICOM directory should be explicit.</param>
        public DicomDirectory(bool explicitVr = true)
        {
            FileMetaInfo.Version = new byte[] { 0x00, 0x01 };
            FileMetaInfo.MediaStorageSOPClassUID = DicomUID.MediaStorageDirectoryStorage;
            FileMetaInfo.MediaStorageSOPInstanceUID = DicomUID.Generate();
            FileMetaInfo.TransferSyntax = explicitVr
                                              ? DicomTransferSyntax.ExplicitVRLittleEndian
                                              : DicomTransferSyntax.ImplicitVRLittleEndian;
            FileMetaInfo.ImplementationClassUID = DicomImplementation.ClassUID;
            FileMetaInfo.ImplementationVersionName = DicomImplementation.Version;

            _directoryRecordSequence = new DicomSequence(DicomTag.DirectoryRecordSequence);

            Dataset.Add(DicomTag.FileSetID, string.Empty)
                .Add<ushort>(DicomTag.FileSetConsistencyFlag, 0)
                .Add(DicomTag.OffsetOfTheFirstDirectoryRecordOfTheRootDirectoryEntity, 0U)
                .Add(DicomTag.OffsetOfTheLastDirectoryRecordOfTheRootDirectoryEntity, 0U)
                .Add(_directoryRecordSequence);
        }

Screenshots or test DICOM files

image

Environment
Fellow Oak DICOM version: 4.0.7
OS: Windows 10 Pro x64
Platform: Microsoft .NET Framework Version 4.8.04084

@gofal
Copy link
Contributor

gofal commented May 20, 2021

To get you right: This exception does not happen in your production code, but you only observe the exception when debugging and viewing the properties of dir in your inspector window?
At least that is what I can reproduce.

The FileSetID shall be an ID for your fileset, e.g. the DICOM Standard says that for burned CDs this FileSetID should be the same als the volume name of the CD. It is not a good idea if fo-dicom would initialize it with something like "DICOMDIR", since it does not make any sense (as if you would initialize every DicomDataset with Patientname "John^Doe").

You expect that the exception does not throw, would you mean that the property should return a string.empty when accessed in code before it was set with a meaningfull value?

@rachelyeshurun
Copy link
Author

The exception does happen in production code as I call this constructor:
DicomDirectory dir = new DicomDirectory();

In fact, I just copied the WriteMedia example here

I understand why it's best to initialize it to empty string, but yes, I think it's better not to throw an exception just for accessing it. Maybe at some later point when empty string FileSetID makes it impossible to continue.

@gofal gofal removed the new label May 25, 2021
@gofal gofal closed this as completed in 6a5de51 May 25, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

2 participants