From c7f128ccfd56f22c456a6cdd8f0cd264ffc07386 Mon Sep 17 00:00:00 2001 From: Ilya Shakhat Date: Tue, 30 Sep 2014 16:23:25 +0400 Subject: [PATCH] Response HTTP 404 if param value is not found Previously all some of wrong values were silently ignored but some resulted in HTTP 500 Server Error. New policy is to validate all values and return HTTP 404 Not Found if there are any wrong values. Closes bug 1373980 Closes bug 1361647 Change-Id: Ib2968b2188070863b2ea6df25c35ad729a72ede5 --- stackalytics/dashboard/decorators.py | 86 ++++++++++++++++-------- stackalytics/dashboard/helpers.py | 17 ++--- stackalytics/dashboard/memory_storage.py | 3 + stackalytics/dashboard/parameters.py | 20 +++--- stackalytics/dashboard/web.py | 14 ++-- tests/api/test_companies.py | 15 ++++- tests/api/test_modules.py | 11 ++- tests/api/test_releases.py | 7 +- tests/unit/test_web_utils.py | 20 +++--- 9 files changed, 122 insertions(+), 71 deletions(-) diff --git a/stackalytics/dashboard/decorators.py b/stackalytics/dashboard/decorators.py index 65c2f8e7e..bc4bd0c35 100644 --- a/stackalytics/dashboard/decorators.py +++ b/stackalytics/dashboard/decorators.py @@ -35,6 +35,35 @@ from stackalytics import version as stackalytics_version LOG = logging.getLogger(__name__) +def _check_param_in(params, name, collection, allow_all=False): + for single in (params.get(name) or []): + single = single.lower() + if allow_all and single == 'all': + continue + if single not in collection: + params[name] = [] + flask.abort(404) + + +def _validate_params(params): + vault_inst = vault.get_vault() + memory_storage_inst = vault.get_memory_storage() + + _check_param_in(params, 'release', vault_inst['releases'], True) + _check_param_in(params, 'project_type', vault_inst['project_types_index']) + _check_param_in(params, 'module', vault_inst['module_id_index']) + _check_param_in(params, 'company', + memory_storage_inst.get_companies_lower()) + _check_param_in(params, 'user_id', memory_storage_inst.get_user_ids()) + _check_param_in(params, 'metric', parameters.METRIC_TO_RECORD_TYPE, True) + + +def _get_single(params): + if params: + return params[0] + return None + + def _prepare_params(kwargs, ignore): params = kwargs.get('_params') @@ -50,6 +79,7 @@ def _prepare_params(kwargs, ignore): params['end_date'] = [utils.round_timestamp_to_day( params['end_date'][0])] + _validate_params(params) kwargs['_params'] = params if ignore: @@ -178,8 +208,7 @@ def record_filter(ignore=None): if 'tm_marks' in metric: filtered_ids = [] - review_nth = int(parameters.get_parameter( - kwargs, 'review_nth')[0]) + review_nth = int(parameters.get_parameter('review_nth')[0]) for record in memory_storage_inst.get_records(record_ids): parent = memory_storage_inst.get_record_by_primary_key( record['review_id']) @@ -371,50 +400,45 @@ def templated(template=None, return_code=200): if ctx is None: ctx = {} + try: + _prepare_params(kwargs, []) + except Exception: + if return_code == 200: + raise # do not re-raise on error page + # put parameters into template - metric = flask.request.args.get('metric') - if metric not in parameters.METRIC_LABELS: - metric = None - ctx['metric'] = metric or parameters.get_default('metric') + ctx['metric'] = parameters.get_single_parameter( + kwargs, 'metric', use_default=True) ctx['metric_label'] = parameters.METRIC_LABELS[ctx['metric']] - project_type = flask.request.args.get('project_type') - if not vault.is_project_type_valid(project_type): - project_type = parameters.get_default('project_type') + project_type = parameters.get_single_parameter( + kwargs, 'project_type', use_default=True) ctx['project_type'] = project_type - release = flask.request.args.get('release') - releases = vault_inst['releases'] - if release: - release = release.lower() - if release != 'all': - if release not in releases: - release = None - else: - release = releases[release]['release_name'] - ctx['release'] = (release or - parameters.get_default('release')).lower() - ctx['review_nth'] = (flask.request.args.get('review_nth') or - parameters.get_default('review_nth')) + ctx['release'] = parameters.get_single_parameter( + kwargs, 'release', use_default=True) - ctx['company'] = parameters.get_single_parameter(kwargs, 'company') - ctx['company_original'] = ( - vault.get_memory_storage().get_original_company_name( - ctx['company'])) + company = parameters.get_single_parameter(kwargs, 'company') + ctx['company'] = company + if company: + ctx['company_original'] = ( + vault.get_memory_storage().get_original_company_name( + ctx['company'])) module = parameters.get_single_parameter(kwargs, 'module') ctx['module'] = module - module_name = None if module and module in vault_inst['module_id_index']: ctx['module_inst'] = vault_inst['module_id_index'][module] - module_name = ctx['module_inst']['module_group_name'] ctx['user_id'] = parameters.get_single_parameter(kwargs, 'user_id') if ctx['user_id']: ctx['user_inst'] = vault.get_user_from_runtime_storage( ctx['user_id']) + ctx['page_title'] = helpers.make_page_title( - ctx['company'], ctx['user_id'], module_name, ctx['release']) + ctx.get('release'), ctx.get('module_inst'), + ctx.get('company_original'), ctx.get('user_inst')) + ctx['stackalytics_version'] = ( stackalytics_version.version_info.version_string()) ctx['stackalytics_release'] = ( @@ -422,6 +446,10 @@ def templated(template=None, return_code=200): ctx['runtime_storage_update_time'] = ( vault_inst['runtime_storage_update_time']) + # deprecated -- top mentor report + ctx['review_nth'] = parameters.get_single_parameter( + kwargs, 'review_nth') + return flask.render_template(template_name, **ctx), return_code return templated_decorated_function diff --git a/stackalytics/dashboard/helpers.py b/stackalytics/dashboard/helpers.py index 49fc19ccd..828acf5c2 100644 --- a/stackalytics/dashboard/helpers.py +++ b/stackalytics/dashboard/helpers.py @@ -237,7 +237,7 @@ def make_link(title, uri=None, options=None): 'metric') param_values = {} for param_name in param_names: - v = parameters.get_parameter({}, param_name, param_name) + v = parameters.get_parameter({}, param_name) if v: param_values[param_name] = ','.join(v) if options: @@ -274,13 +274,10 @@ def make_commit_message(record): return s -def make_page_title(company, user_id, module, release): - if company: - memory_storage = vault.get_memory_storage() - company = memory_storage.get_original_company_name(company) - if company or user_id: - if user_id: - s = vault.get_user_from_runtime_storage(user_id)['user_name'] +def make_page_title(release, module_inst, company, user_inst): + if company or user_inst: + if user_inst: + s = user_inst['user_name'] if company: s += ' (%s)' % company else: @@ -288,8 +285,8 @@ def make_page_title(company, user_id, module, release): else: s = 'OpenStack community' s += ' contribution' - if module: - s += ' to %s' % module + if module_inst: + s += ' to %s' % module_inst['module_group_name'] if release != 'all': s += ' in %s release' % release.capitalize() else: diff --git a/stackalytics/dashboard/memory_storage.py b/stackalytics/dashboard/memory_storage.py index 8b954b21f..6000a893c 100644 --- a/stackalytics/dashboard/memory_storage.py +++ b/stackalytics/dashboard/memory_storage.py @@ -174,6 +174,9 @@ class CachedMemoryStorage(MemoryStorage): def get_companies(self): return self.company_index.keys() + def get_companies_lower(self): + return self.company_name_mapping.keys() + def get_modules(self): return self.module_index.keys() diff --git a/stackalytics/dashboard/parameters.py b/stackalytics/dashboard/parameters.py index 1b35abd65..5b59bf98a 100644 --- a/stackalytics/dashboard/parameters.py +++ b/stackalytics/dashboard/parameters.py @@ -85,25 +85,29 @@ def get_default(param_name): return None -def get_parameter(kwargs, singular_name, plural_name=None, use_default=True): - if singular_name in kwargs: - p = kwargs[singular_name] +def get_parameter(kwargs, name, plural_name=None, use_default=True): + processed_params = kwargs.get('_params') or {} + if name in processed_params: + return processed_params[name] + + if name in kwargs: + p = kwargs[name] else: - p = flask.request.args.get(singular_name) + p = flask.request.args.get(name) if (not p) and plural_name: p = flask.request.args.get(plural_name) if p: return parse.unquote_plus(p).split(',') elif use_default: - default = get_default(singular_name) + default = get_default(name) return [default] if default else [] else: return [] -def get_single_parameter(kwargs, singular_name, use_default=True): - param = get_parameter(kwargs, singular_name, use_default) +def get_single_parameter(kwargs, name, use_default=True): + param = get_parameter(kwargs, name, use_default) if param: return param[0] else: - return '' + return None diff --git a/stackalytics/dashboard/web.py b/stackalytics/dashboard/web.py index 17822c3f7..db4728bc7 100644 --- a/stackalytics/dashboard/web.py +++ b/stackalytics/dashboard/web.py @@ -60,12 +60,6 @@ def widget(): return flask.render_template('widget.html') -@app.errorhandler(404) -@decorators.templated('404.html', 404) -def page_not_found(e): - pass - - # AJAX Handlers --------- def _get_aggregated_stats(records, metric_filter, keys, param_id, @@ -167,7 +161,7 @@ def get_core_engineer_branch(user, modules): @decorators.record_filter() @decorators.aggregate_filter() def get_engineers(records, metric_filter, finalize_handler, **kwargs): - modules_names = parameters.get_parameter({}, 'module', 'modules') + modules_names = parameters.get_parameter(kwargs, 'module') modules = set([m for m, r in vault.resolve_modules(modules_names, [''])]) def postprocessing(record): @@ -190,7 +184,7 @@ def get_engineers(records, metric_filter, finalize_handler, **kwargs): @decorators.jsonify('stats') @decorators.record_filter(ignore=['metric']) def get_engineers_extended(records, **kwargs): - modules_names = parameters.get_parameter({}, 'module', 'modules') + modules_names = parameters.get_parameter(kwargs, 'module') modules = set([m for m, r in vault.resolve_modules(modules_names, [''])]) def postprocessing(record): @@ -299,7 +293,7 @@ def get_companies_json(record_ids, **kwargs): def get_modules_json(record_ids, **kwargs): module_id_index = vault.get_vault()['module_id_index'] - tags = parameters.get_parameter({}, 'tag', 'tags') + tags = parameters.get_parameter(kwargs, 'tag', plural_name='tags') # all modules mentioned in records module_ids = vault.get_memory_storage().get_index_keys_by_record_ids( @@ -414,7 +408,7 @@ def get_bpd(records, **kwargs): @decorators.jsonify() @decorators.record_filter(ignore=['user_id']) def get_users_json(record_ids, **kwargs): - core_in = parameters.get_single_parameter({}, 'core_in') or None + core_in = parameters.get_single_parameter(kwargs, 'core_in') or None valid_modules = set() if core_in: core_in = set(core_in.split(',')) diff --git a/tests/api/test_companies.py b/tests/api/test_companies.py index 4a713a680..9d7896280 100644 --- a/tests/api/test_companies.py +++ b/tests/api/test_companies.py @@ -81,7 +81,20 @@ class TestAPICompanies(test_api.TestAPI): 'uri': 'git://git.openstack.org/openstack/nova.git'}, {'module': 'glance', 'project_type': 'openstack', 'organization': 'openstack', - 'uri': 'git://git.openstack.org/openstack/glance.git'}]}, + 'uri': 'git://git.openstack.org/openstack/glance.git'}], + 'module_groups': { + 'nova': test_api.make_module('nova'), + 'glance': test_api.make_module('glance'), + }, + 'releases': [{'release_name': 'prehistory', + 'end_date': 1234567890}, + {'release_name': 'icehouse', + 'end_date': 1234567890}], + 'project_types': [ + {'id': 'all', 'title': 'All', + 'modules': ['nova', 'glance', 'nova-cli']}, + {'id': 'openstack', 'title': 'OpenStack', + 'modules': ['nova', 'glance']}]}, test_api.make_records(record_type=['commit'], loc=[10, 20, 30], module=['glance'], diff --git a/tests/api/test_modules.py b/tests/api/test_modules.py index a47c68637..3c6386bcd 100644 --- a/tests/api/test_modules.py +++ b/tests/api/test_modules.py @@ -83,7 +83,16 @@ class TestAPIModules(test_api.TestAPI): 'tag': 'group'}, 'nova': test_api.make_module('nova'), 'nova-cli': test_api.make_module('nova-cli'), - }}, + }, + 'releases': [{'release_name': 'prehistory', + 'end_date': 1234567890}, + {'release_name': 'icehouse', + 'end_date': 1234567890}], + 'project_types': [ + {'id': 'all', 'title': 'All', + 'modules': ['nova', 'glance', 'nova-cli']}, + {'id': 'openstack', 'title': 'OpenStack', + 'modules': ['nova', 'glance']}]}, test_api.make_records(record_type=['commit'])): response = self.app.get('/api/1.0/modules/nova') diff --git a/tests/api/test_releases.py b/tests/api/test_releases.py index 60676c330..9b31d5774 100644 --- a/tests/api/test_releases.py +++ b/tests/api/test_releases.py @@ -23,7 +23,12 @@ class TestAPIReleases(test_api.TestAPI): {'releases': [ {'release_name': 'prehistory', 'end_date': 1365033600}, {'release_name': 'havana', 'end_date': 1381968000}, - {'release_name': 'icehouse', 'end_date': 1397692800}]}, + {'release_name': 'icehouse', 'end_date': 1397692800}], + 'project_types': [ + {'id': 'all', 'title': 'All', + 'modules': ['nova', 'glance', 'nova-cli']}, + {'id': 'openstack', 'title': 'OpenStack', + 'modules': ['nova', 'glance']}]}, test_api.make_records(record_type=['commit'])): response = self.app.get('/api/1.0/releases') releases = test_api.load_json(response)['data'] diff --git a/tests/unit/test_web_utils.py b/tests/unit/test_web_utils.py index d83724c98..d64a675b7 100644 --- a/tests/unit/test_web_utils.py +++ b/tests/unit/test_web_utils.py @@ -86,25 +86,23 @@ Implements Blueprint ''' + ( @mock.patch('stackalytics.dashboard.vault.get_vault') @mock.patch('stackalytics.dashboard.vault.get_user_from_runtime_storage') def test_make_page_title(self, user_patch, vault_patch): - memory_storage_mock = mock.Mock() - memory_storage_mock.get_original_company_name = mock.Mock( - return_value='Mirantis' - ) - vault_patch.return_value = {'memory_storage': memory_storage_mock} - user_patch.return_value = {'user_name': 'John Doe'} + user_inst = {'user_name': 'John Doe'} + module_inst = {'module_group_name': 'neutron'} self.assertEqual('OpenStack community contribution in all releases', - helpers.make_page_title('', '', '', 'all')) + helpers.make_page_title('all', None, None, None)) self.assertEqual('OpenStack community contribution in Havana release', - helpers.make_page_title('', '', '', 'Havana')) + helpers.make_page_title('Havana', None, None, None)) self.assertEqual('Mirantis contribution in Havana release', - helpers.make_page_title('Mirantis', '', '', 'Havana')) + helpers.make_page_title( + 'Havana', None, 'Mirantis', None)) self.assertEqual('John Doe contribution in Havana release', - helpers.make_page_title('', 'john_doe', '', 'Havana')) + helpers.make_page_title( + 'Havana', None, None, user_inst)) self.assertEqual( 'John Doe (Mirantis) contribution to neutron in Havana release', helpers.make_page_title( - 'Mirantis', 'John Doe', 'neutron', 'Havana')) + 'Havana', module_inst, 'Mirantis', user_inst)) @mock.patch('flask.request') @mock.patch('stackalytics.dashboard.parameters.get_default')