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

[Abhishek Koneru]

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?

-------------- next part --------------
A non-text attachment was scrubbed...
Name: pki-akoneru-98-2-Added-methods-for-providing-file-input-for-profile-r.patch
Type: text/x-patch
Size: 15123 bytes
Desc: not available
URL: <http://listman.redhat.com/archives/pki-devel/attachments/20140624/fe971a27/attachment.bin>