From 0d1743a83d3a4a495847e7544773c57e3ec5f670 Mon Sep 17 00:00:00 2001 From: ghanshyam Date: Sun, 19 Nov 2017 16:15:39 +0300 Subject: [PATCH] Implement query param schema for volume, snapshot API volume & snapshot API accept query param to filter volumes, snapshots. This commit adds json schema to validating the valid query parameters. There is no change in API behaviour and additionalProperty is kept True for backward compatibility. Partially implements blueprint json-schema-validation-for-query-param Change-Id: I9e91ddabb2e62440f04ab3aac3b7e96a9fdd59dc --- nova/api/openstack/compute/schemas/volumes.py | 17 +++ nova/api/openstack/compute/volumes.py | 5 + .../api/openstack/compute/test_snapshots.py | 80 +++++++++++ .../api/openstack/compute/test_volumes.py | 130 ++++++++++++++++++ 4 files changed, 232 insertions(+) diff --git a/nova/api/openstack/compute/schemas/volumes.py b/nova/api/openstack/compute/schemas/volumes.py index b08ea5f8e51c..36bc1704b807 100644 --- a/nova/api/openstack/compute/schemas/volumes.py +++ b/nova/api/openstack/compute/schemas/volumes.py @@ -90,3 +90,20 @@ create_volume_attachment_v249['properties']['volumeAttachment'][ update_volume_attachment = copy.deepcopy(create_volume_attachment) del update_volume_attachment['properties']['volumeAttachment'][ 'properties']['device'] + +index_query = { + 'type': 'object', + 'properties': { + 'limit': parameter_types.multi_params( + parameter_types.non_negative_integer), + 'offset': parameter_types.multi_params( + parameter_types.non_negative_integer) + }, + # NOTE(gmann): This is kept True to keep backward compatibility. + # As of now Schema validation stripped out the additional parameters and + # does not raise 400. In the future, we may block the additional parameters + # by bump in Microversion. + 'additionalProperties': True +} + +detail_query = index_query diff --git a/nova/api/openstack/compute/volumes.py b/nova/api/openstack/compute/volumes.py index 190110ecb9e8..a1ada8692290 100644 --- a/nova/api/openstack/compute/volumes.py +++ b/nova/api/openstack/compute/volumes.py @@ -132,12 +132,14 @@ class VolumeController(wsgi.Controller): raise exc.HTTPNotFound(explanation=e.format_message()) @wsgi.Controller.api_version("2.1", MAX_PROXY_API_SUPPORT_VERSION) + @validation.query_schema(volumes_schema.index_query) @extensions.expected_errors(()) def index(self, req): """Returns a summary list of volumes.""" return self._items(req, entity_maker=_translate_volume_summary_view) @wsgi.Controller.api_version("2.1", MAX_PROXY_API_SUPPORT_VERSION) + @validation.query_schema(volumes_schema.detail_query) @extensions.expected_errors(()) def detail(self, req): """Returns a detailed list of volumes.""" @@ -262,6 +264,7 @@ class VolumeAttachmentController(wsgi.Controller): super(VolumeAttachmentController, self).__init__() @extensions.expected_errors(404) + @validation.query_schema(volumes_schema.index_query) def index(self, req, server_id): """Returns the list of volume attachments for a given instance.""" context = req.environ['nova.context'] @@ -501,12 +504,14 @@ class SnapshotController(wsgi.Controller): raise exc.HTTPNotFound(explanation=e.format_message()) @wsgi.Controller.api_version("2.1", MAX_PROXY_API_SUPPORT_VERSION) + @validation.query_schema(volumes_schema.index_query) @extensions.expected_errors(()) def index(self, req): """Returns a summary list of snapshots.""" return self._items(req, entity_maker=_translate_snapshot_summary_view) @wsgi.Controller.api_version("2.1", MAX_PROXY_API_SUPPORT_VERSION) + @validation.query_schema(volumes_schema.detail_query) @extensions.expected_errors(()) def detail(self, req): """Returns a detailed list of snapshots.""" diff --git a/nova/tests/unit/api/openstack/compute/test_snapshots.py b/nova/tests/unit/api/openstack/compute/test_snapshots.py index 9b87d5f8917e..077af38b14d4 100644 --- a/nova/tests/unit/api/openstack/compute/test_snapshots.py +++ b/nova/tests/unit/api/openstack/compute/test_snapshots.py @@ -139,6 +139,86 @@ class SnapshotApiTestV21(test.NoDBTestCase): resp_snapshots = resp_dict['snapshots'] self.assertEqual(1, len(resp_snapshots)) + def _test_list_with_invalid_filter(self, url): + prefix = '/os-snapshots' + req = fakes.HTTPRequest.blank(prefix + url) + controller_list = self.controller.index + if 'detail' in url: + controller_list = self.controller.detail + self.assertRaises(exception.ValidationError, + controller_list, req) + + def test_list_with_invalid_non_int_limit(self): + self._test_list_with_invalid_filter('?limit=-9') + + def test_list_with_invalid_string_limit(self): + self._test_list_with_invalid_filter('?limit=abc') + + def test_list_duplicate_query_with_invalid_string_limit(self): + self._test_list_with_invalid_filter( + '?limit=1&limit=abc') + + def test_detail_list_with_invalid_non_int_limit(self): + self._test_list_with_invalid_filter('/detail?limit=-9') + + def test_detail_list_with_invalid_string_limit(self): + self._test_list_with_invalid_filter('/detail?limit=abc') + + def test_detail_list_duplicate_query_with_invalid_string_limit(self): + self._test_list_with_invalid_filter( + '/detail?limit=1&limit=abc') + + def test_list_with_invalid_non_int_offset(self): + self._test_list_with_invalid_filter('?offset=-9') + + def test_list_with_invalid_string_offset(self): + self._test_list_with_invalid_filter('?offset=abc') + + def test_list_duplicate_query_with_invalid_string_offset(self): + self._test_list_with_invalid_filter( + '?offset=1&offset=abc') + + def test_detail_list_with_invalid_non_int_offset(self): + self._test_list_with_invalid_filter('/detail?offset=-9') + + def test_detail_list_with_invalid_string_offset(self): + self._test_list_with_invalid_filter('/detail?offset=abc') + + def test_detail_list_duplicate_query_with_invalid_string_offset(self): + self._test_list_with_invalid_filter( + '/detail?offset=1&offset=abc') + + def _test_list_duplicate_query_parameters_validation(self, url): + params = { + 'limit': 1, + 'offset': 1 + } + controller_list = self.controller.index + if 'detail' in url: + controller_list = self.controller.detail + for param, value in params.items(): + req = fakes.HTTPRequest.blank( + url + '?%s=%s&%s=%s' % + (param, value, param, value)) + controller_list(req) + + def test_list_duplicate_query_parameters_validation(self): + self._test_list_duplicate_query_parameters_validation('/os-snapshots') + + def test_detail_list_duplicate_query_parameters_validation(self): + self._test_list_duplicate_query_parameters_validation( + '/os-snapshots/detail') + + def test_list_with_additional_filter(self): + req = fakes.HTTPRequest.blank( + '/os-snapshots?limit=1&offset=1&additional=something') + self.controller.index(req) + + def test_detail_list_with_additional_filter(self): + req = fakes.HTTPRequest.blank( + '/os-snapshots/detail?limit=1&offset=1&additional=something') + self.controller.detail(req) + class TestSnapshotAPIDeprecation(test.NoDBTestCase): diff --git a/nova/tests/unit/api/openstack/compute/test_volumes.py b/nova/tests/unit/api/openstack/compute/test_volumes.py index 84c69d11ddbd..f9ac90f0a814 100644 --- a/nova/tests/unit/api/openstack/compute/test_volumes.py +++ b/nova/tests/unit/api/openstack/compute/test_volumes.py @@ -338,6 +338,84 @@ class VolumeApiTestV21(test.NoDBTestCase): self.assertIn('Volume 456 could not be found.', encodeutils.safe_decode(resp.body)) + def _test_list_with_invalid_filter(self, url): + prefix = '/os-volumes' + req = fakes.HTTPRequest.blank(prefix + url) + self.assertRaises(exception.ValidationError, + volumes_v21.VolumeController().index, + req) + + def test_list_with_invalid_non_int_limit(self): + self._test_list_with_invalid_filter('?limit=-9') + + def test_list_with_invalid_string_limit(self): + self._test_list_with_invalid_filter('?limit=abc') + + def test_list_duplicate_query_with_invalid_string_limit(self): + self._test_list_with_invalid_filter( + '?limit=1&limit=abc') + + def test_detail_list_with_invalid_non_int_limit(self): + self._test_list_with_invalid_filter('/detail?limit=-9') + + def test_detail_list_with_invalid_string_limit(self): + self._test_list_with_invalid_filter('/detail?limit=abc') + + def test_detail_list_duplicate_query_with_invalid_string_limit(self): + self._test_list_with_invalid_filter( + '/detail?limit=1&limit=abc') + + def test_list_with_invalid_non_int_offset(self): + self._test_list_with_invalid_filter('?offset=-9') + + def test_list_with_invalid_string_offset(self): + self._test_list_with_invalid_filter('?offset=abc') + + def test_list_duplicate_query_with_invalid_string_offset(self): + self._test_list_with_invalid_filter( + '?offset=1&offset=abc') + + def test_detail_list_with_invalid_non_int_offset(self): + self._test_list_with_invalid_filter('/detail?offset=-9') + + def test_detail_list_with_invalid_string_offset(self): + self._test_list_with_invalid_filter('/detail?offset=abc') + + def test_detail_list_duplicate_query_with_invalid_string_offset(self): + self._test_list_with_invalid_filter( + '/detail?offset=1&offset=abc') + + def _test_list_duplicate_query_parameters_validation(self, url): + params = { + 'limit': 1, + 'offset': 1 + } + for param, value in params.items(): + req = fakes.HTTPRequest.blank( + self.url_prefix + url + '?%s=%s&%s=%s' % + (param, value, param, value)) + resp = req.get_response(self.app) + self.assertEqual(200, resp.status_int) + + def test_list_duplicate_query_parameters_validation(self): + self._test_list_duplicate_query_parameters_validation('/os-volumes') + + def test_detail_list_duplicate_query_parameters_validation(self): + self._test_list_duplicate_query_parameters_validation( + '/os-volumes/detail') + + def test_list_with_additional_filter(self): + req = fakes.HTTPRequest.blank(self.url_prefix + + '/os-volumes?limit=1&offset=1&additional=something') + resp = req.get_response(self.app) + self.assertEqual(200, resp.status_int) + + def test_detail_list_with_additional_filter(self): + req = fakes.HTTPRequest.blank(self.url_prefix + + '/os-volumes/detail?limit=1&offset=1&additional=something') + resp = req.get_response(self.app) + self.assertEqual(200, resp.status_int) + class VolumeAttachTestsV21(test.NoDBTestCase): validation_error = exception.ValidationError @@ -700,6 +778,58 @@ class VolumeAttachTestsV21(test.NoDBTestCase): self.attachments, fake_func=fake_swap_volume_for_bdm_not_found) + def _test_list_with_invalid_filter(self, url): + prefix = '/servers/id/os-volume_attachments' + req = fakes.HTTPRequest.blank(prefix + url) + self.assertRaises(exception.ValidationError, + self.attachments.index, + req, + FAKE_UUID) + + def test_list_with_invalid_non_int_limit(self): + self._test_list_with_invalid_filter('?limit=-9') + + def test_list_with_invalid_string_limit(self): + self._test_list_with_invalid_filter('?limit=abc') + + def test_list_duplicate_query_with_invalid_string_limit(self): + self._test_list_with_invalid_filter( + '?limit=1&limit=abc') + + def test_list_with_invalid_non_int_offset(self): + self._test_list_with_invalid_filter('?offset=-9') + + def test_list_with_invalid_string_offset(self): + self._test_list_with_invalid_filter('?offset=abc') + + def test_list_duplicate_query_with_invalid_string_offset(self): + self._test_list_with_invalid_filter( + '?offset=1&offset=abc') + + @mock.patch.object(objects.BlockDeviceMappingList, + 'get_by_instance_uuid') + def test_list_duplicate_query_parameters_validation(self, mock_get): + fake_bdms = objects.BlockDeviceMappingList() + mock_get.return_value = fake_bdms + params = { + 'limit': 1, + 'offset': 1 + } + for param, value in params.items(): + req = fakes.HTTPRequest.blank( + '/servers/id/os-volume_attachments' + '?%s=%s&%s=%s' % + (param, value, param, value)) + self.attachments.index(req, FAKE_UUID) + + @mock.patch.object(objects.BlockDeviceMappingList, + 'get_by_instance_uuid') + def test_list_with_additional_filter(self, mock_get): + fake_bdms = objects.BlockDeviceMappingList() + mock_get.return_value = fake_bdms + req = fakes.HTTPRequest.blank( + '/servers/id/os-volume_attachments?limit=1&additional=something') + self.attachments.index(req, FAKE_UUID) + class VolumeAttachTestsV249(test.NoDBTestCase): validation_error = exception.ValidationError