Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Fix logic to check for empty results in MS/Common #11148

Merged
merged 2 commits into from
May 18, 2022

Conversation

amaltaro
Copy link
Contributor

@amaltaro amaltaro commented May 17, 2022

Fixes #11147

Status

ready

Description

Summary of changes is:

  • check if the HTTP request was successful based solely on the status code (anything other than code 200 is considered as failed)
  • change the exception type
  • marked a few unit tests as integration to avoid making calls to real DBS server

Is it backward compatible (if not, which system it affects?)

YES

Related PRs

None

External dependencies / deployment changes

None

@cmsdmwmbot
Copy link

Jenkins results:

  • Python3 Unit tests: failed
    • 19 new failures
    • 3 tests deleted
    • 7 tests added
    • 2 changes in unstable tests
  • Python3 Pylint check: succeeded
    • 5 warnings
    • 61 comments to review
  • Pylint py3k check: failed
    • 2 warnings
  • Pycodestyle check: succeeded
    • 17 comments to review

Details at https://cmssdt.cern.ch/dmwm-jenkins/view/All/job/DMWM-WMCore-PR-test/13221/artifact/artifacts/PullRequestReport.html

@amaltaro
Copy link
Contributor Author

test this please

@cmsdmwmbot
Copy link

Jenkins results:

  • Python3 Unit tests: succeeded
    • 3 tests deleted
    • 7 tests added
    • 2 changes in unstable tests
  • Python3 Pylint check: succeeded
    • 5 warnings
    • 61 comments to review
  • Pylint py3k check: failed
    • 2 warnings
  • Pycodestyle check: succeeded
    • 17 comments to review

Details at https://cmssdt.cern.ch/dmwm-jenkins/view/All/job/DMWM-WMCore-PR-test/13222/artifact/artifacts/PullRequestReport.html

@cmsdmwmbot
Copy link

Jenkins results:

  • Python3 Unit tests: succeeded
    • 4 tests deleted
    • 8 tests added
  • Python3 Pylint check: succeeded
    • 5 warnings
    • 60 comments to review
  • Pylint py3k check: failed
    • 2 warnings
  • Pycodestyle check: succeeded
    • 12 comments to review

Details at https://cmssdt.cern.ch/dmwm-jenkins/view/All/job/DMWM-WMCore-PR-test/13225/artifact/artifacts/PullRequestReport.html

code = int(row.get('code', 200))
data = row['data']
if (code >= 200 and code < 400) and data in (None, []):
if code >= 400 and 'error' in row:
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@amaltaro this will not work for non 200 requests since the response will (may) contain empty body. For instance, if HTTP server has moved its API to another end-point, then you'll get 301 HTTP StatusMovedPermanently code. (This is common use-case when server only deal with /api but can't handle extra slash like `/api/ and vice versa. Now, the HTTP response body in this case will be empty. And, if we use Python client it will fail later in a code, see:

if hasHTTPFailed(row):
   ...
rows = json.loads(row['data'])
...
for item in rows:

So, if HTTP response body is empty string then json.loads will fail as following:

>>> import json
>>> json.loads('')
Traceback (most recent call last):
  File "<stdin>", line 1, in <module>
  File "/opt/local/Library/Frameworks/Python.framework/Versions/3.9/lib/python3.9/json/__init__.py", line 346, in loads
    return _default_decoder.decode(s)
  File "/opt/local/Library/Frameworks/Python.framework/Versions/3.9/lib/python3.9/json/decoder.py", line 337, in decode
    obj, end = self.raw_decode(s, idx=_w(s, 0).end())
  File "/opt/local/Library/Frameworks/Python.framework/Versions/3.9/lib/python3.9/json/decoder.py", line 355, in raw_decode
    raise JSONDecodeError("Expecting value", s, err.value) from None
json.decoder.JSONDecodeError: Expecting value: line 1 column 1 (char 0)

So, in addition to checking HTTP code we also need to wrap rows = json.loads(row['data']) into try/except block or check that row['data'] is not equal to empty string. Otherwise, we should only concentrate on 200 HTTP code only. I think to satisfy both use-cases it is better to wrap rows = json.loads(row['data']) into try/except block within this module. The except should provide error handling and provide details about code and response body, e.g.

try:
        rows = json.loads(row['data'])
        ...
        for item in rows:
except:
        print("HTTP code {}, response {}".format(row.get('code'), row.get('data'))

amaltaro added 2 commits May 17, 2022 16:13
change check from empty result to http failed

pass HTTP request only for status code 200
update unit tests

update unit tests
@amaltaro
Copy link
Contributor Author

Thank you for your suggestion, Valentin. I have changed the code now to mark HTTP requests as successful only when their status code is 200. Unit test was also updated according to your suggestion. Thanks

@amaltaro amaltaro requested a review from vkuznet May 17, 2022 20:17
@cmsdmwmbot
Copy link

Jenkins results:

  • Python3 Unit tests: succeeded
    • 4 tests deleted
    • 8 tests added
  • Python3 Pylint check: succeeded
    • 5 warnings
    • 60 comments to review
  • Pylint py3k check: failed
    • 2 warnings
  • Pycodestyle check: succeeded
    • 12 comments to review

Details at https://cmssdt.cern.ch/dmwm-jenkins/view/All/job/DMWM-WMCore-PR-test/13231/artifact/artifacts/PullRequestReport.html

@goughes
Copy link
Contributor

goughes commented May 18, 2022

test this please

@cmsdmwmbot
Copy link

Jenkins results:

  • Python3 Unit tests: failed
    • 15 new failures
    • 4 tests deleted
    • 1 tests added
    • 2 changes in unstable tests
  • Python3 Pylint check: succeeded
    • 5 warnings
    • 60 comments to review
  • Pylint py3k check: failed
    • 2 warnings
  • Pycodestyle check: succeeded
    • 12 comments to review

Details at https://cmssdt.cern.ch/dmwm-jenkins/view/All/job/DMWM-WMCore-PR-test/13235/artifact/artifacts/PullRequestReport.html

@amaltaro amaltaro merged commit 7a65b12 into dmwm:master May 18, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Flaw in the isEmptyResults logic for MS/Common tools
4 participants