Repository: pydicom/pynetdicom
Pull request #634 (2021-06-03 02:34:30)

[MRG] Add better handling of invalid A-ASSOCIATE (RJ) parameters during ACSE negotiation
#### Reference issue
Related to #633, handle invalid result, source and diagnostic parameters in the A_ASSOCIATE string methods.

#### Tasks
- [x] Unit tests added that reproduce issue or prove feature is working
- [x] Fix or feature added
- [ ] Unit tests passing and coverage at 100% after adding fix/feature


codecov[bot]: # [Codecov](https://codecov.io/gh/pydicom/pynetdicom/pull/634?src=pr&el=h1&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=pydicom) Report
> Merging [#634](https://codecov.io/gh/pydicom/pynetdicom/pull/634?src=pr&el=desc&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=pydicom) (d92c5c3) into [master](https://codecov.io/gh/pydicom/pynetdicom/commit/21c032fa2d5be9ca436cc894a3877b9a29660529?el=desc&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=pydicom) (21c032f) will **decrease** coverage by `0.15%`.
> The diff coverage is `100.00%`.

```diff
@@             Coverage Diff             @@
##            master     #634      +/-   ##
===========================================
- Coverage   100.00%   99.84%   -0.16%     
===========================================
  Files           27       27              
  Lines         8630     8644      +14     
===========================================
+ Hits          8630     8631       +1     
- Misses           0       13      +13     
```

| [Impacted Files](https://codecov.io/gh/pydicom/pynetdicom/pull/634?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=pydicom) | Coverage Δ | |
|---|---|---|
| [pynetdicom/pdu.py](https://codecov.io/gh/pydicom/pynetdicom/pull/634/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=pydicom#diff-cHluZXRkaWNvbS9wZHUucHk=) | `100.00% <100.00%> (ø)` | |
| [pynetdicom/pdu\_primitives.py](https://codecov.io/gh/pydicom/pynetdicom/pull/634/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=pydicom#diff-cHluZXRkaWNvbS9wZHVfcHJpbWl0aXZlcy5weQ==) | `100.00% <100.00%> (ø)` | |
| [pynetdicom/utils.py](https://codecov.io/gh/pydicom/pynetdicom/pull/634/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=pydicom#diff-cHluZXRkaWNvbS91dGlscy5weQ==) | `84.14% <0.00%> (-15.86%)` | :arrow_down: |


Patch:
diff --git a/docs/changelog/v2.0.0.rst b/docs/changelog/v2.0.0.rst
index 624d439b3..e2a891f9e 100644
--- a/docs/changelog/v2.0.0.rst
+++ b/docs/changelog/v2.0.0.rst
@@ -48,6 +48,8 @@ Enhancements
   option (:issue:`620`)
 * Updated to meet the 2021b version of the DICOM Standard
 * Added type hints
+* Handle non-conformant A-ASSOCIATE (RJ) 'Result', 'Source' and 'Diagnostic'
+  values during ACSE negotiation (:issue:`633`)
 
 Changes
 .......
diff --git a/pynetdicom/pdu.py b/pynetdicom/pdu.py
index d5aff727d..59b4716c7 100644
--- a/pynetdicom/pdu.py
+++ b/pynetdicom/pdu.py
@@ -1225,16 +1225,14 @@ class A_ASSOCIATE_RJ(PDU):
         }
 
         if self.source not in _reasons:
-            LOGGER.error('Invalid value in Source field in '
-                         'A-ASSOCIATE-RJ PDU')
-            raise ValueError('Invalid value in Source field in '
-                             'A-ASSOCIATE-RJ PDU')
+            msg = 'Invalid value in Source field in A-ASSOCIATE-RJ PDU'
+            LOGGER.error(msg)
+            raise ValueError(msg)
 
         if self.reason_diagnostic not in _reasons[self.source]:
-            LOGGER.error('Invalid value in Reason field in '
-                         'A-ASSOCIATE-RJ PDU')
-            raise ValueError('Invalid value in Reason field in '
-                             'A-ASSOCIATE-RJ PDU')
+            msg = 'Invalid value in Reason field in A-ASSOCIATE-RJ PDU'
+            LOGGER.error(msg)
+            raise ValueError(msg)
 
         return _reasons[self.source][self.reason_diagnostic]
 
@@ -1247,10 +1245,9 @@ class A_ASSOCIATE_RJ(PDU):
         }
 
         if self.result not in _results:
-            LOGGER.error('Invalid value in Result field in '
-                         'A-ASSOCIATE-RJ PDU')
-            raise ValueError('Invalid value in Result field in '
-                             'A-ASSOCIATE-RJ PDU')
+            msg = 'Invalid value in Result field in A-ASSOCIATE-RJ PDU'
+            LOGGER.error(msg)
+            raise ValueError(msg)
 
         return _results[self.result]
 
@@ -1264,10 +1261,9 @@ class A_ASSOCIATE_RJ(PDU):
         }
 
         if self.source not in _sources:
-            LOGGER.error('Invalid value in Source field in '
-                         'A-ASSOCIATE-RJ PDU')
-            raise ValueError('Invalid value in Source field in '
-                             'A-ASSOCIATE-RJ PDU')
+            msg = 'Invalid value in Source field in A-ASSOCIATE-RJ PDU'
+            LOGGER.error(msg)
+            raise ValueError(msg)
 
         return _sources[self.source]
 
diff --git a/pynetdicom/pdu_primitives.py b/pynetdicom/pdu_primitives.py
index d412ec77c..5de24ab19 100644
--- a/pynetdicom/pdu_primitives.py
+++ b/pynetdicom/pdu_primitives.py
@@ -345,8 +345,17 @@ class A_ASSOCIATE:
     @property
     def result_str(self) -> str:
         """Return the result as str."""
-        results = {1: "Rejected Permanent", 2: "Rejected Transient"}
-        return results[cast(int, self.result)]
+        results = {
+            0: "Accepted", 1: "Rejected Permanent", 2: "Rejected Transient"
+        }
+
+        if self.result not in results:
+            LOGGER.error(
+                f"Invalid A-ASSOCIATE 'Result' {self.result}"
+            )
+            return "(no value available)"
+
+        return results[self.result]
 
     @property
     def result_source(self) -> Optional[int]:
@@ -387,7 +396,13 @@ class A_ASSOCIATE:
             2: 'Service Provider (ACSE)',
             3: 'Service Provider (Presentation)'
         }
-        return sources[cast(int, self.result_source)]
+        if self.result_source not in sources:
+            LOGGER.error(
+                f"Invalid A-ASSOCIATE 'Result Source' {self.result_source}"
+            )
+            return "(no value available)"
+
+        return sources[self.result_source]
 
     @property
     def diagnostic(self) -> Optional[int]:
@@ -463,7 +478,14 @@ class A_ASSOCIATE:
         }
         result = cast(int, self.result_source)
         diagnostic = cast(int, self.diagnostic)
-        return reasons[result][diagnostic]
+        try:
+            return reasons[result][diagnostic]
+        except KeyError:
+            LOGGER.error(
+                f"Invalid A-ASSOCIATE 'Result Source' {result} and/or "
+                f"'Diagnostic' {diagnostic} values"
+            )
+            return "(no value available)"
 
     @property
     def calling_presentation_address(self) -> Optional[Tuple[str, int]]:


Test patch:
diff --git a/pynetdicom/tests/test_primitives.py b/pynetdicom/tests/test_primitives.py
index ae7ef365e..290b33c45 100644
--- a/pynetdicom/tests/test_primitives.py
+++ b/pynetdicom/tests/test_primitives.py
@@ -1012,6 +1012,35 @@ class TestPrimitive_A_ASSOCIATE:
             b"\x43\x4f\x4d\x5f\x30\x39\x30"
         )
 
+    def test_invalid_result_str(self, caplog):
+        """Test an invalid result value gets logged and doesn't raise."""
+        pdu = A_ASSOCIATE()
+        pdu.result = None
+        with caplog.at_level(logging.ERROR, logger='pynetdicom'):
+            assert pdu.result_str == '(no value available)'
+            assert "Invalid A-ASSOCIATE 'Result' None" in caplog.text
+
+    def test_invalid_source_str(self, caplog):
+        """Test an invalid source value gets logged and doesn't raise."""
+        pdu = A_ASSOCIATE()
+        pdu.result_source = None
+        with caplog.at_level(logging.ERROR, logger='pynetdicom'):
+            assert pdu.source_str == '(no value available)'
+            assert "Invalid A-ASSOCIATE 'Result Source' None" in caplog.text
+
+    def test_invalid_reason_str(self, caplog):
+        """Test an invalid diagnostic value gets logged and doesn't raise."""
+        pdu = A_ASSOCIATE()
+        pdu.result = 1
+        pdu.result_source = 2
+        pdu.diagnostic = 7
+        with caplog.at_level(logging.ERROR, logger='pynetdicom'):
+            assert pdu.reason_str == '(no value available)'
+            assert (
+                "Invalid A-ASSOCIATE 'Result Source' 2 and/or "
+                "'Diagnostic' 7 values"
+            ) in caplog.text
+
 
 class TestPrimitive_A_RELEASE:
     def test_assignment(self):