Fix glance V2 incorrectly implements JSON Patch'add'

According to the RFC for JSON Patch
http://tools.ietf.org/html/rfc6902#section-4.1
the 'add' operation, in the event that the property being
added already exists, should replace the existing value. For backward
compatibility We've fix it only for the latest content type.

Change-Id: Ia4be8b384f8b7f193f139f2206ceea25786b5cc3
Closes-bug: 1250158
DocImpact
Partial-bug: #1277104

glance/api/v2/images.py

@@ -164,14 +164,19 @@ class ImagesController(object):
         path = change['path']
         path_root = path[0]
         value = change['value']
+        json_schema_version = change.get('json_schema_version', 10)
         if path_root == 'locations':
             self._do_add_locations(image, path[1], value)
         else:
-            if (hasattr(image, path_root) or
-                    path_root in image.extra_properties):
+            if ((hasattr(image, path_root) or
+                    path_root in image.extra_properties)
+                    and json_schema_version == 4):
                 msg = _("Property %s already present.")
                 raise webob.exc.HTTPConflict(msg % path_root)
-            image.extra_properties[path_root] = value
+            if hasattr(image, path_root):
+                setattr(image, path_root, value)
+            else:
+                image.extra_properties[path_root] = value
 
     def _do_remove(self, req, image, change):
         path = change['path']
@@ -484,7 +489,8 @@ class RequestDeserializer(wsgi.JSONRequestDeserializer):
 
             # NOTE(zhiyan): the 'path' is a list.
             self._validate_path(op, path)
-            change = {'op': op, 'path': path}
+            change = {'op': op, 'path': path,
+                      'json_schema_version': json_schema_version}
 
             if not op == 'remove':
                 change['value'] = self._get_change_value(raw_change, op)

glance/tests/unit/v2/test_images_resource.py

@@ -1341,14 +1341,47 @@ class TestImagesController(base.IsolatedUnitTest):
         self.assertEqual(output.extra_properties['snitch'], 'golden')
         self.assertNotEqual(output.created_at, output.updated_at)
 
-    def test_update_add_base_property(self):
-        self.db.image_update(None, UUID1, {'properties': {'foo': 'bar'}})
+    def test_update_add_base_property_json_schema_version_4(self):
         request = unit_test_utils.get_fake_request()
-        changes = [{'op': 'add', 'path': ['name'], 'value': 'fedora'}]
+        changes = [{
+            'json_schema_version': 4, 'op': 'add',
+            'path': ['name'], 'value': 'fedora'
+        }]
         self.assertRaises(webob.exc.HTTPConflict, self.controller.update,
                           request, UUID1, changes)
 
-    def test_update_add_property_already_present(self):
+    def test_update_add_extra_property_json_schema_version_4(self):
+        self.db.image_update(None, UUID1, {'properties': {'foo': 'bar'}})
+        request = unit_test_utils.get_fake_request()
+        changes = [{
+            'json_schema_version': 4, 'op': 'add',
+            'path': ['foo'], 'value': 'baz'
+        }]
+        self.assertRaises(webob.exc.HTTPConflict, self.controller.update,
+                          request, UUID1, changes)
+
+    def test_update_add_base_property_json_schema_version_10(self):
+        request = unit_test_utils.get_fake_request()
+        changes = [{
+            'json_schema_version': 10, 'op': 'add',
+            'path': ['name'], 'value': 'fedora'
+        }]
+        output = self.controller.update(request, UUID1, changes)
+        self.assertEqual(output.image_id, UUID1)
+        self.assertEqual(output.name, 'fedora')
+
+    def test_update_add_extra_property_json_schema_version_10(self):
+        self.db.image_update(None, UUID1, {'properties': {'foo': 'bar'}})
+        request = unit_test_utils.get_fake_request()
+        changes = [{
+            'json_schema_version': 10, 'op': 'add',
+            'path': ['foo'], 'value': 'baz'
+        }]
+        output = self.controller.update(request, UUID1, changes)
+        self.assertEqual(output.image_id, UUID1)
+        self.assertEqual(output.extra_properties, {'foo': 'baz'})
+
+    def test_update_add_property_already_present_json_schema_version_4(self):
         request = unit_test_utils.get_fake_request()
         properties = {'foo': 'bar'}
         self.db.image_update(None, UUID1, {'properties': properties})
@@ -1357,11 +1390,28 @@ class TestImagesController(base.IsolatedUnitTest):
         self.assertEqual(output.extra_properties['foo'], 'bar')
 
         changes = [
-            {'op': 'add', 'path': ['foo'], 'value': 'baz'},
+            {'json_schema_version': 4, 'op': 'add',
+             'path': ['foo'], 'value': 'baz'},
         ]
         self.assertRaises(webob.exc.HTTPConflict,
                           self.controller.update, request, UUID1, changes)
 
+    def test_update_add_property_already_present_json_schema_version_10(self):
+        request = unit_test_utils.get_fake_request()
+        properties = {'foo': 'bar'}
+        self.db.image_update(None, UUID1, {'properties': properties})
+
+        output = self.controller.show(request, UUID1)
+        self.assertEqual(output.extra_properties['foo'], 'bar')
+
+        changes = [
+            {'json_schema_version': 10, 'op': 'add',
+             'path': ['foo'], 'value': 'baz'},
+        ]
+        output = self.controller.update(request, UUID1, changes)
+        self.assertEqual(output.image_id, UUID1)
+        self.assertEqual(output.extra_properties, {'foo': 'baz'})
+
     def test_update_add_locations(self):
         new_location = {'url': '%s/fake_location' % BASE_URI, 'metadata': {}}
         request = unit_test_utils.get_fake_request()
@@ -2081,18 +2131,28 @@ class TestImagesDeserializer(test_utils.BaseTestCase):
         request.body = jsonutils.dumps(body)
         output = self.deserializer.update(request)
         expected = {'changes': [
-            {'op': 'replace', 'path': ['name'], 'value': 'fedora'},
-            {'op': 'replace', 'path': ['tags'], 'value': ['king', 'kong']},
-            {'op': 'replace', 'path': ['foo'], 'value': 'bar'},
-            {'op': 'add', 'path': ['bebim'], 'value': 'bap'},
-            {'op': 'remove', 'path': ['sparks']},
-            {'op': 'add', 'path': ['locations', '-'],
+            {'json_schema_version': 10, 'op': 'replace',
+             'path': ['name'], 'value': 'fedora'},
+            {'json_schema_version': 10, 'op': 'replace',
+             'path': ['tags'], 'value': ['king', 'kong']},
+            {'json_schema_version': 10, 'op': 'replace',
+             'path': ['foo'], 'value': 'bar'},
+            {'json_schema_version': 10, 'op': 'add',
+             'path': ['bebim'], 'value': 'bap'},
+            {'json_schema_version': 10, 'op': 'remove',
+             'path': ['sparks']},
+            {'json_schema_version': 10, 'op': 'add',
+             'path': ['locations', '-'],
              'value': {'url': 'scheme3://path3', 'metadata': {}}},
-            {'op': 'add', 'path': ['locations', '10'],
+            {'json_schema_version': 10, 'op': 'add',
+             'path': ['locations', '10'],
              'value': {'url': 'scheme4://path4', 'metadata': {}}},
-            {'op': 'remove', 'path': ['locations', '2']},
-            {'op': 'replace', 'path': ['locations'], 'value': []},
-            {'op': 'replace', 'path': ['locations'],
+            {'json_schema_version': 10, 'op': 'remove',
+             'path': ['locations', '2']},
+            {'json_schema_version': 10, 'op': 'replace',
+             'path': ['locations'], 'value': []},
+            {'json_schema_version': 10, 'op': 'replace',
+             'path': ['locations'],
              'value': [{'url': 'scheme5://path5', 'metadata': {}},
                        {'url': 'scheme6://path6', 'metadata': {}}]},
         ]}
@@ -2119,18 +2179,26 @@ class TestImagesDeserializer(test_utils.BaseTestCase):
         request.body = jsonutils.dumps(body)
         output = self.deserializer.update(request)
         expected = {'changes': [
-            {'op': 'replace', 'path': ['name'], 'value': 'fedora'},
-            {'op': 'replace', 'path': ['tags'], 'value': ['king', 'kong']},
-            {'op': 'replace', 'path': ['foo'], 'value': 'bar'},
-            {'op': 'add', 'path': ['bebim'], 'value': 'bap'},
-            {'op': 'remove', 'path': ['sparks']},
-            {'op': 'add', 'path': ['locations', '-'],
+            {'json_schema_version': 4, 'op': 'replace',
+             'path': ['name'], 'value': 'fedora'},
+            {'json_schema_version': 4, 'op': 'replace',
+             'path': ['tags'], 'value': ['king', 'kong']},
+            {'json_schema_version': 4, 'op': 'replace',
+             'path': ['foo'], 'value': 'bar'},
+            {'json_schema_version': 4, 'op': 'add',
+             'path': ['bebim'], 'value': 'bap'},
+            {'json_schema_version': 4, 'op': 'remove', 'path': ['sparks']},
+            {'json_schema_version': 4, 'op': 'add',
+             'path': ['locations', '-'],
              'value': {'url': 'scheme3://path3', 'metadata': {}}},
-            {'op': 'add', 'path': ['locations', '10'],
+            {'json_schema_version': 4, 'op': 'add',
+             'path': ['locations', '10'],
              'value': {'url': 'scheme4://path4', 'metadata': {}}},
-            {'op': 'remove', 'path': ['locations', '2']},
-            {'op': 'replace', 'path': ['locations'], 'value': []},
-            {'op': 'replace', 'path': ['locations'],
+            {'json_schema_version': 4, 'op': 'remove',
+             'path': ['locations', '2']},
+            {'json_schema_version': 4, 'op': 'replace',
+             'path': ['locations'], 'value': []},
+            {'json_schema_version': 4, 'op': 'replace', 'path': ['locations'],
              'value': [{'url': 'scheme5://path5', 'metadata': {}},
                        {'url': 'scheme6://path6', 'metadata': {}}]},
         ]}
@@ -2156,17 +2224,27 @@ class TestImagesDeserializer(test_utils.BaseTestCase):
         request.body = jsonutils.dumps(body)
         output = self.deserializer.update(request)
         expected = {'changes': [
-            {'op': 'replace', 'path': ['id'], 'value': UUID1},
-            {'op': 'replace', 'path': ['name'], 'value': 'fedora'},
-            {'op': 'replace', 'path': ['visibility'], 'value': 'public'},
-            {'op': 'replace', 'path': ['tags'], 'value': ['king', 'kong']},
-            {'op': 'replace', 'path': ['protected'], 'value': True},
-            {'op': 'replace', 'path': ['container_format'], 'value': 'bare'},
-            {'op': 'replace', 'path': ['disk_format'], 'value': 'raw'},
-            {'op': 'replace', 'path': ['min_ram'], 'value': 128},
-            {'op': 'replace', 'path': ['min_disk'], 'value': 10},
-            {'op': 'replace', 'path': ['locations'], 'value': []},
-            {'op': 'replace', 'path': ['locations'],
+            {'json_schema_version': 10, 'op': 'replace',
+             'path': ['id'], 'value': UUID1},
+            {'json_schema_version': 10, 'op': 'replace',
+             'path': ['name'], 'value': 'fedora'},
+            {'json_schema_version': 10, 'op': 'replace',
+             'path': ['visibility'], 'value': 'public'},
+            {'json_schema_version': 10, 'op': 'replace',
+             'path': ['tags'], 'value': ['king', 'kong']},
+            {'json_schema_version': 10, 'op': 'replace',
+             'path': ['protected'], 'value': True},
+            {'json_schema_version': 10, 'op': 'replace',
+             'path': ['container_format'], 'value': 'bare'},
+            {'json_schema_version': 10, 'op': 'replace',
+             'path': ['disk_format'], 'value': 'raw'},
+            {'json_schema_version': 10, 'op': 'replace',
+             'path': ['min_ram'], 'value': 128},
+            {'json_schema_version': 10, 'op': 'replace',
+             'path': ['min_disk'], 'value': 10},
+            {'json_schema_version': 10, 'op': 'replace',
+             'path': ['locations'], 'value': []},
+            {'json_schema_version': 10, 'op': 'replace', 'path': ['locations'],
              'value': [{'url': 'scheme5://path5', 'metadata': {}},
                        {'url': 'scheme6://path6', 'metadata': {}}]}
         ]}
@@ -2491,7 +2569,8 @@ class TestImagesDeserializerWithExtendedSchema(test_utils.BaseTestCase):
         request.body = jsonutils.dumps(doc)
         output = self.deserializer.update(request)
         expected = {'changes': [
-            {'op': 'add', 'path': ['pants'], 'value': 'off'},
+            {'json_schema_version': 10, 'op': 'add',
+             'path': ['pants'], 'value': 'off'},
         ]}
         self.assertEqual(expected, output)
 
@@ -2555,7 +2634,10 @@ class TestImagesDeserializerWithAdditionalProperties(test_utils.BaseTestCase):
         doc = [{'op': 'add', 'path': '/foo', 'value': 'bar'}]
         request.body = jsonutils.dumps(doc)
         output = self.deserializer.update(request)
-        change = {'op': 'add', 'path': ['foo'], 'value': 'bar'}
+        change = {
+            'json_schema_version': 10, 'op': 'add',
+            'path': ['foo'], 'value': 'bar'
+        }
         self.assertEqual(output, {'changes': [change]})