Repository: pydicom/pydicom

Issue:
Handling of DS too long to be encoded in explicit encoding
<!-- Instructions For Filing a Bug: https://github.com/pydicom/pydicom/blob/master/CONTRIBUTING.md#filing-bugs -->

#### Description
This is probably not a bug, but I'm not sure about the wanted behavior.
An RTPlan dataset encoded as Little Endian Implicit contains multiple values in the DS tag DHV Data (3004,0058) with an overall length not fitting into 2 bytes. Trying to write this as explicit Little Endian fails with an exception (`"ushort format requires 0 &lt;= number &lt;= (0x7fff * 2 + 1)"`) which is raised by the `pack` call in `write_leUS` while trying to write the length.

The standard says for this case in PS3.5, Table 6.2-1 (for VR DS):
```
Note
Data Elements with multiple values using this VR may not be properly encoded if Explicit-VR transfer syntax is used and the VL of this attribute exceeds 65534 bytes.
```
So, as I understand it, this is valid DICOM, that cannot be converted to explicit encoding without data loss.
The question is how to handle this. What comes to mind:
- truncate the value and log a warning
- raise a meaningful exception
- adapt the behavior depending on some config setting

Any thoughts?

<!-- Example: Attribute Error thrown when printing (0x0010, 0x0020) patient Id> 0-->

#### Steps/Code to Reproduce
<!--
Example:
```py
from io import BytesIO
from pydicom import dcmread

bytestream = b'\x02\x00\x02\x00\x55\x49\x16\x00\x31\x2e\x32\x2e\x38\x34\x30\x2e\x31' \
             b'\x30\x30\x30\x38\x2e\x35\x2e\x31\x2e\x31\x2e\x39\x00\x02\x00\x10\x00' \
             b'\x55\x49\x12\x00\x31\x2e\x32\x2e\x38\x34\x30\x2e\x31\x30\x30\x30\x38' \
             b'\x2e\x31\x2e\x32\x00\x20\x20\x10\x00\x02\x00\x00\x00\x01\x00\x20\x20' \
             b'\x20\x00\x06\x00\x00\x00\x4e\x4f\x52\x4d\x41\x4c'

fp = BytesIO(bytestream)
ds = dcmread(fp, force=True)

print(ds.PatientID)
```
If the code is too long, feel free to put it in a public gist and link
it in the issue: https://gist.github.com

When possible use pydicom testing examples to reproduce the errors. Otherwise, provide
an anonymous version of the data in order to replicate the errors.
-->

#### Expected Results
<!-- Please paste or describe the expected results.
Example: No error is thrown and the name of the patient is printed.-->

#### Actual Results
<!-- Please paste or specifically describe the actual output or traceback.
(Use %xmode to deactivate ipython's trace beautifier)
Example: ```AttributeError: 'FileDataset' object has no attribute 'PatientID'```
-->

#### Versions
<!--
Please run the following snippet and paste the output below.
import platform; print(platform.platform())
import sys; print("Python", sys.version)
import pydicom; print("pydicom", pydicom.__version__)
-->


<!-- Thanks for contributing! -->


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

 - There are no PEP8 issues in the file [`pydicom/filewriter.py`](https://github.com/mrbean-bremen/pydicom/blob/e8a8fede5dcf0de023e25729eaca808dac1e452e/pydicom/filewriter.py) !

 - There are no PEP8 issues in the file [`pydicom/tests/test_filewriter.py`](https://github.com/mrbean-bremen/pydicom/blob/e8a8fede5dcf0de023e25729eaca808dac1e452e/pydicom/tests/test_filewriter.py) !


mrbean-bremen: Hm, some of the tests fail consistently - also in master. I have no idea what happens...
mrbean-bremen: The failing builds are unrelated - I created a separate issue for that. 
darcymason: Forgot about this one ... merging now.

Patch:
diff --git a/doc/whatsnew/v1.3.0.rst b/doc/whatsnew/v1.3.0.rst
index 029b08933..28cee247f 100644
--- a/doc/whatsnew/v1.3.0.rst
+++ b/doc/whatsnew/v1.3.0.rst
@@ -15,5 +15,7 @@ Enhancements
 Fixes
 .....
 
+* Raise exception with specific message if value is too large to be written
+  in explicit transfer syntax (:issue:`757`)
 * Do not derive `Dataset` from `dict` - fixes behavior in newer Python versions
-  (:issue:`765`)
\ No newline at end of file
+  (:issue:`765`)
diff --git a/pydicom/filewriter.py b/pydicom/filewriter.py
index ae045069b..f3b8c04c7 100644
--- a/pydicom/filewriter.py
+++ b/pydicom/filewriter.py
@@ -2,6 +2,8 @@
 """Functions related to writing DICOM data."""
 
 from __future__ import absolute_import
+
+import struct
 from struct import pack
 
 from pydicom import compat
@@ -484,7 +486,16 @@ def write_data_element(fp, data_element, encodings=None):
     value_length = buffer.tell()
     if (not fp.is_implicit_VR and VR not in extra_length_VRs and
             not is_undefined_length):
-        fp.write_US(value_length)  # Explicit VR length field is only 2 bytes
+        try:
+            fp.write_US(value_length)  # Explicit VR length field is 2 bytes
+        except struct.error:
+            msg = ('The value for the data element {} exceeds the size '
+                   'of 64 kByte and cannot be written in an explicit transfer '
+                   'syntax. You can save it using Implicit Little Endian '
+                   'transfer syntax, or you have to truncate the value to not '
+                   'exceed the maximum size of 64 kByte.'
+                   .format(data_element.tag))
+            raise ValueError(msg)
     else:
         # write the proper length of the data_element in the length slot,
         # unless is SQ with undefined length.


Test patch:
diff --git a/pydicom/tests/test_filewriter.py b/pydicom/tests/test_filewriter.py
index a3e0ed988..2eaa4d428 100644
--- a/pydicom/tests/test_filewriter.py
+++ b/pydicom/tests/test_filewriter.py
@@ -2419,3 +2419,34 @@ class TestWriteUndefinedLengthPixelData(object):
             ds_unzipped = dcmread(fp)
             for elem_in, elem_out in zip(ds, ds_unzipped):
                 assert elem_in == elem_out
+
+    def test_writing_too_big_data_in_explicit_encoding(self):
+        """Data too large to be written in explicit transfer syntax."""
+        self.fp.is_little_endian = True
+        self.fp.is_implicit_VR = True
+        # make a multi-value larger than 64kB
+        single_value = b'123456.789012345'
+        large_value = b'\\'.join([single_value] * 4500)
+        # can be written with implicit transfer syntax,
+        # where the length field is 4 bytes long
+        pixel_data = DataElement(0x30040058, 'DS',
+                                 large_value,
+                                 is_undefined_length=False)
+        write_data_element(self.fp, pixel_data)
+
+        self.fp = DicomBytesIO()
+        self.fp.is_little_endian = True
+        self.fp.is_implicit_VR = False
+        # shall raise if trying to write it with explicit transfer syntax,
+        # where the length field is 2 bytes long
+        expected_message = (r'The value for the data element \(3004, 0058\) '
+                            r'exceeds the size of 64 kByte ')
+        with pytest.raises(ValueError, match=expected_message):
+            write_data_element(self.fp, pixel_data)
+
+        self.fp = DicomBytesIO()
+        self.fp.is_little_endian = False
+        self.fp.is_implicit_VR = False
+        # we expect the same behavior in Big Endian transfer syntax
+        with pytest.raises(ValueError, match=expected_message):
+            write_data_element(self.fp, pixel_data)