From 19491d79a51fcc8579ad4ee01b78300657abdeea Mon Sep 17 00:00:00 2001 From: Nikhil Mohite Date: Fri, 21 Aug 2020 13:52:05 +0530 Subject: [PATCH] Fixed cognitive complexity issues reported by SonarQube. --- .../foreign_servers/__init__.py | 121 +++++++++++------- web/pgadmin/misc/bgprocess/processes.py | 78 +++++++---- .../tools/schema_diff/directory_compare.py | 55 ++++++-- web/pgadmin/tools/sqleditor/__init__.py | 119 +++++++++++------ 4 files changed, 247 insertions(+), 126 deletions(-) diff --git a/web/pgadmin/browser/server_groups/servers/databases/foreign_data_wrappers/foreign_servers/__init__.py b/web/pgadmin/browser/server_groups/servers/databases/foreign_data_wrappers/foreign_servers/__init__.py index 427d345f6..ccf3c5222 100644 --- a/web/pgadmin/browser/server_groups/servers/databases/foreign_data_wrappers/foreign_servers/__init__.py +++ b/web/pgadmin/browser/server_groups/servers/databases/foreign_data_wrappers/foreign_servers/__init__.py @@ -625,6 +625,63 @@ class ForeignServerView(PGChildNodeView, SchemaDiffObjectCompare): except Exception as e: return internal_server_error(errormsg=str(e)) + @staticmethod + def _validate_fsvr_options(data, old_data): + """ + Validate options for foreign server. + :param data: Data. + :param old_data: old data for get old args. + :return: return is valid ad and change option flag. + """ + required_args = [ + 'name' + ] + for arg in required_args: + if arg not in data: + data[arg] = old_data[arg] + + is_valid_added_options = is_valid_changed_options = False + if 'fsrvoptions' in data and 'added' in data['fsrvoptions']: + is_valid_added_options, data['fsrvoptions']['added'] = \ + validate_options( + data['fsrvoptions']['added'], + 'fsrvoption', + 'fsrvvalue') + + if 'fsrvoptions' in data and 'changed' in data['fsrvoptions']: + is_valid_changed_options, data['fsrvoptions']['changed'] = \ + validate_options( + data['fsrvoptions']['changed'], + 'fsrvoption', + 'fsrvvalue') + + return is_valid_added_options, is_valid_changed_options + + @staticmethod + def _get_fsvr_acl(data): + """ + Iterate and check acl type. + :param data: Data. + :return: + """ + for key in ['fsrvacl']: + if key in data and data[key] is not None: + if 'added' in data[key]: + data[key]['added'] = parse_priv_to_db( + data[key]['added'], + ['U'] + ) + if 'changed' in data[key]: + data[key]['changed'] = parse_priv_to_db( + data[key]['changed'], + ['U'] + ) + if 'deleted' in data[key]: + data[key]['deleted'] = parse_priv_to_db( + data[key]['deleted'], + ['U'] + ) + def get_sql(self, gid, sid, data, did, fid, fsid=None): """ This function will generate sql from model data. @@ -637,11 +694,6 @@ class ForeignServerView(PGChildNodeView, SchemaDiffObjectCompare): fid: foreign data wrapper ID fsid: foreign server ID """ - - required_args = [ - 'name' - ] - if fsid is not None: sql = render_template("/".join([self.template_path, self._PROPERTIES_SQL]), @@ -649,51 +701,19 @@ class ForeignServerView(PGChildNodeView, SchemaDiffObjectCompare): status, res = self.conn.execute_dict(sql) if not status: return internal_server_error(errormsg=res) - if len(res['rows']) == 0: + elif len(res['rows']) == 0: return gone(self.not_found_error_msg()) - - if res['rows'][0]['fsrvoptions'] is not None: + elif res['rows'][0]['fsrvoptions'] is not None: res['rows'][0]['fsrvoptions'] = tokenize_options( res['rows'][0]['fsrvoptions'], 'fsrvoption', 'fsrvvalue' ) - for key in ['fsrvacl']: - if key in data and data[key] is not None: - if 'added' in data[key]: - data[key]['added'] = parse_priv_to_db( - data[key]['added'], - ['U'] - ) - if 'changed' in data[key]: - data[key]['changed'] = parse_priv_to_db( - data[key]['changed'], - ['U'] - ) - if 'deleted' in data[key]: - data[key]['deleted'] = parse_priv_to_db( - data[key]['deleted'], - ['U'] - ) + ForeignServerView._get_fsvr_acl(data) old_data = res['rows'][0] - for arg in required_args: - if arg not in data: - data[arg] = old_data[arg] - - is_valid_added_options = is_valid_changed_options = False - if 'fsrvoptions' in data and 'added' in data['fsrvoptions']: - is_valid_added_options, data['fsrvoptions']['added'] =\ - validate_options( - data['fsrvoptions']['added'], - 'fsrvoption', - 'fsrvvalue') - - if 'fsrvoptions' in data and 'changed' in data['fsrvoptions']: - is_valid_changed_options, data['fsrvoptions']['changed'] =\ - validate_options( - data['fsrvoptions']['changed'], - 'fsrvoption', - 'fsrvvalue') + is_valid_added_options, \ + is_valid_changed_options = \ + ForeignServerView._validate_fsvr_options(data, old_data) sql = render_template( "/".join([self.template_path, self._UPDATE_SQL]), @@ -715,9 +735,7 @@ class ForeignServerView(PGChildNodeView, SchemaDiffObjectCompare): fdw_data = res['rows'][0] - for key in ['fsrvacl']: - if key in data and data[key] is not None: - data[key] = parse_priv_to_db(data[key], ['U']) + ForeignServerView._parse_priv(data) is_valid_options = False if 'fsrvoptions' in data: @@ -733,6 +751,17 @@ class ForeignServerView(PGChildNodeView, SchemaDiffObjectCompare): sql += "\n" return sql, data['name'] + @staticmethod + def _parse_priv(data): + """ + Parse privilege data. + :param data: Data. + :return: + """ + for key in ['fsrvacl']: + if key in data and data[key] is not None: + data[key] = parse_priv_to_db(data[key], ['U']) + @check_precondition def sql(self, gid, sid, did, fid, fsid, json_resp=True): """ diff --git a/web/pgadmin/misc/bgprocess/processes.py b/web/pgadmin/misc/bgprocess/processes.py index 362a931ec..0138394d1 100644 --- a/web/pgadmin/misc/bgprocess/processes.py +++ b/web/pgadmin/misc/bgprocess/processes.py @@ -486,6 +486,26 @@ class BatchProcess(object): 'process_state': self.process_state } + @staticmethod + def _check_start_time(p, data): + """ + Check start time and its related other timing checks. + :param p: Process. + :param data: Data + :return: + """ + if 'start_time' in data and data['start_time']: + p.start_time = data['start_time'] + + # We can't have 'exit_code' without the 'start_time' + if 'exit_code' in data and \ + data['exit_code'] is not None: + p.exit_code = data['exit_code'] + + # We can't have 'end_time' without the 'exit_code'. + if 'end_time' in data and data['end_time']: + p.end_time = data['end_time'] + @staticmethod def update_process_info(p): if p.start_time is None or p.end_time is None: @@ -499,18 +519,7 @@ class BatchProcess(object): data = json.load(fp) # First - check for the existance of 'start_time'. - if 'start_time' in data and data['start_time']: - p.start_time = data['start_time'] - - # We can't have 'exit_code' without the 'start_time' - if 'exit_code' in data and \ - data['exit_code'] is not None: - p.exit_code = data['exit_code'] - - # We can't have 'end_time' without the 'exit_code'. - if 'end_time' in data and data['end_time']: - p.end_time = data['end_time'] - + BatchProcess._check_start_time(p, data) # get the pid of the utility. if 'pid' in data: p.utility_pid = data['pid'] @@ -526,6 +535,33 @@ class BatchProcess(object): return False, False return True, False + @staticmethod + def _check_process_desc(p): + """ + Check process desc instance and return data according to process. + :param p: process + :return: return value for details, type_desc and desc related + to process + """ + desc = loads(p.desc) + details = desc + type_desc = '' + + if isinstance(desc, IProcessDesc): + args = [] + args_csv = StringIO( + p.arguments.encode('utf-8') + if hasattr(p.arguments, 'decode') else p.arguments + ) + args_reader = csv.reader(args_csv, delimiter=str(',')) + for arg in args_reader: + args = args + arg + details = desc.details(p.command, args) + type_desc = desc.type_desc + desc = desc.message + + return desc, details, type_desc + @staticmethod def list(): processes = Process.query.filter_by(user_id=current_user.id) @@ -536,8 +572,7 @@ class BatchProcess(object): status, updated = BatchProcess.update_process_info(p) if not status: continue - - if not changed: + elif not changed: changed = updated if p.start_time is None or ( @@ -551,21 +586,8 @@ class BatchProcess(object): etime = parser.parse(p.end_time or get_current_time()) execution_time = BatchProcess.total_seconds(etime - stime) - desc = loads(p.desc) - details = desc - if isinstance(desc, IProcessDesc): - args = [] - args_csv = StringIO( - p.arguments.encode('utf-8') - if hasattr(p.arguments, 'decode') else p.arguments - ) - args_reader = csv.reader(args_csv, delimiter=str(',')) - for arg in args_reader: - args = args + arg - details = desc.details(p.command, args) - type_desc = desc.type_desc - desc = desc.message + desc, details, type_desc = BatchProcess._check_process_desc(p) res.append({ 'id': p.pid, diff --git a/web/pgadmin/tools/schema_diff/directory_compare.py b/web/pgadmin/tools/schema_diff/directory_compare.py index 44e4ea709..1693ec7a9 100644 --- a/web/pgadmin/tools/schema_diff/directory_compare.py +++ b/web/pgadmin/tools/schema_diff/directory_compare.py @@ -155,6 +155,44 @@ def _get_target_list(removed, target_dict, node, target_params, view_object, return target_only +def _check_add_req_ids(source_dict, target_dict, key, temp_src_params, + temp_tgt_params): + """ + Check for Foreign Data Wrapper ID and Foreign Server ID and update it + in req parameters. + :param source_dict: Source dict for compare schema. + :param target_dict: Target dict for compare schema. + :param key: Key for get obj. + :param temp_src_params: + :param temp_tgt_params: + :return: + """ + if 'fdwid' in source_dict[key]: + temp_src_params['fdwid'] = source_dict[key]['fdwid'] + temp_tgt_params['fdwid'] = target_dict[key]['fdwid'] + # Provide Foreign Server ID + if 'fsid' in source_dict[key]: + temp_src_params['fsid'] = source_dict[key]['fsid'] + temp_tgt_params['fsid'] = target_dict[key]['fsid'] + + +def get_source_target_oid(source_dict, target_dict, key): + """ + Get source and target object ID. + :param source_dict: Source schema diff data. + :param target_dict: Target schema diff data. + :param key: Key. + :return: source and target object ID. + """ + source_object_id = None + target_object_id = None + if 'oid' in source_dict[key]: + source_object_id = source_dict[key]['oid'] + target_object_id = target_dict[key]['oid'] + + return source_object_id, target_object_id + + def _get_identical_and_different_list(intersect_keys, source_dict, target_dict, node, node_label, view_object, **kwargs): @@ -179,11 +217,9 @@ def _get_identical_and_different_list(intersect_keys, source_dict, target_dict, target_params = kwargs['target_params'] group_name = kwargs['group_name'] for key in intersect_keys: - source_object_id = None - target_object_id = None - if 'oid' in source_dict[key]: - source_object_id = source_dict[key]['oid'] - target_object_id = target_dict[key]['oid'] + source_object_id, target_object_id = get_source_target_oid(source_dict, + target_dict, + key) # Recursively Compare the two dictionary if are_dictionaries_identical(dict1[key], dict2[key], @@ -249,13 +285,8 @@ def _get_identical_and_different_list(intersect_keys, source_dict, target_dict, temp_src_params['oid'] = source_object_id temp_tgt_params['oid'] = target_object_id # Provide Foreign Data Wrapper ID - if 'fdwid' in source_dict[key]: - temp_src_params['fdwid'] = source_dict[key]['fdwid'] - temp_tgt_params['fdwid'] = target_dict[key]['fdwid'] - # Provide Foreign Server ID - if 'fsid' in source_dict[key]: - temp_src_params['fsid'] = source_dict[key]['fsid'] - temp_tgt_params['fsid'] = target_dict[key]['fsid'] + _check_add_req_ids(source_dict, target_dict, key, + temp_src_params, temp_tgt_params) source_ddl = view_object.get_sql_from_diff(**temp_src_params) diff_dependencies = view_object.get_dependencies( diff --git a/web/pgadmin/tools/sqleditor/__init__.py b/web/pgadmin/tools/sqleditor/__init__.py index e94a63f8d..ec0395bfc 100644 --- a/web/pgadmin/tools/sqleditor/__init__.py +++ b/web/pgadmin/tools/sqleditor/__init__.py @@ -634,6 +634,29 @@ def generate_client_primary_key_name(columns_info): return temp_key +def _check_and_connect(trans_obj): + """ + Check and connect to the database for transaction. + :param trans_obj: Transaction object. + :return: If any error return error with error msg, + if not then return connection object. + """ + manager = get_driver( + PG_DEFAULT_DRIVER).connection_manager(trans_obj.sid) + if hasattr(trans_obj, 'conn_id'): + conn = manager.connection(did=trans_obj.did, + conn_id=trans_obj.conn_id) + else: + conn = manager.connection(did=trans_obj.did) # default connection + + # Connect to the Server if not connected. + if not conn.connected(): + status, msg = conn.connect() + if not status: + return True, msg, conn + return False, '', conn + + @blueprint.route( '/save/', methods=["PUT", "POST"], endpoint='save' ) @@ -675,21 +698,12 @@ def save(trans_id): } ) - manager = get_driver( - PG_DEFAULT_DRIVER).connection_manager(trans_obj.sid) - if hasattr(trans_obj, 'conn_id'): - conn = manager.connection(did=trans_obj.did, - conn_id=trans_obj.conn_id) - else: - conn = manager.connection(did=trans_obj.did) # default connection + is_error, errmsg, conn = _check_and_connect(trans_obj) + if is_error: + return make_json_response( + data={'status': status, 'result': u"{}".format(errmsg)} + ) - # Connect to the Server if not connected. - if not conn.connected(): - status, msg = conn.connect() - if not status: - return make_json_response( - data={'status': status, 'result': u"{}".format(msg)} - ) status, res, query_results, _rowid = trans_obj.save( changed_data, session_obj['columns_info'], @@ -911,6 +925,52 @@ def set_limit(trans_id): return make_json_response(data={'status': status, 'result': res}) +def _check_for_transaction_before_cancel(trans_id): + """ + Check if transaction exists or not before cancel it. + :param trans_id: Transaction ID for check. + :return: return error is transaction not found, else return grid data. + """ + + if 'gridData' not in session: + return True, '' + + grid_data = session['gridData'] + + # Return from the function if transaction id not found + if str(trans_id) not in grid_data: + return True, '' + + return False, grid_data + + +def _check_and_cancel_transaction(trans_obj, delete_connection, conn, manager): + """ + Check for connection and cancel current transaction. + :param trans_obj: transaction object for cancel. + :param delete_connection: Flag for remove connection. + :param conn: Connection + :param manager: Manager + :return: Return status and result of transaction cancel. + """ + if conn.connected(): + # on successful connection cancel the running transaction + status, result = conn.cancel_transaction( + trans_obj.conn_id, trans_obj.did) + + # Delete connection if we have created it to + # cancel the transaction + if delete_connection: + manager.release(did=trans_obj.did) + else: + status = False + result = gettext( + 'Not connected to server or connection with the server has ' + 'been closed.' + ) + return status, result + + @blueprint.route( '/cancel/', methods=["PUT", "POST"], endpoint='cancel_transaction' @@ -923,17 +983,8 @@ def cancel_transaction(trans_id): Args: trans_id: unique transaction id """ - - if 'gridData' not in session: - return make_json_response( - success=0, - errormsg=gettext('Transaction ID not found in the session.'), - info='DATAGRID_TRANSACTION_REQUIRED', status=404) - - grid_data = session['gridData'] - - # Return from the function if transaction id not found - if str(trans_id) not in grid_data: + is_error, grid_data = _check_for_transaction_before_cancel(trans_id) + if is_error: return make_json_response( success=0, errormsg=gettext('Transaction ID not found in the session.'), @@ -963,21 +1014,9 @@ def cancel_transaction(trans_id): return internal_server_error(errormsg=str(msg)) delete_connection = True - if conn.connected(): - # on successful connection cancel the running transaction - status, result = conn.cancel_transaction( - trans_obj.conn_id, trans_obj.did) - - # Delete connection if we have created it to - # cancel the transaction - if delete_connection: - manager.release(did=trans_obj.did) - else: - status = False - result = gettext( - 'Not connected to server or connection with the server has ' - 'been closed.' - ) + status, result = _check_and_cancel_transaction(trans_obj, + delete_connection, conn, + manager) else: status = False result = gettext(