Repository: klothoplatform/klotho
Pull request #626 (2023-05-23 18:33:34)

fix nil-deref effor in Subnet.Id()
The `TestAllTypesHaveIcons` test calls `Id()` on each resource, to get its type. This was failing for Subnets that hadn't been fully initialized with a VPC yet.

### Standard checks

- **Unit tests**: fixes it
- **Docs**: n/a
- **Backwards compatibility**: no issues



Patch:
diff --git a/pkg/collectionutil/slices.go b/pkg/collectionutil/slices.go
new file mode 100644
index 00000000..bd97c6b1
--- /dev/null
+++ b/pkg/collectionutil/slices.go
@@ -0,0 +1,24 @@
+package collectionutil
+
+// FlattenUnique appends each unique item in each list, in the order in which it first appears.
+//
+// Examples:
+//   - FlattenUnique([]int{1, 2, 3}, []int{4, 3, 4}) => []int{1, 2, 3, 4}
+//   - FlattenUnique([]int{1, 2, 2}, []int{3, 4}) => []int{1, 2, 3, 4}
+func FlattenUnique[E comparable](slices ...[]E) []E {
+	alreadyInResult := make(map[E]struct{})
+
+	var result []E
+	for _, slice := range slices {
+		for _, elem := range slice {
+			_, alreadyIn := alreadyInResult[elem]
+			if !alreadyIn {
+				result = append(result, elem)
+				alreadyInResult[elem] = struct{}{}
+			}
+		}
+
+	}
+
+	return result
+}
diff --git a/pkg/core/resources.go b/pkg/core/resources.go
index 3bc56f94..e949a954 100644
--- a/pkg/core/resources.go
+++ b/pkg/core/resources.go
@@ -25,6 +25,12 @@ type (
 		Id() ResourceId
 	}
 
+	// ExpandableResource is a resource that can generate its own dependencies. See [CreateResource].
+	ExpandableResource[K any] interface {
+		Resource
+		Create(dag *ResourceGraph, param K) error
+	}
+
 	ResourceId struct {
 		Provider string
 		Type     string
diff --git a/pkg/provider/aws/resources/list_all.go b/pkg/provider/aws/resources/list_all.go
index aa154236..8e1b3c2b 100644
--- a/pkg/provider/aws/resources/list_all.go
+++ b/pkg/provider/aws/resources/list_all.go
@@ -50,7 +50,7 @@ func ListAll() []core.Resource {
 		&SecretVersion{},
 		&Secret{},
 		&SecurityGroup{},
-		&Subnet{},
+		&Subnet{Vpc: &Vpc{}},
 		&TargetGroup{},
 		&VpcEndpoint{},
 		&VpcLink{},
diff --git a/pkg/provider/aws/resources/s3.go b/pkg/provider/aws/resources/s3.go
index a54922b9..472b7295 100644
--- a/pkg/provider/aws/resources/s3.go
+++ b/pkg/provider/aws/resources/s3.go
@@ -3,8 +3,10 @@ package resources
 import (
 	"fmt"
 
+	"github.com/klothoplatform/klotho/pkg/collectionutil"
 	"github.com/klothoplatform/klotho/pkg/core"
 	"github.com/klothoplatform/klotho/pkg/sanitization/aws"
+	"github.com/pkg/errors"
 )
 
 var objectSanitizer = aws.S3ObjectSanitizer
@@ -65,6 +67,31 @@ func (bucket *S3Bucket) Id() core.ResourceId {
 	}
 }
 
+type S3BucketCreateParams struct {
+	AppName string
+	Refs    []core.AnnotationKey
+	Name    string
+}
+
+func (bucket *S3Bucket) Create(dag *core.ResourceGraph, params S3BucketCreateParams) error {
+	bucket.Name = bucketSanitizer.Apply(fmt.Sprintf("%s-%s", params.AppName, params.Name))
+
+	if existing := dag.GetResource(bucket.Id()); existing != nil {
+		existingS3, ok := existing.(*S3Bucket)
+		if !ok {
+			return errors.Errorf(`found an existing element at %s, but it was not an S3Bucket`, bucket.Id().String())
+		}
+		// Multiple resources may create the same bucket (today, this specifically happens with our payload bucket). If
+		// that happens, just append the refs and exit early; the rest would have been idempotent.
+		existingS3.ConstructsRef = collectionutil.FlattenUnique(existingS3.ConstructsRef, params.Refs)
+		return nil
+	}
+
+	bucket.ConstructsRef = params.Refs
+	dag.AddResource(bucket)
+	return nil
+}
+
 func NewS3Object(bucket *S3Bucket, objectName string, key string, path string) *S3Object {
 	return &S3Object{
 		Name:          objectSanitizer.Apply(fmt.Sprintf("%s-%s", bucket.Name, objectName)),


Test patch:
diff --git a/pkg/collectionutil/slices_test.go b/pkg/collectionutil/slices_test.go
new file mode 100644
index 00000000..356eee27
--- /dev/null
+++ b/pkg/collectionutil/slices_test.go
@@ -0,0 +1,69 @@
+package collectionutil
+
+import (
+	"testing"
+
+	assert "github.com/stretchr/testify/assert"
+)
+
+func Test_AppendUnique(t *testing.T) {
+	cases := []struct {
+		name   string
+		inputs [][]int
+		b      []int
+		want   []int
+	}{
+		{
+			name: "unique elems",
+			inputs: [][]int{
+				{1, 2, 3},
+				{4, 5, 6},
+			},
+			want: []int{1, 2, 3, 4, 5, 6},
+		},
+		{
+			name: "first is non-unique",
+			inputs: [][]int{
+				{1, 2, 2},
+				{3, 4},
+			},
+			want: []int{1, 2, 3, 4},
+		},
+		{
+			name: "second is non-unique",
+			inputs: [][]int{
+				{1, 2},
+				{3, 3, 4},
+			},
+			want: []int{1, 2, 3, 4},
+		},
+		{
+			name: "both unique but share elements",
+			inputs: [][]int{
+				{1, 2, 3},
+				{4, 3, 5},
+			},
+			want: []int{1, 2, 3, 4, 5},
+		},
+		{
+			name: "one is nil",
+			inputs: [][]int{
+				{1, 2, 3},
+				nil,
+			},
+			want: []int{1, 2, 3},
+		},
+		{
+			name:   "overall is nil",
+			inputs: nil,
+			want:   nil,
+		},
+	}
+	for _, tt := range cases {
+		t.Run(tt.name, func(t *testing.T) {
+			assert := assert.New(t)
+			actual := FlattenUnique(tt.inputs...)
+			assert.Equal(tt.want, actual)
+		})
+	}
+}
diff --git a/pkg/core/coretesting/expandable_resource.go b/pkg/core/coretesting/expandable_resource.go
new file mode 100644
index 00000000..076afe61
--- /dev/null
+++ b/pkg/core/coretesting/expandable_resource.go
@@ -0,0 +1,55 @@
+package coretesting
+
+import (
+	"reflect"
+	"testing"
+
+	"github.com/klothoplatform/klotho/pkg/core"
+	"github.com/stretchr/testify/assert"
+)
+
+type CreateCase[P any, R core.ExpandableResource[P]] struct {
+	Name     string
+	Existing R
+	Params   P
+	Want     ResourcesExpectation
+	Check    func(assertions *assert.Assertions, resource R)
+	WantErr  bool
+}
+
+func (tt CreateCase[P, R]) Run(t *testing.T) {
+	assert := assert.New(t)
+
+	dag := core.NewResourceGraph()
+	// We have to use reflect, since Go doesn't know that R is a pointer, or even something whose zero-type is
+	// comparable (so we can't do "var R zero; if tt.Existing != zero").
+	if !reflect.ValueOf(tt.Existing).IsZero() {
+		dag.AddResource(tt.Existing)
+	}
+
+	var res R
+	rType := reflect.TypeOf(tt.Existing)
+	if rType.Kind() == reflect.Pointer {
+		res = reflect.New(rType.Elem()).Interface().(R)
+	}
+	err := res.Create(dag, tt.Params)
+	if tt.WantErr {
+		assert.Error(err)
+		return
+	} else if !assert.NoError(err) {
+		return
+	}
+
+	tt.Want.Assert(t, dag)
+
+	found := dag.GetResource(res.Id())
+	if !assert.NotNil(found) {
+		return
+	}
+	foundR := found.(R)
+
+	if !assert.NotNil(tt.Check, "bug in test itself: no Case.Check provided!") {
+		return
+	}
+	tt.Check(assert, foundR)
+}
diff --git a/pkg/provider/aws/resources/lambda_test.go b/pkg/provider/aws/resources/lambda_test.go
index 2a669bc2..15e6ea45 100644
--- a/pkg/provider/aws/resources/lambda_test.go
+++ b/pkg/provider/aws/resources/lambda_test.go
@@ -11,15 +11,11 @@ import (
 
 func Test_LambdaCreate(t *testing.T) {
 	eu := &core.ExecutionUnit{AnnotationKey: core.AnnotationKey{ID: "test", Capability: annotation.ExecutionUnitCapability}}
-	cases := []struct {
-		name    string
-		lambda  *LambdaFunction
-		want    coretesting.ResourcesExpectation
-		wantErr bool
-	}{
+
+	cases := []coretesting.CreateCase[LambdaCreateParams, *LambdaFunction]{
 		{
-			name: "nil lambda",
-			want: coretesting.ResourcesExpectation{
+			Name: "nil lambda",
+			Want: coretesting.ResourcesExpectation{
 				Nodes: []string{
 					"aws:ecr_image:my-app-test",
 					"aws:ecr_repo:my-app",
@@ -34,44 +30,26 @@ func Test_LambdaCreate(t *testing.T) {
 					{Source: "aws:lambda_function:my-app-test", Destination: "aws:log_group:my-app-test"},
 				},
 			},
+			Check: func(assert *assert.Assertions, lambda *LambdaFunction) {
+				assert.Equal(lambda.Name, "my-app-test")
+				assert.ElementsMatch(lambda.ConstructsRef, []core.AnnotationKey{eu.AnnotationKey})
+			},
 		},
 		{
-			name:    "existing lambda",
-			lambda:  &LambdaFunction{Name: "my-app-test"},
-			wantErr: true,
+			Name:     "existing lambda",
+			Existing: &LambdaFunction{Name: "my-app-test"},
+			WantErr:  true,
 		},
 	}
 	for _, tt := range cases {
-		t.Run(tt.name, func(t *testing.T) {
-
-			assert := assert.New(t)
-			dag := core.NewResourceGraph()
-
-			if tt.lambda != nil {
-				dag.AddResource(tt.lambda)
-			}
-
-			metadata := LambdaCreateParams{
+		t.Run(tt.Name, func(t *testing.T) {
+			tt.Params = LambdaCreateParams{
 				AppName: "my-app",
 				Refs:    []core.AnnotationKey{eu.AnnotationKey},
 				Name:    eu.ID,
 			}
-			lambda := &LambdaFunction{}
-			err := lambda.Create(dag, metadata)
-			if tt.wantErr {
-				assert.Error(err)
-				return
-			}
-			if !assert.NoError(err) {
-				return
-			}
-			tt.want.Assert(t, dag)
-
-			graphLambda := dag.GetResource(lambda.Id())
-			lambda = graphLambda.(*LambdaFunction)
 
-			assert.Equal(lambda.Name, "my-app-test")
-			assert.ElementsMatch(lambda.ConstructsRef, []core.AnnotationKey{eu.AnnotationKey})
+			tt.Run(t)
 		})
 	}
 }
diff --git a/pkg/provider/aws/resources/s3_test.go b/pkg/provider/aws/resources/s3_test.go
new file mode 100644
index 00000000..e89153e3
--- /dev/null
+++ b/pkg/provider/aws/resources/s3_test.go
@@ -0,0 +1,65 @@
+package resources
+
+import (
+	"testing"
+
+	"github.com/klothoplatform/klotho/pkg/annotation"
+	"github.com/klothoplatform/klotho/pkg/core"
+	"github.com/klothoplatform/klotho/pkg/core/coretesting"
+	"github.com/stretchr/testify/assert"
+)
+
+func Test_S3BucketCreate(t *testing.T) {
+	annotationKey := core.AnnotationKey{ID: "test", Capability: annotation.ExecutionUnitCapability}
+	cases := []coretesting.CreateCase[S3BucketCreateParams, *S3Bucket]{
+		{
+			Name: "single payloads bucket",
+			Want: coretesting.ResourcesExpectation{
+				Nodes: []string{
+					"aws:s3_bucket:my-app-payloads",
+				},
+				Deps: nil,
+			},
+			Check: func(assert *assert.Assertions, bucket *S3Bucket) {
+				assert.Equal("my-app-payloads", bucket.Name)
+				assert.ElementsMatch(bucket.ConstructsRef, []core.AnnotationKey{annotationKey})
+			},
+		},
+		{
+			Name: "two payloads buckets converge",
+			Existing: &S3Bucket{
+				Name: "my-app-payloads",
+				ConstructsRef: []core.AnnotationKey{core.AnnotationKey{
+					ID:         "some-other-eu",
+					Capability: annotation.ExecutionUnitCapability}},
+			},
+			Want: coretesting.ResourcesExpectation{
+				Nodes: []string{
+					"aws:s3_bucket:my-app-payloads",
+				},
+				Deps: nil,
+			},
+			Check: func(assert *assert.Assertions, bucket *S3Bucket) {
+				assert.Equal("my-app-payloads", bucket.Name)
+				assert.ElementsMatch(bucket.ConstructsRef,
+					[]core.AnnotationKey{
+						annotationKey,
+						core.AnnotationKey{
+							ID:         "some-other-eu",
+							Capability: annotation.ExecutionUnitCapability},
+					},
+				)
+			},
+		},
+	}
+	for _, tt := range cases {
+		t.Run(tt.Name, func(t *testing.T) {
+			tt.Params = S3BucketCreateParams{
+				AppName: "my-app",
+				Refs:    []core.AnnotationKey{annotationKey},
+				Name:    "payloads",
+			}
+			tt.Run(t)
+		})
+	}
+}