From 67d5360571ef5dcff5bf8a2774ae6d4d34084760 Mon Sep 17 00:00:00 2001 From: Alan Malta Rodrigues Date: Thu, 15 Oct 2020 13:03:52 +0200 Subject: [PATCH 1/2] Properly validate RequestPriority parameter improve error message cast using type from stdbase use type check remove blank space fix message --- src/python/WMCore/ReqMgr/Service/Request.py | 8 +++++-- src/python/WMCore/ReqMgr/Utils/Validation.py | 23 +++++++++++++++----- 2 files changed, 24 insertions(+), 7 deletions(-) diff --git a/src/python/WMCore/ReqMgr/Service/Request.py b/src/python/WMCore/ReqMgr/Service/Request.py index 93877676a6..acd350d672 100644 --- a/src/python/WMCore/ReqMgr/Service/Request.py +++ b/src/python/WMCore/ReqMgr/Service/Request.py @@ -22,7 +22,8 @@ REQUEST_STATE_TRANSITION, ACTIVE_STATUS) from WMCore.ReqMgr.DataStructs.RequestType import REQUEST_TYPES from WMCore.ReqMgr.Utils.Validation import (validate_request_create_args, validate_request_update_args, - validate_clone_create_args, validateOutputDatasets, workqueue_stat_validation) + validate_clone_create_args, validateOutputDatasets, + validate_request_priority, workqueue_stat_validation) from WMCore.Services.RequestDB.RequestDBWriter import RequestDBWriter from WMCore.Services.WorkQueue.WorkQueue import WorkQueue @@ -409,14 +410,16 @@ def _handleNoStatusUpdate(self, workload, request_args, dn): if 'RequestPriority' in request_args: # Yes, we completely ignore any other arguments posted by the user (web UI case) request_args = {'RequestPriority': request_args['RequestPriority']} + validate_request_priority(request_args) # must update three places: GQ elements, workload_cache and workload spec self.gq_service.updatePriority(workload.name(), request_args['RequestPriority']) report = self.reqmgr_db_service.updateRequestProperty(workload.name(), request_args, dn) workload.setPriority(request_args['RequestPriority']) workload.saveCouchUrl(workload.specUrl()) - cherrypy.log('Updated priority of "%s" to %s' % (workload.name(), request_args['RequestPriority'])) + cherrypy.log('Updated priority of "{}" to: {}'.format(workload.name(), request_args['RequestPriority'])) elif workqueue_stat_validation(request_args): report = self.reqmgr_db_service.updateRequestStats(workload.name(), request_args) + cherrypy.log('Updated workqueue statistics of "{}", with: {}'.format(workload.name(), request_args)) else: msg = "There are invalid arguments for no-status update: %s" % request_args raise InvalidSpecParameterValue(msg) @@ -431,6 +434,7 @@ def _handleAssignmentApprovedTransition(self, workload, request_args, dn): msg = "There are invalid arguments for assignment-approved transition: %s" % request_args raise InvalidSpecParameterValue(msg) + validate_request_priority(request_args) report = self.reqmgr_db_service.updateRequestProperty(workload.name(), request_args, dn) return report diff --git a/src/python/WMCore/ReqMgr/Utils/Validation.py b/src/python/WMCore/ReqMgr/Utils/Validation.py index 75873ec3a5..641a216529 100644 --- a/src/python/WMCore/ReqMgr/Utils/Validation.py +++ b/src/python/WMCore/ReqMgr/Utils/Validation.py @@ -13,6 +13,7 @@ from WMCore.ReqMgr.Tools.cms import releases, architectures, dashboardActivities from WMCore.Services.DBS.DBS3Reader import getDataTiers from WMCore.WMFactory import WMFactory +from WMCore.WMSpec.StdSpecs.StdBase import StdBase from WMCore.WMSpec.WMWorkload import WMWorkloadHelper from WMCore.WMSpec.WMWorkloadTools import loadSpecClassByType, setArgumentsWithDefault from WMCore.Cache.GenericDataCache import GenericDataCache, MemoryCacheStruct @@ -63,15 +64,27 @@ def validate_request_update_args(request_args, config, reqmgr_db_service, param) elif request_args["RequestStatus"] == 'assigned': workload.validateArgumentForAssignment(request_args) - # TODO: fetch it from the assignment arg definition - if 'RequestPriority' in request_args: - request_args['RequestPriority'] = int(request_args['RequestPriority']) - if (lambda x: (x >= 0 and x < 1e6))(request_args['RequestPriority']) is False: - raise InvalidSpecParameterValue("RequestPriority must be an integer between 0 and 1e6") + validate_request_priority(request_args) return workload, request_args +def validate_request_priority(reqArgs): + """ + Validate the RequestPriority argument against its definition + in StdBase + :param reqArgs: dictionary of user request arguments + :return: nothing, but raises an exception in case of an invalid value + """ + if 'RequestPriority' in reqArgs: + reqPrioDefin = StdBase.getWorkloadCreateArgs()['RequestPriority'] + if not isinstance(reqArgs['RequestPriority'], reqPrioDefin['type']): + msg = "RequestPriority must be of integer type, not: {}".format(type(reqArgs['RequestPriority'])) + raise InvalidSpecParameterValue(msg) + if reqPrioDefin['validate'](reqArgs['RequestPriority']) is False: + raise InvalidSpecParameterValue("RequestPriority must be an integer between 0 and 999999") + + def validate_request_create_args(request_args, config, reqmgr_db_service, *args, **kwargs): """ *arg and **kwargs are only for the interface From 094c3ab6df24c1245c5d3fe9e448d8a7a17f144b Mon Sep 17 00:00:00 2001 From: Alan Malta Rodrigues Date: Thu, 15 Oct 2020 13:04:13 +0200 Subject: [PATCH 2/2] created unit test fix unit tests --- .../WMCore_t/ReqMgr_t/Utils_t/Validation_t.py | 26 ++++++++++++++++++- 1 file changed, 25 insertions(+), 1 deletion(-) diff --git a/test/python/WMCore_t/ReqMgr_t/Utils_t/Validation_t.py b/test/python/WMCore_t/ReqMgr_t/Utils_t/Validation_t.py index 22723f2709..99ec8e5490 100644 --- a/test/python/WMCore_t/ReqMgr_t/Utils_t/Validation_t.py +++ b/test/python/WMCore_t/ReqMgr_t/Utils_t/Validation_t.py @@ -7,8 +7,8 @@ import unittest -from WMCore.ReqMgr.Utils.Validation import validateOutputDatasets from WMCore.ReqMgr.DataStructs.RequestError import InvalidSpecParameterValue +from WMCore.ReqMgr.Utils.Validation import validateOutputDatasets, validate_request_priority class ValidationTests(unittest.TestCase): @@ -48,6 +48,30 @@ def testValidateOutputDatasets(self): with self.assertRaises(InvalidSpecParameterValue): validateOutputDatasets(outputDsets, dbsUrl) + def testRequestPriorityValidation(self): + """ + Test the `validate_request_priority` function, which validates the + RequestPriority parameter + :return: nothing, raises an exception if there are problems + """ + # test valid cases, integer in the range of [0, 1e6] + for goodPrio in [0, 100, int(1e6 - 1)]: + reqArgs = {'RequestPriority': goodPrio} + print(reqArgs) + validate_request_priority(reqArgs) + + # test invalid ranges + for badPrio in [-10, 1e6, 1e7]: + reqArgs = {'RequestPriority': badPrio} + with self.assertRaises(InvalidSpecParameterValue): + validate_request_priority(reqArgs) + + # test invalid data types + for badPrio in ["1234", 1234.35, 1e6, [123]]: + reqArgs = {'RequestPriority': badPrio} + with self.assertRaises(InvalidSpecParameterValue): + validate_request_priority(reqArgs) + if __name__ == '__main__': unittest.main()