[Pki-devel] [PATCH] 98 Allow users to provide input in a file to crate/modify a profile

[Fraser Tweedale]

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?