From 7c8f27b284b37b3048942b4f4cde16bd241b6a78 Mon Sep 17 00:00:00 2001 From: Dylan Ratcliffe Date: Fri, 28 Jun 2024 12:28:23 +0000 Subject: [PATCH] Fixed potential panic --- cmd/changes_submit_plan.go | 20 ++++- cmd/changes_submit_plan_test.go | 128 ++++++++++++++++++++++++++++++++ cmd/plan_mapper_test.go | 105 -------------------------- 3 files changed, 146 insertions(+), 107 deletions(-) diff --git a/cmd/changes_submit_plan.go b/cmd/changes_submit_plan.go index cf4fb8c2..2a28a03b 100644 --- a/cmd/changes_submit_plan.go +++ b/cmd/changes_submit_plan.go @@ -278,8 +278,24 @@ func removeUnknownFields(before, after *structpb.Value, afterUnknown interface{} // Do nothing continue } else { - // Recurse into the nested fields - err := removeUnknownFields(before.GetListValue().GetValues()[i], after.GetListValue().GetValues()[i], v) + // Make sure that the before and after both actually have a + // valid list item at this position, if they don't we can just + // pass `nil` to the `removeUnknownFields` function and it'll + // handle it + beforeListValues := before.GetListValue().GetValues() + afterListValues := after.GetListValue().GetValues() + var nestedBeforeValue *structpb.Value + var nestedAfterValue *structpb.Value + + if len(beforeListValues) > i { + nestedBeforeValue = beforeListValues[i] + } + + if len(afterListValues) > i { + nestedAfterValue = afterListValues[i] + } + + err := removeUnknownFields(nestedBeforeValue, nestedAfterValue, v) if err != nil { return err } diff --git a/cmd/changes_submit_plan_test.go b/cmd/changes_submit_plan_test.go index 24fbb64b..c0a4e2ed 100644 --- a/cmd/changes_submit_plan_test.go +++ b/cmd/changes_submit_plan_test.go @@ -2,8 +2,10 @@ package cmd import ( "context" + "encoding/json" "testing" + "github.com/overmindtech/sdp-go" "github.com/sirupsen/logrus" "github.com/stretchr/testify/require" ) @@ -144,3 +146,129 @@ func TestExtractProviderNameFromConfigKey(t *testing.T) { }) } } + +func TestRemoveKnownAfterApply(t *testing.T) { + before, err := sdp.ToAttributes(map[string]interface{}{ + "string_value": "foo", + "int_value": 42, + "bool_value": true, + "float_value": 3.14, + "list_value": []interface{}{ + "foo", + "bar", + }, + "map_value": map[string]interface{}{ + "foo": "bar", + "bar": "baz", + }, + "map_value2": map[string]interface{}{ + "ding": map[string]interface{}{ + "foo": "bar", + }, + }, + "nested_list": []interface{}{ + []interface{}{}, + []interface{}{ + "foo", + "bar", + }, + }, + }) + if err != nil { + t.Fatal(err) + } + + after, err := sdp.ToAttributes(map[string]interface{}{ + "string_value": "bar", // I want to see a diff here + "int_value": nil, // These are going to be known after apply + "bool_value": nil, // These are going to be known after apply + "float_value": 3.14, + "list_value": []interface{}{ + "foo", + "bar", + "baz", // So is this one + }, + "map_value": map[string]interface{}{ // This whole thing will be known after apply + "foo": "bar", + }, + "map_value2": map[string]interface{}{ + "ding": map[string]interface{}{ + "foo": nil, // This will be known after apply + }, + }, + "nested_list": []interface{}{ + []interface{}{ + "foo", + }, + }, + }) + if err != nil { + t.Fatal(err) + } + + afterUnknown := json.RawMessage(`{ + "int_value": true, + "bool_value": true, + "float_value": false, + "list_value": [ + false, + false, + true + ], + "map_value": true, + "map_value2": { + "ding": { + "foo": true + } + }, + "nested_list": [ + [ + false, + true + ], + [ + false, + true + ] + ] + }`) + + err = removeKnownAfterApply(before, after, afterUnknown) + if err != nil { + t.Fatal(err) + } + + if _, err := before.Get("int_value"); err == nil { + t.Errorf("Expected int_value to be removed from the before, but it's still there") + } + + if _, err := before.Get("bool_value"); err == nil { + t.Errorf("Expected bool_value to be removed from the before, but it's still there") + } + + if _, err := after.Get("int_value"); err == nil { + t.Errorf("Expected int_value to be removed from the after, but it's still there") + } + + if _, err := after.Get("bool_value"); err == nil { + t.Errorf("Expected bool_value to be removed from the after, but it's still there") + } + + if list, err := before.Get("list_value"); err != nil { + t.Errorf("Expected list_value to be there, but it's not: %v", err) + } else { + + if len(list.([]interface{})) != 2 { + t.Error("Expected list_value to have 2 elements") + } + } + + if list, err := after.Get("list_value"); err != nil { + t.Errorf("Expected list_value to be there, but it's not: %v", err) + } else { + if len(list.([]interface{})) != 2 { + t.Error("Expected list_value to have 2 elements") + } + } + +} diff --git a/cmd/plan_mapper_test.go b/cmd/plan_mapper_test.go index 5069e610..af993d86 100644 --- a/cmd/plan_mapper_test.go +++ b/cmd/plan_mapper_test.go @@ -2,7 +2,6 @@ package cmd import ( "context" - "encoding/json" "testing" "github.com/overmindtech/sdp-go" @@ -207,107 +206,3 @@ func TestPlanMappingResultNumFuncs(t *testing.T) { t.Errorf("Expected 1 unsupported, got %v", result.NumUnsupported()) } } - -func TestRemoveKnownAfterApply(t *testing.T) { - before, err := sdp.ToAttributes(map[string]interface{}{ - "string_value": "foo", - "int_value": 42, - "bool_value": true, - "float_value": 3.14, - "list_value": []interface{}{ - "foo", - "bar", - }, - "map_value": map[string]interface{}{ - "foo": "bar", - "bar": "baz", - }, - "map_value2": map[string]interface{}{ - "ding": map[string]interface{}{ - "foo": "bar", - }, - }, - }) - if err != nil { - t.Fatal(err) - } - - after, err := sdp.ToAttributes(map[string]interface{}{ - "string_value": "bar", // I want to see a diff here - "int_value": nil, // These are going to be known after apply - "bool_value": nil, // These are going to be known after apply - "float_value": 3.14, - "list_value": []interface{}{ - "foo", - "bar", - "baz", // So is this one - }, - "map_value": map[string]interface{}{ // This whole thing will be known after apply - "foo": "bar", - }, - "map_value2": map[string]interface{}{ - "ding": map[string]interface{}{ - "foo": nil, // This will be known after apply - }, - }, - }) - if err != nil { - t.Fatal(err) - } - - afterUnknown := json.RawMessage(`{ - "int_value": true, - "bool_value": true, - "float_value": false, - "list_value": [ - false, - false, - true - ], - "map_value": true, - "map_value2": { - "ding": { - "foo": true - } - } - }`) - - err = removeKnownAfterApply(before, after, afterUnknown) - if err != nil { - t.Fatal(err) - } - - if _, err := before.Get("int_value"); err == nil { - t.Errorf("Expected int_value to be removed from the before, but it's still there") - } - - if _, err := before.Get("bool_value"); err == nil { - t.Errorf("Expected bool_value to be removed from the before, but it's still there") - } - - if _, err := after.Get("int_value"); err == nil { - t.Errorf("Expected int_value to be removed from the after, but it's still there") - } - - if _, err := after.Get("bool_value"); err == nil { - t.Errorf("Expected bool_value to be removed from the after, but it's still there") - } - - if list, err := before.Get("list_value"); err != nil { - t.Errorf("Expected list_value to be there, but it's not: %v", err) - } else { - - if len(list.([]interface{})) != 2 { - t.Error("Expected list_value to have 2 elements") - } - } - - if list, err := after.Get("list_value"); err != nil { - t.Errorf("Expected list_value to be there, but it's not: %v", err) - } else { - if len(list.([]interface{})) != 2 { - t.Error("Expected list_value to have 2 elements") - } - } - -}