From f7cb058c8eb0c20e048d2afd04358969b7823e16 Mon Sep 17 00:00:00 2001
From: Bryn Cooke <BrynCooke@gmail.com>
Date: Mon, 28 Nov 2022 09:56:31 +0000
Subject: [PATCH] Fixes #2123 (#2162)
MIME-Version: 1.0
Content-Type: text/plain; charset=UTF-8
Content-Transfer-Encoding: 8bit

Issue was introduced with #2116 but no release had this in.

Move operations would insert data in the config due to the delete magic
value always getting added. Now we check before adding such values.

We may need to move to fluvio-jolt longer term.

<!--
First, 🌠 thank you 🌠 for considering a contribution to Apollo!

Some of this information is also included in the /CONTRIBUTING.md file
at the
root of this repository.  We suggest you read it!

  https://github.com/apollographql/router/blob/HEAD/CONTRIBUTING.md

Here are some important details to keep in mind:

* ⏰ Your time is important
To save your precious time, if the contribution you are making will
take more than an hour, please make sure it has been discussed in an
        issue first. This is especially true for feature requests!

* 💡 Features
Feature requests can be created and discussed within a GitHub Issue.
Be sure to search for existing feature requests (and related issues!)
prior to opening a new request. If an existing issue covers the need,
please upvote that issue by using the 👍 emote, rather than opening a
        new issue.

* 🕷 Bug fixes
These can be created and discussed in this repository. When fixing a
bug,
please _try_ to add a test which verifies the fix. If you cannot, you
should
still submit the PR but we may still ask you (and help you!) to create a
test.

* 📖 Contribution guidelines
Follow https://github.com/apollographql/router/blob/HEAD/CONTRIBUTING.md
when submitting a pull request. Make sure existing tests still pass, and
add
        tests for all new behavior.

* ✏️ Explain your pull request
Describe the big picture of your changes here to communicate to what
        your pull request is meant to accomplish. Provide 🔗 links 🔗 to
associated issues! Documentation in the docs/ directory should be
updated
        as necessary.  Finally, a /CHANGELOG.md entry should be added.

We hope you will find this to be a positive experience! Contribution can
be
intimidating and we hope to alleviate that pain as much as possible.
Without
following these guidelines, you may be missing context that can help you
succeed
with your contribution, which is why we encourage discussion first.
Ultimately,
there is no guarantee that we will be able to merge your pull-request,
but by
following these guidelines we can try to avoid disappointment.

-->

Co-authored-by: bryn <bryn@apollographql.com>
---
 NEXT_CHANGELOG.md                             |  2 +-
 ...pgrade__test__move_non_existent_field.snap |  7 +++
 apollo-router/src/configuration/upgrade.rs    | 54 ++++++++++++++-----
 3 files changed, 50 insertions(+), 13 deletions(-)
 create mode 100644 apollo-router/src/configuration/snapshots/apollo_router__configuration__upgrade__test__move_non_existent_field.snap

diff --git a/NEXT_CHANGELOG.md b/NEXT_CHANGELOG.md
index c86d9ac0e8..1508b75baa 100644
--- a/NEXT_CHANGELOG.md
+++ b/NEXT_CHANGELOG.md
@@ -186,7 +186,7 @@ From the CLI users can run:
 
 There are situations where comments and whitespace are not preserved. This may be improved in future.
 
-By [@bryncooke](https://github.com/bryncooke) in https://github.com/apollographql/router/pull/2116
+By [@bryncooke](https://github.com/bryncooke) in https://github.com/apollographql/router/pull/2116, https://github.com/apollographql/router/pull/2162
 
 ### *Experimental* subgraph request retry ([Issue #338](https://github.com/apollographql/router/issues/338), [Issue #1956](https://github.com/apollographql/router/issues/1956))
 
diff --git a/apollo-router/src/configuration/snapshots/apollo_router__configuration__upgrade__test__move_non_existent_field.snap b/apollo-router/src/configuration/snapshots/apollo_router__configuration__upgrade__test__move_non_existent_field.snap
new file mode 100644
index 0000000000..58f3973636
--- /dev/null
+++ b/apollo-router/src/configuration/snapshots/apollo_router__configuration__upgrade__test__move_non_existent_field.snap
@@ -0,0 +1,7 @@
+---
+source: apollo-router/src/configuration/upgrade.rs
+expression: "apply_migration(&json!({ \"should\" : \"stay\" }),\n        &Migration::builder().action(Action::Move {\n                            from: \"obj.field1\".to_string(),\n                            to: \"new.obj.field1\".to_string(),\n                        }).description(\"move field1\").build()).expect(\"expected successful migration\")"
+---
+{
+  "should": "stay"
+}
diff --git a/apollo-router/src/configuration/upgrade.rs b/apollo-router/src/configuration/upgrade.rs
index 9ed15c5ea8..43c8ff40ab 100644
--- a/apollo-router/src/configuration/upgrade.rs
+++ b/apollo-router/src/configuration/upgrade.rs
@@ -70,22 +70,37 @@ fn apply_migration(config: &Value, migration: &Migration) -> Result<Value, Confi
     for action in &migration.actions {
         match action {
             Action::Delete { path } => {
-                // Deleting isn't actually supported by protus so we add a magic value to delete later
-                transformer_builder = transformer_builder.add_action(
-                    Parser::parse(REMOVAL_EXPRESSION, path).expect("migration must be valid"),
-                );
+                if !jsonpath_lib::select(config, &format!("$.{}", path))
+                    .unwrap_or_default()
+                    .is_empty()
+                {
+                    // Deleting isn't actually supported by protus so we add a magic value to delete later
+                    transformer_builder = transformer_builder.add_action(
+                        Parser::parse(REMOVAL_EXPRESSION, path).expect("migration must be valid"),
+                    );
+                }
             }
             Action::Copy { from, to } => {
-                transformer_builder = transformer_builder
-                    .add_action(Parser::parse(from, to).expect("migration must be valid"));
+                if !jsonpath_lib::select(config, &format!("$.{}", from))
+                    .unwrap_or_default()
+                    .is_empty()
+                {
+                    transformer_builder = transformer_builder
+                        .add_action(Parser::parse(from, to).expect("migration must be valid"));
+                }
             }
             Action::Move { from, to } => {
-                transformer_builder = transformer_builder
-                    .add_action(Parser::parse(from, to).expect("migration must be valid"));
-                // Deleting isn't actually supported by protus so we add a magic value to delete later
-                transformer_builder = transformer_builder.add_action(
-                    Parser::parse(REMOVAL_EXPRESSION, from).expect("migration must be valid"),
-                );
+                if !jsonpath_lib::select(config, &format!("$.{}", from))
+                    .unwrap_or_default()
+                    .is_empty()
+                {
+                    transformer_builder = transformer_builder
+                        .add_action(Parser::parse(from, to).expect("migration must be valid"));
+                    // Deleting isn't actually supported by protus so we add a magic value to delete later
+                    transformer_builder = transformer_builder.add_action(
+                        Parser::parse(REMOVAL_EXPRESSION, from).expect("migration must be valid"),
+                    );
+                }
             }
         }
     }
@@ -258,6 +273,21 @@ mod test {
         .expect("expected successful migration"));
     }
 
+    #[test]
+    fn move_non_existent_field() {
+        insta::assert_json_snapshot!(apply_migration(
+            &json!({"should": "stay"}),
+            &Migration::builder()
+                .action(Action::Move {
+                    from: "obj.field1".to_string(),
+                    to: "new.obj.field1".to_string()
+                })
+                .description("move field1")
+                .build(),
+        )
+        .expect("expected successful migration"));
+    }
+
     #[test]
     fn move_array_element() {
         insta::assert_json_snapshot!(apply_migration(