Repository: dmwm/wmcore
Issue: Flaw in the isEmptyResults logic for MS/Common tools

**Impact of the bug**
Microservices

**Describe the bug**
Data is returned as a JSON object by pycurl_manager module, which means this check for empty results:
https://github.com/dmwm/WMCore/blob/master/src/python/WMCore/MicroService/Tools/Common.py#L49

is missing one - or probably two extra checks - for json empty objects like 'null' and '[]'.

**How to reproduce it**
Query DBS with `from WMCore.Services.pycurl_manager import getdata` for an invalid input data

**Expected behavior**
We should not consider a 200 status code with output `'[]'` (JSON object) as failure in:
https://github.com/dmwm/WMCore/blob/master/src/python/WMCore/MicroService/Tools/Common.py#L49

So we need to either add a couple of other possible results in that list in the `if` statement. Or add a logic of json.load and check (likely in a try/except block).
 
**Additional context and error message**
Error message from MSTransferor logs:
```
2022-05-17 12:13:00,261:INFO:Common: Executing 13 requests against DBS 'blockparents' API
Failure in findBlockParents for block /NoBPTX/Run2018D-12Nov2019_UL2018-v1/MINIAOD#be382968-82b7-4e65-b73f-57aa0b83d0e4. Error: 'this_block_name'
2022-05-17 12:13:00,461:ERROR:MSManager: MS daemon error: 'NoneType' object has no attribute 'items'
Traceback (most recent call last):
  File "/data/srv/HG2205e/sw/slc7_amd64_gcc630/cms/reqmgr2ms/1.0.3.pre7/lib/python3.8/site-packages/WMCore/MicroService/MSManager.py", line 41, in daemon
    func(reqStatus)
  File "/data/srv/HG2205e/sw/slc7_amd64_gcc630/cms/reqmgr2ms/1.0.3.pre7/lib/python3.8/site-packages/WMCore/MicroService/MSManager.py", line 179, in transferor
    res = self.msTransferor.execute(reqStatus)
  File "/data/srv/HG2205e/sw/slc7_amd64_gcc630/cms/reqmgr2ms/1.0.3.pre7/lib/python3.8/site-packages/WMCore/MicroService/MSTransferor/MSTransferor.py", line 193, in execute
    reqResults = self.reqInfo(reqSlice)
  File "/data/srv/HG2205e/sw/slc7_amd64_gcc630/cms/reqmgr2ms/1.0.3.pre7/lib/python3.8/site-packages/WMCore/MicroService/MSTransferor/RequestInfo.py", line 69, in __call__
    self.unified(workflows)
  File "/data/srv/HG2205e/sw/slc7_amd64_gcc630/cms/reqmgr2ms/1.0.3.pre7/lib/python3.8/site-packages/WMCore/MicroService/MSTransferor/RequestInfo.py", line 151, in unified
    self.setParentChildBlocks(workflows, parentageMap)
  File "/data/srv/HG2205e/sw/slc7_amd64_gcc630/cms/reqmgr2ms/1.0.3.pre7/lib/python3.8/site-packages/WMCore/MicroService/MSTransferor/RequestInfo.py", line 506, in setParentChildBlocks
    wflow.setChildToParentBlocks(parentageMap[wflow.getInputDataset()])
  File "/data/srv/HG2205e/sw/slc7_amd64_gcc630/cms/reqmgr2ms/1.0.3.pre7/lib/python3.8/site-packages/WMCore/MicroService/MSTransferor/DataStructs/Workflow.py", line 321, in setChildToParentBlocks
    for child, parents in viewitems(blocksDict):
  File "/data/srv/HG2205e/sw/slc7_amd64_gcc630/external/py3-future/0.18.2-comp/lib/python3.8/site-packages/future/utils/__init__.py", line 277, in viewitems
    func = obj.items
AttributeError: 'NoneType' object has no attribute 'items'
```


Changed files:
src/python/WMCore/MicroService/Tools/Common.py, test/python/WMCore_t/MicroService_t/Tools_t/Common_t.py

diff --git a/src/python/WMCore/MicroService/Tools/Common.py b/src/python/WMCore/MicroService/Tools/Common.py
index 70ece3599..32cdf9725 100644
--- a/src/python/WMCore/MicroService/Tools/Common.py
+++ b/src/python/WMCore/MicroService/Tools/Common.py
@@ -34,21 +34,19 @@ STEP_PAT = re.compile(r'Step[0-9]')
 TASK_PAT = re.compile(r'Task[0-9]')
 
 
-def isEmptyResults(row):
+def hasHTTPFailed(row):
     """
-    _isEmptyResults_
+    Evaluates whether the HTTP request through PyCurl failed or not.
 
-    Evaluates whether row data contains empty result set
-    :return: bool
+    :param row: dictionary data returned from pycurl_manager module
+    :return: a boolean confirming failure or not
     """
     if 'data' not in row:
-        raise Exception("provided result dict does not contain 'data' key")
-    # if code is not present in row it means it was success (HTTP status code 200)
-    code = int(row.get('code', 200))
-    data = row['data']
-    if (code >= 200 and code < 400) and data in (None, []):
         return True
-    return False
+    if int(row.get('code', 200)) == 200:
+        return False
+    return True
+
 
 def getMSLogger(verbose, logger=None):
     """
@@ -82,7 +80,7 @@ def dbsInfo(datasets, dbsUrl):
 
     for row in data:
         dataset = row['url'].split('=')[-1]
-        if isEmptyResults(row):
+        if hasHTTPFailed(row):
             print("FAILURE: dbsInfo for %s. Error: %s %s" % (dataset, row.get('code'), row.get('error')))
             continue
         rows = json.loads(row['data'])
@@ -251,7 +249,7 @@ def getBlocksByDsetAndRun(datasetName, runList, dbsUrl):
 
     for row in data:
         dataset = row['url'].rsplit('=')[-1]
-        if isEmptyResults(row):
+        if hasHTTPFailed(row):
             msg = "Failure in getBlocksByDsetAndRun for %s. Error: %s %s" % (dataset,
                                                                              row.get('code'),
                                                                              row.get('error'))
@@ -280,13 +278,13 @@ def getFileLumisInBlock(blocks, dbsUrl, validFileOnly=1):
 
     for row in data:
         blockName = unquote(row['url'].rsplit('=')[-1])
-        if isEmptyResults(row):
+        if hasHTTPFailed(row):
             msg = "Failure in getFileLumisInBlock for block %s. Error: %s %s" % (blockName,
                                                                                  row.get('code'),
                                                                                  row.get('error'))
             raise RuntimeError(msg)
         rows = json.loads(row['data'])
-        rows = aggFileLumis(rows) # adjust to DBS Go server output
+        rows = aggFileLumis(rows)  # adjust to DBS Go server output
         runLumisByBlock.setdefault(blockName, [])
         for item in rows:
             runLumisByBlock[blockName].append(item)
@@ -308,7 +306,7 @@ def findBlockParents(blocks, dbsUrl):
     for row in data:
         blockName = unquote(row['url'].rsplit('=')[-1])
         dataset = blockName.split("#")[0]
-        if isEmptyResults(row):
+        if hasHTTPFailed(row):
             print("Failure in findBlockParents for block %s. Error: %s %s" % (blockName,
                                                                               row.get('code'),
                                                                               row.get('error')))
@@ -342,7 +340,7 @@ def getRunsInBlock(blocks, dbsUrl):
     data = multi_getdata(urls, ckey(), cert())
     for row in data:
         blockName = unquote(row['url'].rsplit('=')[-1])
-        if isEmptyResults(row):
+        if hasHTTPFailed(row):
             msg = "Failure in getRunsInBlock for block %s. Error: %s %s" % (blockName,
                                                                             row.get('code'),
                                                                             row.get('error'))
@@ -387,10 +385,10 @@ def eventsLumisInfo(inputs, dbsUrl, validFileOnly=0, sumOverLumi=0):
     data = multi_getdata(urls, ckey(), cert())
     for row in data:
         data = unquote(row['url'].split('=')[-1])
-        if isEmptyResults(row):
+        if hasHTTPFailed(row):
             print("FAILURE: eventsLumisInfo for %s. Error: %s %s" % (data,
-                                                                    row.get('code'),
-                                                                    row.get('error')))
+                                                                     row.get('code'),
+                                                                     row.get('error')))
             continue
         rows = json.loads(row['data'])
         for item in rows:
@@ -594,7 +592,7 @@ def findParent(datasets, dbsUrl):
 
     for row in data:
         dataset = row['url'].split('=')[-1]
-        if isEmptyResults(row):
+        if hasHTTPFailed(row):
             print("Failure in findParent for dataset %s. Error: %s %s" % (dataset,
                                                                           row.get('code'),
                                                                           row.get('error')))
diff --git a/test/python/WMCore_t/MicroService_t/Tools_t/Common_t.py b/test/python/WMCore_t/MicroService_t/Tools_t/Common_t.py
index b80f133aa..3f6a737a7 100644
--- a/test/python/WMCore_t/MicroService_t/Tools_t/Common_t.py
+++ b/test/python/WMCore_t/MicroService_t/Tools_t/Common_t.py
@@ -5,10 +5,13 @@ Author: Valentin Kuznetsov <vkuznet [AT] gmail [DOT] com>
 """
 from __future__ import division, print_function
 
+import json
 import unittest
 
-from WMCore.MicroService.Tools.Common import \\
-        dbsInfo, getEventsLumis, findParent, isEmptyResults
+from nose.plugins.attrib import attr
+
+from WMCore.MicroService.Tools.Common import (dbsInfo, getEventsLumis,
+                                              findParent, hasHTTPFailed)
 
 
 class CommonTest(unittest.TestCase):
@@ -20,6 +23,7 @@ class CommonTest(unittest.TestCase):
                          '/ZMM_14TeV/Summer12-DESIGN42_V17_SLHCTk-v1/GEN-SIM']
         self.child = ['/SingleElectron/Run2016B-18Apr2017_ver2-v1/AOD']
 
+    @attr("integration")
     def testDbsInfo(self):
         "Test function for dbsInfo()"
         datasetBlocks, datasetSizes, datasetTransfers = dbsInfo(self.datasets, self.dbsUrl)
@@ -31,36 +35,46 @@ class CommonTest(unittest.TestCase):
         self.assertEqual(expect, sizes)
         self.assertEqual(len(self.datasets), len(datasetTransfers))
 
+    @attr("integration")
     def testGetEventsLumis(self):
         "Test function for getEventsLumis()"
         totEvts = totLumis = 0
         for dataset in self.datasets:
             nevts, nlumis = getEventsLumis(dataset, self.dbsUrl)
+            print("dataset: {} with evts: {} and lumis: {}".format(dataset, nevts, nlumis))
             totEvts += nevts
             totLumis += nlumis
-        expect = 10250 + 10616
+        expect = 7398 + 0
         self.assertEqual(expect, totEvts)
-        expect = 22 + 10
+        expect = 16 + 0
         self.assertEqual(expect, totLumis)
 
+    @attr("integration")
     def test_findParent(self):
         "Test function for findParent()"
         parents = findParent(self.child, self.dbsUrl)
         self.assertEqual(parents[self.child[0]], '/SingleElectron/Run2016B-v2/RAW')
 
-    def test_isEmptyResults(self):
-        row = {'code': 200, 'data': None}
-        result = isEmptyResults(row)
-        self.assertEqual(result, True)
-        row = {'code': 200, 'data': []}
-        result = isEmptyResults(row)
-        self.assertEqual(result, True)
-        row = {'code': 400, 'data': None}
-        result = isEmptyResults(row)
-        self.assertEqual(result, False)
-        row = {'code': 400, 'data': []}
-        result = isEmptyResults(row)
-        self.assertEqual(result, False)
+    def testHasHTTPFailed(self):
+        """Test the hasHTTPFailed method"""
+        self.assertTrue(hasHTTPFailed({}))
+        self.assertTrue(hasHTTPFailed({'data': '', 'code': 301, 'error': ''}))
+        self.assertTrue(hasHTTPFailed({'data': '', 'code': 301, 'error': 'blah'}))
+        self.assertTrue(hasHTTPFailed({'data': 1, 'code': 400, 'error': 'blah'}))
+        self.assertTrue(hasHTTPFailed({'data': 1, 'code': 500, 'error': []}))
+        self.assertTrue(hasHTTPFailed({'data': 1, 'code': 500, 'error': None}))
+
+        self.assertFalse(hasHTTPFailed({'data': []}))
+        self.assertFalse(hasHTTPFailed({'data': None}))
+        self.assertFalse(hasHTTPFailed({'data': json.dumps([])}))
+        self.assertFalse(hasHTTPFailed({'data': json.dumps(None)}))
+        self.assertFalse(hasHTTPFailed({'data': ['blah']}))
+        self.assertFalse(hasHTTPFailed({'data': 'blah'}))
+        self.assertFalse(hasHTTPFailed({'data': json.dumps('blah')}))
+        self.assertFalse(hasHTTPFailed({'data': 'blah', 'code': 200}))
+        # result below should never happen, but let's test the status code
+        self.assertFalse(hasHTTPFailed({'data': 1, 'code': 200, 'error': 'blah'}))
+
 
 if __name__ == '__main__':
     unittest.main()