[Pki-devel] [PATCH] 98-2 Fixes for comments on patch 98

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?