From 4151486bd49dcee063e8173d604139c65db36e12 Mon Sep 17 00:00:00 2001 From: Henry Gessau Date: Wed, 16 Nov 2016 11:33:37 -0500 Subject: [PATCH] Remove REVERSED_PLURALS and get_resource_info() By changing pecan to store plugins by collection (plural) instead of resource (singular) we don't need REVERSED_PLURALS and get_resource_info(). These are not used outside of neutron core. Consequently, this exposed a bug with some controllers not honoring an extension's intention of disallowing pagination and sorting. This is also fixed in this patch to get successful test runs. Co-Authored-By: Brandon Logan Change-Id: I9d529d0a2ad369e2be0a8df3c6f06a6532e8b13d --- neutron/api/v2/attributes.py | 17 ----------------- neutron/api/v2/base.py | 8 ++++++++ neutron/pecan_wsgi/controllers/resource.py | 3 ++- neutron/pecan_wsgi/controllers/utils.py | 2 +- neutron/pecan_wsgi/hooks/policy_enforcement.py | 14 ++++++++------ neutron/pecan_wsgi/hooks/quota_enforcement.py | 3 ++- neutron/pecan_wsgi/startup.py | 15 ++++++++++----- .../tests/functional/pecan_wsgi/test_hooks.py | 3 ++- neutron/tests/unit/api/v2/test_attributes.py | 18 ------------------ 9 files changed, 33 insertions(+), 50 deletions(-) diff --git a/neutron/api/v2/attributes.py b/neutron/api/v2/attributes.py index 42d5bd628c2..fcc9d6df6e9 100644 --- a/neutron/api/v2/attributes.py +++ b/neutron/api/v2/attributes.py @@ -297,9 +297,6 @@ PLURALS = {NETWORKS: NETWORK, 'allocation_pools': 'allocation_pool', 'fixed_ips': 'fixed_ip', 'extensions': 'extension'} -# Store singular/plural mappings. This dictionary is populated by -# get_resource_info -REVERSED_PLURALS = {} def get_collection_info(collection): @@ -310,20 +307,6 @@ def get_collection_info(collection): return RESOURCE_ATTRIBUTE_MAP.get(collection) -def get_resource_info(resource): - """Helper function to retrieve attribute info - - :param resource: resource name - """ - plural_name = REVERSED_PLURALS.get(resource) - if not plural_name: - for (plural, singular) in PLURALS.items(): - if singular == resource: - plural_name = plural - REVERSED_PLURALS[resource] = plural_name - return RESOURCE_ATTRIBUTE_MAP.get(plural_name) - - def fill_default_value(attr_info, res_dict, exc_cls=ValueError, check_allow_post=True): diff --git a/neutron/api/v2/base.py b/neutron/api/v2/base.py index 97c8ccaf083..fb265d36490 100644 --- a/neutron/api/v2/base.py +++ b/neutron/api/v2/base.py @@ -75,6 +75,14 @@ class Controller(object): def member_actions(self): return self._member_actions + @property + def allow_pagination(self): + return self._allow_pagination + + @property + def allow_sorting(self): + return self._allow_sorting + def _init_policy_attrs(self): """Create the list of attributes required by policy. diff --git a/neutron/pecan_wsgi/controllers/resource.py b/neutron/pecan_wsgi/controllers/resource.py index 25bcc0eaf80..3858149d347 100644 --- a/neutron/pecan_wsgi/controllers/resource.py +++ b/neutron/pecan_wsgi/controllers/resource.py @@ -122,7 +122,8 @@ class CollectionsController(utils.NeutronPecanController): # NOTE(tonytan4ever): item needs to share the same # parent as collection parent_resource=self.parent, - member_actions=self._member_actions), remainder) + member_actions=self._member_actions, + plugin=self.plugin), remainder) @utils.expose(generic=True) def index(self, *args, **kwargs): diff --git a/neutron/pecan_wsgi/controllers/utils.py b/neutron/pecan_wsgi/controllers/utils.py index 64e9f2650a0..be1aaa0a5d6 100644 --- a/neutron/pecan_wsgi/controllers/utils.py +++ b/neutron/pecan_wsgi/controllers/utils.py @@ -150,7 +150,7 @@ class NeutronPecanController(object): def plugin(self): if not self._plugin: self._plugin = manager.NeutronManager.get_plugin_for_resource( - self.resource) + self.collection) return self._plugin @property diff --git a/neutron/pecan_wsgi/hooks/policy_enforcement.py b/neutron/pecan_wsgi/hooks/policy_enforcement.py index ee3fa6a3fb8..0191a61c04a 100644 --- a/neutron/pecan_wsgi/hooks/policy_enforcement.py +++ b/neutron/pecan_wsgi/hooks/policy_enforcement.py @@ -36,10 +36,11 @@ def _custom_getter(resource, resource_id): return quota.get_tenant_quotas(resource_id)[quotasv2.RESOURCE_NAME] -def fetch_resource(state, neutron_context, controller, resource, resource_id, +def fetch_resource(method, neutron_context, controller, + collection, resource, resource_id, parent_id=None): field_list = [] - if state.request.method == 'PUT': + if method == 'PUT': attrs = controller.resource_info if not attrs: # this isn't a request for a normal resource. it could be @@ -50,7 +51,7 @@ def fetch_resource(state, neutron_context, controller, resource, resource_id, field_list = [name for (name, value) in attrs.items() if (value.get('required_by_policy') or value.get('primary_key') or 'default' not in value)] - plugin = manager.NeutronManager.get_plugin_for_resource(resource) + plugin = manager.NeutronManager.get_plugin_for_resource(collection) if plugin: if utils.is_member_action(controller): getter = controller.parent_controller.plugin_shower @@ -109,8 +110,9 @@ class PolicyHook(hooks.PecanHook): item = {} resource_id = state.request.context.get('resource_id') parent_id = state.request.context.get('parent_id') - resource_obj = fetch_resource(state, neutron_context, controller, - resource, resource_id, + method = state.request.method + resource_obj = fetch_resource(method, neutron_context, controller, + collection, resource, resource_id, parent_id=parent_id) if resource_obj: original_resources.append(resource_obj) @@ -169,7 +171,7 @@ class PolicyHook(hooks.PecanHook): # in the single case, we enforce which raises on violation # in the plural case, we just check so violating items are hidden policy_method = policy.enforce if is_single else policy.check - plugin = manager.NeutronManager.get_plugin_for_resource(resource) + plugin = manager.NeutronManager.get_plugin_for_resource(collection) try: resp = [self._get_filtered_item(state.request, controller, resource, collection, item) diff --git a/neutron/pecan_wsgi/hooks/quota_enforcement.py b/neutron/pecan_wsgi/hooks/quota_enforcement.py index efa359b8af6..9b0bf944ac7 100644 --- a/neutron/pecan_wsgi/hooks/quota_enforcement.py +++ b/neutron/pecan_wsgi/hooks/quota_enforcement.py @@ -31,11 +31,12 @@ class QuotaEnforcementHook(hooks.PecanHook): priority = 130 def before(self, state): + collection = state.request.context.get('collection') resource = state.request.context.get('resource') items = state.request.context.get('resources') if state.request.method != 'POST' or not resource or not items: return - plugin = manager.NeutronManager.get_plugin_for_resource(resource) + plugin = manager.NeutronManager.get_plugin_for_resource(collection) # Store requested resource amounts grouping them by tenant deltas = collections.Counter(map(lambda x: x['tenant_id'], items)) # Perform quota enforcement diff --git a/neutron/pecan_wsgi/startup.py b/neutron/pecan_wsgi/startup.py index 0a553c805fe..4eab83b3342 100644 --- a/neutron/pecan_wsgi/startup.py +++ b/neutron/pecan_wsgi/startup.py @@ -44,15 +44,14 @@ def initialize_all(): plugin=plugin) manager.NeutronManager.set_controller_for_resource( collection, new_controller) - manager.NeutronManager.set_plugin_for_resource(resource, plugin) + manager.NeutronManager.set_plugin_for_resource(collection, plugin) pecanized_resources = ext_mgr.get_pecan_resources() for pec_res in pecanized_resources: - resource = attributes.PLURALS[pec_res.collection] manager.NeutronManager.set_controller_for_resource( pec_res.collection, pec_res.controller) manager.NeutronManager.set_plugin_for_resource( - resource, pec_res.plugin) + pec_res.collection, pec_res.plugin) # Now build Pecan Controllers and routes for all extensions resources = ext_mgr.get_resources() @@ -84,10 +83,16 @@ def initialize_all(): plugin = legacy_controller.plugin attr_info = legacy_controller.attr_info member_actions = legacy_controller.member_actions + pagination = legacy_controller.allow_pagination + sorting = legacy_controller.allow_sorting new_controller = res_ctrl.CollectionsController( collection, resource, resource_info=attr_info, - parent_resource=parent_resource, member_actions=member_actions) - manager.NeutronManager.set_plugin_for_resource(resource, plugin) + parent_resource=parent_resource, member_actions=member_actions, + plugin=plugin, allow_pagination=pagination, + allow_sorting=sorting) + # new_controller.collection has replaced hyphens with underscores + manager.NeutronManager.set_plugin_for_resource( + new_controller.collection, plugin) if path_prefix: manager.NeutronManager.add_resource_for_path_prefix( collection, path_prefix) diff --git a/neutron/tests/functional/pecan_wsgi/test_hooks.py b/neutron/tests/functional/pecan_wsgi/test_hooks.py index 0eda686f31b..056a1b25639 100644 --- a/neutron/tests/functional/pecan_wsgi/test_hooks.py +++ b/neutron/tests/functional/pecan_wsgi/test_hooks.py @@ -114,7 +114,8 @@ class TestPolicyEnforcementHook(test_functional.PecanFunctionalTest): self.mock_plugin = mock.Mock() attributes.RESOURCE_ATTRIBUTE_MAP.update(self.FAKE_RESOURCE) attributes.PLURALS['mehs'] = 'meh' - manager.NeutronManager.set_plugin_for_resource('meh', self.mock_plugin) + manager.NeutronManager.set_plugin_for_resource('mehs', + self.mock_plugin) fake_controller = resource.CollectionsController('mehs', 'meh') manager.NeutronManager.set_controller_for_resource( 'mehs', fake_controller) diff --git a/neutron/tests/unit/api/v2/test_attributes.py b/neutron/tests/unit/api/v2/test_attributes.py index 3c959caccd2..d6223aff5cb 100644 --- a/neutron/tests/unit/api/v2/test_attributes.py +++ b/neutron/tests/unit/api/v2/test_attributes.py @@ -13,7 +13,6 @@ # License for the specific language governing permissions and limitations # under the License. -import mock from neutron_lib.api import converters from neutron_lib import constants from neutron_lib import exceptions as n_exc @@ -156,20 +155,3 @@ class TestHelpers(base.DietTestCase): def test_get_collection_info_missing(self): self.assertFalse(attributes.get_collection_info('meh')) - - def test_get_resource_info(self): - attributes.REVERSED_PLURALS.pop('port', None) - attrs = attributes.get_resource_info('port') - self._verify_port_attributes(attrs) - # verify side effect - self.assertIn('port', attributes.REVERSED_PLURALS) - - def test_get_resource_info_missing(self): - self.assertFalse(attributes.get_resource_info('meh')) - - def test_get_resource_info_cached(self): - with mock.patch('neutron.api.v2.attributes.PLURALS') as mock_plurals: - attributes.REVERSED_PLURALS['port'] = 'ports' - attrs = attributes.get_resource_info('port') - self._verify_port_attributes(attrs) - self.assertEqual(0, mock_plurals.items.call_count)