Repository: goadesign/goa

Issue:
Sending `null` values in arrays that are required causes a panic
If you declare something like this:
```go
var Item = Type("Item", func() {
	Attribute("id", String, "ID of the item", func() {
		Example("item-1234")
	})
	Attribute("name", String, "Name of the item", func() {
		Example("Example Item")
	})
	Attribute("description", String, "Description of the item", func() {
		Example("This is an example item.")
	})
	
	Required("id", "name", "description")
})

var Payload = Type("Payload", func() {
	Attribute("items", ArrayOf(Item))
	Required("items")
})
```

And then spend a payload with an array with a null item:
```
{"items": [null]}
```

We've noticed you get a nil pointer dereference from the generated `unmarshalPayloadRequestBodyToPayload` function, as goa assumes each array item is an object.

The expected behaviour would be
1. Goa should not panic, it should return an array with nils (i.e. it's up to the API handler to handle this)
2. There should be a validation rule that makes it possible to specify that elements in an array cannot be nil 

Discussion:
@raphael: Thanks for opening the issue!

* #3830 prevents the panic
* #3834 introduces a new DSL `ArrayOfRequired` which makes it possible to define arrays that must not include nil elements

Solution patch:
diff --git a/codegen/templates/validation/array.go.tpl b/codegen/templates/validation/array.go.tpl
index 27f8ac4b..2332fa52 100644
--- a/codegen/templates/validation/array.go.tpl
+++ b/codegen/templates/validation/array.go.tpl
@@ -1,3 +1,10 @@
 for _, e := range {{ .target }} {
+{{- if .nonNullableElems }}
+	if e == nil {
+		err = goa.MergeErrors(err, goa.MissingFieldError("{{ .context }}", "[*]"))
+	}
+{{- end }}
+{{- if .validation }}
 {{ .validation }}
+{{- end }}
 }
\ No newline at end of file
diff --git a/codegen/validation.go b/codegen/validation.go
index a8cafb67..5774c8f4 100644
--- a/codegen/validation.go
+++ b/codegen/validation.go
@@ -126,7 +126,8 @@ func recurseValidationCode(att *expr.AttributeExpr, put expr.UserType, attCtx *A
 			}
 		}
 	case expr.IsArray(att.Type):
-		elem := expr.AsArray(att.Type).ElemType
+		arr := expr.AsArray(att.Type)
+		elem := arr.ElemType
 		ctx := attCtx
 		if ctx.Pointer && expr.IsPrimitive(elem.Type) {
 			// Array elements of primitive type are never pointers
@@ -134,9 +135,14 @@ func recurseValidationCode(att *expr.AttributeExpr, put expr.UserType, attCtx *A
 			ctx.Pointer = false
 		}
 		val := validateAttribute(ctx, elem, put, "e", context+"[*]", true, view)
-		if val != "" {
+		if val != "" || arr.NonNullableElems {
 			newline()
-			data := map[string]any{"target": target, "validation": val}
+			data := map[string]any{
+				"target":           target,
+				"validation":       val,
+				"nonNullableElems": arr.NonNullableElems,
+				"context":          context,
+			}
 			if err := arrayValT.Execute(buf, data); err != nil {
 				panic(err) // bug
 			}
diff --git a/dsl/user_type.go b/dsl/user_type.go
index 0c7041be..3f864cfe 100644
--- a/dsl/user_type.go
+++ b/dsl/user_type.go
@@ -163,6 +163,35 @@ func ArrayOf(v any, fn ...func()) *expr.Array {
 	return &expr.Array{ElemType: &at}
 }
 
+// ArrayOfRequired creates an array type from its element type where elements
+// cannot be null.
+//
+// ArrayOfRequired may be used wherever types can.
+// The first argument is the type of the array elements specified by name or by
+// reference.
+// The second argument is an optional function that defines validations for the
+// array elements.
+//
+// The difference between ArrayOf and ArrayOfRequired is that ArrayOfRequired
+// generates validation code that rejects null elements in the array.
+//
+// Examples:
+//
+//	var Names = ArrayOfRequired(String, func() {
+//	    Pattern("[a-zA-Z]+") // Validates elements of the array
+//	})
+//
+//	var Account = Type("Account", func() {
+//	    Attribute("bottles", ArrayOfRequired(Bottle), "Account bottles", func() {
+//	        MinLength(1) // Validates array as a whole
+//	    })
+//	})
+func ArrayOfRequired(v any, fn ...func()) *expr.Array {
+	arr := ArrayOf(v, fn...)
+	arr.NonNullableElems = true
+	return arr
+}
+
 // MapOf creates a map from its key and element types.
 //
 // MapOf may be used wherever types can.
diff --git a/expr/attribute.go b/expr/attribute.go
index 42ff21e3..2e959ed0 100644
--- a/expr/attribute.go
+++ b/expr/attribute.go
@@ -187,17 +187,19 @@ func TaggedAttribute(a *AttributeExpr, tag string) string {
 	return ""
 }
 
-// WalkAttribute iterates over the given attribute, its bases and references
+// walkAttribute iterates over the given attribute, its bases and references
 // (if any). It calls the given function giving each attribute as it iterates.
 // It stops if the given attribute is not an object type or there is no more
 // attribute to iterate over or if the iterator function returned an error. It
 // is generally used in implementing the Validator interface since attribute
 // bases and references are only merged during Finalize. It is not a recursive
 // implementation.
-func WalkAttribute(att *AttributeExpr, it func(name string, a *AttributeExpr) error) error {
+// Note: keep this function private as it does not walk through all types.
+// External packages should use the codegen.Walk function instead.
+func walkAttribute(att *AttributeExpr, it func(name string, a *AttributeExpr) error) error {
 	switch dt := att.Type.(type) {
 	case UserType:
-		if err := WalkAttribute(dt.Attribute(), it); err != nil {
+		if err := walkAttribute(dt.Attribute(), it); err != nil {
 			return err
 		}
 	case *Object:
@@ -208,12 +210,12 @@ func WalkAttribute(att *AttributeExpr, it func(name string, a *AttributeExpr) er
 		}
 	}
 	for _, b := range att.Bases {
-		if err := WalkAttribute(&AttributeExpr{Type: b}, it); err != nil {
+		if err := walkAttribute(&AttributeExpr{Type: b}, it); err != nil {
 			return err
 		}
 	}
 	for _, r := range att.References {
-		if err := WalkAttribute(&AttributeExpr{Type: r}, it); err != nil {
+		if err := walkAttribute(&AttributeExpr{Type: r}, it); err != nil {
 			return err
 		}
 	}
diff --git a/expr/method.go b/expr/method.go
index f4af6bbe..ac9abdb0 100644
--- a/expr/method.go
+++ b/expr/method.go
@@ -242,7 +242,7 @@ func (m *MethodExpr) validateErrors() *eval.ValidationErrors {
 			// presence of struct:error:name meta in the object type.
 			if i != j && e.Type == e2.Type && IsObject(e.Type) {
 				var found bool
-				WalkAttribute(e.AttributeExpr, func(_ string, att *AttributeExpr) error { // nolint: errcheck
+				walkAttribute(e.AttributeExpr, func(_ string, att *AttributeExpr) error { // nolint: errcheck
 					if _, ok := att.Meta["struct:error:name"]; ok {
 						found = true
 						return fmt.Errorf("struct:error:name found: stop iteration")
diff --git a/expr/result_type.go b/expr/result_type.go
index f7d80f17..8a098a77 100644
--- a/expr/result_type.go
+++ b/expr/result_type.go
@@ -180,7 +180,7 @@ func (rt *ResultTypeExpr) Finalize() {
 	rt.ensureDefaultView()
 	rt.UserTypeExpr.Finalize()
 	seen := make(map[string]struct{})
-	WalkAttribute(rt.AttributeExpr, func(_ string, att *AttributeExpr) error { // nolint: errcheck
+	walkAttribute(rt.AttributeExpr, func(_ string, att *AttributeExpr) error { // nolint: errcheck
 		if rt, ok := att.Type.(*ResultTypeExpr); ok {
 			if _, ok := seen[rt.Identifier]; !ok {
 				seen[rt.Identifier] = struct{}{}
diff --git a/expr/service.go b/expr/service.go
index 494b52bb..43887d95 100644
--- a/expr/service.go
+++ b/expr/service.go
@@ -106,7 +106,7 @@ func (s *ServiceExpr) Finalize() {
 func (e *ErrorExpr) Validate() error {
 	verr := new(eval.ValidationErrors)
 	var errField string
-	WalkAttribute(e.AttributeExpr, func(name string, att *AttributeExpr) error { // nolint: errcheck
+	walkAttribute(e.AttributeExpr, func(name string, att *AttributeExpr) error { // nolint: errcheck
 		if _, ok := att.Meta["struct:error:name"]; ok {
 			if errField != "" {
 				verr.Add(e, "duplicate error names in type %q", e.Type.Name())
diff --git a/expr/types.go b/expr/types.go
index 37aa4354..f959b637 100644
--- a/expr/types.go
+++ b/expr/types.go
@@ -32,7 +32,8 @@ type (
 
 	// Array is the type used to describe field arrays or repeated fields.
 	Array struct {
-		ElemType *AttributeExpr
+		ElemType         *AttributeExpr
+		NonNullableElems bool
 	}
 
 	// Map is the type used to describe maps of fields.



Test patch:
diff --git a/codegen/testdata/golden/validation_array-required-non-nullable-elems.go.golden b/codegen/testdata/golden/validation_array-required-non-nullable-elems.go.golden
new file mode 100644
index 00000000..287870a9
--- /dev/null
+++ b/codegen/testdata/golden/validation_array-required-non-nullable-elems.go.golden
@@ -0,0 +1,18 @@
+func Validate() (err error) {
+	if target.RequiredArray == nil {
+		err = goa.MergeErrors(err, goa.MissingFieldError("required_array", "target"))
+	}
+	if len(target.RequiredArray) < 1 {
+		err = goa.MergeErrors(err, goa.InvalidLengthError("target.required_array", target.RequiredArray, len(target.RequiredArray), 1, true))
+	}
+	for _, e := range target.RequiredArray {
+		if e == nil {
+			err = goa.MergeErrors(err, goa.MissingFieldError("target.required_array", "[*]"))
+		}
+		if e != nil {
+			if err2 := ValidateInteger(e); err2 != nil {
+				err = goa.MergeErrors(err, err2)
+			}
+		}
+	}
+}
diff --git a/codegen/testdata/validation_types_dsl.go b/codegen/testdata/validation_types_dsl.go
index 92da6ce8..13d198dd 100644
--- a/codegen/testdata/validation_types_dsl.go
+++ b/codegen/testdata/validation_types_dsl.go
@@ -97,6 +97,13 @@ var ValidationTypesDSL = func() {
 			Required("required_array")
 		})
 
+		_ = Type("ArrayRequired", func() {
+			Attribute("required_array", ArrayOfRequired(IntegerT), func() {
+				MinLength(1)
+			})
+			Required("required_array")
+		})
+
 		_ = Type("Map", func() {
 			Attribute("required_map", MapOf(Int, String), func() {
 				MinLength(5)
diff --git a/codegen/validation_test.go b/codegen/validation_test.go
index 80075c83..a652266b 100644
--- a/codegen/validation_test.go
+++ b/codegen/validation_test.go
@@ -13,19 +13,20 @@ func TestRecursiveValidationCode(t *testing.T) {
 	var (
 		scope = NewNameScope()
 
-		integerT = root.UserType("Integer")
-		stringT  = root.UserType("String")
-		floatT   = root.UserType("Float")
-		aliasT   = root.UserType("AliasType")
-		userT    = root.UserType("UserType")
-		arrayUT  = root.UserType("ArrayUserType")
-		arrayT   = root.UserType("Array")
-		mapT     = root.UserType("Map")
-		unionT   = root.UserType("Union")
-		rtT      = root.UserType("Result")
-		rtcolT   = root.UserType("Collection")
-		colT     = root.UserType("TypeWithCollection")
-		deepT    = root.UserType("Deep")
+		integerT  = root.UserType("Integer")
+		stringT   = root.UserType("String")
+		floatT    = root.UserType("Float")
+		aliasT    = root.UserType("AliasType")
+		userT     = root.UserType("UserType")
+		arrayUT   = root.UserType("ArrayUserType")
+		arrayT    = root.UserType("Array")
+		arrayReqT = root.UserType("ArrayRequired")
+		mapT      = root.UserType("Map")
+		unionT    = root.UserType("Union")
+		rtT       = root.UserType("Result")
+		rtcolT    = root.UserType("Collection")
+		colT      = root.UserType("TypeWithCollection")
+		deepT     = root.UserType("Deep")
 	)
 	cases := []struct {
 		Name       string
@@ -51,6 +52,7 @@ func TestRecursiveValidationCode(t *testing.T) {
 		{"array-required", arrayT, true, false, false},
 		{"array-pointer", arrayT, false, true, false},
 		{"array-use-default", arrayT, false, false, true},
+		{"array-required-non-nullable-elems", arrayReqT, true, false, false},
 		{"map-required", mapT, true, false, false},
 		{"map-pointer", mapT, false, true, false},
 		{"map-use-default", mapT, false, false, true},