weixin_39789499
weixin_39789499
2020-12-29 05:52

All currently implemented UID Generate methods cannot guarantee to generate valid DICOM UIDs

All currently implemented DicomUID.Generate methods cannot guarantee that a valid DICOM unique UID is generated.

A DICOM UID generator should: 1) generate process wide unique IDs 2) generate system wide unique IDs (more strict then rule 1) 3) generate universal wide unique IDs (more strict then rule 2) 4) Follow the format specifications of the DICOM standard.

Examples (based on source code of v4.0.0):


public static DicomUID Generate(string name) // class DicomUID
{
    if (string.IsNullOrEmpty(RootUID))
    {
        RootUID = "1.2.826.0.1.3680043.2.1343.1";
    }

    var uid = $"{RootUID}.{DateTime.UtcNow}.{DateTime.UtcNow.Ticks}";

    return new DicomUID(uid, name, DicomUidType.SOPInstance);
}

This method fails for rule 1. The resolution of Ticks is not small enough to guarantee that 2 calls quickly after each other to this method won't generate the same UID.


public static DicomUID Generate() // class DicomUID
{
    var generator = new DicomUIDGenerator();
    return DicomUIDGenerator.GenerateNew();
}

This method uses DicomUIDGenerator.GeneratorNew() and passes rule 1 but fails for rule 2 (see below)


public static DicomUID GenerateNew() // DicomUIDGenerator
{
    lock (_lock)
    {
        var ticks = DateTime.UtcNow.Subtract(Y2K).Ticks;
        if (ticks <= _lastTicks) ticks = _lastTicks + 1;
        _lastTicks = ticks;

        var str = ticks.ToString();
        if (str.EndsWith("0000")) str = str.Substring(0, str.Length - 4);

        var uid = new StringBuilder();
        uid.Append(InstanceRootUID.UID).Append('.').Append(str);

        return new DicomUID(uid.ToString(), "SOP Instance UID", DicomUidType.SOPInstance);
    }
}

This method passes rule 1, but fails rule 2. Running 2 DICOM applications that need to generate UIDs on the same system or on different computers have a high change of generating them at the same time, causing the generation of duplicate UIDs. Note: most computers have their time synced, so UTC time is almost the same on many systems.


public static DicomUID GenerateDerivedFromUUID()  // DicomUIDGenerator
{            
    var guid = Guid.NewGuid().ToByteArray();
    var bigint = new System.Numerics.BigInteger(guid);
    if (bigint < 0) bigint = -bigint;
    var uid = "2.25." + bigint;

    return new DicomUID(uid, "Local UID", DicomUidType.Unknown);
}

This method passes 1, 2 but fails 4 and probably fails 3. DICOM requires a decimal representation of a Universally Unique Identifier (UUID) (see PS 3.5, B.2). ToByteArray() returns the bytes of the Guid in a special order (see .NET docs and wikipedia), which needs to be transformed before it can be passed into the constructor of BigInteger (which expects the array in little endian order). The order of the bytes is essential to keep the transformed UUID unique. Swapping them may cause conflicts with conforming UID generators.

Recommendation: The GenerateDerivedFromUUID() method (when fixed) can supports almost all use cases for UID generation. The exceptional use cases (private SOP class, transfer syntax, etc) are mostly generated in a manual way. An easy solution is to only use this method in the fo-dicom library and remove all the other generator functions.

Note: an identical issue (#546) about this topic was opened in the past, but only solved a problem with rule 1.

该提问来源于开源项目:fo-dicom/fo-dicom

  • 点赞
  • 写回答
  • 关注问题
  • 收藏
  • 复制链接分享
  • 邀请回答

9条回答

  • weixin_39789499 weixin_39789499 4月前

    Prefix "2.25" is required by the DICOM standard when the algorithm to generate UIDs as described in DICOM Part 5, appendix B.2 (page 107) is used.

    点赞 评论 复制链接分享
  • weixin_39864373 weixin_39864373 4月前

    Very good point. And thanks for also providing a fix with the issue description. Would you mind to fix the method and create a pull request?

    点赞 评论 复制链接分享
  • weixin_39789499 weixin_39789499 4月前

    Sure, can create fix and pull request. Already cloned the repro.

    点赞 评论 复制链接分享
  • weixin_39789499 weixin_39789499 4月前

    The second fix would be forwarding the call in Generate() in class DicomUID to GenerateDerivedFromUUID and remove the Generate(string name) overload.

    In essence this would mean that the class DicomUIDGenerator becomes obsolete when GenerateDerivedFromUUID is moved to DicomUID.

    Remove the public class DicomUIDGenerator and/or the method Generate(string name) would be a breaking API change. What is the preferred policy for that?

    点赞 评论 复制链接分享
  • weixin_39864373 weixin_39864373 4月前

    Therre are still helper methods in DicomUIDGenerator like RegenerateAll or the mapping of sourceuid and destinationuid. I would not remove them without any reason.

    The policy with breaking changes is, that the methods are marked as Obsolete and kept - if possible. When refactoring or extending later then the obsolete methods may be deleted if they dont fit any more. The geneation of UID may brake in some circumstances and therefore is far not optimal and has to be replaced, but most of the time the old generation will work fine, And as I understand the generation of UIDs with the reading Root UID (organisational root) will be removed, but I know that some archives rely on that images have this fix prefix and with that routing and storage rules are configured. Users need the chance to have time move to the new API. So please dont delete it but mark it as obsolete. All the internal UID generations of course will then use the new implementation.

    点赞 评论 复制链接分享
  • weixin_39789499 weixin_39789499 4月前

    I will then just update only the forwarding of DicomUID.Generate() to GenerateDerivedFromUUID and mark the other 2 "create" functions obsolete.

    Do you want a new pull request or shall I extend the current one?

    off topic: DicomUIDGenerator.RegenerateAll is a strange function. There are no unit tests for it, so its use case is unclear to me. I can understand the use case of updating the StudyUid, SeriesUid and InstanceUid for an instance, but not all "unknown" uids. As a member of DicomUIDGenerator it is also hard for end-users to discover, and it creates a dependency from DicomUIDGenerator on the class DicomDataSet. Maybe this method would be a better member of the DicomDatasetExtensions class.

    点赞 评论 复制链接分享
  • weixin_39864373 weixin_39864373 4月前

    please extend the current pull request. Thanks a lot!

    点赞 评论 复制链接分享
  • weixin_39789499 weixin_39789499 4月前

    done

    点赞 评论 复制链接分享
  • weixin_39774556 weixin_39774556 4月前

    why is there this "2.25" prefix all the time? Where is the root-UID gone?

    see some parts in #862

    点赞 评论 复制链接分享

相关推荐