diff --git a/src/DIRAC/RequestManagementSystem/Client/Operation.py b/src/DIRAC/RequestManagementSystem/Client/Operation.py index 03571413233..2df5896245d 100644 --- a/src/DIRAC/RequestManagementSystem/Client/Operation.py +++ b/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 diff --git a/src/DIRAC/RequestManagementSystem/Client/Request.py b/src/DIRAC/RequestManagementSystem/Client/Request.py index c3241f9ac1d..224ec370869 100644 --- a/src/DIRAC/RequestManagementSystem/Client/Request.py +++ b/src/DIRAC/RequestManagementSystem/Client/Request.py @@ -86,14 +86,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() diff --git a/src/DIRAC/RequestManagementSystem/Client/test/Test_Operation.py b/src/DIRAC/RequestManagementSystem/Client/test/Test_Operation.py index c92e40ba4ab..479a8680959 100644 --- a/src/DIRAC/RequestManagementSystem/Client/test/Test_Operation.py +++ b/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 diff --git a/src/DIRAC/RequestManagementSystem/Client/test/Test_Request.py b/src/DIRAC/RequestManagementSystem/Client/test/Test_Request.py index 86b929fa466..dcb790b4fb4 100644 --- a/src/DIRAC/RequestManagementSystem/Client/test/Test_Request.py +++ b/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 diff --git a/src/DIRAC/RequestManagementSystem/Service/ReqProxyHandler.py b/src/DIRAC/RequestManagementSystem/Service/ReqProxyHandler.py index 595c2eb8a32..1eafe496185 100644 --- a/src/DIRAC/RequestManagementSystem/Service/ReqProxyHandler.py +++ b/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()) diff --git a/src/DIRAC/RequestManagementSystem/Service/test/Test_ReqProxyHandler.py b/src/DIRAC/RequestManagementSystem/Service/test/Test_ReqProxyHandler.py new file mode 100644 index 00000000000..1d070a343da --- /dev/null +++ b/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", "Owner": "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.Owner, cached.OwnerGroup) == ("user", "group") diff --git a/src/DIRAC/RequestManagementSystem/private/RequestTask.py b/src/DIRAC/RequestManagementSystem/private/RequestTask.py index b452b59d2fe..26cb4a89a9e 100644 --- a/src/DIRAC/RequestManagementSystem/private/RequestTask.py +++ b/src/DIRAC/RequestManagementSystem/private/RequestTask.py @@ -271,6 +271,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 diff --git a/src/DIRAC/RequestManagementSystem/private/test/Test_RequestTask.py b/src/DIRAC/RequestManagementSystem/private/test/Test_RequestTask.py index 70245220f6a..6d7c6125ece 100644 --- a/src/DIRAC/RequestManagementSystem/private/test/Test_RequestTask.py +++ b/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__":