Add support for XML file upload pre-change (DCNE-150) #77
Conversation
akinross
requested review from
lhercot,
shrsr,
akinross,
anvitha-jain,
samiib and
gmicol
plugins/modules/nd_pcv.py
Outdated
| tree = ndi.construct_tree(extract_data) | ||
| ndi.create_structured_data(tree, file) | ||
| # Check if file extension is XML. If XML, skip JSON validation/transformation | ||
| if not file.endswith(".xml"): |
There was a problem hiding this comment.
Should we base the content validation on the file extension?
I think we could achieve a nicer user experience by catching the JSON exception and checking if it is an actual XML document.
What do you think?
There was a problem hiding this comment.
It's another option, indeed. I am not sure how common it is for customers to use an XML file without the proper extension, but of course that would be a more solid experience. Any ideas how to check if it's a valid XML file, other than checking the content for XML tags?
There was a problem hiding this comment.
Think there are more ways of achieving this but I would say either lxml (https://lxml.de/validation.html) or xmlschema (https://xmlschema.readthedocs.io/en/latest/usage.html#validation)
samiib
changed the title
akinross
force-pushed
the
Fix-76-nd_pcv-fails-when-file-is-XML
branch
from
e8568bb to
26b37aa
Compare
|
Have modified the code according to prev conversation. Now the module is able to detect the type of file and deal with no extension or wrong extension files. |
akinross
requested review from
akinross,
lhercot,
shrsr,
anvitha-jain,
samiib and
gmicol
| """This method handles file download and upload operations | ||
| :arg method (str): Method can be GET or POST | ||
| :arg path (str): Path should be the resource path | ||
| :arg file (str): The absolute file path of the target file | ||
| :arg file_ext (str): The file extension, with leading dot, to be used for the file. If file has already an extension, it will be replaced |
There was a problem hiding this comment.
should we add a check that the file extension leads with a dot? possibly error or inject when this is not done.
There was a problem hiding this comment.
Sounds good, added
plugins/httpapi/nd.py
Outdated
| @@ -254,6 +255,14 @@ def send_file_request(self, method, path, file=None, data=None, remote_path=None | |||
| self.method = "GET" | |||
| if method is not None: | |||
| self.method = method | |||
|
|
|||
There was a problem hiding this comment.
Please run black -l 159 to format these changes.
plugins/modules/nd_pcv.py
Outdated
| try: | ||
| file_ext = ".json" | ||
| extract_data = ndi.load(open(file)) | ||
| except: |
There was a problem hiding this comment.
| except: | |
| except BaseException: |
I think this will also fix another sanity issue.
plugins/modules/nd_pcv.py
Outdated
| @@ -43,7 +43,7 @@ | |||
| aliases: [ site ] | |||
| file: | |||
| description: | |||
| - Optional parameter if creating new pre-change analysis from file. | |||
| - Optional parameter if creating new pre-change analysis from file. XML and JSON files are supported. If no file extension is provided, the file is assumed to be JSON. | |||
There was a problem hiding this comment.
| - Optional parameter if creating new pre-change analysis from file. XML and JSON files are supported. If no file extension is provided, the file is assumed to be JSON. | |
| - Optional parameter if creating new pre-change analysis from file. | |
| - XML and JSON files are supported. If no file extension is provided, the file is assumed to be JSON. |
To fix the long line sanity issue.
fixes #76