[8.0] fix (RMS): stop the ReqProxy from dropping the operations of cached requests

src/DIRAC/RequestManagementSystem/Client/Operation.py

@@ -99,14 +99,12 @@ def __init__(self, fromDict=None):
         elif not isinstance(fromDict, dict):
             fromDict = {}
 
-        if "Files" in fromDict:
-            for fileDict in fromDict.get("Files", []):
-                self.addFile(File(fileDict))
-
-            del fromDict["Files"]
+        for fileDict in fromDict.get("Files", []):
+            self.addFile(File(fileDict))
 
+        # Don't modify fromDict, callers may still be using it
         for key, value in fromDict.items():
-            if value:
+            if key != "Files" and value:
                 setattr(self, key, value)
 
     # # protected methods for parent only

src/DIRAC/RequestManagementSystem/Client/Request.py

@@ -92,14 +92,12 @@ def __init__(self, fromDict=None):
         elif not isinstance(fromDict, dict):
             fromDict = {}
 
-        if "Operations" in fromDict:
-            for opDict in fromDict.get("Operations", []):
-                self += Operation(opDict)
-
-            del fromDict["Operations"]
+        for opDict in fromDict.get("Operations", []):
+            self += Operation(opDict)
 
+        # Don't modify fromDict, callers may still be using it
         for key, value in fromDict.items():
-            if value:
+            if key != "Operations" and value:
                 setattr(self, key, value)
 
         self._notify()

src/DIRAC/RequestManagementSystem/Client/test/Test_Operation.py

@@ -7,6 +7,8 @@
 
     Operation test cases
 """
+import copy
+
 import pytest
 
 from DIRAC.RequestManagementSystem.Client.File import File
@@ -132,3 +134,12 @@ def test_List():
     # opID set
     op.OperationID = 1
     del op[0]
+
+
+def test_ctorDoesNotModifyInput():
+    """c'tor must not modify the dict it is given"""
+    opDict = {"Type": "ReplicateAndRegister", "TargetSE": "CERN-USER", "Files": [{"LFN": "/a/b/c"}]}
+    expected = copy.deepcopy(opDict)
+    operation = Operation(opDict)
+    assert opDict == expected
+    assert len(operation) == 1

src/DIRAC/RequestManagementSystem/Client/test/Test_Request.py

@@ -8,6 +8,7 @@
     test cases for Request class
 """
 
+import copy
 import datetime
 
 from DIRAC.RequestManagementSystem.Client.Operation import Operation
@@ -134,6 +135,19 @@ def test_CtorSerilization():
     req = Request(fromJSON)
 
 
+def test_CtorDoesNotModifyInput():
+    """c'tor must not modify the dict it is given"""
+    reqDict = {
+        "RequestName": "test",
+        "Operations": [{"Type": "ReplicateAndRegister", "TargetSE": "CERN-USER", "Files": [{"LFN": "/a/b/c"}]}],
+    }
+    expected = copy.deepcopy(reqDict)
+    req = Request(reqDict)
+    assert reqDict == expected
+    assert len(req) == 1
+    assert len(req[0]) == 1
+
+
 def test_Props():
     """props"""
     # # valid values

src/DIRAC/RequestManagementSystem/Service/ReqProxyHandler.py

@@ -178,7 +178,8 @@ def export_putRequest(self, requestJSON):
         gLogger.info("putRequest: got request", f"{requestName}")
 
         # We only need the object to check the authorization
-        request = Request(requestDict)
+        # Build it from the JSON rather than requestDict, as requestDict is what gets saved
+        request = Request(requestJSON)
         # Check whether the credentials in the Requests are correct and allowed to be set
         isAuthorized = RequestValidator.setAndCheckRequestOwner(request, self.getRemoteCredentials())
 

src/DIRAC/RequestManagementSystem/Service/test/Test_ReqProxyHandler.py

@@ -0,0 +1,32 @@
+"""Tests for ReqProxyHandler"""
+import json
+
+from DIRAC.RequestManagementSystem.Client.File import File
+from DIRAC.RequestManagementSystem.Client.Operation import Operation
+from DIRAC.RequestManagementSystem.Client.Request import Request
+from DIRAC.RequestManagementSystem.Service import ReqProxyHandler as ReqProxyModule
+from DIRAC.RequestManagementSystem.Service.ReqProxyHandler import ReqProxyHandler
+
+
+def test_putRequestKeepsOperations(tmp_path, monkeypatch):
+    """The cached request must contain the same operations as the one which was received"""
+    monkeypatch.setattr(ReqProxyHandler, "cacheDir", classmethod(lambda cls: str(tmp_path)))
+    monkeypatch.setattr(ReqProxyModule.RequestValidator, "setAndCheckRequestOwner", lambda request, creds: True)
+    handler = object.__new__(ReqProxyHandler)
+    handler.getRemoteCredentials = lambda: {}
+
+    request = Request({"RequestName": "test", "OwnerDN": "/DC=ch/CN=user", "OwnerGroup": "group"})
+    op = Operation({"Type": "ReplicateAndRegister", "TargetSE": "CERN-USER"})
+    op.addFile(File({"LFN": "/a/b/c"}))
+    request.addOperation(op)
+    request.addOperation(Operation({"Type": "ForwardDISET", "Arguments": "x"}))
+
+    res = handler.export_putRequest(request.toJSON()["Value"])
+    assert res["OK"], res
+    assert res["Value"] == {"set": False, "saved": True}
+
+    (cachedFile,) = tmp_path.iterdir()
+    cached = Request(json.loads(cachedFile.read_text()))
+    assert [o.Type for o in cached] == ["ReplicateAndRegister", "ForwardDISET"]
+    assert [f.LFN for f in cached[0]] == ["/a/b/c"]
+    assert (cached.OwnerDN, cached.OwnerGroup) == ("/DC=ch/CN=user", "group")

src/DIRAC/RequestManagementSystem/private/RequestTask.py

@@ -281,6 +281,13 @@ def __call__(self):
                 self.log.error("Cannot get waiting operation", operation["Message"])
                 return operation
             operation = operation["Value"]
+            if operation is None:
+                # e.g. the request has no operations at all: there is nothing we can do with it,
+                # so keep it for a human to look at rather than picking it up again immediately
+                self.log.error("Request has no operation to execute, delaying it", self.request.RequestName)
+                self.request.Error = "Request has no operation to execute"
+                self.request.delayNextExecution(24 * 60)
+                return S_OK(self.request)
             self.log.info("executing operation", f"{operation.Type}")
 
             # # and handler for it

src/DIRAC/RequestManagementSystem/private/test/Test_RequestTask.py

@@ -6,6 +6,7 @@
 
     test cases for RequestTask class
 """
+import datetime
 import unittest
 import importlib
 from unittest.mock import Mock, MagicMock
@@ -72,6 +73,27 @@ def testAPI(self):
         ret = self.task.setupProxy()
         print(ret)
 
+    def testNoOperation(self):
+        """a request without any operation is delayed instead of crashing"""
+        rt = importlib.import_module("DIRAC.RequestManagementSystem.private.RequestTask")
+        rt.Operations = self.mockOps
+        rt.CS = MagicMock()
+
+        emptyReq = Request({"RequestName": "empty", "OwnerGroup": "lhcb_user", "Owner": "chaen"})
+        self.task = RequestTask(
+            emptyReq.toJSON()["Value"],
+            self.handlersDict,
+            "csPath",
+            "RequestManagement/RequestExecutingAgent",
+            requestClient=self.mockRC,
+        )
+        self.task.setupProxy = Mock(return_value={"OK": True, "Value": {"Shifter": []}})
+        ret = self.task()
+        self.assertTrue(ret["OK"], ret)
+        self.assertEqual(ret["Value"].Status, "Waiting")
+        self.assertEqual(ret["Value"].Error, "Request has no operation to execute")
+        self.assertGreater(ret["Value"].NotBefore, datetime.datetime.utcnow() + datetime.timedelta(hours=23))
+
 
 # # tests execution
 if __name__ == "__main__":