Please review the patch with fixes for comments on patch 98. Issues addressed: 1. Added a try catch block for issue 1(as noted below) caught by ftweedal(nice catch!). 2. Made the data_format mandatory for file_input methods. 3. Added code to create a profile with PolicyOutput and PolicySets. -- Abhishek Please apply patch 97 before applying this patch. On Fri, 2014-06-20 at 12:02 +1000, Fraser Tweedale wrote:
On Thu, Jun 19, 2014 at 09:58:43AM -0400, Abhishek Koneru wrote:
Please review the attached patch which adds methods that allow users to pass the profile data in a file.
Also attached two sample xml files, original.xml and modified.xml. Place them in /tmp before running the profile.py module.
Thanks, Abhishek
Hi Abhishek,
Patch applied and __main__ example run. Seems to do what it says on the tin; the usual "haven't used it in anger" caveat ^_^. Review comments inline below.
Fraser
+ @pki.handle_exceptions() def modify_profile(self, profile_data): """ Modify an existing profile. """ - if profile_data is None: - raise ValueError("No ProfileData specified") + return self._send_profile_request(profile_data, 'modify') + + def _send_request_in_file(self, path_to_file, data_format, operation): + + if path_to_file is None: + raise ValueError("File path must be specified.") + + if data_format not in ['xml', 'json']: + raise ValueError("Unsupported data type: " + str(data_format)) + + if operation not in ['create', 'modify']: + raise ValueError("Invalid operation specified: " + str(operation)) + + data = None + with open(path_to_file) as input_file: + data = input_file.read() + + if data_format == 'xml': + self.headers['Content-type'] = 'application/xml'
1)
Overwriting self.headers['Content-type'] is problematic. For example, if the data cannot be parsed or lacks an 'id' key, an exception will be raised and the ProfileClient will be stuck with the wrong Content-Type header.
Possible solutions:
- use try/finally or a context manager to ensure the header gets reset to 'application/json' even if an exception is raised. - Modify the _put and _post methods to include an optional argument to override the content-type header. To avoid special-cases too many things, this arg could even be a dict that can be merged with the default headers, e.g.:
def _post(self, url, payload=None, headers=None): self.account_client.login() headers = dict(self.headers, **(headers or {})) r = self.connection.post(url, payload, headers, query_params) ...
Then callers can supply custom headers for a single request without potentially affecting other requests.
+ + r = None + # Sending the data to the server. + if operation == 'create': + r = self._post(self.profiles_url, data) + else: + profile_id = None + if data_format == 'xml': + profile_id = etree.fromstring(data).get('id') + else: + profile_id = json.loads(data)['id'] + + if profile_id is None: + raise ValueError('Profile Id is not specified.') + url = self.profiles_url + '/' + profile_id + r = self._put(url, data) + + # Reset the Content-type header to json(As used by other methods). + if data_format == 'xml': + self.headers['Content-type'] = 'application/json'
- url = self.profiles_url + '/' + str(profile_data.profile_id) - profile_object = json.dumps(profile_data, cls=encoder.CustomTypeEncoder, - sort_keys=True) - r = self._put(url, profile_object) return Profile.from_json(r.json())
+ @pki.handle_exceptions() + def create_profile_using_file_input(self, path_to_file, data_format="xml"): + """ + Create a new profile from a profile object stored in a file. + Acceptable data formats - json, xml. + """ + return self._send_request_in_file(path_to_file, data_format, 'create') +
2)
Default ``data_format="xml"`` makes it too easy for people to misuse the API, e.g. a path to a JSON file, but no ``data_format`` kwarg given, resulting in default "xml" being used. I suggest either making it a compulsory argument, or making its default ``None`` and, if no explicit ``data_format`` arg given, using the file extension.
+ @pki.handle_exceptions() + def modify_profile_using_file_input(self, path_to_file, data_format="xml"): + """ + Modify a profile from a profile object stored in a file. + Acceptable data formats - json, xml. + """ + return self._send_request_in_file(path_to_file, data_format, 'modify') + + @pki.handle_exceptions() def delete_profile(self, profile_id): """ Delete a profile. @@ -1185,6 +1253,23 @@ def main(): # pylint: disable-msg=W0703 except Exception as e: print str(e) + print + + # Creating a profile from file + print('Creating a profile using file input.') + print('------------------------------------') + original = profile_client.create_profile_using_file_input( + '/tmp/original.xml') + print(original) + print + + # Modifying a profile from file + print('Modifying a profile using file input.') + print('------------------------------------') + modified = profile_client.modify_profile_using_file_input( + '/tmp/modified.xml') + print(modified) + print
3)
Nit-pick: could you delete the created profile here so that the example can be run multiple times?