[bugfix] Fix sanitize function for nd_rest module (DCNE-684)#210
[bugfix] Fix sanitize function for nd_rest module (DCNE-684)#210gmicol wants to merge 2 commits intoCiscoDevNet:masterfrom
Conversation
…when The API-returned object to sanitize is None.
gmicol
requested review from
akinross,
anvitha-jain,
lhercot,
sajagana,
samiib and
shrsr
as code owners
plugins/module_utils/nd.py
Outdated
| """Clean up a Python object of type list or dict from specific keys, values and None values if specified""" | ||
| if isinstance(obj_to_sanitize, dict): | ||
| if obj_to_sanitize is None: | ||
| return {} |
There was a problem hiding this comment.
Why determine to return a dict and not a list because the function cleans list or dict? Why not return None in this case?
There was a problem hiding this comment.
None makes definitely more sense. Changed it
There was a problem hiding this comment.
@gmicol I checked the modules to see where sanitize() is being used and looks like nd_rest is the only module using it so far. However, In nd_rest could you please verify whether we could replace:
if sanitize(nd.result["jsondata"], ND_REST_KEYS_TO_SANITIZE) != nd.previous and method != "GET":
nd.result["changed"] = True
with
if nd.existing != nd.previous and method != "GET":
nd.result["changed"] = True
| """Clean up a Python object of type list or dict from specific keys, values and None values if specified""" | ||
| if isinstance(obj_to_sanitize, dict): | ||
| if obj_to_sanitize is None: | ||
| return None |
There was a problem hiding this comment.
Although None makes sense in isolation here, I tend to agree with the initial change of returning {}. This is based on how sanitize is used in nd_rest.
Since existing & previous all default to dict() there might be issues related to comparison of {} != None.
It could surface issues like always setting changed=True on line 279 in nd_rest.py for certain cases. Although it might only be an issue for edge cases.
There was a problem hiding this comment.
I tend to disagree with that statement. I think nd_rest should return exactly what the API is returning: If a POST or PUT request has been made and the API returns no content, we should return None as well.
If we go back to the initial change of returning to an empty dict, we face the opposite issue of the one you are mentioning: changed will always be equal to False on line 279 if the API returns None even though potential changes have been pushed.
it's true that might be issues related to comparison of {} != None. However, I think there would just a few of edge cases compared to the opposite scenario. I would rather indicate to our users that a change has potentially been made rather than the opposite.
But it raises an issue on how we want the nd_rest module to behave when indicating a change, knowing that the API is very inconsistent.
There was a problem hiding this comment.
There are places we adjust what the API returns for usability. Example in other modules that have comma separated string properties which get converted to lists. Or error messages that get additional validation. Since nd_rest is simply a passthrough for the API we just need to make sure the module clearly shows what the API is returning. I can see changed being false is also a potential usability issue aswell.
Does a None in the existing/previous results returned as explicit null or get omitted?
| def sanitize(obj_to_sanitize, keys=None, values=None, recursive=True, remove_none_values=True): | ||
| """Clean up a Python object of type list or dict from specific keys, values and None values if specified""" | ||
| if isinstance(obj_to_sanitize, dict): | ||
| if obj_to_sanitize is None: |
There was a problem hiding this comment.
I can see an opportunity to add tests for this to increase test coverage of nd_rest.
6 participants
fix #181
The
sanitizefunction will now return an empty dict when theobj_to_sanitizeis None: Deals with cases where the API returns None.