Repository: pydicom/pydicom

Issue:
Crash with specific UIDs
The following crashes with infinite recursion under Python 2.7:
``` python
    def test_name_with_equal_hash(self):
        uid = UID('1.3.12.2.1107.5.2.18.41538.2017072416190348328326500')
        assert uid.name == '1.3.12.2.1107.5.2.18.41538.2017072416190348328326500'
```
If accessing the name, it is checked if the UID string is one of the predefined strings. 
The given UID has the same hash as a predefined UID (Basic Voice Audio Waveform Storage), and therefore the `__eq__` operator is invoked. This operator compares the UID names, what leads to the recursion.

In 0.99 and previous versions, the crash happened in `UID.__init()`, with `AttributeError: 'UID' object has no attribute 'name'` (happened in the field with SOPInstanceUID).
In Python 3, the string hash has changed, so it does not occur there, at least for this specific UID.


Discussion:
pep8speaks: Hello @mrbean-bremen! Thanks for submitting the PR.

 - In the file [`pydicom/tests/test_uid.py`](https://github.com/pydicom/pydicom/blob/43d2c913f18863dfff70f8a052187f422f067a0e/pydicom/tests/test_uid.py), following are the PEP8 issues :

> [Line 64:16](https://github.com/pydicom/pydicom/blob/43d2c913f18863dfff70f8a052187f422f067a0e/pydicom/tests/test_uid.py#L64): [E714](https://duckduckgo.com/?q=pep8%20E714) test for object identity should be 'is not'
> [Line 187:80](https://github.com/pydicom/pydicom/blob/43d2c913f18863dfff70f8a052187f422f067a0e/pydicom/tests/test_uid.py#L187): [E501](https://duckduckgo.com/?q=pep8%20E501) line too long (80 > 79 characters)
> [Line 190:80](https://github.com/pydicom/pydicom/blob/43d2c913f18863dfff70f8a052187f422f067a0e/pydicom/tests/test_uid.py#L190): [E501](https://duckduckgo.com/?q=pep8%20E501) line too long (81 > 79 characters)




scaramallion: Apart from that, looks good
massich: There are several things that I don't understand. First of all, I can't reproduce the error. But appart from that i don't understand the implementation of [UID's `__eq__` function](https://github.com/pydicom/pydicom/blob/7783dce0013448e9c1acdb1b06e4ffbe97c3d334/pydicom/uid.py#L63) which ends up bringing up the problems here: ` if str.__str__(self) in UID_dictionary:`

Couldn't it be easier to have an implentation of `__eq__` that looked more like this:
```py
def __eq__(self, other):
     """Return True if `self` or `self.name` equals `other`."""
     if isinstance(other, UID): 
          return self.name == other.name
     elif isinstance(other, str):
          return self.name == other
     else:
          raise NotImplementedError
```
mrbean-bremen: First, this would not work as intended, if `self` is a predefined UID and `other` is the UID string of that uid.
Second, this would not fix the infinite recursion while accessing `self.name`.
massich: (following from [this](https://github.com/pydicom/pydicom/pull/500#discussion_r137766395) conversation)

>> And using, lambda _: 88888888888 as the new `__hash__` function is clear that I'm using a constant arbitrary value.
>
>No, we need to use the hash of an existing uid.

I still don't see it. I've to say though that at some point I recall seing somewhere that its iterated through the `UID_dictionary` but I can recall where now (and I couldn't find it). 

I think that a constant value should be enough. But I might be wrong for what I just said. Anyhow if an arbitrary constant cannot be used I would make more explicit that we are testing to an arbitrary hash of an uid existing in the dictionary. 

```py
  def test_name_with_equal_hash(self):
￼          """Test that UID name works for UID with same hash as predefined UID.

        This is a regression test from #499 where infinite recursion ... (little description needed)
￼          """
       class MockedUID(UID):
            def __hash__(self):
                ARBITRARY_UID_HASH_CONST = hash(UID_dictionary[0])
                return ARBITRARY_UID_HASH_CONST

        uid = MockedUID('1.2.3')
        assert uid.name == '1.2.3'
```

If declaring a constant seems overkill then: `return hash(UID_dictionary[0])` + a comment saying that any hash from a uid within UID_dictionary would do the job. (In other words a comment that convince the reader why `return 42` or `return hash('42')` is not suited)
massich: But I understand that we might be nitpicking here, and this is just a bugfix. 
mrbean-bremen: @massich - is that ok for you?
massich: I would still do `hash(UID_dictionary[0])` instead of JPEGLOSSLY since it is more clear. But again, this is nitpicking.
mrbean-bremen: >I would still do hash(UID_dictionary[0]) instead of JPEGLOSSLY since it is more clear.

I didn't do this because `UID_dictionary` is in a private package and not supposed to be accessed.

Patch:
diff --git a/pydicom/uid.py b/pydicom/uid.py
index 483159e47..741450672 100644
--- a/pydicom/uid.py
+++ b/pydicom/uid.py
@@ -162,15 +162,16 @@ class UID(str):
     @property
     def name(self):
         """Return the UID name from the UID dictionary."""
-        if self in UID_dictionary:
+        uid_string = str.__str__(self)
+        if uid_string in UID_dictionary:
             return UID_dictionary[self][0]
 
-        return str.__str__(self)
+        return uid_string
 
     @property
     def type(self):
         """Return the UID type from the UID dictionary."""
-        if self in UID_dictionary:
+        if str.__str__(self) in UID_dictionary:
             return UID_dictionary[self][1]
 
         return ''
@@ -178,7 +179,7 @@ class UID(str):
     @property
     def info(self):
         """Return the UID info from the UID dictionary."""
-        if self in UID_dictionary:
+        if str.__str__(self) in UID_dictionary:
             return UID_dictionary[self][2]
 
         return ''
@@ -186,7 +187,7 @@ class UID(str):
     @property
     def is_retired(self):
         """Return True if the UID is retired, False otherwise or if private."""
-        if self in UID_dictionary:
+        if str.__str__(self) in UID_dictionary:
             return bool(UID_dictionary[self][3])
 
         return False


Test patch:
diff --git a/pydicom/tests/test_uid.py b/pydicom/tests/test_uid.py
index f035c4cdd..2d7938b89 100644
--- a/pydicom/tests/test_uid.py
+++ b/pydicom/tests/test_uid.py
@@ -3,7 +3,7 @@
 
 import pytest
 
-from pydicom.uid import UID, generate_uid, PYDICOM_ROOT_UID
+from pydicom.uid import UID, generate_uid, PYDICOM_ROOT_UID, JPEGLSLossy
 
 
 class TestGenerateUID(object):
@@ -61,7 +61,6 @@ class TestUID(object):
         assert not self.uid == 'Explicit VR Little Endian'
         # Issue 96
         assert not self.uid == 3
-        assert not self.uid is None
 
     def test_inequality(self):
         """Test that UID.__ne__ works."""
@@ -73,7 +72,6 @@ class TestUID(object):
         assert self.uid != 'Explicit VR Little Endian'
         # Issue 96
         assert self.uid != 3
-        assert self.uid is not None
 
     def test_hash(self):
         """Test that UID.__hash_- works."""
@@ -183,6 +181,21 @@ class TestUID(object):
         assert self.uid.name == 'Implicit VR Little Endian'
         assert UID('1.2.840.10008.5.1.4.1.1.2').name == 'CT Image Storage'
 
+    def test_name_with_equal_hash(self):
+        """Test that UID name works for UID with same hash as predefined UID.
+        """
+        class MockedUID(UID):
+            # Force the UID to return the same hash as one of the
+            # uid dictionary entries (any will work).
+            # The resulting hash collision forces the usage of the `eq`
+            # operator while checking for containment in the uid dictionary
+            # (regression test for issue #499)
+            def __hash__(self):
+                return hash(JPEGLSLossy)
+
+        uid = MockedUID('1.2.3')
+        assert uid.name == '1.2.3'
+
     def test_type(self):
         """Test that UID.type works."""
         assert self.uid.type == 'Transfer Syntax'