From 2603ba2a24c231a797cfe7b7d441849f7488628c Mon Sep 17 00:00:00 2001 From: bryn Date: Fri, 25 Nov 2022 17:23:19 +0000 Subject: [PATCH 1/2] Fixes #2123 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. --- NEXT_CHANGELOG.md | 2 +- apollo-router/src/configuration/upgrade.rs | 54 +++++++++++++++++----- 2 files changed, 43 insertions(+), 13 deletions(-) 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/upgrade.rs b/apollo-router/src/configuration/upgrade.rs index 9ed15c5ea8..52ca692ed2 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 { - // 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, 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, 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, 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!({}), + &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( From 8d7e7c8a4625a4b57e4202f2e2f70ca45cfdf7e7 Mon Sep 17 00:00:00 2001 From: bryn Date: Mon, 28 Nov 2022 09:22:59 +0000 Subject: [PATCH 2/2] Fix tests --- ...iguration__upgrade__test__move_non_existent_field.snap | 7 +++++++ apollo-router/src/configuration/upgrade.rs | 8 ++++---- 2 files changed, 11 insertions(+), 4 deletions(-) create mode 100644 apollo-router/src/configuration/snapshots/apollo_router__configuration__upgrade__test__move_non_existent_field.snap 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 52ca692ed2..43c8ff40ab 100644 --- a/apollo-router/src/configuration/upgrade.rs +++ b/apollo-router/src/configuration/upgrade.rs @@ -70,7 +70,7 @@ fn apply_migration(config: &Value, migration: &Migration) -> Result { - if !jsonpath_lib::select(config, path) + if !jsonpath_lib::select(config, &format!("$.{}", path)) .unwrap_or_default() .is_empty() { @@ -81,7 +81,7 @@ fn apply_migration(config: &Value, migration: &Migration) -> Result { - if !jsonpath_lib::select(config, from) + if !jsonpath_lib::select(config, &format!("$.{}", from)) .unwrap_or_default() .is_empty() { @@ -90,7 +90,7 @@ fn apply_migration(config: &Value, migration: &Migration) -> Result { - if !jsonpath_lib::select(config, from) + if !jsonpath_lib::select(config, &format!("$.{}", from)) .unwrap_or_default() .is_empty() { @@ -276,7 +276,7 @@ mod test { #[test] fn move_non_existent_field() { insta::assert_json_snapshot!(apply_migration( - &json!({}), + &json!({"should": "stay"}), &Migration::builder() .action(Action::Move { from: "obj.field1".to_string(),