Avoid raising exception when ReqMgr2 request is not found #11159
Conversation
|
Tested in my VM with (and without the detail option) and this is how the response will look like when request cannot be found: |
|
Jenkins results:
|
There was a problem hiding this comment.
@amaltaro , there are several issues with the code:
- line 80 returns a dict, while line 82 returns a list. Inconsistent data-type from the same API is a bad practice since it forces clients to handle (parse) different data-types.
- on the same line you change result local variable by re-assigning to it different values, instead it is better to just return this alue
- there is no docstring for this function, and since you are touching it , then it is better to add a docstring
- I suggest to add comment about data-type of the doc returned by
getDocAPI. - if
result = self.couchDB.getDoc(requestName)is a dict then it is better to useresult.get('RequestName', ...)call where you should supply default (proper) value (data-type), e.g.result.get('RequestName', "")ifRequestNameis a string (I have not idea what should be returned, a string or some sort of the object). - in
elif resultit is unclear a type of result, someone can think it is a boolean or empty list or empty dict, at least you should add a proper comment over there or better check results data-type and value.
|
Thanks for a prompt review! See below some follow up to the points you raised.
I understand and I also agree with this. However, I am not changing it here because it might affect clients that are already consuming such information and the present format for years. Other than the comment below, I made further modifications and it's ready for another review. Note, however, that this PR is meant to only fix the exception raised on the server side. |
|
Jenkins results:
|
There was a problem hiding this comment.
Alan, the changes are fine but you can still improve things by changing this block:
if detail:
result.pop('_attachments', None)
result.pop('_rev', None)
result = {result['RequestName']: result}
else:
result = [result['RequestName']]
return result
to
if detail:
result.pop('_attachments', None)
result.pop('_rev', None)
return {result['RequestName']: result}
return[result['RequestName']]
In other words, you don't need to create redundant local variable, assign to it results and then return it. Less code, no copying, no re-assignment, less if/else block.
some of Valentins suggestions Valentins return suggestion
|
Thanks @vkuznet . I have applied the changes you suggested, but in the end it's a tradeoff of having multiple return statements spread in the method versus leaving the final return to the end of the method. |
|
Jenkins results:
|
Fixes #11158
Status
ready
Description
If data for the request name provided cannot be retrieved, return and empty object (dict) to the client. Keeping the server log clean as well.
Is it backward compatible (if not, which system it affects?)
YES
Related PRs
None
External dependencies / deployment changes
None