Repository: pydicom/pydicom

Issue:
Error in to_json
https://github.com/pydicom/pydicom/blob/b5d0da082bff04a4f88eee320c241db25c008b08/pydicom/dataset.py#L1889

Hi,
I am getting an error here with a Siemens Dicom file:
'bytes' object has no attribute 'encode'

The data element is the following:
`data_element
(0029, 1010) [CSA Image Header Info]             OB: Array of 9948 elements
VM:1
VR:'OB'
descripWidth:35
file_tell:3864
is_raw:False
is_retired:False
is_undefined_length:False
`


Discussion:
darcymason: @pieper, would appreciate your review on this.  I'll review as well when I get a chance.
pieper: Looks good to me - thanks 👍 
mrbean-bremen: I added all possible VRs to the round-trip test (single-value only), and found only one problem - actually the same as before with PN under Python 2, but in the other direction.

Patch:
diff --git a/doc/whatsnew/v1.4.0.rst b/doc/whatsnew/v1.4.0.rst
new file mode 100644
index 000000000..46b56f74a
--- /dev/null
+++ b/doc/whatsnew/v1.4.0.rst
@@ -0,0 +1,9 @@
+Version 1.4.0
+=================================
+
+Changelog
+---------
+
+Fixes
+.....
+* Fixed handling of binary values in json encoding (:issue:`887`)
diff --git a/pydicom/dataelem.py b/pydicom/dataelem.py
index 4d633f3af..56d05bee6 100644
--- a/pydicom/dataelem.py
+++ b/pydicom/dataelem.py
@@ -212,8 +212,10 @@ class DataElement(object):
                 raise TypeError(fmt.format(value_key, tag))
         elif value_key in {'InlineBinary', 'BulkDataURI'}:
             if isinstance(value, list):
-                fmt = '"{}" of data element "{}" must be a string.'
-                raise TypeError(fmt.format(value_key, tag))
+                fmt = '"{}" of data element "{}" must be a {}.'
+                expected_type = ('string' if value_key == 'BulkDataURI'
+                                 else 'bytes-like object')
+                raise TypeError(fmt.format(value_key, tag, expected_type))
         if vr == 'SQ':
             elem_value = []
             for value_item in value:
@@ -229,7 +231,7 @@ class DataElement(object):
                         if len(unique_value_keys) == 0:
                             logger.debug(
                                 'data element has neither key "{}".'.format(
-                                    '" nor "'.join(supported_keys)
+                                    '" nor "'.join(jsonrep.JSON_VALUE_KEYS)
                                 )
                             )
                             elem = DataElement(tag=tag, value='', VR=vr)
@@ -271,7 +273,7 @@ class DataElement(object):
                             'no bulk data URI handler provided for retrieval '
                             'of value of data element "{}"'.format(tag)
                         )
-                        elem_value = ''
+                        elem_value = b''
                     else:
                         elem_value = bulk_data_uri_handler(value)
                 else:
@@ -281,7 +283,7 @@ class DataElement(object):
                         elem_value = value
             else:
                 elem_value = value
-        if not value:
+        if elem_value is None:
             logger.warning('missing value for data element "{}"'.format(tag))
             elem_value = ''
 
diff --git a/pydicom/dataset.py b/pydicom/dataset.py
index 80a8f3d3c..efc56d935 100644
--- a/pydicom/dataset.py
+++ b/pydicom/dataset.py
@@ -343,12 +343,10 @@ class Dataset(dict):
     """
     indent_chars = "   "
 
-    # TODO: this the correct list?
     _BINARY_VR_VALUES = ['OW', 'OB', 'OD', 'OF', 'OL', 'UN',
-                         'OW/OB', 'OW or OB', 'OB or OW', 'US or SS']
-
-    _VRs_TO_BE_FLOATS = ["DS", "FL", "FD", ]
-    _VRs_TO_BE_INTS = ["IS", "SL", "SS", "UL", "US", ]
+                         'OB or OW', 'US or OW', 'US or SS or OW']
+    _VRs_TO_BE_FLOATS = ['DS', 'FL', 'FD', ]
+    _VRs_TO_BE_INTS = ['IS', 'SL', 'SS', 'UL', 'US', 'US or SS']
 
     # Order of keys is significant!
     _JSON_VALUE_KEYS = ('Value', 'BulkDataURI', 'InlineBinary', )
@@ -1886,7 +1884,7 @@ class Dataset(dict):
         json_element = {'vr': data_element.VR, }
         if data_element.VR in Dataset._BINARY_VR_VALUES:
             if data_element.value is not None:
-                binary_value = data_element.value.encode('utf-8')
+                binary_value = data_element.value
                 encoded_value = base64.b64encode(binary_value).decode('utf-8')
                 if len(encoded_value) > bulk_data_threshold:
                     if bulk_data_element_handler is None:
@@ -1918,18 +1916,21 @@ class Dataset(dict):
             ]
             json_element['Value'] = value
         elif data_element.VR == 'PN':
-            if data_element.value is not None:
-                if len(data_element.value.components) > 2:
+            elem_value = data_element.value
+            if elem_value is not None:
+                if compat.in_py2:
+                    elem_value = PersonNameUnicode(elem_value, 'UTF8')
+                if len(elem_value.components) > 2:
                     json_element['Value'] = [
-                        {'Phonetic': data_element.value.components[2], },
+                        {'Phonetic': elem_value.components[2], },
                     ]
-                elif len(data_element.value.components) > 1:
+                elif len(elem_value.components) > 1:
                     json_element['Value'] = [
-                        {'Ideographic': data_element.value.components[1], },
+                        {'Ideographic': elem_value.components[1], },
                     ]
                 else:
                     json_element['Value'] = [
-                        {'Alphabetic': data_element.value.components[0], },
+                        {'Alphabetic': elem_value.components[0], },
                     ]
         else:
             if data_element.value is not None:


Test patch:
diff --git a/pydicom/tests/test_dataset.py b/pydicom/tests/test_dataset.py
index daad03c97..7fd3d09c4 100644
--- a/pydicom/tests/test_dataset.py
+++ b/pydicom/tests/test_dataset.py
@@ -1403,33 +1403,6 @@ class DatasetTests(unittest.TestCase):
         ds2.update(ds)
         assert ds2.PatientName == 'TestC'
 
-    def test_json_roundtrip(self):
-        ds = Dataset()
-        ds.add_new(0x00080005, 'CS', 'ISO_IR 100')
-        ds.add_new(0x00090010, 'LO', 'Creator 1.0')
-        ds.add_new(0x00091001, 'SH', 'Version1')
-        ds.add_new(0x00090011, 'LO', 'Creator 2.0')
-        ds.add_new(0x00091101, 'SH', 'Version2')
-        ds.add_new(0x00091102, 'US', 2)
-
-        jsonmodel = ds.to_json()
-        ds2 = Dataset.from_json(jsonmodel)
-
-        assert ds2.SpecificCharacterSet == ['ISO_IR 100']
-
-    def test_json_private_DS_VM(self):
-        test1_json = get_testdata_files("test1.json")[0]
-        if compat.in_py2:
-            jsonmodel = open(test1_json, 'r').read().encode("utf8")
-        else:
-            jsonmodel = open(test1_json, 'r').read()
-        ds = Dataset.from_json(jsonmodel)
-        import json
-        jsonmodel2 = ds.to_json(dump_handler=lambda d: json.dumps(d, indent=2))
-        ds2 = Dataset.from_json(jsonmodel2)
-
-        assert ds.PatientIdentityRemoved == 'YES'
-        assert ds2.PatientIdentityRemoved == 'YES'
 
 class DatasetElementsTests(unittest.TestCase):
     """Test valid assignments of data elements"""
diff --git a/pydicom/tests/test_json.py b/pydicom/tests/test_json.py
index ef27f5c3d..ae4e6c78f 100644
--- a/pydicom/tests/test_json.py
+++ b/pydicom/tests/test_json.py
@@ -1,3 +1,5 @@
+# -*- coding: utf-8 -*-
+# Copyright 2008-2019 pydicom authors. See LICENSE file for details.
 import pytest
 from pydicom.data import get_testdata_files
 from pydicom.dataelem import DataElement
@@ -6,38 +8,75 @@ from pydicom import compat
 
 from pydicom.valuerep import PersonNameUnicode, PersonName3
 
+
 def test_json_PN():
     s = open(get_testdata_files("test_PN.json")[0], "r").read()
     ds = Dataset.from_json(s)
-    assert isinstance(ds[0x00080090].value, (PersonNameUnicode, PersonName3))
-    assert isinstance(ds[0x00100010].value, (PersonNameUnicode, PersonName3))
+    assert isinstance(ds[0x00080090].value,
+                      (PersonNameUnicode, PersonName3))
+    assert isinstance(ds[0x00100010].value,
+                      (PersonNameUnicode, PersonName3))
     inner_seq = ds[0x04000561].value[0][0x04000550]
     dataelem = inner_seq[0][0x00100010]
     assert isinstance(dataelem.value, (PersonNameUnicode, PersonName3))
 
+
 @pytest.mark.skipif(compat.in_py2,
                     reason='JSON conversion not yet working in Python 2')
 def test_dataelem_from_json():
     tag = 0x0080090
     vr = "PN"
-    value = [{"Alphabetic":""}]
+    value = [{"Alphabetic": ""}]
     dataelem = DataElement.from_json(Dataset, tag, vr, value, "Value")
     assert isinstance(dataelem.value, (PersonNameUnicode, PersonName3))
 
+
 def test_json_roundtrip():
     ds = Dataset()
     ds.add_new(0x00080005, 'CS', 'ISO_IR 100')
     ds.add_new(0x00090010, 'LO', 'Creator 1.0')
     ds.add_new(0x00091001, 'SH', 'Version1')
+    ds.add_new(0x00091002, 'OB', b'BinaryContent')
+    ds.add_new(0x00091003, 'OW', b'\x0102\x3040\x5060')
+    ds.add_new(0x00091004, 'OF', b'\x00\x01\x02\x03\x04\x05\x06\x07')
+    ds.add_new(0x00091005, 'OD', b'\x00\x01\x02\x03\x04\x05\x06\x07'
+                                 b'\x01\x01\x02\x03\x04\x05\x06\x07')
+    ds.add_new(0x00091006, 'OL', b'\x00\x01\x02\x03\x04\x05\x06\x07'
+                                 b'\x01\x01\x02\x03')
+    ds.add_new(0x00091007, 'UI', '1.2.3.4.5.6')
+    ds.add_new(0x00091008, 'DA', '20200101')
+    ds.add_new(0x00091009, 'TM', '115500')
+    ds.add_new(0x0009100a, 'DT', '20200101115500.000000')
+    ds.add_new(0x0009100b, 'UL', 3000000000)
+    ds.add_new(0x0009100c, 'SL', -2000000000)
+    ds.add_new(0x0009100d, 'US', 40000)
+    ds.add_new(0x0009100e, 'SS', -22222)
+    ds.add_new(0x0009100f, 'FL', 3.14)
+    ds.add_new(0x00091010, 'FD', 3.14159265)
+    ds.add_new(0x00091011, 'CS', 'TEST MODE')
+    ds.add_new(0x00091012, 'PN', 'CITIZEN^1')
+    ds.add_new(0x00091013, 'PN', u'Yamada^Tarou=山田^太郎=やまだ^たろう')
+    ds.add_new(0x00091014, 'IS', '42')
+    ds.add_new(0x00091015, 'DS', '3.14159265')
+    ds.add_new(0x00091016, 'AE', b'CONQUESTSRV1')
+    ds.add_new(0x00091017, 'AS', '055Y')
+    ds.add_new(0x00091018, 'LT', 50 * u'Калинка,')
+    ds.add_new(0x00091019, 'UC', 'LONG CODE VALUE')
+    ds.add_new(0x0009101a, 'UN', b'\x0102\x3040\x5060')
+    ds.add_new(0x0009101b, 'UR', 'https://example.com')
+    ds.add_new(0x0009101c, 'AT', b'\x10\x00\x20\x00\x10\x00\x30\x00\x10')
+    ds.add_new(0x0009101d, 'ST', 100 * u'علي بابا')
+    ds.add_new(0x0009101e, 'SH', u'Διονυσιος')
     ds.add_new(0x00090011, 'LO', 'Creator 2.0')
     ds.add_new(0x00091101, 'SH', 'Version2')
     ds.add_new(0x00091102, 'US', 2)
 
-    jsonmodel = ds.to_json()
+    jsonmodel = ds.to_json(bulk_data_threshold=100)
     ds2 = Dataset.from_json(jsonmodel)
 
     assert ds2.SpecificCharacterSet == ['ISO_IR 100']
 
+
 def test_json_private_DS_VM():
     test1_json = get_testdata_files("test1.json")[0]
     jsonmodel = open(test1_json, 'r').read()