fixup DicomAnonymiser CTP implementation

.editorconfig

@@ -33,3 +33,4 @@ dotnet_diagnostic.SYSLIB1045.severity = none
 dotnet_diagnostic.IDE0005.severity = error
 
 csharp_style_namespace_declarations = file_scoped:error
+dotnet_style_namespace_match_folder = true:error

.github/workflows/main.yml

@@ -95,6 +95,17 @@ jobs:
       - name: "[linux] install RDMP databases"
         if: ${{ matrix.os == 'linux' }}
         run: ${{ env.rdmp-cli-dir }}/rdmp install --createdatabasetimeout 180 ${{ env.rdmp_conn_str }} TEST_
+      - name: Setup Java JDK
+        uses: actions/[email protected]
+        with:
+          java-version: ${{ env.java-version }}
+          distribution: ${{ env.java-distribution }}
+      - name: "Download ctp-anon-cli jar"
+        run: |
+          set -euxo pipefail
+          ctp_jar_ver=$(grep -E 'TEST_CTP_JAR_VERSION = "(.*)";' tests/SmiServices.IntegrationTests/Microservices/DicomAnonymiser/FixtureSetup.cs | cut -d'"' -f2)
+          ./bin/smi/downloadCtpAnonJar.py "${ctp_jar_ver}"
+          java -jar "./data/ctp/ctp-anon-cli-${ctp_jar_ver}.jar" --version
       - name: show dotnet info
         run: dotnet --info
       - name: build, test, and package dotnet

Directory.Build.props

@@ -5,6 +5,7 @@
     <DebugSymbols>true</DebugSymbols>
     <DebugType>full</DebugType>
     <DisableImplicitNuGetFallbackFolder>true</DisableImplicitNuGetFallbackFolder>
+    <EnforceCodeStyleInBuild>true</EnforceCodeStyleInBuild>
     <GenerateAssemblyInfo>false</GenerateAssemblyInfo>
     <IsPackable>false</IsPackable>
     <IsPublishable>false</IsPublishable>

bin/smi/downloadCtpAnonJar.py

@@ -0,0 +1,37 @@
+#!/usr/bin/env python3
+import argparse
+import os
+import sys
+import urllib.request
+from collections.abc import Sequence
+
+sys.path.append(os.path.join(os.path.dirname(__file__), ".."))
+import common  # noqa: E402
+
+_CTP_JAR_DIR = f"{common.PROJ_ROOT}/data/ctp"
+
+
+def main(argv: Sequence[str] | None = None) -> int:
+
+    parser = argparse.ArgumentParser()
+    parser.add_argument(
+        "version",
+    )
+    args = parser.parse_args(argv)
+
+    url = (
+        "https://github.com/SMI/ctp-anon-cli/releases/download/"
+        f"v{args.version}/ctp-anon-cli-{args.version}.jar"
+    )
+    file = os.path.join(_CTP_JAR_DIR, f"ctp-anon-cli-{args.version}.jar")
+    if os.path.isfile(file):
+        return 0
+
+    print(f"Downloading {url} to {file}")
+    urllib.request.urlretrieve(url, file)
+
+    return 0
+
+
+if __name__ == "__main__":
+    raise SystemExit(main())

data/ctp/.gitignore

@@ -0,0 +1 @@
+*.jar

docs/services/ctp-anonymiser.md

@@ -37,4 +37,4 @@ removed all PII from the file, it will not proceed if it cannot insert redacted
 
 ## CLI Options
 
-The anonymisation script can be specified using the `-a` option. An example script can be viewed [here](/data/ctp/ctp-whitelist.script).
+The anonymisation script can be specified using the `-a` option. An example script can be viewed [here](/data/ctp/ctp-allowlist.script).

news/2053-feature.md

@@ -0,0 +1 @@
+Finish dicom-anonymiser CTP implementation and expand tests

src/SmiServices/Microservices/DicomAnonymiser/Anonymisers/AnonymiserFactory.cs

@@ -14,8 +14,8 @@ public static IDicomAnonymiser CreateAnonymiser(GlobalOptions options)
         return anonymiserType switch
         {
             AnonymiserType.DefaultAnonymiser => new DefaultAnonymiser(options),
-            // TODO(rkm 2021-12-07) Can remove the LGTM ignore once an AnonymiserType is implemented
-            _ => throw new NotImplementedException($"No case for AnonymiserType '{anonymiserType}'"), // lgtm[cs/constant-condition]
+            AnonymiserType.SmiCtpAnonymiser => new SmiCtpAnonymiser(options),
+            _ => throw new NotImplementedException($"No case for AnonymiserType '{anonymiserType}'"),
         };
     }
 }

src/SmiServices/Microservices/DicomAnonymiser/Anonymisers/AnonymiserType.cs

@@ -6,5 +6,8 @@ public enum AnonymiserType
     /// Unused placeholder value
     /// </summary>
     None = 0,
+
     DefaultAnonymiser = 1,
+
+    SmiCtpAnonymiser = 2,
 }

src/SmiServices/Microservices/DicomAnonymiser/Anonymisers/DefaultAnonymiser.cs

@@ -2,137 +2,43 @@
 using SmiServices.Common.Messages.Extraction;
 using SmiServices.Common.Options;
 using System;
-using System.Collections.Generic;
-using System.Diagnostics;
 using System.IO.Abstractions;
 
 namespace SmiServices.Microservices.DicomAnonymiser.Anonymisers;
 
-public class DefaultAnonymiser : IDicomAnonymiser
+public class DefaultAnonymiser : IDicomAnonymiser, IDisposable
 {
     private readonly ILogger _logger = LogManager.GetCurrentClassLogger();
-    private readonly DicomAnonymiserOptions _options;
-    private const string _bash = "/bin/bash";
+    private readonly SmiCtpAnonymiser _ctpAnonymiser;
 
     public DefaultAnonymiser(GlobalOptions globalOptions)
     {
-        if (globalOptions.DicomAnonymiserOptions == null)
-            throw new ArgumentNullException(nameof(globalOptions));
+        var dicomAnonymiserOptions = globalOptions.DicomAnonymiserOptions!;
 
-        if (globalOptions.LoggingOptions == null)
-            throw new ArgumentNullException(nameof(globalOptions));
-
-        _options = globalOptions.DicomAnonymiserOptions;
-    }
-
-    /// <summary>
-    /// Creates a process with the given parameters
-    /// </summary>
-    private static Process CreateProcess(string fileName, string arguments, string? workingDirectory = null, Dictionary<string, string>? environmentVariables = null)
-    {
-        var process = new Process
-        {
-            StartInfo = new ProcessStartInfo
-            {
-                FileName = fileName,
-                Arguments = arguments,
-                UseShellExecute = false,
-                RedirectStandardOutput = true,
-                RedirectStandardError = true,
-                WorkingDirectory = workingDirectory ?? string.Empty
-            }
-        };
-
-        if (environmentVariables != null)
-        {
-            foreach (var variable in environmentVariables)
-            {
-                process.StartInfo.EnvironmentVariables[variable.Key] = variable.Value;
-            }
-        }
-
-        return process;
-    }
-
-    private Process CreateCTPAnonProcess(IFileInfo sourceFile, IFileInfo destFile)
-    {
-        string arguments = $"-jar {_options.CtpAnonCliJar} -a {_options.CtpAllowlistScript} -s false {sourceFile} {destFile}";
-
-        return CreateProcess("java", arguments);
-    }
-
-    private Process CreatePixelAnonProcess(IFileInfo sourceFile, IFileInfo destFile)
-    {
-        string activateCommand = $"source {_options.VirtualEnvPath}/bin/activate";
-        string arguments = $"-c \"{activateCommand} && {_options.DicomPixelAnonPath}/dicom_pixel_anon.sh -o {destFile} {sourceFile}\"";
-
-        return CreateProcess(_bash, arguments, _options.DicomPixelAnonPath);
-    }
-
-    // TODO (da 2024-02-23) Use StructuredReports repository to access SRAnonTool
-    private Process CreateSRAnonProcess(IFileInfo sourceFile, IFileInfo destFile)
-    {
-        string arguments = $"{_options.SRAnonymiserToolPath} -i {sourceFile} -o {destFile} -s /Users/daniyalarshad/EPCC/github/NationalSafeHaven/opt/semehr";
-        _ = new Dictionary<string, string> { { "SMI_ROOT", $"{_options.SmiServicesPath}" } };
-
-        return CreateProcess(_bash, arguments);
+        _ctpAnonymiser = new SmiCtpAnonymiser(globalOptions);
     }
 
     /// <summary>
     /// Anonymises a DICOM file based on image modality
     /// </summary>
-    public ExtractedFileStatus Anonymise(ExtractFileMessage message, IFileInfo sourceFile, IFileInfo destFile, out string anonymiserStatusMessage)
+    public ExtractedFileStatus Anonymise(IFileInfo sourceFile, IFileInfo destFile, string modality, out string? anonymiserStatusMessage)
     {
-        _logger.Info($"Anonymising {sourceFile} to {destFile}");
-
-        if (!RunProcessAndCheckSuccess(CreateCTPAnonProcess(sourceFile, destFile), "CTP Anonymiser"))
+        var status = _ctpAnonymiser.Anonymise(sourceFile, destFile, modality, out string? ctpStatusMessage);
+        if (status != ExtractedFileStatus.Anonymised)
         {
-            anonymiserStatusMessage = "Error running CTP anonymiser";
-            return ExtractedFileStatus.ErrorWontRetry;
+            anonymiserStatusMessage = ctpStatusMessage;
+            return status;
         }
 
-        if (message.Modality == "SR")
-        {
-            if (!RunProcessAndCheckSuccess(CreateSRAnonProcess(sourceFile, destFile), "SR Anonymiser"))
-            {
-                anonymiserStatusMessage = "Error running SR anonymiser";
-                return ExtractedFileStatus.ErrorWontRetry;
-            }
-        }
-        else
-        {
-            if (!RunProcessAndCheckSuccess(CreatePixelAnonProcess(sourceFile, destFile), "Pixel Anonymiser"))
-            {
-                anonymiserStatusMessage = "Error running PIXEL anonymiser";
-                return ExtractedFileStatus.ErrorWontRetry;
-            }
-        }
+        // TODO(rkm 2024-12-17) Implement SR anon here (instead of via CTP), and add pixel anonymiser
 
-        anonymiserStatusMessage = "Anonymisation successful";
+        anonymiserStatusMessage = null;
         return ExtractedFileStatus.Anonymised;
     }
 
-    /// <summary>
-    /// Runs a process and logs the result
-    /// </summary>
-    private bool RunProcessAndCheckSuccess(Process process, string processName)
+    public void Dispose()
     {
-        process.Start();
-        process.WaitForExit();
-
-        var returnCode = process.ExitCode.ToString();
-        LogProcessResult(processName, returnCode, process);
-
-        return returnCode == "0";
+        GC.SuppressFinalize(this);
+        _ctpAnonymiser.Dispose();
     }
-
-    /// <summary>
-    /// Logs the result of a process
-    /// </summary>
-    private void LogProcessResult(string processName, string returnCode, Process process)
-    {
-        var output = returnCode == "0" ? process.StandardOutput.ReadToEnd() : process.StandardError.ReadToEnd();
-        _logger.Info($"{(returnCode == "0" ? "SUCCESS" : "ERROR")} [{processName}]: Return Code {returnCode}\n{output}");
-    }
-
 }

src/SmiServices/Microservices/DicomAnonymiser/Anonymisers/IDicomAnonymiser.cs

@@ -6,13 +6,13 @@ namespace SmiServices.Microservices.DicomAnonymiser.Anonymisers;
 public interface IDicomAnonymiser
 {
     /// <summary>
-    /// Anonymise the specified <paramref name="sourceFile"/> to <paramref name="destFile"></paramref> based on the provided <paramref name="message"/> modality.
+    /// Anonymise the specified <paramref name="sourceFile"/> to <paramref name="destFile"></paramref>.
     /// Implementations should assume that <paramref name="sourceFile"/> already exists, and <paramref name="destFile"></paramref> does not exist. 
     /// </summary>
-    /// <param name="message"></param>
     /// <param name="sourceFile"></param>
     /// <param name="destFile"></param>
+    /// <param name="modality"></param>
     /// <param name="anonymiserStatusMessage"></param>
     /// <returns></returns>
-    ExtractedFileStatus Anonymise(ExtractFileMessage message, IFileInfo sourceFile, IFileInfo destFile, out string anonymiserStatusMessage);
+    ExtractedFileStatus Anonymise(IFileInfo sourceFile, IFileInfo destFile, string modality, out string? anonymiserStatusMessage);
 }

src/SmiServices/Microservices/DicomAnonymiser/Anonymisers/SmiCtpAnonymiser.cs

@@ -0,0 +1,112 @@
+using NLog;
+using SmiServices.Common.Messages.Extraction;
+using SmiServices.Common.Options;
+using SmiServices.Microservices.DicomAnonymiser.Helpers;
+using System;
+using System.Diagnostics;
+using System.IO;
+using System.IO.Abstractions;
+using System.Threading;
+
+namespace SmiServices.Microservices.DicomAnonymiser.Anonymisers;
+
+public class SmiCtpAnonymiser : IDicomAnonymiser, IDisposable
+{
+    private readonly ILogger _logger = LogManager.GetCurrentClassLogger();
+    private readonly Process _ctpProcess;
+
+    // NOTE(rkm 2025-01-08) This sometimes takes more than 10s in CI for some reason
+    private readonly TimeSpan CTP_TIMEOUT = TimeSpan.FromSeconds(20);
+
+    public SmiCtpAnonymiser(GlobalOptions globalOptions)
+    {
+        var dicomAnonymiserOptions = globalOptions.DicomAnonymiserOptions ?? throw new ArgumentException($"{nameof(globalOptions.DicomAnonymiserOptions)} was null", nameof(globalOptions));
+
+        if (!File.Exists(dicomAnonymiserOptions.CtpAnonCliJar))
+            throw new ArgumentException($"{nameof(dicomAnonymiserOptions.CtpAnonCliJar)} '{dicomAnonymiserOptions.CtpAnonCliJar}' does not exist", nameof(globalOptions));
+
+        if (!File.Exists(dicomAnonymiserOptions.CtpAllowlistScript))
+            throw new ArgumentException($"{nameof(dicomAnonymiserOptions.CtpAllowlistScript)} '{dicomAnonymiserOptions.CtpAllowlistScript}' does not exist", nameof(globalOptions));
+
+        var srAnonTool = "false";
+        if (!string.IsNullOrWhiteSpace(dicomAnonymiserOptions.SRAnonymiserToolPath))
+        {
+            if (!File.Exists(dicomAnonymiserOptions.SRAnonymiserToolPath))
+                throw new ArgumentException($"{nameof(dicomAnonymiserOptions.SRAnonymiserToolPath)} '{dicomAnonymiserOptions.SRAnonymiserToolPath}' does not exist", nameof(globalOptions));
+            srAnonTool = dicomAnonymiserOptions.SRAnonymiserToolPath;
+        }
+
+        var ctpArgs = $"-jar {dicomAnonymiserOptions.CtpAnonCliJar} --anon-script {dicomAnonymiserOptions.CtpAllowlistScript} --sr-anon-tool {srAnonTool} --daemonize";
+        _logger.Info($"Starting ctp with: java {ctpArgs}");
+
+        var ready = false;
+        _ctpProcess = ProcessWrapper.CreateProcess("java", ctpArgs);
+        _ctpProcess.OutputDataReceived += OnCtpOutputDataReceived;
+        _ctpProcess.ErrorDataReceived += (_, args) => _logger.Debug(args.Data);
+        _ctpProcess.Start();
+        _ctpProcess.BeginOutputReadLine();
+        _ctpProcess.BeginErrorReadLine();
+
+        lock (_ctpProcess)
+            Monitor.Wait(_ctpProcess, CTP_TIMEOUT);
+
+        if (!ready)
+        {
+            _ctpProcess.Kill();
+            throw new Exception($"Did not receive READY before timeout");
+        }
+
+        void OnCtpOutputDataReceived(object _, DataReceivedEventArgs args)
+        {
+            _logger.Debug($"[ctp-anon-cli stdout] {args.Data}");
+            if ("READY" != args.Data) return;
+
+            ready = true;
+            lock (_ctpProcess)
+                Monitor.Pulse(_ctpProcess);
+        }
+    }
+
+    public ExtractedFileStatus Anonymise(IFileInfo sourceFile, IFileInfo destFile, string modality, out string? anonymiserStatusMessage)
+    {
+        var args = $"{sourceFile.FullName} {destFile.FullName}";
+        string? result = null;
+
+        _ctpProcess.OutputDataReceived += CtpProcessOnOutputDataReceived;
+
+        _logger.Debug($"[ctp-anon-cli stdin ] {args}");
+        _ctpProcess.StandardInput.WriteLine(args);
+
+        lock (args)
+            Monitor.Wait(args);
+
+        _ctpProcess.OutputDataReceived -= CtpProcessOnOutputDataReceived;
+
+        ExtractedFileStatus status;
+        if (result == "OK")
+        {
+            anonymiserStatusMessage = null;
+            status = ExtractedFileStatus.Anonymised;
+        }
+        else
+        {
+            anonymiserStatusMessage = result;
+            status = ExtractedFileStatus.ErrorWontRetry;
+        }
+
+        return status;
+
+        void CtpProcessOnOutputDataReceived(object _, DataReceivedEventArgs e)
+        {
+            result = e.Data;
+            lock (args)
+                Monitor.Pulse(args);
+        }
+    }
+
+    public void Dispose()
+    {
+        GC.SuppressFinalize(this);
+        _ctpProcess.Dispose();
+    }
+}