Fixed potential panic

overmindtech · Jun 28, 2024 · 7c8f27b · 7c8f27b
1 parent 0cc2070
commit 7c8f27b
Show file tree

Hide file tree

Showing 3 changed files with 146 additions and 107 deletions.
diff --git 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
@@ -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
@@ -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")
-		}
-	}
-
-}