From 991120e12bf0912344a48b84bfe13a5f697bcf0a Mon Sep 17 00:00:00 2001 From: zelima Date: Wed, 3 May 2017 14:07:39 +0400 Subject: [PATCH] [error handling][xl]: refs #312 - controllers refactored to handle internal server errors without try/catch - new tests, as previous ones were not full covering all cases - tests refactored to assert new error messages --- app/auth/annotations.py | 11 +- app/auth/controllers.py | 193 +++++++++++++---------------- app/auth/models.py | 6 +- app/package/controllers.py | 189 +++++++++++----------------- app/package/models.py | 204 ++++++++++++++----------------- app/profile/controllers.py | 14 +-- app/search/controllers.py | 19 ++- app/search/models.py | 4 +- app/site/controllers.py | 19 +-- tests/auth/test_controller.py | 26 ++-- tests/package/test_controller.py | 63 +++++++--- tests/package/test_models.py | 14 +-- 12 files changed, 338 insertions(+), 424 deletions(-) diff --git a/app/auth/annotations.py b/app/auth/annotations.py index 677da59..6dae671 100644 --- a/app/auth/annotations.py +++ b/app/auth/annotations.py @@ -9,7 +9,7 @@ from flask import current_app as app from flask import request, _request_ctx_stack -from app.utils import handle_error +from app.utils import InvalidUsage from app.auth.models import JWT from app.auth.authorization import is_authorize from app.package.models import Package @@ -38,7 +38,7 @@ def wrapped(*args, **kwargs): user_id = user_info['user'] status = check_is_authorized(action, kwargs['publisher'], kwargs['package'], user_id) if not status: - return handle_error("NOT_ALLOWED", "The operation is not allowed", 403) + raise InvalidUsage("The operation is not allowed", 403) return f(*args, **kwargs) return wrapped return wrapper @@ -54,12 +54,11 @@ def get_user_from_jwt(req, api_key): token = req.values.get('jwt') if not token: - return False, handle_error('authorization_header_missing', - 'Authorization header is expected', 401) + raise InvalidUsage('Authorization header is expected', 401) try: return True, jwt_helper.decode(token) except Exception as e: - return False, handle_error('jwt_error', e.message, 400) + raise InvalidUsage(e.message, 400) def check_is_authorized(action, publisher, package=None, user_id=None): @@ -73,6 +72,6 @@ def check_is_authorized(action, publisher, package=None, user_id=None): publisher_name = publisher instance = Publisher.query.filter_by(name=publisher_name).one() else: - return handle_error("INVALID_ENTITY", "{e} is not a valid one".format(e=entity_str), 401) + raise InvalidUsage("{e} is not a valid one".format(e=entity_str), 401) return is_authorize(user_id, instance, action) diff --git a/app/auth/controllers.py b/app/auth/controllers.py index f5088b0..da1e49d 100644 --- a/app/auth/controllers.py +++ b/app/auth/controllers.py @@ -12,7 +12,7 @@ from app.profile.models import User from app.auth.models import JWT, FileData from app.package.models import Package -from app.utils import handle_error +from app.utils import InvalidUsage from app.auth.annotations import check_is_authorized, get_user_from_jwt auth_blueprint = Blueprint('auth', __name__, url_prefix='/api/auth') @@ -49,36 +49,29 @@ def callback_handling(): description: Returns back email, nickname, picture, name """ - try: - github = app.config['github'] - resp = github.authorized_response() - if resp is None or resp.get('access_token') is None: - return handle_error('Access Denied', - request.args.get('error_description'), - 400) - session['github_token'] = (resp['access_token'], '') - user_info = github.get('user') - user_info = user_info.data - # in case user Email is not public - if not user_info.get('email'): - emails = github.get('user/emails').data - if not len(emails): - return handle_error('Not Found', 'Email not found', 404) - for email in emails: - if email.get('primary'): - user_info['email'] = email.get('email') - user = User().create_or_update_user_from_callback(user_info) - jwt_helper = JWT(app.config['JWT_SEED'], user.id) - session.pop('github_token', None) - g.current_user = user - resp = make_response(render_template("dashboard.html", - title='Dashboard'), 200) - resp.set_cookie('jwt', jwt_helper.encode()) - return resp - except Exception as e: - app.logger.error(e) - return handle_error('GENERIC_ERROR', e.message, 500) - + github = app.config['github'] + resp = github.authorized_response() + if resp is None or resp.get('access_token') is None: + raise InvalidUsage('Access Denied', 400) + session['github_token'] = (resp['access_token'], '') + user_info = github.get('user') + user_info = user_info.data + # in case user Email is not public + if not user_info.get('email'): + emails = github.get('user/emails').data + if not len(emails): + raise InvalidUsage('Email Not Found', 404) + for email in emails: + if email.get('primary'): + user_info['email'] = email.get('email') + user = User().create_or_update_user_from_callback(user_info) + jwt_helper = JWT(app.config['JWT_SEED'], user.id) + session.pop('github_token', None) + g.current_user = user + resp = make_response(render_template("dashboard.html", + title='Dashboard'), 200) + resp.set_cookie('jwt', jwt_helper.encode()) + return resp @auth_blueprint.route("/token", methods=['POST']) def get_jwt(): @@ -122,54 +115,40 @@ def get_jwt(): description: Secret key do not match """ - try: - data = request.get_json() - user_name = data.get('username', None) - email = data.get('email', None) - secret = data.get('secret', None) - verify = False - user_id = None - if user_name is None and email is None: - return handle_error('INVALID_INPUT', - 'User name or email both can not be empty', - 400) - - if secret is None: - return handle_error('INVALID_INPUT', - 'secret can not be empty', - 400) - elif user_name is not None: - try: - user = User.query.filter_by(name=user_name).one() - except NoResultFound as e: - app.logger.error(e) - return handle_error('USER_NOT_FOUND', - 'user does not exists', - 404) - if secret == user.secret: - verify = True - user_id = user.id - elif email is not None: - try: - user = User.query.filter_by(email=email).one() - except NoResultFound as e: - app.logger.error(e) - return handle_error('USER_NOT_FOUND', - 'user does not exists', - 404) - if secret == user.secret: - verify = True - user_id = user.id - if verify: - return jsonify({'token': JWT(app.config['JWT_SEED'], - user_id).encode()}), 200 - else: - return handle_error('SECRET_ERROR', - 'Secret key do not match', - 403) - except Exception as e: - app.logger.error(e) - return handle_error('GENERIC_ERROR', e.message, 500) + data = request.get_json() + user_name = data.get('username', None) + email = data.get('email', None) + secret = data.get('secret', None) + verify = False + user_id = None + if user_name is None and email is None: + raise InvalidUsage('User name or email both can not be empty', 400) + + if secret is None: + raise InvalidUsage('Secret can not be empty', 400) + elif user_name is not None: + try: + user = User.query.filter_by(name=user_name).one() + except NoResultFound as e: + app.logger.error(e) + raise InvalidUsage('user does not exists', 404) + if secret == user.secret: + verify = True + user_id = user.id + elif email is not None: + try: + user = User.query.filter_by(email=email).one() + except NoResultFound as e: + app.logger.error(e) + raise InvalidUsage('user does not exists', 404) + if secret == user.secret: + verify = True + user_id = user.id + if verify: + return jsonify({'token': JWT(app.config['JWT_SEED'], + user_id).encode()}), 200 + else: + raise InvalidUsage('Secret key do not match', 403) @auth_blueprint.route("/login", methods=['GET']) @@ -211,33 +190,29 @@ def authorize_upload(): 500: description: Internal Server Error """ - try: - user_id = None - jwt_status, user_info = get_user_from_jwt(request, app.config['JWT_SEED']) - if jwt_status: - user_id = user_info['user'] - - data = request.get_json() - metadata, filedata = data['metadata'], data['filedata'] - publisher, package_name = metadata['owner'], metadata['name'] - res_payload = {'filedata': {}} - - if Package.is_package_exists(publisher, package_name): - status = check_is_authorized('Package::Update', publisher, package_name, user_id) - else: - status = check_is_authorized('Package::Create', publisher, package_name, user_id) - - if not status: - return handle_error('UN-AUTHORIZE', 'not authorized to upload data', 400) - - for relative_path in filedata.keys(): - response = FileData(package_name=package_name, - publisher=publisher, - relative_path=relative_path, - props=filedata[relative_path]) - res_payload['filedata'][relative_path] = response.build_file_information() - - return jsonify(res_payload), 200 - except Exception as e: - app.logger.error(e) - return handle_error('GENERIC_ERROR', e.message, 500) + user_id = None + jwt_status, user_info = get_user_from_jwt(request, app.config['JWT_SEED']) + if jwt_status: + user_id = user_info['user'] + + data = request.get_json() + metadata, filedata = data['metadata'], data['filedata'] + publisher, package_name = metadata['owner'], metadata['name'] + res_payload = {'filedata': {}} + + if Package.is_package_exists(publisher, package_name): + status = check_is_authorized('Package::Update', publisher, package_name, user_id) + else: + status = check_is_authorized('Package::Create', publisher, package_name, user_id) + + if not status: + raise InvalidUsage('Not authorized to upload data', 400) + + for relative_path in filedata.keys(): + response = FileData(package_name=package_name, + publisher=publisher, + relative_path=relative_path, + props=filedata[relative_path]) + res_payload['filedata'][relative_path] = response.build_file_information() + + return jsonify(res_payload), 200 diff --git a/app/auth/models.py b/app/auth/models.py index 90905a5..d3b4bcd 100644 --- a/app/auth/models.py +++ b/app/auth/models.py @@ -7,7 +7,7 @@ import datetime import jwt from app.package.models import BitStore - +from app.utils import InvalidUsage class JWT(object): @@ -40,9 +40,9 @@ def decode(self, token): algorithm=self.algorithm, issuer=self.issuer) except jwt.ExpiredSignature: - raise Exception("token is expired") + raise InvalidUsage("Token is expired") except jwt.DecodeError: - raise Exception('token signature is invalid') + raise InvalidUsage('Token signature is invalid') except Exception: raise Exception('Unable to parse authentication token.') diff --git a/app/package/controllers.py b/app/package/controllers.py index 26a6bcf..e01b633 100644 --- a/app/package/controllers.py +++ b/app/package/controllers.py @@ -15,7 +15,7 @@ from app.package.models import BitStore, Package, PackageStateEnum from app.profile.models import Publisher, User from app.auth.annotations import requires_auth, is_allowed -from app.utils import handle_error +from app.utils import InvalidUsage from app.auth.annotations import check_is_authorized, get_user_from_jwt package_blueprint = Blueprint('package', __name__, url_prefix='/api/package') @@ -73,21 +73,15 @@ def tag_data_package(publisher, package): description: Status of the operation default: OK """ - try: - data = request.get_json() - if 'version' not in data: - return handle_error('ATTRIBUTE_MISSING', 'version not found', 400) + data = request.get_json() + if 'version' not in data: + raise InvalidUsage('version not found', 400) - bitstore = BitStore(publisher, package) - status_db = Package.create_or_update_tag(publisher, package, data['version']) - status_bitstore = bitstore.copy_to_new_version(data['version']) + bitstore = BitStore(publisher, package) + status_db = Package.create_or_update_tag(publisher, package, data['version']) + status_bitstore = bitstore.copy_to_new_version(data['version']) - if status_db is False or status_bitstore is False: - raise Exception("failed to tag data package") - return jsonify({"status": "OK"}), 200 - except Exception as e: - app.logger.error(e) - return handle_error('GENERIC_ERROR', e.message, 500) + return jsonify({"status": "OK"}), 200 @package_blueprint.route("//", methods=["DELETE"]) @@ -127,19 +121,11 @@ def delete_data_package(publisher, package): type: string default: OK """ - try: - bitstore = BitStore(publisher=publisher, package=package) - status_acl = bitstore.change_acl('private') - status_db = Package.change_status(publisher, package, PackageStateEnum.deleted) - if status_acl and status_db: - return jsonify({"status": "OK"}), 200 - if not status_acl: - raise Exception('Failed to change acl') - if not status_db: - raise Exception('Failed to change status') - except Exception as e: - app.logger.error(e) - return handle_error('GENERIC_ERROR', e.message, 500) + bitstore = BitStore(publisher=publisher, package=package) + status_acl = bitstore.change_acl('private') + status_db = Package.change_status(publisher, package, PackageStateEnum.deleted) + if status_acl and status_db: + return jsonify({"status": "OK"}), 200 @package_blueprint.route("///undelete", methods=["POST"]) @@ -181,19 +167,11 @@ def undelete_data_package(publisher, package): default: OK """ - try: - bitstore = BitStore(publisher=publisher, package=package) - status_acl = bitstore.change_acl('public-read') - status_db = Package.change_status(publisher, package, PackageStateEnum.active) - if status_acl and status_db: - return jsonify({"status": "OK"}), 200 - if not status_acl: - raise Exception('Failed to change acl') - if not status_db: - raise Exception('Failed to change status') - except Exception as e: - app.logger.error(e) - return handle_error('GENERIC_ERROR', e.message, 500) + bitstore = BitStore(publisher=publisher, package=package) + status_acl = bitstore.change_acl('public-read') + status_db = Package.change_status(publisher, package, PackageStateEnum.active) + if status_acl and status_db: + return jsonify({"status": "OK"}), 200 @package_blueprint.route("///purge", methods=["DELETE"]) @@ -234,20 +212,11 @@ def purge_data_package(publisher, package): type: string default: OK """ - try: - bitstore = BitStore(publisher=publisher, package=package) - status_acl = bitstore.delete_data_package() - status_db = Package.delete_data_package(publisher, package) - if status_acl and status_db: - return jsonify({"status": "OK"}), 200 - if not status_acl: - raise Exception('Failed to delete from s3') - if not status_db: - raise Exception('Failed to delete from db') - except Exception as e: - app.logger.error(e) - return handle_error('GENERIC_ERROR', e.message, 500) - + bitstore = BitStore(publisher=publisher, package=package) + status_acl = bitstore.delete_data_package() + status_db = Package.delete_data_package(publisher, package) + if status_acl and status_db: + return jsonify({"status": "OK"}), 200 @package_blueprint.route("/upload", methods=["POST"]) @requires_auth @@ -280,38 +249,33 @@ def finalize_publish(): 500: description: Internal Server Error """ - try: - data = request.get_json() - datapackage_url = data['datapackage'] - publisher, package, version = BitStore.extract_information_from_s3_url(datapackage_url) - user_id = None - jwt_status, user_info = get_user_from_jwt(request, app.config['JWT_SEED']) - if jwt_status: - user_id = user_info['user'] - - if Package.is_package_exists(publisher, package): - status = check_is_authorized('Package::Update', publisher, package, user_id) - else: - status = check_is_authorized('Package::Create', publisher, package, user_id) + data = request.get_json() + datapackage_url = data['datapackage'] + publisher, package, version = BitStore.extract_information_from_s3_url(datapackage_url) + user_id = None + jwt_status, user_info = get_user_from_jwt(request, app.config['JWT_SEED']) + if jwt_status: + user_id = user_info['user'] - if not status: - return handle_error('UN-AUTHORIZE', 'not authorized to upload data', 400) + if Package.is_package_exists(publisher, package): + status = check_is_authorized('Package::Update', publisher, package, user_id) + else: + status = check_is_authorized('Package::Create', publisher, package, user_id) - bit_store = BitStore(publisher, package) - b = bit_store.get_metadata_body() - body = json.loads(b) - if body is not None: - bit_store.change_acl('public-read') - readme = bit_store.get_s3_object(bit_store.get_readme_object_key()) - Package.create_or_update(name=package, publisher_name=publisher, - descriptor=body, readme=readme) - return jsonify({"status": "queued"}), 200 + if not status: + raise InvalidUsage('Not authorized to upload data', 400) - raise Exception("Failed to get data from s3") + bit_store = BitStore(publisher, package) + b = bit_store.get_metadata_body() + body = json.loads(b) + if body is not None: + bit_store.change_acl('public-read') + readme = bit_store.get_s3_object(bit_store.get_readme_object_key()) + Package.create_or_update(name=package, publisher_name=publisher, + descriptor=body, readme=readme) + return jsonify({"status": "queued"}), 200 - except Exception as e: - app.logger.error(e) - return handle_error('GENERIC_ERROR', e.message, 500) + raise InvalidUsage("Failed to get data from s3") @package_blueprint.route("//", methods=["GET"]) @@ -348,27 +312,22 @@ def get_metadata(publisher, package): 404: description: No metadata found for the package """ - try: - data = Package.query.join(Publisher).\ - filter(Publisher.name == publisher, - Package.name == package, - Package.status == PackageStateEnum.active).\ - first() - if data is None: - return handle_error('DATA_NOT_FOUND', - 'No metadata found for the package', - 404) - tag = filter(lambda t: t.tag == 'latest', data.tags)[0] - metadata = { - 'id': data.id, - 'name': data.name, - 'publisher': data.publisher.name, - 'readme': tag.readme or '', - 'descriptor': tag.descriptor - } - return jsonify(metadata), 200 - except Exception as e: - return handle_error('GENERIC_ERROR', e.message, 500) + data = Package.query.join(Publisher).\ + filter(Publisher.name == publisher, + Package.name == package, + Package.status == PackageStateEnum.active).\ + first() + if data is None: + raise InvalidUsage('No metadata found for the package', 404) + tag = filter(lambda t: t.tag == 'latest', data.tags)[0] + metadata = { + 'id': data.id, + 'name': data.name, + 'publisher': data.publisher.name, + 'readme': tag.readme or '', + 'descriptor': tag.descriptor + } + return jsonify(metadata), 200 @package_blueprint.route("/", methods=["GET"]) @@ -402,18 +361,12 @@ def get_all_metadata_names_for_publisher(publisher): 404: description: No Data Package Found For The Publisher """ - try: - metadata = Package.query.join(Publisher).\ - with_entities(Package.name).\ - filter(Publisher.name == publisher).all() - if len(metadata) is 0: - return handle_error('DATA_NOT_FOUND', - 'No Data Package Found For The Publisher', - 404) - keys = [] - for d in metadata: - keys.append(d[0]) - return jsonify({'data': metadata}), 200 - except Exception as e: - app.logger.error(e) - return handle_error('GENERIC_ERROR', e.message, 500) + metadata = Package.query.join(Publisher).\ + with_entities(Package.name).\ + filter(Publisher.name == publisher).all() + if len(metadata) is 0: + raise InvalidUsage('No Data Package Found For The Publisher', 404) + keys = [] + for d in metadata: + keys.append(d[0]) + return jsonify({'data': metadata}), 200 diff --git a/app/package/models.py b/app/package/models.py index 55ccfcb..6b73d01 100644 --- a/app/package/models.py +++ b/app/package/models.py @@ -14,6 +14,7 @@ from sqlalchemy.orm import relationship from app.profile.models import Publisher from app.database import db +from botocore.exceptions import ClientError class BitStore(object): @@ -66,7 +67,9 @@ def get_s3_object(self, key): s3_client = app.config['S3'] response = s3_client.get_object(Bucket=bucket_name, Key=key) return response['Body'].read() - except Exception: + except ClientError as e: + if e.response['Error']['Code'] != 'NoSuchKey': + raise e return None def get_readme_object_key(self): @@ -77,7 +80,7 @@ def get_readme_object_key(self): :return: Value of the readme key if found else None :rtype: None or Str """ - readme_key = None + readme_key = 'None' prefix = self.build_s3_key('') bucket_name = app.config['S3_BUCKET_NAME'] s3_client = app.config['S3'] @@ -159,24 +162,21 @@ def delete_data_package(self): This method is used for Hard delete data packages. :return: Status True if able to delete or False if exception """ - try: - bucket_name = app.config['S3_BUCKET_NAME'] - s3_client = app.config['S3'] + bucket_name = app.config['S3_BUCKET_NAME'] + s3_client = app.config['S3'] + + keys = [] + list_objects = s3_client.list_objects(Bucket=bucket_name, + Prefix=self.build_s3_base_prefix()) + if list_objects is not None and 'Contents' in list_objects: + for ob in s3_client \ + .list_objects(Bucket=bucket_name, + Prefix=self.build_s3_base_prefix())['Contents']: + keys.append(dict(Key=ob['Key'])) + + s3_client.delete_objects(Bucket=bucket_name, Delete=dict(Objects=keys)) + return True - keys = [] - list_objects = s3_client.list_objects(Bucket=bucket_name, - Prefix=self.build_s3_base_prefix()) - if list_objects is not None and 'Contents' in list_objects: - for ob in s3_client \ - .list_objects(Bucket=bucket_name, - Prefix=self.build_s3_base_prefix())['Contents']: - keys.append(dict(Key=ob['Key'])) - - s3_client.delete_objects(Bucket=bucket_name, Delete=dict(Objects=keys)) - return True - except Exception as e: - app.logger.error(e) - return False def change_acl(self, acl): """ @@ -185,49 +185,41 @@ def change_acl(self, acl): This method is used for Soft delete data packages. :return: Status True if able to delete or False if exception """ - try: - bucket_name = app.config['S3_BUCKET_NAME'] - s3_client = app.config['S3'] + bucket_name = app.config['S3_BUCKET_NAME'] + s3_client = app.config['S3'] - keys = [] - list_objects = s3_client.list_objects(Bucket=bucket_name, - Prefix=self.build_s3_base_prefix()) - if list_objects is not None and 'Contents' in list_objects: - for ob in s3_client \ - .list_objects(Bucket=bucket_name, - Prefix=self.build_s3_base_prefix())['Contents']: - keys.append(ob['Key']) - - for key in keys: - s3_client.put_object_acl(Bucket=bucket_name, Key=key, - ACL=acl) - except Exception as e: - app.logger.error(e) - return False + keys = [] + list_objects = s3_client.list_objects(Bucket=bucket_name, + Prefix=self.build_s3_base_prefix()) + if list_objects is not None and 'Contents' in list_objects: + for ob in s3_client \ + .list_objects(Bucket=bucket_name, + Prefix=self.build_s3_base_prefix())['Contents']: + keys.append(ob['Key']) + + for key in keys: + s3_client.put_object_acl(Bucket=bucket_name, Key=key, + ACL=acl) return True def copy_to_new_version(self, version): - try: - bucket_name = app.config['S3_BUCKET_NAME'] - s3_client = app.config['S3'] - latest_keys = [] - list_objects = s3_client.list_objects(Bucket=bucket_name, - Prefix=self.build_s3_versioned_prefix()) - if list_objects is not None and 'Contents' in list_objects: - for ob in s3_client \ - .list_objects(Bucket=bucket_name, - Prefix=self.build_s3_versioned_prefix())['Contents']: - latest_keys.append(ob['Key']) - for key in latest_keys: - versioned_key = key.replace('/latest/', '/{0}/'.format(version)) - copy_source = {'Bucket': bucket_name, 'Key': key} - s3_client.copy_object(Bucket=bucket_name, - Key=versioned_key, - CopySource=copy_source) - return True - except Exception as e: - app.logger.error(e) - return False + bucket_name = app.config['S3_BUCKET_NAME'] + s3_client = app.config['S3'] + latest_keys = [] + list_objects = s3_client.list_objects(Bucket=bucket_name, + Prefix=self.build_s3_versioned_prefix()) + if list_objects is not None and 'Contents' in list_objects: + for ob in s3_client \ + .list_objects(Bucket=bucket_name, + Prefix=self.build_s3_versioned_prefix())['Contents']: + latest_keys.append(ob['Key']) + for key in latest_keys: + versioned_key = key.replace('/latest/', '/{0}/'.format(version)) + copy_source = {'Bucket': bucket_name, 'Key': key} + s3_client.copy_object(Bucket=bucket_name, + Key=versioned_key, + CopySource=copy_source) + return True @staticmethod def extract_information_from_s3_url(url): @@ -267,33 +259,29 @@ class Package(db.Model): @staticmethod def create_or_update_tag(publisher_name, package_name, tag): - try: - package = Package.query.join(Publisher)\ - .filter(Publisher.name == publisher_name, - Package.name == package_name).one() + package = Package.query.join(Publisher)\ + .filter(Publisher.name == publisher_name, + Package.name == package_name).one() - data_latest = PackageTag.query.join(Package)\ - .filter(Package.id == package.id, - PackageTag.tag == 'latest').one() + data_latest = PackageTag.query.join(Package)\ + .filter(Package.id == package.id, + PackageTag.tag == 'latest').one() - tag_instance = PackageTag.query.join(Package) \ - .filter(Package.id == package.id, - PackageTag.tag == tag).first() - - update_props = ['descriptor', 'readme', 'package_id'] - if tag_instance is None: - tag_instance = PackageTag() - - for update_prop in update_props: - setattr(tag_instance, update_prop, getattr(data_latest, update_prop)) - tag_instance.tag = tag - - db.session.add(tag_instance) - db.session.commit() - return True - except Exception as e: - app.logger.error(e) - return False + tag_instance = PackageTag.query.join(Package) \ + .filter(Package.id == package.id, + PackageTag.tag == tag).first() + + update_props = ['descriptor', 'readme', 'package_id'] + if tag_instance is None: + tag_instance = PackageTag() + + for update_prop in update_props: + setattr(tag_instance, update_prop, getattr(data_latest, update_prop)) + tag_instance.tag = tag + + db.session.add(tag_instance) + db.session.commit() + return True @staticmethod def create_or_update(name, publisher_name, **kwargs): @@ -335,17 +323,13 @@ def change_status(publisher_name, package_name, status=PackageStateEnum.active): :param status: status of the package :return: If success True else False """ - try: - data = Package.query.join(Publisher). \ - filter(Publisher.name == publisher_name, - Package.name == package_name).one() - data.status = status - db.session.add(data) - db.session.commit() - return True - except Exception as e: - app.logger.error(e) - return False + data = Package.query.join(Publisher). \ + filter(Publisher.name == publisher_name, + Package.name == package_name).one() + data.status = status + db.session.add(data) + db.session.commit() + return True @staticmethod def delete_data_package(publisher_name, package_name): @@ -356,18 +340,14 @@ def delete_data_package(publisher_name, package_name): :param package_name: package name :return: If success True else False """ - try: - data = Package.query.join(Publisher). \ - filter(Publisher.name == publisher_name, - Package.name == package_name).one() - package_id = data.id - Package.query.filter(Package.id == package_id).delete() - # db.session.delete(meta_data) - db.session.commit() - return True - except Exception as e: - app.logger.error(e) - return False + data = Package.query.join(Publisher). \ + filter(Publisher.name == publisher_name, + Package.name == package_name).one() + package_id = data.id + Package.query.filter(Package.id == package_id).delete() + # db.session.delete(meta_data) + db.session.commit() + return True @staticmethod def get_package(publisher_name, package_name): @@ -377,14 +357,10 @@ def get_package(publisher_name, package_name): :param package_name: package name :return: data package object based on the filter. """ - try: - instance = Package.query.join(Publisher) \ - .filter(Package.name == package_name, - Publisher.name == publisher_name).one() - return instance - except Exception as e: - app.logger.error(e) - return None + instance = Package.query.join(Publisher) \ + .filter(Package.name == package_name, + Publisher.name == publisher_name).one_or_none() + return instance @staticmethod def is_package_exists(publisher_name, package_name): diff --git a/app/profile/controllers.py b/app/profile/controllers.py index a12c683..c697730 100644 --- a/app/profile/controllers.py +++ b/app/profile/controllers.py @@ -6,7 +6,7 @@ from flask import Blueprint, jsonify from flask import current_app as app -from app.utils import handle_error +from app.utils import InvalidUsage from app.profile.models import Publisher profile_blueprint = Blueprint('profile', __name__, url_prefix='/api/profile') @@ -62,11 +62,7 @@ def get_publisher_profile(name): type: string default: SUCCESS """ - try: - info = Publisher.get_publisher_info(name) - if info is None: - return handle_error("NOT_FOUND", "publisher not found", 404) - return jsonify(dict(data=info, status="SUCCESS")) - except Exception as e: - app.logger.error(e) - return handle_error('GENERIC_ERROR', e.message, 500) + info = Publisher.get_publisher_info(name) + if info is None: + raise InvalidUsage("Publisher not found", 404) + return jsonify(dict(data=info, status="SUCCESS")) diff --git a/app/search/controllers.py b/app/search/controllers.py index 8a95fcc..139ac21 100644 --- a/app/search/controllers.py +++ b/app/search/controllers.py @@ -6,7 +6,6 @@ from flask import Blueprint, request, jsonify from flask import current_app as app -from app.utils import handle_error from app.search.models import DataPackageQuery search_blueprint = Blueprint('search', __name__, url_prefix='/api/search') @@ -41,15 +40,11 @@ def search_packages(): properties: type: object """ - try: - q = request.args.get('q') - if q is None: - q = '' - limit = request.args.get('limit') + q = request.args.get('q') + if q is None: + q = '' + limit = request.args.get('limit') - result = DataPackageQuery(query_string=q.strip(), - limit=limit).get_data() - return jsonify(dict(items=result, total_count=len(result))) - except Exception as e: - app.logger.error(e) - return handle_error('GENERIC_ERROR', e.message, 500) + result = DataPackageQuery(query_string=q.strip(), + limit=limit).get_data() + return jsonify(dict(items=result, total_count=len(result))) diff --git a/app/search/models.py b/app/search/models.py index a46061e..1f46ddf 100644 --- a/app/search/models.py +++ b/app/search/models.py @@ -9,7 +9,7 @@ from sqlalchemy import or_ from app.package.models import Package, PackageTag, PackageStateEnum from app.profile.models import Publisher - +from app.utils import InvalidUsage class DataPackageQuery(object): @@ -29,7 +29,7 @@ def _build_sql_query(self, query, query_filters): if filter_class == 'publisher': sa_filters.append(Publisher.name == filter_term) else: - raise Exception("not supported any other filter right now") + raise InvalidUsage("not supported any other filter right now") if len(sa_filters) > 0: sql_query = sql_query.filter(or_(*sa_filters)) diff --git a/app/site/controllers.py b/app/site/controllers.py index ff5d13c..56cf9b7 100644 --- a/app/site/controllers.py +++ b/app/site/controllers.py @@ -14,6 +14,7 @@ from app.search.models import DataPackageQuery from app.utils.helpers import text_to_markdown, dp_in_readme from BeautifulSoup import BeautifulSoup +from app.utils import InvalidUsage site_blueprint = Blueprint('site', __name__) @@ -48,15 +49,15 @@ def datapackage_show(publisher, package): Loads datapackage page for given owner """ - metadata = json.loads( - app.test_client(). \ - get('/api/package/{publisher}/{package}'. \ - format(publisher=publisher, package=package)).data) - try: - if metadata['error_code'] == 'DATA_NOT_FOUND': - return "404 Not Found", 404 - except: - pass + response = app.test_client(). \ + get('/api/package/{publisher}/{package}'. \ + format(publisher=publisher, package=package)) + + if response.status_code == 404: + raise InvalidUsage("Page Not Found", 404) + + metadata = json.loads(response.data) + packaged = Packaged(metadata) dataset = packaged.construct_dataset(request.url_root) diff --git a/tests/auth/test_controller.py b/tests/auth/test_controller.py index cdc3211..63ab345 100644 --- a/tests/auth/test_controller.py +++ b/tests/auth/test_controller.py @@ -47,7 +47,7 @@ def test_throw_400_if_user_name_and_email_is_none(self): content_type='application/json') data = json.loads(rv.data) assert rv.status_code == 400 - assert data['error_code'] == 'INVALID_INPUT' + assert data['message'] == 'User name or email both can not be empty' def test_throw_400_if_secret_is_none(self): rv = self.client.post(self.auth_token_url, @@ -58,7 +58,7 @@ def test_throw_400_if_secret_is_none(self): content_type='application/json') assert rv.status_code == 400 data = json.loads(rv.data) - assert data['error_code'] == 'INVALID_INPUT' + assert data['message'] == 'Secret can not be empty' def test_throw_404_if_user_id_do_not_exists(self): rv = self.client.post(self.auth_token_url, @@ -70,7 +70,7 @@ def test_throw_404_if_user_id_do_not_exists(self): content_type='application/json') data = json.loads(rv.data) self.assertEqual(rv.status_code, 404) - self.assertEqual(data['error_code'], 'USER_NOT_FOUND') + self.assertEqual(data['message'], 'user does not exists') def test_throw_404_if_user_email_do_not_exists(self): rv = self.client.post(self.auth_token_url, @@ -82,7 +82,7 @@ def test_throw_404_if_user_email_do_not_exists(self): content_type='application/json') data = json.loads(rv.data) self.assertEqual(rv.status_code, 404) - self.assertEqual(data['error_code'], 'USER_NOT_FOUND') + self.assertEqual(data['message'], 'user does not exists') def test_throw_403_if_user_name_and_secret_key_does_not_match(self): rv = self.client.post(self.auth_token_url, @@ -94,7 +94,7 @@ def test_throw_403_if_user_name_and_secret_key_does_not_match(self): content_type='application/json') data = json.loads(rv.data) self.assertEqual(rv.status_code, 403) - self.assertEqual(data['error_code'], 'SECRET_ERROR') + self.assertEqual(data['message'], 'Secret key do not match') def test_throw_403_if_email_and_secret_key_does_not_match(self): rv = self.client.post(self.auth_token_url, @@ -106,15 +106,15 @@ def test_throw_403_if_email_and_secret_key_does_not_match(self): content_type='application/json') data = json.loads(rv.data) self.assertEqual(rv.status_code, 403) - self.assertEqual(data['error_code'], 'SECRET_ERROR') + self.assertEqual(data['message'], 'Secret key do not match') - def test_throw_500_if_exception_occours(self): + def test_throw_400_if_bad_request(self): rv = self.client.post(self.auth_token_url, data="'username': None,", content_type='application/json') data = json.loads(rv.data) - self.assertEqual(rv.status_code, 500) - self.assertEqual(data['error_code'], 'GENERIC_ERROR') + self.assertEqual(rv.status_code, 400) + self.assertEqual(data['message'], 'Bad Request') def test_return_200_if_email_and_secret_matches(self): rv = self.client.post(self.auth_token_url, @@ -150,7 +150,9 @@ def test_throw_500_if_error_getting_user_info_from_oauth(self, authorized_respon data = json.loads(response.data) - self.assertEqual(data['error_code'], 'GENERIC_ERROR') + print (data) + + self.assertEqual(data['message'], 'Internal Server Error') self.assertEqual(response.status_code, 500) @patch('flask_oauthlib.client.OAuthRemoteApp.authorized_response') @@ -170,7 +172,7 @@ def test_throw_404_if_email_not_found(self, get_user,authorized_response): }.get(k, 'unhandled request %s'%k) response = self.client.get('/api/auth/callback?code=123') data = json.loads(response.data) - self.assertEqual(data['error_code'], 'Not Found') + self.assertEqual(data['message'], 'Email Not Found') self.assertEqual(response.status_code, 404) @patch('flask_oauthlib.client.OAuthRemoteApp.authorized_response') @@ -180,7 +182,7 @@ def test_throw_400_access_denied_if_authorized_response_is_none(self, authorized data = json.loads(response.data) - self.assertEqual(data['error_code'], 'Access Denied') + self.assertEqual(data['message'], 'Access Denied') self.assertEqual(response.status_code, 400) @patch('flask_oauthlib.client.OAuthRemoteApp.authorized_response') diff --git a/tests/package/test_controller.py b/tests/package/test_controller.py index 92b7090..3b62b82 100644 --- a/tests/package/test_controller.py +++ b/tests/package/test_controller.py @@ -30,7 +30,7 @@ def test_throw_404_if_meta_data_not_found(self): get('/api/package/%s/%s' % (self.publisher, self.package)) data = json.loads(response.data) self.assertEqual(response.status_code, 404) - self.assertEqual(data['error_code'], 'DATA_NOT_FOUND') + self.assertEqual(data['message'], 'No metadata found for the package') def test_return_200_if_meta_data_found(self): descriptor = {'name': 'test description'} @@ -383,10 +383,14 @@ def tearDown(self): class SoftDeleteTestCase(unittest.TestCase): publisher_name = 'test_publisher' + unauthorized_name = 'not_allowed_user' + package = 'test_package' url = "/api/package/{pub}/{pac}".format(pub=publisher_name, pac=package) user_id = 1 + user_id_member = 2 + user_member_name = 'test_user' jwt_url = '/api/auth/token' def setUp(self): @@ -400,16 +404,27 @@ def setUp(self): self.user.email, self.user.name, self.user.secret = \ 'test@test.com', self.publisher_name, 'super_secret' + self.user_member = User() + self.user_member.id = self.user_id_member + self.user_member.email, self.user_member.name, self.user_member.secret = \ + 'test1@test.com', self.user_member_name, 'super_secret' + self.publisher = Publisher(name=self.publisher_name) + self.publisher_1 = Publisher(name=self.user_member_name) association = PublisherUser(role=UserRoleEnum.owner) association.publisher = self.publisher + association1 = PublisherUser(role=UserRoleEnum.member) + association1.publisher = self.publisher_1 + metadata = Package(name=self.package) self.publisher.packages.append(metadata) self.user.publishers.append(association) + self.user_member.publishers.append(association1) db.session.add(self.user) + db.session.add(self.user_member) db.session.commit() response = self.client.post(self.jwt_url, data=json.dumps({ @@ -418,8 +433,16 @@ def setUp(self): }), content_type='application/json') data = json.loads(response.data) - self.jwt = data['token'] - self.auth = "%s" % self.jwt + self.auth = data['token'] + + response = self.client.post(self.jwt_url, + data=json.dumps({ + 'username': self.user_member_name, + 'secret': 'super_secret' + }), + content_type='application/json') + data = json.loads(response.data) + self.jwt_member = data['token'] @patch('app.package.models.BitStore.change_acl') @patch('app.package.models.Package.change_status') @@ -435,39 +458,46 @@ def test_return_200_if_all_goes_well(self, change_status, change_acl): def test_return_403_not_allowed_to_do_operation(self, change_status, change_acl): change_acl.return_value = True change_status.return_value = True + auth = "%s" % self.jwt_member - response = self.client.delete(self.url) + response = self.client.delete(self.url, headers=dict(Authorization=auth)) self.assertEqual(response.status_code, 403) + @patch('app.package.models.BitStore.change_acl') + @patch('app.package.models.Package.change_status') + def test_return_401_if_not_header(self, change_status, change_acl): + change_acl.return_value = True + change_status.return_value = True + + response = self.client.delete(self.url) + self.assertEqual(response.status_code, 401) + @patch('app.package.models.BitStore.change_acl') @patch('app.package.models.Package.change_status') def test_throw_500_if_change_acl_fails(self, change_status, change_acl): - change_acl.return_value = False + change_acl.side_effect = Exception('failed') change_status.return_value = True response = self.client.delete(self.url, headers=dict(Authorization=self.auth)) data = json.loads(response.data) self.assertEqual(response.status_code, 500) - self.assertEqual(data['message'], 'Failed to change acl') @patch('app.package.models.BitStore.change_acl') @patch('app.package.models.Package.change_status') def test_throw_500_if_change_status_fails(self, change_status, change_acl): change_acl.return_value = True - change_status.return_value = False + change_status.side_effect = Exception('failed') response = self.client.delete(self.url, headers=dict(Authorization=self.auth)) data = json.loads(response.data) self.assertEqual(response.status_code, 500) - self.assertEqual(data['message'], 'Failed to change status') @patch('app.package.models.BitStore.change_acl') @patch('app.package.models.Package.change_status') def test_throw_generic_error_if_internal_error(self, change_status, change_acl): change_acl.side_effect = Exception('failed') - change_status.return_value = False + change_status.side_effect = Exception('failed') response = self.client.delete(self.url, headers=dict(Authorization=self.auth)) data = json.loads(response.data) self.assertEqual(response.status_code, 500) - self.assertEqual(data['message'], 'failed') def tearDown(self): with self.app.app_context(): @@ -548,24 +578,22 @@ def test_return_200_if_all_goes_well(self, db_delete, bitstore_delete): @patch('app.package.models.BitStore.delete_data_package') @patch('app.package.models.Package.delete_data_package') def test_throw_500_if_change_acl_fails(self, db_delete, bitstore_delete): - bitstore_delete.return_value = False + bitstore_delete.side_effect = Exception('failed') db_delete.return_value = True auth = "%s" % self.jwt response = self.client.delete(self.url, headers={'Auth-Token': auth}) data = json.loads(response.data) self.assertEqual(response.status_code, 500) - self.assertEqual(data['message'], 'Failed to delete from s3') @patch('app.package.models.BitStore.delete_data_package') @patch('app.package.models.Package.delete_data_package') def test_throw_500_if_change_status_fails(self, db_delete, bitstore_delete): bitstore_delete.return_value = True - db_delete.return_value = False + db_delete.side_effect = Exception('failed') auth = "%s" % self.jwt response = self.client.delete(self.url, headers={'Auth-Token': auth}) data = json.loads(response.data) self.assertEqual(response.status_code, 500) - self.assertEqual(data['message'], 'Failed to delete from db') @patch('app.package.models.BitStore.delete_data_package') @patch('app.package.models.Package.delete_data_package') @@ -576,7 +604,6 @@ def test_throw_generic_error_if_internal_error(self, db_delete, bitstore_delete) response = self.client.delete(self.url, headers={'Auth-Token': auth}) data = json.loads(response.data) self.assertEqual(response.status_code, 500) - self.assertEqual(data['message'], 'failed') @patch('app.package.models.BitStore.delete_data_package') @patch('app.package.models.Package.delete_data_package') @@ -815,13 +842,13 @@ def test_throw_400_if_version_missing(self, create_or_update_tag, headers=dict(Authorization=self.auth)) self.assertEqual(response.status_code, 400) data = json.loads(response.data) - self.assertEqual('ATTRIBUTE_MISSING', data['error_code']) + self.assertEqual('version not found', data['message']) @patch('app.package.models.BitStore.copy_to_new_version') @patch('app.package.models.Package.create_or_update_tag') def test_throw_500_if_failed_to_tag(self, create_or_update_tag, copy_to_new_version): - copy_to_new_version.return_value = False + copy_to_new_version.side_effect = Exception('failed') create_or_update_tag.return_value = True response = self.client.post(self.url, data=json.dumps({ @@ -877,7 +904,7 @@ def test_throw_403_if_not_owner_or_member_of_publisher(self): @patch('app.package.models.Package.create_or_update_tag') def test_allow_if_member_of_publisher(self, create_or_update_tag, copy_to_new_version): - copy_to_new_version.return_value = False + copy_to_new_version.side_effect = Exception('failed') create_or_update_tag.return_value = True response = self.client.post(self.jwt_url, data=json.dumps({ diff --git a/tests/package/test_models.py b/tests/package/test_models.py index 278e07d..5491f99 100644 --- a/tests/package/test_models.py +++ b/tests/package/test_models.py @@ -149,7 +149,7 @@ def test_return_none_if_no_readme_found(self): s3.create_bucket(Bucket=bucket_name) read_me_key = bit_store.build_s3_key('test.md') s3.put_object(Bucket=bucket_name, Key=read_me_key, Body='') - self.assertEqual(bit_store.get_readme_object_key(), None) + self.assertEqual(bit_store.get_readme_object_key(), 'None') @mock_s3 def test_return_none_if_object_found(self): @@ -161,7 +161,7 @@ def test_return_none_if_object_found(self): read_me_key = bit_store.build_s3_key('test.md') s3.put_object(Bucket=bucket_name, Key=read_me_key, Body='') self.assertEqual(bit_store.get_s3_object(read_me_key + "testing"), None) - self.assertEqual(bit_store.get_s3_object(None), None) + self.assertEqual(bit_store.get_s3_object('None'), None) @mock_s3 def test_change_acl(self): @@ -405,11 +405,6 @@ def test_change_status(self): Package.name == self.package_one).one() self.assertEqual(PackageStateEnum.active, data.status) - def test_return_false_if_failed_to_change_status(self): - status = Package.change_status(self.publisher_one, 'fake_package', - status='active') - self.assertFalse(status) - def test_return_true_if_delete_data_package_success(self): status = Package.delete_data_package(self.publisher_one, self.package_one) @@ -421,11 +416,6 @@ def test_return_true_if_delete_data_package_success(self): data = Publisher.query.all() self.assertEqual(2, len(data)) - def test_return_false_if_error_occur(self): - status = Package.delete_data_package("fake_package", - self.package_one) - self.assertFalse(status) - def test_should_populate_new_versioned_data_package(self): Package.create_or_update_tag(self.publisher_one, self.package_two, 'tag_one')