Remove special treatment of .json for API objects

- /v1/nodes/test.json will now only mean node with the name
"test.json"
- /v1/nodes/test.json.json will mean a node with the name
"test.json.json" and,
- /v1/nodes/test will mean a node with the name "test".

So /v1/nodes/test.json will no longer default to "test" and
will HTTP 404 unless a node with the name "test" actually exists.

This also removes the backward compatibility with the
guess_content_type_from_ext feature

Closes-Bug: #1748224

Change-Id: If4b3a23e2a09065f5e063e66cff66b96af4d3393

api-ref/source/baremetal-api-v1-nodes.inc

@@ -18,6 +18,15 @@ capable of running an Operating System. Each Node must be associated with a
   the ``node_ident``. Responses clearly indicate whether a given field is a
   ``uuid`` or a ``name``.
 
+.. versionchanged:: 1.91
+  In older API versions, we have a pecan feature enabled that strips .json
+  extensions from the end of a resource reference query and treat it as if it
+  was referenced by just its UUID or ``node_ident``. E.g.
+  ``0178-0c2c-9c26-ca69-3011-a9dd.json``, is treated as
+  ``0178-0c2c-9c26-ca69-3011-a9dd``. This feature is now disabled in newer API
+  versions.
+
+
 Depending on the Roles assigned to the authenticated OpenStack User, and upon
 the configuration of the Bare Metal service, API responses may change. For
 example, the default value of the "show_password" settings cause all API

doc/source/contributor/webapi-version-history.rst

@@ -2,6 +2,23 @@
 REST API Version History
 ========================
 
+1.91 (Dalmatian)
+-----------------------
+
+Removes special treatment of .json for API objects
+
+* ``/v1/nodes/test.json`` will now only mean node with the name
+  "test.json"
+* ``/v1/nodes/test.json``.json will mean a node with the name
+  "test.json.json" and,
+* ``/v1/nodes/test`` will mean a node with the name "test".
+
+So ``/v1/nodes/test.json`` will no longer default to "test" and
+will HTTP 404 unless a node with the name "test" actually exists.
+
+This also removes the backward compatibility with the
+``guess_content_type_from_ext`` feature
+
 1.90 (Caracal)
 -----------------------
 

ironic/api/controllers/v1/versions.py

@@ -220,6 +220,7 @@ MINOR_87_SERVICE = 87
 MINOR_88_PORT_NAME = 88
 MINOR_89_ATTACH_DETACH_VMEDIA = 89
 MINOR_90_OVN_VTEP = 90
+MINOR_91_DOT_JSON = 91
 
 # When adding another version, update:
 # - MINOR_MAX_VERSION
@@ -227,7 +228,7 @@ MINOR_90_OVN_VTEP = 90
 #   explanation of what changed in the new version
 # - common/release_mappings.py, RELEASE_MAPPING['master']['api']
 
-MINOR_MAX_VERSION = MINOR_90_OVN_VTEP
+MINOR_MAX_VERSION = MINOR_91_DOT_JSON
 
 # String representations of the minor and maximum versions
 _MIN_VERSION_STRING = '{}.{}'.format(BASE_VERSION, MINOR_1_INITIAL_VERSION)

ironic/api/middleware/json_ext.py

@@ -10,8 +10,11 @@
 # License for the specific language governing permissions and limitations
 # under the License.
 
+import microversion_parse as mvp
 from oslo_log import log
 
+from ironic.api.controllers import base
+from ironic.api.controllers.v1 import versions
 from ironic.common import utils
 
 
@@ -32,12 +35,26 @@ class JsonExtensionMiddleware(object):
 
     def __call__(self, env, start_response):
         path = utils.safe_rstrip(env.get('PATH_INFO'), '/')
+
         if path and path.endswith('.json'):
-            LOG.debug('Stripping .json prefix from %s for compatibility '
+            version_string = self.transform_header(base.Version.string)
-                      'with pecan', path)
+
-            env['PATH_INFO'] = path[:-5]
+            minor = None
-            env['HAS_JSON_SUFFIX'] = True
+            request_version = env.get(version_string, '')
+            if request_version:
+                _, minor = mvp.parse_version_string(request_version)
+
+            env['HAS_JSON_SUFFIX'] = False
+            if minor and minor < versions.MINOR_91_DOT_JSON:
+                LOG.debug('Stripping .json prefix from %s for compatibility '
+                          'with pecan', path)
+                env['PATH_INFO'] = path[:-5]
+                env['HAS_JSON_SUFFIX'] = True
         else:
             env['HAS_JSON_SUFFIX'] = False
 
         return self.app(env, start_response)
+
+    def transform_header(self, version_string):
+        """Transforms version string to HTTP header format."""
+        return 'HTTP_%s' % version_string.replace('-', '_').upper()

ironic/common/release_mappings.py

@@ -705,7 +705,7 @@ RELEASE_MAPPING = {
         }
     },
     'master': {
-        'api': '1.90',
+        'api': '1.91',
         'rpc': '1.59',
         'objects': {
             'Allocation': ['1.1'],

ironic/tests/unit/api/controllers/v1/test_allocation.py

@@ -75,12 +75,21 @@ class TestListAllocations(test_api_base.BaseApiTest):
         self.assertNotIn('node_id', data)
 
     def test_get_one_with_json(self):
+        headers = {api_base.Version.string: '1.90'}
         allocation = obj_utils.create_test_allocation(self.context,
                                                       node_id=self.node.id)
         data = self.get_json('/allocations/%s.json' % allocation.uuid,
-                             headers=self.headers)
+                             headers=headers)
         self.assertEqual(allocation.uuid, data['uuid'])
 
+    def test_get_one_with_json_not_found(self):
+        allocation = obj_utils.create_test_allocation(self.context,
+                                                      node_id=self.node.id)
+        data = self.get_json('/allocations/%s.json' % allocation.uuid,
+                             headers=self.headers,
+                             expect_errors=True)
+        self.assertEqual(http_client.NOT_FOUND, data.status_code)
+
     def test_get_one_with_json_in_name(self):
         allocation = obj_utils.create_test_allocation(self.context,
                                                       name='pg.json',
@@ -89,6 +98,14 @@ class TestListAllocations(test_api_base.BaseApiTest):
                              headers=self.headers)
         self.assertEqual(allocation.uuid, data['uuid'])
 
+    def test_get_one_with_double_json_in_name(self):
+        allocation = obj_utils.create_test_allocation(self.context,
+                                                      name='pg.json.json',
+                                                      node_id=self.node.id)
+        data = self.get_json('/allocations/%s' % allocation.uuid,
+                             headers=self.headers)
+        self.assertEqual(allocation.uuid, data['uuid'])
+
     def test_get_one_with_suffix(self):
         allocation = obj_utils.create_test_allocation(self.context,
                                                       name='pg.1',
@@ -1312,10 +1329,16 @@ class TestDelete(test_api_base.BaseApiTest):
         self.assertTrue(mock_destroy.called)
 
     def test_delete_allocation_by_name_with_json(self, mock_destroy):
+        headers = {api_base.Version.string: '1.90'}
         self.delete('/allocations/%s.json' % self.allocation.name,
-                    headers=self.headers)
+                    headers=headers)
         self.assertTrue(mock_destroy.called)
 
+    def test_delete_allocation_by_name_with_json_not_found(self, mock_destroy):
+        res = self.delete('/allocations/%s.json' % self.allocation.name,
+                          headers=self.headers, expect_errors=True)
+        self.assertEqual(http_client.NOT_FOUND, res.status_code)
+
     def test_delete_allocation_by_name_not_existed(self, mock_destroy):
         res = self.delete('/allocations/%s' % 'blah', expect_errors=True,
                           headers=self.headers)

ironic/tests/unit/api/controllers/v1/test_deploy_template.py

@@ -77,11 +77,18 @@ class TestListDeployTemplates(BaseDeployTemplatesAPITest):
             self.assertEqual(t_dict_step['priority'], t_step['priority'])
 
     def test_get_one_with_json(self):
+        headers = {api_base.Version.string: '1.90'}
         template = obj_utils.create_test_deploy_template(self.context)
         data = self.get_json('/deploy_templates/%s.json' % template.uuid,
-                             headers=self.headers)
+                             headers=headers)
         self.assertEqual(template.uuid, data['uuid'])
 
+    def test_get_one_with_json_not_found(self):
+        template = obj_utils.create_test_deploy_template(self.context)
+        response = self.get_json('/deploy_templates/%s.json' % template.uuid,
+                                 headers=self.headers, expect_errors=True)
+        self.assertEqual(http_client.NOT_FOUND, response.status_int)
+
     def test_get_one_with_suffix(self):
         template = obj_utils.create_test_deploy_template(self.context,
                                                          name='CUSTOM_DT1')
@@ -378,17 +385,30 @@ class TestPatch(BaseDeployTemplatesAPITest):
         self.assertEqual(steps, response.json['steps'])
 
     def test_update_by_name_with_json(self, mock_save):
+        headers = {api_base.Version.string: '1.90'}
         interface = 'bios'
         path = '/deploy_templates/%s.json' % self.template.name
         response = self.patch_json(path,
                                    [{'path': '/steps/0/interface',
                                      'value': interface,
                                      'op': 'replace'}],
-                                   headers=self.headers)
+                                   headers=headers)
         self.assertEqual('application/json', response.content_type)
         self.assertEqual(http_client.OK, response.status_code)
         self.assertEqual(interface, response.json['steps'][0]['interface'])
 
+    def test_update_by_name_with_json_not_found(self, mock_save):
+        interface = 'bios'
+        path = '/deploy_templates/%s.json' % self.template.name
+        response = self.patch_json(path,
+                                   [{'path': '/steps/0/interface',
+                                     'value': interface,
+                                     'op': 'replace'}],
+                                   headers=self.headers,
+                                   expect_errors=True)
+        self.assertEqual('application/json', response.content_type)
+        self.assertEqual(http_client.NOT_FOUND, response.status_code)
+
     def test_update_name_standard_trait(self, mock_save):
         name = 'HW_CPU_X86_VMX'
         patch = [{'path': '/name', 'value': name, 'op': 'replace'}]
@@ -938,20 +958,34 @@ class TestDelete(BaseDeployTemplatesAPITest):
                                       obj_fields.NotificationStatus.END)])
 
     def test_delete_by_uuid_with_json(self, mock_destroy):
+        headers = {api_base.Version.string: '1.90'}
         self.delete('/deploy_templates/%s.json' % self.template.uuid,
-                    headers=self.headers)
+                    headers=headers)
         mock_destroy.assert_called_once_with(mock.ANY)
 
+    def test_delete_by_uuid_with_json_not_found(self, mock_destroy):
+        response = self.delete('/deploy_templates/%s.json' %
+                               self.template.uuid, headers=self.headers,
+                               expect_errors=True)
+        self.assertEqual(http_client.NOT_FOUND, response.status_int)
+
     def test_delete_by_name(self, mock_destroy):
         self.delete('/deploy_templates/%s' % self.template.name,
                     headers=self.headers)
         mock_destroy.assert_called_once_with(mock.ANY)
 
     def test_delete_by_name_with_json(self, mock_destroy):
+        headers = {api_base.Version.string: '1.90'}
         self.delete('/deploy_templates/%s.json' % self.template.name,
-                    headers=self.headers)
+                    headers=headers)
         mock_destroy.assert_called_once_with(mock.ANY)
 
+    def test_delete_by_name_with_json_not_found(self, mock_destroy):
+        response = self.delete('/deploy_templates/%s.json' %
+                               self.template.name, headers=self.headers,
+                               expect_errors=True)
+        self.assertEqual(http_client.NOT_FOUND, response.status_int)
+
     def test_delete_invalid_api_version(self, mock_dpt):
         response = self.delete('/deploy_templates/%s' % self.template.uuid,
                                expect_errors=True,

ironic/tests/unit/api/controllers/v1/test_node.py

@@ -245,15 +245,6 @@ class TestListNodes(test_api_base.BaseApiTest):
         self.assertEqual('******', data['instance_info']['image_url'])
         self.assertEqual('bar', data['instance_info']['foo'])
 
-    def test_get_one_with_json(self):
-        # Test backward compatibility with guess_content_type_from_ext
-        node = obj_utils.create_test_node(self.context,
-                                          chassis_id=self.chassis.id)
-        data = self.get_json(
-            '/nodes/%s.json' % node.uuid,
-            headers={api_base.Version.string: str(api_v1.max_version())})
-        self.assertEqual(node.uuid, data['uuid'])
-
     def test_get_one_with_json_in_name(self):
         # Test that it is possible to name a node ending with .json
         node = obj_utils.create_test_node(self.context,
@@ -264,26 +255,45 @@ class TestListNodes(test_api_base.BaseApiTest):
             headers={api_base.Version.string: str(api_v1.max_version())})
         self.assertEqual(node.uuid, data['uuid'])
 
+    def test_get_one_with_double_json_in_name(self):
+        # Check that .json.json is treated as is
+        headers = {api_base.Version.string: str(api_v1.max_version())}
+        node = obj_utils.create_test_node(self.context,
+                                          name='node.json.json',
+                                          chassis_id=self.chassis.id)
+        data = self.get_json('/nodes/%s' % node.name, headers=headers)
+        self.assertEqual(node.uuid, data['uuid'])
+
+    def test_get_one_with_json(self):
+        # Test backward compatibility with guess_content_type_from_ext
+        headers = {api_base.Version.string: '1.90'}
+        node = obj_utils.create_test_node(self.context,
+                                          chassis_id=self.chassis.id)
+        data = self.get_json('/nodes/%s.json' % node.uuid, headers=headers)
+        self.assertEqual(node.uuid, data['uuid'])
+
+    def test_get_one_with_json_not_found(self):
+        # Check that appending .json extension fails in newer api
+        node = obj_utils.create_test_node(self.context,
+                                          name='node',
+                                          chassis_id=self.chassis.id)
+        data = self.get_json(
+            '/nodes/%s.json' % node.uuid,
+            headers={api_base.Version.string: str(api_v1.max_version())},
+            expect_errors=True)
+        self.assertEqual(http_client.NOT_FOUND, data.status_code)
+
     def test_get_one_with_suffix(self):
         # This tests that we don't mess with mime-like suffixes
         node = obj_utils.create_test_node(self.context,
                                           name='test.1',
                                           chassis_id=self.chassis.id)
+
         data = self.get_json(
             '/nodes/%s' % node.name,
             headers={api_base.Version.string: str(api_v1.max_version())})
         self.assertEqual(node.uuid, data['uuid'])
 
-    def test_get_one_with_double_json(self):
-        # Check that .json is only stripped once
-        node = obj_utils.create_test_node(self.context,
-                                          name='node.json',
-                                          chassis_id=self.chassis.id)
-        data = self.get_json(
-            '/nodes/%s.json' % node.name,
-            headers={api_base.Version.string: str(api_v1.max_version())})
-        self.assertEqual(node.uuid, data['uuid'])
-
     def _test_node_field_hidden_in_lower_version(self, field,
                                                  old_version, new_version):
         node = obj_utils.create_test_node(self.context)
@@ -4211,7 +4221,7 @@ class TestPatch(test_api_base.BaseApiTest):
                                           uuid=uuidutils.generate_uuid())
         mock_cmnpar.return_value = node
         self.mock_update_node.return_value = node
-        headers = {api_base.Version.string: str(api_v1.max_version())}
+        headers = {api_base.Version.string: '1.90'}
         response = self.patch_json('/nodes/%s' % node.uuid,
                                    [{'path': '/description',
                                      'value': 'foo',
@@ -4245,7 +4255,7 @@ class TestPatch(test_api_base.BaseApiTest):
                                           uuid=uuidutils.generate_uuid())
         mock_cmnpar.return_value = node
         self.mock_update_node.return_value = node
-        headers = {api_base.Version.string: str(api_v1.max_version())}
+        headers = {api_base.Version.string: '1.90'}
         response = self.patch_json('/nodes/%s' % node.uuid,
                                    [{'path': '/extra/foo',
                                      'value': 'bar',
@@ -4263,7 +4273,7 @@ class TestPatch(test_api_base.BaseApiTest):
                                           uuid=uuidutils.generate_uuid())
         mock_cmnpar.return_value = node
         self.mock_update_node.return_value = node
-        headers = {api_base.Version.string: str(api_v1.max_version())}
+        headers = {api_base.Version.string: '1.90'}
         response = self.patch_json('/nodes/%s' % node.uuid,
                                    [{'path': '/instance_info/foo',
                                      'value': 'bar',
@@ -4277,12 +4287,13 @@ class TestPatch(test_api_base.BaseApiTest):
 
     @mock.patch.object(api_utils, 'check_multiple_node_policies_and_retrieve',
                        autospec=True)
-    def test_patch_policy_update_generic_and_extra(self, mock_cmnpar):
+    def test_patch_policy_update_generic_and_extra(
+            self, mock_cmnpar):
         node = obj_utils.create_test_node(self.context,
                                           uuid=uuidutils.generate_uuid())
         mock_cmnpar.return_value = node
         self.mock_update_node.return_value = node
-        headers = {api_base.Version.string: str(api_v1.max_version())}
+        headers = {api_base.Version.string: '1.90'}
         response = self.patch_json('/nodes/%s' % node.uuid,
                                    [{'path': '/description',
                                      'value': 'foo',
@@ -4299,12 +4310,13 @@ class TestPatch(test_api_base.BaseApiTest):
 
     @mock.patch.object(api_utils, 'check_multiple_node_policies_and_retrieve',
                        autospec=True)
-    def test_patch_policy_update_generic_and_instance_info(self, mock_cmnpar):
+    def test_patch_policy_update_generic_and_instance_info(
+            self, mock_cmnpar):
         node = obj_utils.create_test_node(self.context,
                                           uuid=uuidutils.generate_uuid())
         mock_cmnpar.return_value = node
         self.mock_update_node.return_value = node
-        headers = {api_base.Version.string: str(api_v1.max_version())}
+        headers = {api_base.Version.string: '1.90'}
         response = self.patch_json('/nodes/%s' % node.uuid,
                                    [{'path': '/description',
                                      'value': 'foo',
@@ -4321,12 +4333,13 @@ class TestPatch(test_api_base.BaseApiTest):
 
     @mock.patch.object(api_utils, 'check_multiple_node_policies_and_retrieve',
                        autospec=True)
-    def test_patch_policy_update_extra_and_instance_info(self, mock_cmnpar):
+    def test_patch_policy_update_extra_and_instance_info(
+            self, mock_cmnpar):
         node = obj_utils.create_test_node(self.context,
                                           uuid=uuidutils.generate_uuid())
         mock_cmnpar.return_value = node
         self.mock_update_node.return_value = node
-        headers = {api_base.Version.string: str(api_v1.max_version())}
+        headers = {api_base.Version.string: '1.90'}
         response = self.patch_json('/nodes/%s' % node.uuid,
                                    [{'path': '/extra/foo',
                                      'value': 'bar',
@@ -4350,7 +4363,7 @@ class TestPatch(test_api_base.BaseApiTest):
                                           uuid=uuidutils.generate_uuid())
         mock_cmnpar.return_value = node
         self.mock_update_node.return_value = node
-        headers = {api_base.Version.string: str(api_v1.max_version())}
+        headers = {api_base.Version.string: '1.90'}
         response = self.patch_json('/nodes/%s' % node.uuid,
                                    [{'path': '/description',
                                      'value': 'foo',

ironic/tests/unit/api/controllers/v1/test_portgroup.py

@@ -84,10 +84,11 @@ class TestListPortgroups(test_api_base.BaseApiTest):
         self.assertNotIn('node_id', data)
 
     def test_get_one_with_json(self):
+        headers = {api_base.Version.string: '1.90'}
         portgroup = obj_utils.create_test_portgroup(self.context,
                                                     node_id=self.node.id)
         data = self.get_json('/portgroups/%s.json' % portgroup.uuid,
-                             headers=self.headers)
+                             headers=headers)
         self.assertEqual(portgroup.uuid, data['uuid'])
 
     def test_get_one_with_json_in_name(self):
@@ -98,6 +99,23 @@ class TestListPortgroups(test_api_base.BaseApiTest):
                              headers=self.headers)
         self.assertEqual(portgroup.uuid, data['uuid'])
 
+    def test_get_one_with_double_json_in_name(self):
+        portgroup = obj_utils.create_test_portgroup(self.context,
+                                                    name='pg.json.json',
+                                                    node_id=self.node.id)
+        data = self.get_json('/portgroups/%s' % portgroup.uuid,
+                             headers=self.headers)
+        self.assertEqual(portgroup.uuid, data['uuid'])
+
+    def test_get_one_with_json_not_found(self):
+        portgroup = obj_utils.create_test_portgroup(self.context,
+                                                    name='pg',
+                                                    node_id=self.node.id)
+        data = self.get_json('/portgroups/%s.json' % portgroup.uuid,
+                             headers=self.headers,
+                             expect_errors=True)
+        self.assertEqual(http_client.NOT_FOUND, data.status_code)
+
     def test_get_one_with_suffix(self):
         portgroup = obj_utils.create_test_portgroup(self.context,
                                                     name='pg.1',
@@ -637,13 +655,14 @@ class TestPatch(test_api_base.BaseApiTest):
 
     def test_update_byname_with_json(self, mock_upd):
         extra = {'foo': 'bar'}
+        headers = {api_base.Version.string: '1.90'}
         mock_upd.return_value = self.portgroup
         mock_upd.return_value.extra = extra
         response = self.patch_json('/portgroups/%s.json' % self.portgroup.name,
                                    [{'path': '/extra/foo',
                                      'value': 'bar',
                                      'op': 'add'}],
-                                   headers=self.headers)
+                                   headers=headers)
         self.assertEqual('application/json', response.content_type)
         self.assertEqual(http_client.OK, response.status_code)
         self.assertEqual(extra, response.json['extra'])
@@ -1413,8 +1432,9 @@ class TestDelete(test_api_base.BaseApiTest):
         self.assertTrue(mock_dpt.called)
 
     def test_delete_portgroup_byname_with_json(self, mock_dpt):
+        headers = {api_base.Version.string: '1.90'}
         self.delete('/portgroups/%s.json' % self.portgroup.name,
-                    headers=self.headers)
+                    headers=headers)
         self.assertTrue(mock_dpt.called)
 
     def test_delete_portgroup_byname_not_existed(self, mock_dpt):

ironic/tests/unit/api/controllers/v1/test_ramdisk.py

@@ -234,12 +234,13 @@ class TestHeartbeat(test_api_base.BaseApiTest):
 
     @mock.patch.object(rpcapi.ConductorAPI, 'heartbeat', autospec=True)
     def test_ok_with_json(self, mock_heartbeat):
+        headers = {api_base.Version.string: '1.90'}
         node = obj_utils.create_test_node(self.context)
         response = self.post_json(
             '/heartbeat/%s.json' % node.uuid,
             {'callback_url': 'url',
              'agent_token': 'maybe some magic'},
-            headers={api_base.Version.string: str(api_v1.max_version())})
+            headers=headers)
         self.assertEqual(http_client.ACCEPTED, response.status_int)
         self.assertEqual(b'', response.body)
         mock_heartbeat.assert_called_once_with(mock.ANY, mock.ANY,

ironic/tests/unit/api/test_acl_basic.yaml

@@ -40,8 +40,8 @@ public_api_v1_slash:
 
 public_api_v1_json:
   path: /v1.json
-  assert_status: 200
+  assert_status: 404
-  
+
 public_api_v1_xml:
   path: /v1.xml
   assert_status: 404

releasenotes/notes/stop-special-handling-of-json-extension-c69af7da3abaeaac.yaml

@@ -0,0 +1,12 @@
+---
+fixes:
+  - |
+    Special treatment of .json is now disabled for nodes with .json
+    extension in URL field.
+
+    See `bug 1748224 <https://bugs.launchpad.net/ironic/+bug/1748224>`_ for more details.
+upgrade:
+  - |
+    API version 1.91 removes special treatment given to URLs ending in ".json".
+    Operators desiring the previous behavior can request API version 1.90 or
+    earlier.