Skip to content

Commit 82ee5d6

Browse files
authored
Fix JsonMove._get_value to Support Both String and Integer List Indices (#283)
<!-- Please make sure you've read and understood our contributing guidelines: https://github.com/Azure/SONiC/blob/gh-pages/CONTRIBUTING.md failure_prs.log skip_prs.log Make sure all your commits include a signature generated with `git commit -s` ** If this is a bug fix, make sure your description includes "closes #xxxx", "fixes #xxxx" or "resolves #xxxx" so that GitHub automatically closes the related issue when the PR is merged. If you are adding/modifying/removing any command or utility script, please also make sure to add/modify/remove any unit tests from the tests directory as appropriate. If you are modifying or removing an existing 'show', 'config' or 'sonic-clear' subcommand, or you are adding a new subcommand, please make sure you also update the Command Line Reference Guide (doc/Command-Reference.md) to reflect your changes. Please provide the following information: --> ### What I did: Issue: sonic-net/sonic-utilities#4221 - Updated [JsonMove._get_value](vscode-file://vscode-app/c:/Program%20Files/Microsoft%20VS%20Code/resources/app/out/vs/code/electron-browser/workbench/workbench.html) to handle both string and integer indices when traversing lists in config data. - Adjusted related unit tests to reflect the new behavior. ### How I did it: - Modified the traversal logic to convert string tokens to integers when accessing lists, allowing both "1" and 1 as valid indices. - Removed the test expecting a TypeError for integer indices and added assertions for both string and integer index access. ### How to verify it: Patched change in lab device, confirmed. ```shell admin@STR-SN5640-RDMA-1:~$ cat /usr/local/lib/python3.11/dist-packages/generic_config_updater/patch_sorter.py | grep -C 2 "int(token)" for token in tokens: if isinstance(config, list): token = int(token) config = config[token] admin@STR-SN5640-RDMA-1:~$ cat t_tc_to_queue_map_modify.json [ { "op": "replace", "path": "/TC_TO_QUEUE_MAP/AZURE/8", "value": "8" }, { "op": "add", "path": "/TC_TO_QUEUE_MAP/AZURE/7", "value": "7" } ] admin@STR-SN5640-RDMA-1:~$ sudo config apply-patch -v t_tc_to_queue_map_modify.json Patch Applier: localhost: Patch application starting. Patch Applier: localhost: Patch: [{"op": "replace", "path": "/TC_TO_QUEUE_MAP/AZURE/8", "value": "8"}, {"op": "add", "path": "/TC_TO_QUEUE_MAP/AZURE/7", "value": "7"}] Patch Applier: localhost getting current config db. Patch Applier: localhost: simulating the target full config after applying the patch. Patch Applier: localhost: validating all JsonPatch operations are permitted on the specified fields Patch Applier: localhost: validating target config does not have empty tables, since they do not show up in ConfigDb. Patch Applier: localhost: sorting patch updates. Patch Sorter - Strict: Validating patch is not making changes to tables without YANG models. Patch Sorter - Strict: Validating target config according to YANG models. Patch Sorter - Strict: Sorting patch updates. Patch Applier: The localhost patch was converted into 1 change: Patch Applier: localhost: applying 1 change in order: Patch Applier: failure_prs.log skip_prs.log [{"op": "replace", "path": "/TC_TO_QUEUE_MAP/AZURE/7", "value": "7"}, {"op": "replace", "path": "/TC_TO_QUEUE_MAP/AZURE/8", "value": "8"}] Patch Applier: localhost: verifying patch updates are reflected on ConfigDB. Patch Applier: localhost patch application completed. Patch applied successfully. ``` Also run the updated unit tests and all tests should pass, confirming the fix. #### Previous command output (if the output of a command-line utility has changed) #### New command output (if the output of a command-line utility has changed)
1 parent 8bd0058 commit 82ee5d6

2 files changed

Lines changed: 56 additions & 2 deletions

File tree

generic_config_updater/patch_sorter.py

Lines changed: 1 addition & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -98,10 +98,9 @@ def _to_jsonpatch_operation(self, diff, op_type, current_config_tokens, target_c
9898
@staticmethod
9999
def _get_value(config, tokens):
100100
for token in tokens:
101-
if isinstance(token, str) and token.isnumeric():
101+
if isinstance(config, list):
102102
token = int(token)
103103
config = config[token]
104-
105104
return copy.deepcopy(config)
106105

107106
@staticmethod
Lines changed: 55 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,55 @@
1+
import unittest
2+
from generic_config_updater.patch_sorter import JsonMove
3+
4+
5+
class TestJsonMoveGetValue(unittest.TestCase):
6+
def setUp(self):
7+
self.config = {
8+
"table1": {
9+
"key1": {
10+
"field": "value1"
11+
},
12+
"1": {
13+
"field": "value2"
14+
}
15+
},
16+
"table2": [
17+
{"name": "item0"},
18+
{"name": "item1"}
19+
]
20+
}
21+
22+
def test_get_value_dict(self):
23+
tokens = ["table1", "key1", "field"]
24+
self.assertEqual(JsonMove._get_value(self.config, tokens), "value1")
25+
26+
def test_get_value_dict_numeric_string(self):
27+
tokens = ["table1", "1", "field"]
28+
self.assertEqual(JsonMove._get_value(self.config, tokens), "value2")
29+
30+
def test_get_value_list(self):
31+
# Should allow both int and string index for list
32+
tokens = ["table2", 1, "name"]
33+
self.assertEqual(JsonMove._get_value(self.config, tokens), "item1")
34+
tokens = ["table2", "1", "name"]
35+
self.assertEqual(JsonMove._get_value(self.config, tokens), "item1")
36+
37+
def test_get_value_missing_key(self):
38+
tokens = ["table1", "not_exist"]
39+
with self.assertRaises(KeyError):
40+
JsonMove._get_value(self.config, tokens)
41+
42+
def test_get_value_list_invalid_index(self):
43+
tokens = ["table2", "10", "name"]
44+
with self.assertRaises(IndexError):
45+
JsonMove._get_value(self.config, tokens)
46+
47+
def test_get_value_non_container(self):
48+
tokens = ["table1", "key1", "field", "extra"]
49+
with self.assertRaises(TypeError):
50+
JsonMove._get_value(self.config, tokens)
51+
52+
def test_get_value_dict_numeric_keys(self):
53+
self.config["7"] = {"8": "30"}
54+
tokens = ["7", "8"]
55+
self.assertEqual(JsonMove._get_value(self.config, tokens), "30")

0 commit comments

Comments
 (0)