Merge pull request #16 from NHSDigital/fix/make-put-request-to-manage

Amend DicomUploader to PUT to Manage

Author: steventux

.env.development

@@ -4,6 +4,8 @@ AZURE_RELAY_HYBRID_CONNECTION=name-of-your-choice-relay-test-hc
 AZURE_RELAY_KEY_NAME=RootManageSharedAccessKey
 AZURE_RELAY_SHARED_ACCESS_KEY=YOUR_SHARED_ACCESS_KEY_HERE
 
+CLOUD_API_ENDPOINT=https://localhost:8000/api/v1/dicom
+CLOUD_API_TOKEN=testtoken
 # MWL Server Configuration
 MWL_AET=SCREENING_MWL
 MWL_PORT=4243

compose.yml

@@ -11,6 +11,8 @@ services:
       - pacs-storage:/var/lib/pacs/storage
       - pacs-db:/var/lib/pacs
     environment:
+      - CLOUD_API_ENDPOINT=${CLOUD_API_ENDPOINT}
+      - CLOUD_API_TOKEN=${CLOUD_API_TOKEN}
       - PACS_AET=SCREENING_PACS
       - PACS_PORT=4244
       - PACS_STORAGE_PATH=/var/lib/pacs/storage

src/services/dicom/dicom_uploader.py

@@ -16,39 +16,36 @@
 
 class DICOMUploader:
     def __init__(self, api_endpoint: str | None = None, timeout: int = 30, verify_ssl: bool = True):
-        self.api_endpoint = api_endpoint or os.getenv("CLOUD_API_ENDPOINT", "http://localhost:8000/api/dicom/upload/")
+        self.api_endpoint = api_endpoint or os.getenv("CLOUD_API_ENDPOINT", "http://localhost:8000/api/v1/dicom")
         self.timeout = timeout
         self.verify_ssl = verify_ssl
 
-    def upload_dicom(self, sop_instance_uid: str, dicom_bytes: bytes, action_id: Optional[str]) -> bool:
-        if not action_id:
-            logger.warning(f"No action_id for {sop_instance_uid}, upload will be rejected by server")
-
-        headers = {
-            "X-Source-Message-ID": action_id or "",
+    def headers(self) -> dict:
+        return {
+            "Authorization": f"Bearer {os.getenv('CLOUD_API_TOKEN', '')}",
         }
 
-        # Wrap bytes in BytesIO stream - Django expects a file-like object
-        file_stream = io.BytesIO(dicom_bytes)
+    def upload_dicom(self, sop_instance_uid: str, dicom_stream: io.BufferedReader, action_id: Optional[str]) -> bool:
+        if not action_id:
+            logger.error(f"No action_id for {sop_instance_uid}, upload will be rejected by server")
+            return False
+
         files = {
-            "file": (f"{sop_instance_uid}.dcm", file_stream),
+            "file": (f"{sop_instance_uid}.dcm", dicom_stream),
         }
 
         try:
-            logger.info(
-                f"Uploading {sop_instance_uid} to {self.api_endpoint} "
-                f"(size: {len(dicom_bytes)} bytes, action_id: {action_id})"
-            )
+            logger.info(f"Uploading {sop_instance_uid} to {self.api_endpoint}/{action_id}")
 
-            response = requests.post(
-                self.api_endpoint,
+            response = requests.put(
+                f"{self.api_endpoint}/{action_id}",
                 files=files,
-                headers=headers,
                 timeout=self.timeout,
                 verify=self.verify_ssl,
+                headers=self.headers(),
             )
 
-            if response.status_code in (200, 201, 204):
+            if response.status_code == 201:
                 logger.info(f"Successfully uploaded {sop_instance_uid} (status: {response.status_code})")
                 return True
             else:

src/services/dicom/upload_processor.py

@@ -97,12 +97,9 @@ def upload_instance(self, instance: dict) -> bool:
                 self._mark_failed(sop_instance_uid, error, attempt_count + 1)
                 return False
 
-            dicom_bytes = dicom_path.read_bytes()
-            logger.debug(f"Read {len(dicom_bytes)} bytes from {dicom_path}")
-
             action_id = self.mwl_storage.get_source_message_id(accession_number) if accession_number else None
 
-            if self.uploader.upload_dicom(sop_instance_uid, dicom_bytes, action_id):
+            if self.uploader.upload_dicom(sop_instance_uid, open(dicom_path, "rb"), action_id):
                 self.pacs_storage.mark_upload_complete(sop_instance_uid)
                 logger.info(f"Successfully uploaded {sop_instance_uid}")
                 return True

tests/services/dicom/test_dicom_uploader.py

@@ -1,80 +1,87 @@
 import io
+import tempfile
 from unittest.mock import Mock, patch
 
+import pydicom
+import pytest
 import requests
 
 from services.dicom.dicom_uploader import DICOMUploader
 
 
 class TestDICOMUploader:
-    @patch("services.dicom.dicom_uploader.requests.post")
-    def test_upload_success(self, mock_post):
+    @pytest.fixture
+    def dicom_file(self):
+        tf = tempfile.NamedTemporaryFile(delete=False)
+        tf.write(b"fake dicom data")
+        tf.close()
+        yield tf.name
+
+    @patch("services.dicom.dicom_uploader.requests.put")
+    def test_upload_success(self, mock_put, dicom_file):
         mock_response = Mock()
         mock_response.status_code = 201
-        mock_post.return_value = mock_response
+        mock_put.return_value = mock_response
+
+        sop_instance_uid = pydicom.uid.generate_uid()
 
         uploader = DICOMUploader(api_endpoint="http://test.com/api/upload")
 
-        dicom_bytes = b"fake dicom data"
         result = uploader.upload_dicom(
-            sop_instance_uid="1.2.3.4.5",  # gitleaks:allow
-            dicom_bytes=dicom_bytes,
+            sop_instance_uid=sop_instance_uid,
+            dicom_stream=open(dicom_file, "rb"),
             action_id="ACTION123",
         )
 
         assert result is True
-        mock_post.assert_called_once()
+        mock_put.assert_called_once_with(
+            "http://test.com/api/upload/ACTION123",
+            files=mock_put.call_args[1]["files"],
+            timeout=30,
+            verify=True,
+            headers=uploader.headers(),
+        )
 
-        # Verify multipart form upload with BytesIO stream
-        call_kwargs = mock_post.call_args[1]
-        assert call_kwargs["headers"]["X-Source-Message-ID"] == "ACTION123"
+        call_kwargs = mock_put.call_args[1]
         assert "files" in call_kwargs
         file_tuple = call_kwargs["files"]["file"]
-        assert file_tuple[0] == "1.2.3.4.5.dcm"  # gitleaks:allow
-        assert isinstance(file_tuple[1], io.BytesIO)  # stream
-        assert file_tuple[1].getvalue() == dicom_bytes  # content
-
-    @patch("services.dicom.dicom_uploader.requests.post")
-    def test_upload_without_action_id(self, mock_post):
-        """Upload without action_id sends empty header (server will reject)."""
-        mock_response = Mock()
-        mock_response.status_code = 400
-        mock_response.text = "Missing X-Source-Message-ID header"
-        mock_post.return_value = mock_response
+        assert file_tuple[0] == f"{sop_instance_uid}.dcm"
+        assert isinstance(file_tuple[1], io.BufferedReader)
+        assert file_tuple[1].read() == open(dicom_file, "rb").read()
 
+    def test_upload_without_action_id(self, dicom_file):
+        """Upload without action_id does not make request."""
         uploader = DICOMUploader()
-        result = uploader.upload_dicom(sop_instance_uid="1.2.3", dicom_bytes=b"data", action_id=None)
+        result = uploader.upload_dicom(sop_instance_uid="1.2.3", dicom_stream=open(dicom_file, "rb"), action_id=None)
 
         assert result is False
-        call_kwargs = mock_post.call_args[1]
-        assert call_kwargs["headers"]["X-Source-Message-ID"] == ""
 
-    @patch("services.dicom.dicom_uploader.requests.post")
-    def test_upload_failure_status_code(self, mock_post):
+    @patch("services.dicom.dicom_uploader.requests.put")
+    def test_upload_failure_status_code(self, mock_put, dicom_file):
         mock_response = Mock()
         mock_response.status_code = 500
         mock_response.text = "Internal server error"
-        mock_post.return_value = mock_response
+        mock_put.return_value = mock_response
 
         uploader = DICOMUploader()
-        result = uploader.upload_dicom("1.2.3", b"data", None)
+        result = uploader.upload_dicom("1.2.3", open(dicom_file, "rb"), None)
 
         assert result is False
 
-    @patch("services.dicom.dicom_uploader.requests.post")
-    def test_upload_timeout(self, mock_post):
-        mock_post.side_effect = requests.exceptions.Timeout()
+    @patch("services.dicom.dicom_uploader.requests.put")
+    def test_upload_timeout(self, mock_put, dicom_file):
+        mock_put.side_effect = requests.exceptions.Timeout()
 
         uploader = DICOMUploader(timeout=5)
-        result = uploader.upload_dicom("1.2.3", b"data", None)
+        result = uploader.upload_dicom("1.2.3", open(dicom_file, "rb"), None)
 
         assert result is False
 
-    @patch("services.dicom.dicom_uploader.requests.post")
-    def test_upload_network_error(self, mock_post):
-        mock_post.side_effect = requests.exceptions.ConnectionError()
+    @patch("services.dicom.dicom_uploader.requests.put")
+    def test_upload_network_error(self, mock_put, dicom_file):
+        mock_put.side_effect = requests.exceptions.ConnectionError()
 
         uploader = DICOMUploader()
-        result = uploader.upload_dicom("1.2.3", b"data", None)
+        result = uploader.upload_dicom("1.2.3", open(dicom_file, "rb"), None)
 
         assert result is False

tests/services/dicom/test_upload_processor.py

@@ -1,5 +1,5 @@
 from pathlib import Path
-from unittest.mock import Mock, patch
+from unittest.mock import Mock, mock_open, patch
 
 import pytest
 
@@ -62,8 +62,9 @@ def test_process_batch_processes_all_instances(self, processor, mock_pacs_storag
             },
         ]
         mock_uploader.upload_dicom.return_value = True
+        mo = mock_open(read_data=b"dicom data")
 
-        with patch.object(Path, "exists", return_value=True), patch.object(Path, "read_bytes", return_value=b"dicom"):
+        with patch.object(Path, "exists", return_value=True), patch("builtins.open", mo):
             result = processor.process_batch(limit=10)
 
         assert result == 2
@@ -79,16 +80,14 @@ def test_upload_instance_success(self, processor, mock_pacs_storage, mock_mwl_st
         mock_mwl_storage.get_source_message_id.return_value = "ACTION123"
         mock_uploader.upload_dicom.return_value = True
 
-        with (
-            patch.object(Path, "exists", return_value=True),
-            patch.object(Path, "read_bytes", return_value=b"dicom data"),
-        ):
+        mo = mock_open(read_data=b"dicom data")
+        with patch.object(Path, "exists", return_value=True), patch("builtins.open", mo):
             result = processor.upload_instance(instance)
 
         assert result is True
         mock_pacs_storage.mark_upload_started.assert_called_once_with("1.2.3.4")  # gitleaks:allow
         mock_pacs_storage.mark_upload_complete.assert_called_once_with("1.2.3.4")  # gitleaks:allow
-        mock_uploader.upload_dicom.assert_called_once_with("1.2.3.4", b"dicom data", "ACTION123")  # gitleaks:allow
+        mock_uploader.upload_dicom.assert_called_once_with("1.2.3.4", mo(), "ACTION123")  # gitleaks:allow
 
     def test_upload_instance_file_not_found(self, processor, mock_pacs_storage):
         instance = {