Repository: pydicom/pydicom

Issue:
Add Tag and VR to the bulk data handling in `from_json`
Currently, if you convert back to a Dataset format from a JSON format, you MUST re-hydrate all of the bulk data URI's or you will loose the information.

This causes a problem if you just wish to use the Dataset's header (maybe to extract some data, or rearrange some data), because now you have to pay the cost of getting all the pixel data and then handling the pixel data again upon conversion back to JSON

**Describe the solution you'd like**
Add the tag and the vr to the bulk data handler in `from_json` (this can be done in a backwards compatible way). This will allow the user to store the BulkDataURI's by tag in a map, return dummy data large enough to trigger the bulk handling when to_json is called next, and to use the map to convert back to the original URI's when bulk handling is triggered from to_json.

I'm going to drop a PR tomorrow that does this in a fully backward compatible, non-breaking fashion.


Discussion:
pep8speaks: Hello @joseph-wortmann! Thanks for opening this PR. We checked the lines you've touched for [PEP 8](https://www.python.org/dev/peps/pep-0008) issues, and found:





* In the file [`pydicom/jsonrep.py`](https://github.com/pydicom/pydicom/blob/b657e5a28600ba620d299a3e748e78ae42114d14/pydicom/jsonrep.py):

> [Line 61:14](https://github.com/pydicom/pydicom/blob/b657e5a28600ba620d299a3e748e78ae42114d14/pydicom/jsonrep.py#L61): [W291](https://duckduckgo.com/?q=pep8%20W291) trailing whitespace
> [Line 62:23](https://github.com/pydicom/pydicom/blob/b657e5a28600ba620d299a3e748e78ae42114d14/pydicom/jsonrep.py#L62): [W291](https://duckduckgo.com/?q=pep8%20W291) trailing whitespace
> [Line 63:13](https://github.com/pydicom/pydicom/blob/b657e5a28600ba620d299a3e748e78ae42114d14/pydicom/jsonrep.py#L63): [W291](https://duckduckgo.com/?q=pep8%20W291) trailing whitespace
> [Line 64:12](https://github.com/pydicom/pydicom/blob/b657e5a28600ba620d299a3e748e78ae42114d14/pydicom/jsonrep.py#L64): [W291](https://duckduckgo.com/?q=pep8%20W291) trailing whitespace
> [Line 65:15](https://github.com/pydicom/pydicom/blob/b657e5a28600ba620d299a3e748e78ae42114d14/pydicom/jsonrep.py#L65): [W291](https://duckduckgo.com/?q=pep8%20W291) trailing whitespace
> [Line 69:55](https://github.com/pydicom/pydicom/blob/b657e5a28600ba620d299a3e748e78ae42114d14/pydicom/jsonrep.py#L69): [W291](https://duckduckgo.com/?q=pep8%20W291) trailing whitespace




codecov[bot]: # [Codecov](https://codecov.io/gh/pydicom/pydicom/pull/1228?src=pr&el=h1) Report
> :exclamation: No coverage uploaded for pull request base (`master@8112bb6`). [Click here to learn what that means](https://docs.codecov.io/docs/error-reference#section-missing-base-commit).
> The diff coverage is `100.00%`.

[![Impacted file tree graph](https://codecov.io/gh/pydicom/pydicom/pull/1228/graphs/tree.svg?width=650&height=150&src=pr&token=YciLV15Lwr)](https://codecov.io/gh/pydicom/pydicom/pull/1228?src=pr&el=tree)

```diff
@@            Coverage Diff            @@
##             master    #1228   +/-   ##
=========================================
  Coverage          ?   95.13%           
=========================================
  Files             ?       57           
  Lines             ?     9203           
  Branches          ?        0           
=========================================
  Hits              ?     8755           
  Misses            ?      448           
  Partials          ?        0           
```


| [Impacted Files](https://codecov.io/gh/pydicom/pydicom/pull/1228?src=pr&el=tree) | Coverage Δ | |
|---|---|---|
| [pydicom/dataset.py](https://codecov.io/gh/pydicom/pydicom/pull/1228/diff?src=pr&el=tree#diff-cHlkaWNvbS9kYXRhc2V0LnB5) | `97.50% <ø> (ø)` | |
| [pydicom/dataelem.py](https://codecov.io/gh/pydicom/pydicom/pull/1228/diff?src=pr&el=tree#diff-cHlkaWNvbS9kYXRhZWxlbS5weQ==) | `97.65% <100.00%> (ø)` | |
| [pydicom/jsonrep.py](https://codecov.io/gh/pydicom/pydicom/pull/1228/diff?src=pr&el=tree#diff-cHlkaWNvbS9qc29ucmVwLnB5) | `93.69% <100.00%> (ø)` | |

------

[Continue to review full report at Codecov](https://codecov.io/gh/pydicom/pydicom/pull/1228?src=pr&el=continue).
> **Legend** - [Click here to learn more](https://docs.codecov.io/docs/codecov-delta)
> `Δ = absolute <relative> (impact)`, `ø = not affected`, `? = missing data`
> Powered by [Codecov](https://codecov.io/gh/pydicom/pydicom/pull/1228?src=pr&el=footer). Last update [8112bb6...b657e5a](https://codecov.io/gh/pydicom/pydicom/pull/1228?src=pr&el=lastupdated). Read the [comment docs](https://docs.codecov.io/docs/pull-request-comments).

joseph-wortmann: @mrbean-bremen added

Patch:
diff --git a/.gitignore b/.gitignore
index 113fdf395..b49f46acd 100644
--- a/.gitignore
+++ b/.gitignore
@@ -51,3 +51,6 @@ pydicom/.mypy_cache/*
 
 # vscode
 .vscode/*
+
+# virtualenv
+env/
diff --git a/doc/release_notes/v2.1.0.rst b/doc/release_notes/v2.1.0.rst
index 3b90717ab..1d8fe9a34 100644
--- a/doc/release_notes/v2.1.0.rst
+++ b/doc/release_notes/v2.1.0.rst
@@ -56,6 +56,10 @@ Enhancements
   :class:`~pydicom.fileset.FileSet` class (:issue:`9`, :issue:`243`,
   :issue:`1093`)
 * Added :attr:`~pydicom.uid.AllTransferSyntaxes`
+* Added another signature to the `bulk_data_uri_handler` in `from_json` to
+  allow for the communication of not just the URI but also the tag and VR
+  to the handler. Previous handlers will work as expected, new signature
+  handlers will get the additional information.
 
 Changes
 .......
diff --git a/doc/tutorials/dicom_json.rst b/doc/tutorials/dicom_json.rst
index 051c532ce..798668895 100644
--- a/doc/tutorials/dicom_json.rst
+++ b/doc/tutorials/dicom_json.rst
@@ -94,3 +94,14 @@ the stored ``BulkDataURI``:
   >>>     "00091002": {"vr": "OB", "BulkDataURI": "https://my.wado.org/123"}
   >>> }
   >>> ds = Dataset.from_json(json_data, bulk_data_uri_handler=bulk_data_reader)
+
+or, if you need to also know the tag and the vr, in addition to the stored
+``BulkDataURI``:
+
+  >>> def bulk_data_reader(tag, vr, bulk_data_uri):
+  >>>     return data_retrieved_for_tag_and_vr_via_uri(tag, vr, bulk_data_uri)
+  >>>
+  >>> json_data = {
+  >>>     "00091002": {"vr": "OB", "BulkDataURI": "https://my.wado.org/123"}
+  >>> }
+  >>> ds = Dataset.from_json(json_data, bulk_data_uri_handler=bulk_data_reader)
diff --git a/pydicom/dataelem.py b/pydicom/dataelem.py
index 588d31520..6673d6a1e 100644
--- a/pydicom/dataelem.py
+++ b/pydicom/dataelem.py
@@ -231,7 +231,12 @@ class DataElement:
         vr: str,
         value: object,
         value_key: Union[str, None],
-        bulk_data_uri_handler: Optional[Callable[[str], object]] = None
+        bulk_data_uri_handler: Optional[
+            Union[
+                Callable[[BaseTag, str, str], object],
+                Callable[[str], object]
+            ]
+        ] = None
     ) -> _DataElement:
         """Return a :class:`DataElement` from JSON.
 
@@ -251,7 +256,8 @@ class DataElement:
             Key of the data element that contains the value
             (options: ``{"Value", "InlineBinary", "BulkDataURI"}``)
         bulk_data_uri_handler: callable or None
-            Callable function that accepts the "BulkDataURI" of the JSON
+            Callable function that accepts either the tag, vr and "BulkDataURI"
+            or just the "BulkDataURI" of the JSON
             representation of a data element and returns the actual value of
             that data element (retrieved via DICOMweb WADO-RS)
 
@@ -260,8 +266,14 @@ class DataElement:
         DataElement
         """
         # TODO: test wado-rs retrieve wrapper
-        converter = JsonDataElementConverter(dataset_class, tag, vr, value,
-                                             value_key, bulk_data_uri_handler)
+        converter = JsonDataElementConverter(
+            dataset_class,
+            tag,
+            vr,
+            value,
+            value_key,
+            bulk_data_uri_handler
+        )
         elem_value = converter.get_element_values()
         try:
             return cls(tag=tag, value=elem_value, VR=vr)
diff --git a/pydicom/dataset.py b/pydicom/dataset.py
index b01029bd0..8dfcd8261 100644
--- a/pydicom/dataset.py
+++ b/pydicom/dataset.py
@@ -2218,7 +2218,12 @@ class Dataset(Dict[BaseTag, _DatasetValue]):
     def from_json(
         cls: Type[_Dataset],
         json_dataset: Union[Dict[str, bytes], str],
-        bulk_data_uri_handler: Optional[Callable[[bytes], object]] = None
+        bulk_data_uri_handler: Optional[
+            Union[
+                Callable[[BaseTag, str, str], object],
+                Callable[[str], object]
+            ]
+        ] = None
     ) -> _Dataset:
         """Add elements to the :class:`Dataset` from DICOM JSON format.
 
@@ -2232,7 +2237,8 @@ class Dataset(Dict[BaseTag, _DatasetValue]):
             :class:`dict` or :class:`str` representing a DICOM Data Set
             formatted based on the DICOM JSON Model.
         bulk_data_uri_handler : callable, optional
-            Callable function that accepts the "BulkDataURI" of the JSON
+            Callable function that accepts either the tag, vr and "BulkDataURI"
+            or just the "BulkDataURI" of the JSON
             representation of a data element and returns the actual value of
             data element (retrieved via DICOMweb WADO-RS).
 
@@ -2263,7 +2269,7 @@ class Dataset(Dict[BaseTag, _DatasetValue]):
     def to_json_dict(
         self,
         bulk_data_threshold: int = 1024,
-        bulk_data_element_handler: Optional[Callable[[DataElement], bytes]] = None  # noqa
+        bulk_data_element_handler: Optional[Callable[[DataElement], str]] = None  # noqa
     ) -> _Dataset:
         """Return a dictionary representation of the :class:`Dataset`
         conforming to the DICOM JSON Model as described in the DICOM
@@ -2301,7 +2307,7 @@ class Dataset(Dict[BaseTag, _DatasetValue]):
     def to_json(
         self,
         bulk_data_threshold: int = 1024,
-        bulk_data_element_handler: Optional[Callable[[DataElement], bytes]] = None,  # noqa
+        bulk_data_element_handler: Optional[Callable[[DataElement], str]] = None,  # noqa
         dump_handler: Optional[Callable[["Dataset"], str]] = None
     ) -> str:
         """Return a JSON representation of the :class:`Dataset`.
diff --git a/pydicom/jsonrep.py b/pydicom/jsonrep.py
index 44b08a70e..c6ef519c3 100644
--- a/pydicom/jsonrep.py
+++ b/pydicom/jsonrep.py
@@ -2,9 +2,12 @@
 """Methods for converting Datasets and DataElements to/from json"""
 
 import base64
+from inspect import signature
+import inspect
+from typing import Callable, Optional, Union
 import warnings
 
-from pydicom.valuerep import PersonName
+from pydicom.tag import BaseTag
 
 # Order of keys is significant!
 JSON_VALUE_KEYS = ('Value', 'BulkDataURI', 'InlineBinary',)
@@ -54,8 +57,20 @@ class JsonDataElementConverter:
     .. versionadded:: 1.4
     """
 
-    def __init__(self, dataset_class, tag, vr, value, value_key,
-                 bulk_data_uri_handler):
+    def __init__(
+        self,
+        dataset_class,
+        tag,
+        vr,
+        value,
+        value_key,
+        bulk_data_uri_handler: Optional[
+            Union[
+                Callable[[BaseTag, str, str], object],
+                Callable[[str], object]
+            ]
+        ] = None
+    ):
         """Create a new converter instance.
 
         Parameters
@@ -63,7 +78,7 @@ class JsonDataElementConverter:
         dataset_class : dataset.Dataset derived class
             Class used to create sequence items.
         tag : BaseTag
-            The data element tag.
+            The data element tag or int.
         vr : str
             The data element value representation.
         value : list
@@ -72,7 +87,8 @@ class JsonDataElementConverter:
             Key of the data element that contains the value
             (options: ``{"Value", "InlineBinary", "BulkDataURI"}``)
         bulk_data_uri_handler: callable or None
-            Callable function that accepts the "BulkDataURI" of the JSON
+            Callable function that accepts either the tag, vr and "BulkDataURI"
+            or just the "BulkDataURI" of the JSON
             representation of a data element and returns the actual value of
             that data element (retrieved via DICOMweb WADO-RS)
         """
@@ -81,7 +97,15 @@ class JsonDataElementConverter:
         self.vr = vr
         self.value = value
         self.value_key = value_key
-        self.bulk_data_uri_handler = bulk_data_uri_handler
+        if (
+            bulk_data_uri_handler and
+            len(signature(bulk_data_uri_handler).parameters) == 1
+        ):
+            def wrapped_bulk_data_handler(tag, vr, value):
+                return bulk_data_uri_handler(value)
+            self.bulk_data_element_handler = wrapped_bulk_data_handler
+        else:
+            self.bulk_data_element_handler = bulk_data_uri_handler
 
     def get_element_values(self):
         """Return a the data element value or list of values.
@@ -124,13 +148,13 @@ class JsonDataElementConverter:
             if not isinstance(value, str):
                 fmt = '"{}" of data element "{}" must be a string.'
                 raise TypeError(fmt.format(self.value_key, self.tag))
-            if self.bulk_data_uri_handler is None:
+            if self.bulk_data_element_handler is None:
                 warnings.warn(
                     'no bulk data URI handler provided for retrieval '
                     'of value of data element "{}"'.format(self.tag)
                 )
                 return empty_value_for_VR(self.vr, raw=True)
-            return self.bulk_data_uri_handler(value)
+            return self.bulk_data_element_handler(self.tag, self.vr, value)
         return empty_value_for_VR(self.vr)
 
     def get_regular_element_value(self, value):


Test patch:
diff --git a/pydicom/tests/test_json.py b/pydicom/tests/test_json.py
index 83a6d7bd0..50ce34cc2 100644
--- a/pydicom/tests/test_json.py
+++ b/pydicom/tests/test_json.py
@@ -334,7 +334,18 @@ class TestBinary:
             Dataset.from_json(ds_json)
 
     def test_bulk_data_reader_is_called(self):
-        def bulk_data_reader(_):
+        def bulk_data_reader(value):
+            return b'xyzzy'
+
+        json_data = {
+            "00091002": {"vr": "OB", "BulkDataURI": "https://a.dummy.url"}
+        }
+        ds = Dataset().from_json(json.dumps(json_data), bulk_data_reader)
+
+        assert b'xyzzy' == ds[0x00091002].value
+
+    def test_bulk_data_reader_is_called_2(self):
+        def bulk_data_reader(tag, vr, value):
             return b'xyzzy'
 
         json_data = {