From b3a50524dfdb6d1ad5f2a5162b36897c23c71bf1 Mon Sep 17 00:00:00 2001
From: Mohamed Ghoneim <ghooo.ghooo@gmail.com>
Date: Mon, 27 Dec 2021 14:49:48 -0800
Subject: [PATCH] [GCU] Using simulated config instead of target config when
 validating replace operation in NoDependencyMoveValidator (#1987)

#### What I did
Using `SimulatedConfig` instead of `TargetConfig` for validating a move using `NoDependencyMoveValidator`

SimulatedConfig: config after applying the given move
TargetConfig: the final config state that's required by the patch

The problem is if the moves is to update a list item, the list item location in the `TargetConfig` might have different location in the `CurrentConfig`.

The reason for that is the `TargetConfig` has the final outcome after applying the `patch`, but the move might be just making a small change towards this goal.

Example:
Assume current_config
```
{
    "VLAN": {
        "Vlan100": {
            "vlanid": "100",
            "dhcp_servers": [
                "192.0.0.1",
                "192.0.0.2"
            ]
        }
    }
}
```
TargetConfig:
```
{
    "VLAN": {
        "Vlan100": {
            "vlanid": "100",
            "dhcp_servers": [
                "192.0.0.3"
            ]
        }
    }
}
```
Move:
```python
ps.JsonMove(diff, OperationType.REPLACE, ["VLAN", "Vlan100", "dhcp_servers", 1], ["VLAN", "Vlan100", "dhcp_servers", 0])
```


The move means:
```
Replace the value in CurrentConfig that has path `/VLAN/Vlan100/dhcp_servers/1` with
the value from the TargetConfig that has the path `/VLAN/Vlan100/dhcp_servers/0`
```
Notice how the array index in CurrentConfig does not exist in TargetConfig

Instead of using TargetConfig to validate, use SimulatedConfig which is the config after applying the move. In this case it would be:
```
{
    "VLAN": {
        "Vlan100": {
            "vlanid": "100",
            "dhcp_servers": [
                "192.0.0.1",
                "192.0.0.3"
            ]
        }
    }
}
```

#### How I did it
Replace `diff.target_config` with `simulated_config`

#### How to verify it
added a unit-test
---
 generic_config_updater/patch_sorter.py        |  6 ++--
 .../patch_sorter_test.py                      | 31 +++++++++++++++++++
 2 files changed, 35 insertions(+), 2 deletions(-)

diff --git a/generic_config_updater/patch_sorter.py b/generic_config_updater/patch_sorter.py
index 1b1e5e1dd3fb..ca750723187d 100644
--- a/generic_config_updater/patch_sorter.py
+++ b/generic_config_updater/patch_sorter.py
@@ -551,10 +551,12 @@ def _validate_replace(self, move, diff):
         simulated_config = move.apply(diff.current_config)
         deleted_paths, added_paths = self._get_paths(diff.current_config, simulated_config, [])
 
+        # For deleted paths, we check the current config has no dependencies between nodes under the removed path
         if not self._validate_paths_config(deleted_paths, diff.current_config):
             return False
 
-        if not self._validate_paths_config(added_paths, diff.target_config):
+        # For added paths, we check the simulated config has no dependencies between nodes under the added path
+        if not self._validate_paths_config(added_paths, simulated_config):
             return False
 
         return True
@@ -1020,7 +1022,7 @@ def __init__(self, move_wrapper):
         self.move_wrapper = move_wrapper
         self.mem = {}
 
-    def rec(self, diff):
+    def sort(self, diff):
         if diff.has_no_diff():
             return []
 
diff --git a/tests/generic_config_updater/patch_sorter_test.py b/tests/generic_config_updater/patch_sorter_test.py
index 72275f2d95fe..cd9786fdf1fe 100644
--- a/tests/generic_config_updater/patch_sorter_test.py
+++ b/tests/generic_config_updater/patch_sorter_test.py
@@ -1054,6 +1054,37 @@ def test_validate__replace_whole_config_item_same_ref_same__true(self):
         # Act and assert
         self.assertTrue(self.validator.validate(move, diff))
 
+    def test_validate__replace_list_item_different_location_than_target_and_no_deps__true(self):
+        # Arrange
+        current_config = {
+            "VLAN": {
+                "Vlan100": {
+                    "vlanid": "100",
+                    "dhcp_servers": [
+                        "192.0.0.1",
+                        "192.0.0.2"
+                    ]
+                }
+            }
+        }
+        target_config = {
+            "VLAN": {
+                "Vlan100": {
+                    "vlanid": "100",
+                    "dhcp_servers": [
+                        "192.0.0.3"
+                    ]
+                }
+            }
+        }
+        diff = ps.Diff(current_config, target_config)
+        # the target tokens point to location 0 which exist in target_config
+        # but the replace operation is operating on location 1 in current_config
+        move = ps.JsonMove(diff, OperationType.REPLACE, ["VLAN", "Vlan100", "dhcp_servers", 1], ["VLAN", "Vlan100", "dhcp_servers", 0])
+
+        # Act and assert
+        self.assertTrue(self.validator.validate(move, diff))
+
     def prepare_config(self, config, patch):
         return patch.apply(config)