Add support for location parameters in v2 commands

Currently glanceclient's v2 commands don't support modification
operations on an image's location attribute - the argparse specification
for the location attribute of the image-update command causes the image
id argument to be included in list of locations and so the command
parsing fails (because it causes the image id to appear to be missing).

Furthermore even if the 'locations' argument were to be accepted by
argparse (e.g. by changing the argument specs and using --id to specify
the image id) the command would still fail because the arguments are
passed directly to the schema which expects the value of the 'locations'
argument to be a valid dictionary (there is nobody to convert the
argument string to a python dictionary that the schema expects).

This commit adds the following location related commands to
glanceclient:
    --location-add: Add a new location to the list of image locations.
    --location-delete: Remove an existing location from the list of
        image locations.
    --location-update: Update the metadata of existing location.

The glanceclient.v2.images.Controller class has been agumented with
three new methods to support the commands listed above:
    - add_location
    - delete_locations
    - update_location

The server has not been modified, i.e. all location related API requests
are passed to the server via HTTP PATCH requests and handled by the
server's image update function.

The v2 'image' and 'shell' related tests have also been supplemented.

Note that in order to use these options the server must be first
configured to expose location related info to the clients (i.e.
'show_multiple_locations' must be set to 'True").

I also added a mailmap entry for myself.

DocImpact
Closes-bug: #1271452
Co-Author: David Koo (koofoss) <david.koo@huawei.com>

Change-Id: Id1f320af05d9344645836359758e4aa227aafc69

6 files changed, 360 insertions, 33 deletions

diff --git a/.mailmap b/.mailmap
index 04abdc4..1a348b9 100644
--- a/.mailmap
+++ b/.mailmap
@@ -1,5 +1,4 @@
-# Format is:
-# <preferred e-mail> <other e-mail 1>
-# <preferred e-mail> <other e-mail 2>
+# "man git-shortlog" for reference
 <mr.alex.meade@gmail.com> <hatboy112@yahoo.com>
 <mr.alex.meade@gmail.com> <alex.meade@rackspace.com>
+David Koo <david.koo@huawei.com> <kpublicmail@gmail.com>
diff --git a/glanceclient/common/utils.py b/glanceclient/common/utils.py
index 8993cba..6cdc364 100644
--- a/glanceclient/common/utils.py
+++ b/glanceclient/common/utils.py
@@ -325,3 +325,13 @@ def strip_version(endpoint):
     if re.match('v\d+\.?\d*', url_bits[-1]):
         endpoint = '/'.join(url_bits[:-1])
     return endpoint
+
+
+def print_image(image_obj, max_col_width=None):
+    ignore = ['self', 'access', 'file', 'schema']
+    image = dict([item for item in six.iteritems(image_obj)
+                  if item[0] not in ignore])
+    if str(max_col_width).isdigit():
+        print_dict(image, max_column_width=max_col_width)
+    else:
+        print_dict(image)
diff --git a/glanceclient/v2/images.py b/glanceclient/v2/images.py
index ab07a6b..cb57143 100644
--- a/glanceclient/v2/images.py
+++ b/glanceclient/v2/images.py
@@ -13,12 +13,14 @@
 #    License for the specific language governing permissions and limitations
 #    under the License.
 
+import json
 import six
 from six.moves.urllib import parse
 
 import warlock
 
 from glanceclient.common import utils
+from glanceclient import exc
 from glanceclient.openstack.common import strutils
 
 DEFAULT_PAGE_SIZE = 20
@@ -170,3 +172,94 @@ class Controller(object):
         # we need to fetch the image again to get a clean history. This is
         # an obvious optimization for warlock
         return self.get(image_id)
+
+    def _get_image_with_locations_or_fail(self, image_id):
+        image = self.get(image_id)
+        if getattr(image, 'locations', None) is None:
+            raise exc.HTTPBadRequest('The administrator has disabled '
+                                     'API access to image locations')
+        return image
+
+    def _send_image_update_request(self, image_id, patch_body):
+        url = '/v2/images/%s' % image_id
+        hdrs = {'Content-Type': 'application/openstack-images-v2.1-json-patch'}
+        self.http_client.raw_request('PATCH', url,
+                                     headers=hdrs,
+                                     body=json.dumps(patch_body))
+
+    def add_location(self, image_id, url, metadata):
+        """Add a new location entry to an image's list of locations.
+
+        It is an error to add a URL that is already present in the list of
+        locations.
+
+        :param image_id: ID of image to which the location is to be added.
+        :param url: URL of the location to add.
+        :param metadata: Metadata associated with the location.
+        :returns: The updated image
+        """
+        image = self._get_image_with_locations_or_fail(image_id)
+        url_list = [l['url'] for l in image.locations]
+        if url in url_list:
+            err_str = 'A location entry at %s already exists' % url
+            raise exc.HTTPConflict(err_str)
+
+        add_patch = [{'op': 'add', 'path': '/locations/-',
+                      'value': {'url': url, 'metadata': metadata}}]
+        self._send_image_update_request(image_id, add_patch)
+        return self.get(image_id)
+
+    def delete_locations(self, image_id, url_set):
+        """Remove one or more location entries of an image.
+
+        :param image_id: ID of image from which locations are to be removed.
+        :param url_set: set of URLs of location entries to remove.
+        :returns: None
+        """
+        image = self._get_image_with_locations_or_fail(image_id)
+        current_urls = [l['url'] for l in image.locations]
+
+        missing_locs = url_set.difference(set(current_urls))
+        if missing_locs:
+            raise exc.HTTPNotFound('Unknown URL(s): %s' % list(missing_locs))
+
+        # NOTE: warlock doesn't generate the most efficient patch for remove
+        # operations (it shifts everything up and deletes the tail elements) so
+        # we do it ourselves.
+        url_indices = [current_urls.index(url) for url in url_set]
+        url_indices.sort(reverse=True)
+        patches = [{'op': 'remove', 'path': '/locations/%s' % url_idx}
+                   for url_idx in url_indices]
+        self._send_image_update_request(image_id, patches)
+
+    def update_location(self, image_id, url, metadata):
+        """Update an existing location entry in an image's list of locations.
+
+        The URL specified must be already present in the image's list of
+        locations.
+
+        :param image_id: ID of image whose location is to be updated.
+        :param url: URL of the location to update.
+        :param metadata: Metadata associated with the location.
+        :returns: The updated image
+        """
+        image = self._get_image_with_locations_or_fail(image_id)
+        url_map = dict([(l['url'], l) for l in image.locations])
+        if url not in url_map:
+            raise exc.HTTPNotFound('Unknown URL: %s' % url)
+
+        if url_map[url]['metadata'] == metadata:
+            return image
+
+        # NOTE: The server (as of now) doesn't support modifying individual
+        # location entries. So we must:
+        #   1. Empty existing list of locations.
+        #   2. Send another request to set 'locations' to the new list
+        #      of locations.
+        url_map[url]['metadata'] = metadata
+        patches = [{'op': 'replace',
+                    'path': '/locations',
+                    'value': p} for p in ([], list(url_map.values()))]
+        self._send_image_update_request(image_id, patches)
+
+        return self.get(image_id)
diff --git a/glanceclient/v2/shell.py b/glanceclient/v2/shell.py
index 3515bf3..cc9139d 100644
--- a/glanceclient/v2/shell.py
+++ b/glanceclient/v2/shell.py
@@ -19,7 +19,6 @@ from glanceclient import exc
 import json
 import os
 from os.path import expanduser
-import six
 
 IMAGE_SCHEMA = None
 
@@ -54,14 +53,11 @@ def do_image_create(gc, args):
         fields[key] = value
 
     image = gc.images.create(**fields)
-    ignore = ['self', 'access', 'file', 'schema']
-    image = dict([item for item in six.iteritems(image)
-                  if item[0] not in ignore])
-    utils.print_dict(image)
+    utils.print_image(image)
 
 
 @utils.arg('id', metavar='<IMAGE_ID>', help='ID of image to update.')
-@utils.schema_args(get_image_schema, omit=['id'])
+@utils.schema_args(get_image_schema, omit=['id', 'locations'])
 @utils.arg('--property', metavar="<key=value>", action='append',
            default=[], help=('Arbitrary property to associate with image.'
                              ' May be used multiple times.'))
@@ -85,10 +81,7 @@ def do_image_update(gc, args):
 
     image_id = fields.pop('id')
     image = gc.images.update(image_id, remove_properties, **fields)
-    ignore = ['self', 'access', 'file', 'schema']
-    image = dict([item for item in six.iteritems(image)
-                  if item[0] not in ignore])
-    utils.print_dict(image)
+    utils.print_image(image)
 
 
 @utils.arg('--page-size', metavar='<SIZE>', default=None, type=int,
@@ -125,9 +118,7 @@ def do_image_show(gc, args):
     """Describe a specific image."""
     image = gc.images.get(args.id)
     ignore = ['self', 'access', 'file', 'schema']
-    image = dict([item for item in six.iteritems(image) if item[0] not in
-                  ignore])
-    utils.print_dict(image, max_column_width=int(args.max_column_width))
+    utils.print_image(image, int(args.max_column_width))
 
 
 @utils.arg('--image-id', metavar='<IMAGE_ID>', required=True,
@@ -263,3 +254,48 @@ def do_image_tag_delete(gc, args):
         utils.exit('Unable to delete tag. Specify image_id and tag_value')
     else:
         gc.image_tags.delete(args.image_id, args.tag_value)
+
+
+@utils.arg('--url', metavar='<URL>', required=True,
+           help='URL of location to add.')
+@utils.arg('--metadata', metavar='<STRING>', default='{}',
+           help=('Metadata associated with the location. '
+                 'Must be a valid JSON object (default: %(default)s)'))
+@utils.arg('id', metavar='<ID>',
+           help='ID of image to which the location is to be added.')
+def do_location_add(gc, args):
+    """Add a location (and related metadata) to an image."""
+    try:
+        metadata = json.loads(args.metadata)
+    except ValueError:
+        utils.exit('Metadata is not a valid JSON object.')
+    else:
+        image = gc.images.add_location(args.id, args.url, metadata)
+        utils.print_dict(image)
+
+
+@utils.arg('--url', metavar='<URL>', action='append', required=True,
+           help='URL of location to remove. May be used multiple times.')
+@utils.arg('id', metavar='<ID>',
+           help='ID of image whose locations are to be removed.')
+def do_location_delete(gc, args):
+    """Remove locations (and related metadata) from an image."""
+    image = gc.images.delete_locations(args.id, set(args.url))
+
+
+@utils.arg('--url', metavar='<URL>', required=True,
+           help='URL of location to update.')
+@utils.arg('--metadata', metavar='<STRING>', default='{}',
+           help=('Metadata associated with the location. '
+                 'Must be a valid JSON object (default: %(default)s)'))
+@utils.arg('id', metavar='<ID>',
+           help='ID of image whose location is to be updated.')
+def do_location_update(gc, args):
+    """Update metadata of an image's location."""
+    try:
+        metadata = json.loads(args.metadata)
+    except ValueError:
+        utils.exit('Metadata is not a valid JSON object.')
+    else:
+        image = gc.images.update_location(args.id, args.url, metadata)
+        utils.print_dict(image)
diff --git a/tests/v2/test_images.py b/tests/v2/test_images.py
index b76267c..149e096 100644
--- a/tests/v2/test_images.py
+++ b/tests/v2/test_images.py
@@ -14,11 +14,13 @@
 #    under the License.
 
 import errno
+import json
 import testtools
 
 import six
 import warlock
 
+from glanceclient import exc
 from glanceclient.v2 import images
 from tests import utils
 
@@ -303,12 +305,43 @@ fixtures = {
             {'images': []},
         ),
     },
+    '/v2/images/a2b83adc-888e-11e3-8872-78acc0b951d8': {
+        'GET': (
+            {},
+            {
+                'id': 'a2b83adc-888e-11e3-8872-78acc0b951d8',
+                'name': 'image-location-tests',
+                'locations': [{u'url': u'http://foo.com/',
+                               u'metadata': {u'foo': u'foometa'}},
+                              {u'url': u'http://bar.com/',
+                               u'metadata': {u'bar': u'barmeta'}}],
+            },
+        ),
+        'PATCH': (
+            {},
+            '',
+        )
+    },
 }
 
 
 fake_schema = {
     'name': 'image',
-    'properties': {'id': {}, 'name': {}},
+    'properties': {
+        'id': {},
+        'name': {},
+        'locations': {
+            'type': 'array',
+            'items': {
+                'type': 'object',
+                'properties': {
+                    'metadata': {'type': 'object'},
+                    'url': {'type': 'string'},
+                },
+                'required': ['url', 'metadata'],
+            },
+        },
+    },
     'additionalProperties': {'type': 'string'}
 }
 FakeModel = warlock.model_factory(fake_schema)
@@ -626,3 +659,105 @@ class TestController(testtools.TestCase):
         params = {'name': 'pong', 'bad_prop': False}
         with testtools.ExpectedException(TypeError):
             self.controller.update(image_id, **params)
+
+    def test_location_ops_when_server_disabled_location_ops(self):
+        # Location operations should not be allowed if server has not
+        # enabled location related operations
+        image_id = '3a4560a1-e585-443e-9b39-553b46ec92d1'
+        estr = 'The administrator has disabled API access to image locations'
+        url = 'http://bar.com/'
+        meta = {'bar': 'barmeta'}
+
+        e = self.assertRaises(exc.HTTPBadRequest,
+                              self.controller.add_location,
+                              image_id, url, meta)
+        self.assertTrue(estr in str(e))
+
+        e = self.assertRaises(exc.HTTPBadRequest,
+                              self.controller.delete_locations,
+                              image_id, set([url]))
+        self.assertTrue(estr in str(e))
+
+        e = self.assertRaises(exc.HTTPBadRequest,
+                              self.controller.update_location,
+                              image_id, url, meta)
+        self.assertTrue(estr in str(e))
+
+    def _empty_get(self, image_id):
+        return ('GET', '/v2/images/%s' % image_id, {}, None)
+
+    def _patch_req(self, image_id, patch_body):
+        c_type = 'application/openstack-images-v2.1-json-patch'
+        return ('PATCH',
+                '/v2/images/%s' % image_id,
+                {'Content-Type': c_type},
+                json.dumps(patch_body))
+
+    def test_add_location(self):
+        image_id = 'a2b83adc-888e-11e3-8872-78acc0b951d8'
+        new_loc = {'url': 'http://spam.com/', 'metadata': {'spam': 'ham'}}
+        add_patch = {'path': '/locations/-', 'value': new_loc, 'op': 'add'}
+        image = self.controller.add_location(image_id, **new_loc)
+        self.assertEqual(self.api.calls, [
+            self._empty_get(image_id),
+            self._patch_req(image_id, [add_patch]),
+            self._empty_get(image_id)
+        ])
+
+    def test_add_duplicate_location(self):
+        image_id = 'a2b83adc-888e-11e3-8872-78acc0b951d8'
+        new_loc = {'url': 'http://foo.com/', 'metadata': {'foo': 'newfoo'}}
+        err_str = 'A location entry at %s already exists' % new_loc['url']
+
+        err = self.assertRaises(exc.HTTPConflict,
+                                self.controller.add_location,
+                                image_id, **new_loc)
+        self.assertIn(err_str, str(err))
+
+    def test_remove_location(self):
+        image_id = 'a2b83adc-888e-11e3-8872-78acc0b951d8'
+        url_set = set(['http://foo.com/', 'http://bar.com/'])
+        del_patches = [{'path': '/locations/1', 'op': 'remove'},
+                       {'path': '/locations/0', 'op': 'remove'}]
+        image = self.controller.delete_locations(image_id, url_set)
+        self.assertEqual(self.api.calls, [
+            self._empty_get(image_id),
+            self._patch_req(image_id, del_patches)
+        ])
+
+    def test_remove_missing_location(self):
+        image_id = 'a2b83adc-888e-11e3-8872-78acc0b951d8'
+        url_set = set(['http://spam.ham/'])
+        err_str = 'Unknown URL(s): %s' % list(url_set)
+
+        err = self.assertRaises(exc.HTTPNotFound,
+                                self.controller.delete_locations,
+                                image_id, url_set)
+        self.assertTrue(err_str in str(err))
+
+    def test_update_location(self):
+        image_id = 'a2b83adc-888e-11e3-8872-78acc0b951d8'
+        new_loc = {'url': 'http://foo.com/', 'metadata': {'spam': 'ham'}}
+        fixture_idx = '/v2/images/%s' % (image_id)
+        orig_locations = fixtures[fixture_idx]['GET'][1]['locations']
+        loc_map = dict([(l['url'], l) for l in orig_locations])
+        loc_map[new_loc['url']] = new_loc
+        mod_patch = [{'path': '/locations', 'op': 'replace',
+                      'value': []},
+                     {'path': '/locations', 'op': 'replace',
+                      'value': list(loc_map.values())}]
+        image = self.controller.update_location(image_id, **new_loc)
+        self.assertEqual(self.api.calls, [
+            self._empty_get(image_id),
+            self._patch_req(image_id, mod_patch),
+            self._empty_get(image_id)
+        ])
+
+    def test_update_missing_location(self):
+        image_id = 'a2b83adc-888e-11e3-8872-78acc0b951d8'
+        new_loc = {'url': 'http://spam.com/', 'metadata': {'spam': 'ham'}}
+        err_str = 'Unknown URL: %s' % new_loc['url']
+        err = self.assertRaises(exc.HTTPNotFound,
+                                self.controller.update_location,
+                                image_id, **new_loc)
+        self.assertTrue(err_str in str(err))
diff --git a/tests/v2/test_shell_v2.py b/tests/v2/test_shell_v2.py
index ff14470..0c70b8b 100644
--- a/tests/v2/test_shell_v2.py
+++ b/tests/v2/test_shell_v2.py
@@ -14,6 +14,7 @@
 #    License for the specific language governing permissions and limitations
 #    under the License.
 
+import json
 import mock
 import six
 import testtools
@@ -53,7 +54,7 @@ class ShellV2Test(testtools.TestCase):
         utils.print_dict = mock.Mock()
         utils.save_image = mock.Mock()
 
-    def _test_with_few_arguments(self, func, func_args, err_msg):
+    def assert_exits_with_msg(self, func, func_args, err_msg):
         with mock.patch.object(utils, 'exit') as mocked_utils_exit:
             mocked_utils_exit.return_value = '%s' % err_msg
 
@@ -207,6 +208,59 @@ class ShellV2Test(testtools.TestCase):
             utils.print_dict.assert_called_once_with({
                 'id': 'pass', 'name': 'IMG-01', 'disk_format': 'vhd'})
 
+    def test_do_location_add_update_with_invalid_json_metadata(self):
+        args = self._make_args({'id': 'pass',
+                                'url': 'http://foo/bar',
+                                'metadata': '{1, 2, 3}'})
+        self.assert_exits_with_msg(test_shell.do_location_add,
+                                   args,
+                                   'Metadata is not a valid JSON object.')
+        self.assert_exits_with_msg(test_shell.do_location_update,
+                                   args,
+                                   'Metadata is not a valid JSON object.')
+
+    def test_do_location_add(self):
+        gc = self.gc
+        loc = {'url': 'http://foo.com/', 'metadata': {'foo': 'bar'}}
+        args = self._make_args({'id': 'pass',
+                                'url': loc['url'],
+                                'metadata': json.dumps(loc['metadata'])})
+        with mock.patch.object(gc.images, 'add_location') as mocked_addloc:
+            expect_image = {'id': 'pass', 'locations': [loc]}
+            mocked_addloc.return_value = expect_image
+
+            test_shell.do_location_add(self.gc, args)
+            mocked_addloc.assert_called_once_with('pass',
+                                                  loc['url'],
+                                                  loc['metadata'])
+            utils.print_dict.assert_called_once_with(expect_image)
+
+    def test_do_location_delete(self):
+        gc = self.gc
+        loc_set = set(['http://foo/bar', 'http://spam/ham'])
+        args = self._make_args({'id': 'pass', 'url': loc_set})
+
+        with mock.patch.object(gc.images, 'delete_locations') as mocked_rmloc:
+            expect_image = {'id': 'pass', 'locations': []}
+            test_shell.do_location_delete(self.gc, args)
+            mocked_rmloc.assert_called_once_with('pass', loc_set)
+
+    def test_do_location_update(self):
+        gc = self.gc
+        loc = {'url': 'http://foo.com/', 'metadata': {'foo': 'bar'}}
+        args = self._make_args({'id': 'pass',
+                                'url': loc['url'],
+                                'metadata': json.dumps(loc['metadata'])})
+        with mock.patch.object(gc.images, 'update_location') as mocked_modloc:
+            expect_image = {'id': 'pass', 'locations': [loc]}
+            mocked_modloc.return_value = expect_image
+
+            test_shell.do_location_update(self.gc, args)
+            mocked_modloc.assert_called_once_with('pass',
+                                                  loc['url'],
+                                                  loc['metadata'])
+            utils.print_dict.assert_called_once_with(expect_image)
+
     def test_do_explain(self):
         input = {
             'page_size': 18,
@@ -292,9 +346,9 @@ class ShellV2Test(testtools.TestCase):
         args = self._make_args({'image_id': None, 'member_id': 'MEM-01'})
         msg = 'Unable to create member. Specify image_id and member_id'
 
-        self._test_with_few_arguments(func=test_shell.do_member_create,
-                                      func_args=args,
-                                      err_msg=msg)
+        self.assert_exits_with_msg(func=test_shell.do_member_create,
+                                   func_args=args,
+                                   err_msg=msg)
 
     def test_do_member_update(self):
         input = {
@@ -322,9 +376,9 @@ class ShellV2Test(testtools.TestCase):
         msg = 'Unable to update member. Specify image_id, member_id' \
               ' and member_status'
 
-        self._test_with_few_arguments(func=test_shell.do_member_update,
-                                      func_args=args,
-                                      err_msg=msg)
+        self.assert_exits_with_msg(func=test_shell.do_member_update,
+                                   func_args=args,
+                                   err_msg=msg)
 
     def test_do_member_delete(self):
         args = self._make_args({'image_id': 'IMG-01', 'member_id': 'MEM-01'})
@@ -337,9 +391,9 @@ class ShellV2Test(testtools.TestCase):
         args = self._make_args({'image_id': None, 'member_id': 'MEM-01'})
         msg = 'Unable to delete member. Specify image_id and member_id'
 
-        self._test_with_few_arguments(func=test_shell.do_member_delete,
-                                      func_args=args,
-                                      err_msg=msg)
+        self.assert_exits_with_msg(func=test_shell.do_member_delete,
+                                   func_args=args,
+                                   err_msg=msg)
 
     def test_image_tag_update(self):
         args = self._make_args({'image_id': 'IMG-01', 'tag_value': 'tag01'})
@@ -355,9 +409,9 @@ class ShellV2Test(testtools.TestCase):
         args = self._make_args({'image_id': None, 'tag_value': 'tag01'})
         msg = 'Unable to update tag. Specify image_id and tag_value'
 
-        self._test_with_few_arguments(func=test_shell.do_image_tag_update,
-                                      func_args=args,
-                                      err_msg=msg)
+        self.assert_exits_with_msg(func=test_shell.do_image_tag_update,
+                                   func_args=args,
+                                   err_msg=msg)
 
     def test_image_tag_delete(self):
         args = self._make_args({'image_id': 'IMG-01', 'tag_value': 'tag01'})
@@ -372,6 +426,6 @@ class ShellV2Test(testtools.TestCase):
         args = self._make_args({'image_id': 'IMG-01', 'tag_value': None})
         msg = 'Unable to delete tag. Specify image_id and tag_value'
 
-        self._test_with_few_arguments(func=test_shell.do_image_tag_delete,
-                                      func_args=args,
-                                      err_msg=msg)
+        self.assert_exits_with_msg(func=test_shell.do_image_tag_delete,
+                                   func_args=args,
+                                   err_msg=msg)