From c554f6da1ecf8394074efbe03fafebe73333cb82 Mon Sep 17 00:00:00 2001 From: ghooo Date: Tue, 22 Jun 2021 02:29:27 -0700 Subject: [PATCH] addressing comments --- generic_config_updater/gu_common.py | 67 +++++++----------------- generic_config_updater/patch_sorter.py | 72 +++++++++++++------------- 2 files changed, 56 insertions(+), 83 deletions(-) diff --git a/generic_config_updater/gu_common.py b/generic_config_updater/gu_common.py index 4f4aa3bf93..37515bd848 100644 --- a/generic_config_updater/gu_common.py +++ b/generic_config_updater/gu_common.py @@ -20,7 +20,7 @@ class JsonChange: It provides a single function to apply the change to a given JSON object. """ def __init__(self, patch): - self.patch=patch + self.patch = patch def apply(self, config): return self.patch.apply(config) @@ -230,7 +230,7 @@ def get_path_tokens(self, path): # src: https://tools.ietf.org/html/rfc6901 mod_tokens = [] for token in tokens: - mod_token = str(token) + mod_token = token mod_token = mod_token.replace("~0", "~") mod_token = mod_token.replace("~1", "/") mod_tokens.append(mod_token) @@ -312,7 +312,7 @@ def _get_xpath_single_quote_str_end(self, start, xpath): # libyang implements XPATH 1.0 which does not escape single quotes # libyang src: https://netopeer.liberouter.org/doc/libyang/master/html/howtoxpath.html # XPATH 1.0 src: https://www.w3.org/TR/1999/REC-xpath-19991116/#NT-Literal - idx=idx+1 + idx = idx+1 return idx @@ -324,7 +324,7 @@ def _get_xpath_double_quote_str_end(self, start, xpath): # libyang implements XPATH 1.0 which does not escape double quotes # libyang src: https://netopeer.liberouter.org/doc/libyang/master/html/howtoxpath.html # XPATH 1.0 src: https://www.w3.org/TR/1999/REC-xpath-19991116/#NT-Literal - idx=idx+1 + idx = idx+1 return idx @@ -392,7 +392,7 @@ def _find_leafref_paths(self, path, config): ref_xpaths = [] for xpath in leaf_xpaths: - ref_xpaths.extend(self._find_leaf_dependencies(sy, xpath)) + ref_xpaths.extend(sy.find_data_dependencies(xpath)) ref_paths = [] for ref_xpath in ref_xpaths: @@ -402,7 +402,7 @@ def _find_leafref_paths(self, path, config): return set(ref_paths) def _get_inner_leaf_xpaths(self, xpath, sy): - if xpath=="/": # Point to Root element which contains all xpaths + if xpath == "/": # Point to Root element which contains all xpaths nodes = sy.root.tree_for() else: # Otherwise get all nodes that match xpath nodes = sy.root.find_path(xpath).data() @@ -415,37 +415,7 @@ def _get_inner_leaf_xpaths(self, xpath, sy): def _is_leaf_node(self, node): schema = node.schema() - return ly.LYS_LEAF == schema.nodetype() # or ly.LYS_LEAFLIST == schema.nodetype() - - # TODO: YANG SONiC lib has this function but with a bug in handling null backlinks. - # It is OK to have null backlinks if there is no backlinks. - def _find_leaf_dependencies(self, sy, data_xpath): - ref_list = [] - node = sy.root - try: - data_node = sy._find_data_node(data_xpath) - except Exception as e: - print("find_data_dependencies(): Failed to find data node from xpath: {}".format(data_xapth)) - return ref_list - - try: - value = str(sy._find_data_node_value(data_xpath)) - - schema_node = ly.Schema_Node_Leaf(data_node.schema()) - backlinks = schema_node.backlinks() - if backlinks is not None and backlinks.number() > 0: - for link in backlinks.schema(): - node_set = node.find_path(link.path()) - for data_set in node_set.data(): - data_set.schema() - casted = data_set.subtype() - if value == casted.value_str(): - ref_list.append(data_set.path()) - except Exception as e: - raise GenericConfigUpdaterError("Failed to find node or dependencies for \ - {}\n{}".format(data_xpath, str(e))) - - return ref_list + return ly.LYS_LEAF == schema.nodetype() def convert_path_to_xpath(self, path, config, sy): """ @@ -500,10 +470,10 @@ def _get_xpath_tokens_from_container(self, model, token_index, path_tokens, conf def _get_xpath_tokens_from_list(self, model, token_index, path_tokens, config): list_name = model['@name'] - tableKey=path_tokens[token_index] + tableKey = path_tokens[token_index] listKeys = model['key']['@value'] keyDict = self._extractKey(tableKey, listKeys) - keyTokens=[f"[{key}='{keyDict[key]}']" for key in keyDict] + keyTokens = [f"[{key}='{keyDict[key]}']" for key in keyDict] item_token = f"{list_name}{''.join(keyTokens)}" xpath_tokens = [item_token] @@ -512,7 +482,7 @@ def _get_xpath_tokens_from_list(self, model, token_index, path_tokens, config): # Example: # path: /VLAN/Vlan1000 # xpath: /sonic-vlan:sonic-vlan/VLAN/VLAN_LIST[name='Vlan1000'] - if len(path_tokens)-1==token_index: + if len(path_tokens)-1 == token_index: return xpath_tokens new_xpath_tokens = self._get_xpath_tokens_from_leaf(model, token_index+1, path_tokens,config[path_tokens[token_index]]) @@ -544,10 +514,13 @@ def _get_xpath_tokens_from_leaf(self, model, token_index, path_tokens, config): # Example: # path: /VLAN/Vlan1000/dhcp_servers # xpath: /sonic-vlan:sonic-vlan/VLAN/VLAN_LIST[name='Vlan1000']/dhcp_servers - if len(path_tokens)-1==token_index: + if len(path_tokens)-1 == token_index: return [token] list_config = config[token] - value=list_config[int(path_tokens[token_index+1])] + value = list_config[int(path_tokens[token_index+1])] + # To get a leaf-list instance with the value 'val' + # /module-name:container/leaf-list[.='val'] + # Source: Check examples in https://netopeer.liberouter.org/doc/libyang/master/html/howto_x_path.html return [f"{token}[.='{value}']"] raise ValueError("Token not found") @@ -571,11 +544,11 @@ def _get_list_model(self, model, token_index, path_tokens): clist = model.get('list') # Container contains a single list, just return it # TODO: check if matching also by name is necessary - if isinstance(clist, dict): # and clist['@name'] == token+"_LIST": + if isinstance(clist, dict): return clist if isinstance(clist, list): - configdb_values_str=path_tokens[token_index+1] + configdb_values_str = path_tokens[token_index+1] # Format: "value1|value2|value|..." configdb_values = configdb_values_str.split("|") for list_model in clist: @@ -663,13 +636,13 @@ def _get_path_tokens_from_list(self, model, token_index, xpath_tokens, config): if len(xpath_tokens)-1 == token_index: return path_tokens - nxt_token = xpath_tokens[token_index+1] + next_token = xpath_tokens[token_index+1] # if the target node is a key, then it does not have a correspondene to path. # Just return the current 'key1|key2|..' token as it already refers to the keys # Example where the target node is 'name' which is a key in VLAN_MEMBER_LIST: # xpath: /sonic-vlan:sonic-vlan/VLAN_MEMBER/VLAN_MEMBER_LIST[name='Vlan1000'][port='Ethernet8']/name # path: /VLAN_MEMBER/Vlan1000|Ethernet8 - if nxt_token in key_dict: + if next_token in key_dict: return path_tokens new_path_tokens = self._get_path_tokens_from_leaf(model, token_index+1, xpath_tokens, config[path_token]) @@ -730,7 +703,7 @@ def _extract_key_dict(self, list_token): # the findall groups would be ('name', 'Vlan1000'), ('port', 'Ethernet8') key_value_pattern = "\[([^=]+)='([^']*)'\]" matches = re.findall(key_value_pattern, all_key_value) - key_dict={} + key_dict = {} for item in matches: key = item[0] value = item[1] diff --git a/generic_config_updater/patch_sorter.py b/generic_config_updater/patch_sorter.py index e773936615..8bf99ba004 100644 --- a/generic_config_updater/patch_sorter.py +++ b/generic_config_updater/patch_sorter.py @@ -51,19 +51,19 @@ class JsonMove: """ def __init__(self, diff, op_type, current_config_tokens, target_config_tokens=None): operation = JsonMove._to_jsonpatch_operation(diff, op_type, current_config_tokens, target_config_tokens) - self.patch=jsonpatch.JsonPatch([operation]) - self.op_type=operation[OperationWrapper.OP_KEYWORD] - self.path=operation[OperationWrapper.PATH_KEYWORD] - self.value=operation.get(OperationWrapper.VALUE_KEYWORD, None) + self.patch = jsonpatch.JsonPatch([operation]) + self.op_type = operation[OperationWrapper.OP_KEYWORD] + self.path = operation[OperationWrapper.PATH_KEYWORD] + self.value = operation.get(OperationWrapper.VALUE_KEYWORD, None) - self.op_type=op_type - self.current_config_tokens=current_config_tokens - self.target_config_tokens=target_config_tokens + self.op_type = op_type + self.current_config_tokens = current_config_tokens + self.target_config_tokens = target_config_tokens @staticmethod def _to_jsonpatch_operation(diff, op_type, current_config_tokens, target_config_tokens): - operation_wrapper=OperationWrapper() - path_addressing=PathAddressing() + operation_wrapper = OperationWrapper() + path_addressing = PathAddressing() if op_type == OperationType.REMOVE: path = path_addressing.create_path(current_config_tokens) @@ -82,7 +82,7 @@ def _to_jsonpatch_operation(diff, op_type, current_config_tokens, target_config_ @staticmethod def _get_value(config, tokens): for token in tokens: - config=config[token] + config = config[token] return copy.deepcopy(config) @@ -113,8 +113,8 @@ def _to_jsonpatch_add_operation(diff, current_config_tokens, target_config_token Instead we convert to: {"op":"add", "path":"/dict1", "value":{"key11": "value11"}} """ - operation_wrapper=OperationWrapper() - path_addressing=PathAddressing() + operation_wrapper = OperationWrapper() + path_addressing = PathAddressing() # if path refers to whole config i.e. no tokens, then just create the operation if not current_config_tokens: @@ -208,7 +208,7 @@ def _to_jsonpatch_add_operation(diff, current_config_tokens, target_config_token break if not(was_list) and token not in current_ptr: break - current_ptr=current_ptr[token] + current_ptr = current_ptr[token] op_type = OperationType.ADD new_path = path_addressing.create_path(new_tokens) @@ -227,7 +227,7 @@ def from_patch(patch): @staticmethod def from_operation(operation): - path_addressing=PathAddressing() + path_addressing = PathAddressing() op_type = OperationType[operation[OperationWrapper.OP_KEYWORD].upper()] path = operation[OperationWrapper.PATH_KEYWORD] if op_type in [OperationType.ADD, OperationType.REPLACE]: @@ -242,10 +242,10 @@ def from_operation(operation): current_config = {} current_config_ptr = current_config for token in tokens[:-1]: - target_config_ptr[token]={} - current_config_ptr[token]={} - target_config_ptr=target_config_ptr[token] - current_config_ptr=current_config_ptr[token] + target_config_ptr[token] = {} + current_config_ptr[token] = {} + target_config_ptr = target_config_ptr[token] + current_config_ptr = current_config_ptr[token] if tokens: target_config_ptr[tokens[-1]] = value @@ -253,7 +253,7 @@ def from_operation(operation): # whole-config, just use value target_config = value - current_config_tokens=tokens + current_config_tokens = tokens if op_type in [OperationType.ADD, OperationType.REPLACE]: target_config_tokens = tokens else: @@ -279,7 +279,7 @@ def __eq__(self, other): return False def __hash__(self): - return hash(self.op_type) ^ hash(self.path) ^ hash(json.dumps(self.value)) + return hash((self.op_type, self.path, json.dumps(self.value))) class MoveWrapper: def __init__(self, move_generators, move_extenders, move_validators): @@ -339,7 +339,7 @@ class FullConfigMoveValidator: A class to validate that full config is valid according to YANG models after applying the move. """ def __init__(self, config_wrapper): - self.config_wrapper=config_wrapper + self.config_wrapper = config_wrapper def validate(self, move, diff): simulated_config = move.apply(diff.current_config) @@ -376,7 +376,7 @@ class CreateOnlyMoveValidator: a modified create-only field. """ def __init__(self, path_addressing): - self.path_addressing=path_addressing + self.path_addressing = path_addressing def validate(self, move, diff): if move.op_type != OperationType.REPLACE: @@ -416,10 +416,10 @@ def _value_exist_but_different(self, tokens, current_config_ptr, simulated_confi if mod_token not in simulated_config_ptr: return False - current_config_ptr=current_config_ptr[mod_token] - simulated_config_ptr=simulated_config_ptr[mod_token] + current_config_ptr = current_config_ptr[mod_token] + simulated_config_ptr = simulated_config_ptr[mod_token] - return current_config_ptr!=simulated_config_ptr + return current_config_ptr != simulated_config_ptr class NoDependencyMoveValidator: """ @@ -428,8 +428,8 @@ class NoDependencyMoveValidator: way dependent configs are never updated together. """ def __init__(self, path_addressing, config_wrapper): - self.path_addressing=path_addressing - self.config_wrapper=config_wrapper + self.path_addressing = path_addressing + self.config_wrapper = config_wrapper def validate(self, move, diff): operation_type = move.op_type @@ -579,7 +579,7 @@ class LowLevelMoveGenerator: where the path of the move does not have children. """ def __init__(self, path_addressing): - self.path_addressing=path_addressing + self.path_addressing = path_addressing def generate(self, diff): single_run_generator = SingleRunLowLevelMoveGenerator(diff, self.path_addressing) for move in single_run_generator.generate(): @@ -591,7 +591,7 @@ class SingleRunLowLevelMoveGenerator: """ def __init__(self, diff, path_addressing): self.diff = diff - self.path_addressing=path_addressing + self.path_addressing = path_addressing def generate(self): current_ptr = self.diff.current_config @@ -848,7 +848,7 @@ class DeleteRefsMoveExtender: A class to extend the given DELETE move by adding DELETE moves to configs referring to the path in the move. """ def __init__(self, path_addressing): - self.path_addressing=path_addressing + self.path_addressing = path_addressing def extend(self, move, diff): operation_type = move.op_type @@ -950,15 +950,15 @@ def rec(self, diff): return bst_moves class Algorithm(Enum): - DFS=1 - BFS=2 - MEMOIZATION=3 + DFS = 1 + BFS = 2 + MEMOIZATION = 3 class SortAlgorithmFactory: def __init__(self, operation_wrapper, config_wrapper, path_addressing): - self.operation_wrapper=operation_wrapper - self.config_wrapper=config_wrapper - self.path_addressing=path_addressing + self.operation_wrapper = operation_wrapper + self.config_wrapper = config_wrapper + self.path_addressing = path_addressing def create(self, algorithm=Algorithm.DFS): move_generators = [LowLevelMoveGenerator(self.path_addressing)]