Repository: goadesign/goa (go)

Issue:
Panic occurs in payload validation with OneOf (HTTP)
# Bug?

In HTTP transport, the validation of Payload containing `OneOf` is generated as follows:

```Go
// ValidatePetOwnerRequestBody runs the validations defined on
// PetOwnerRequestBody
func ValidatePetOwnerRequestBody(body *PetOwnerRequestBody) (err error) {
	if body.Pet.Type == nil {
		err = goa.MergeErrors(err, goa.MissingFieldError("Type", "body.pet"))
	}
	if body.Pet.Value == nil {
		err = goa.MergeErrors(err, goa.MissingFieldError("Value", "body.pet"))
	}
	if body.Pet.Type != nil {
		if !(*body.Pet.Type == "dog" || *body.Pet.Type == "cat") {
			err = goa.MergeErrors(err, goa.InvalidEnumValueError("body.pet.Type", *body.Pet.Type, []interface{}{"dog", "cat"}))
		}
	}
	return
}
```

The generated verification does not perform a nil check on `body.Pet`, which may cause a panic if the payload is invalid.

# Expected generated codes

```Go
// ValidatePetOwnerRequestBody runs the validations defined on
// PetOwnerRequestBody
func ValidatePetOwnerRequestBody(body *PetOwnerRequestBody) (err error) {
	if body.Pet == nil || body.Pet.Type == nil {
		err = goa.MergeErrors(err, goa.MissingFieldError("Type", "body.pet"))
	}
	if body.Pet == nil || body.Pet.Value == nil {
		err = goa.MergeErrors(err, goa.MissingFieldError("Value", "body.pet"))
	}
	if body.Pet != nil && body.Pet.Type != nil {
		if !(*body.Pet.Type == "dog" || *body.Pet.Type == "cat") {
			err = goa.MergeErrors(err, goa.InvalidEnumValueError("body.pet.Type", *body.Pet.Type, []interface{}{"dog", "cat"}))
		}
	}
	return
}
```

# Sample Minimal Design

```go
package design

import (
	. "goa.design/goa/v3/dsl"
)

var Dog = Type("Dog", func() {
	Description("Dogs are cool")
	Field(1, "Name")
	Field(2, "Breed")
	Required("Name", "Breed")
})

var Cat = Type("Cat", func() {
	Description("Cats are cool too")
	Field(1, "Name")
	Field(2, "Age", Int)
	Required("Name", "Age")
})

var PetOwner = Type("PetOwner", func() {
	Description("A pet owner")
	OneOf("pet", func() {
		Description("The owner's pet")
		Field(1, "dog", Dog)
		Field(2, "cat", Cat)
	})
})

var _ = Service("pet-hotel", func() {
	Method("check-in", func() {
		Payload(func() {
			Field("1", "body", PetOwner)
			Required("body")
		})
		Result(PetOwner)
		HTTP(func() {
			POST("/check-in")
			Response(StatusOK)
		})
		GRPC(func() {
			Response(CodeOK)
		})
	})
})
```

## Execution example

### OK
```shell
curl -X POST localhost:8888/check-in -d '{"body":{"pet":{"Type":"dog","Value":"{\"Name\":\"POCHI\",\"Breed\":\"AKITA\"}"}}}'
{"pet":{"Type":"dog","Value":"{\"Name\":\"POCHI\",\"Breed\":\"AKITA\"}"}}
```

### Missing `body` (Error Response)
```shell
$ curl -X POST localhost:8888/check-in -d '{"pet":{"Type":"dog","Value":"{\"Name\":\"POCHI\",\"Breed\":\"AKITA\"}"}}'
{"name":"missing_field","id":"yLEPEv6N","message":"\"body\" is missing from body","temporary":false,"timeout":false,"fault":false}
```

### Missing `pet` (❗PANIC❗)
```shell
$ curl -X POST localhost:8888/check-in -d '{"body":{"hello":{"Type":"dog","Value":"{\"Name\":\"POCHI\",\"Breed\":\"AKITA\"}"}}}'
curl: (52) Empty reply from server
```

Server log
```shell
[pethotelapi] 16:18:22 id=wHhoJHII req=POST /check-in from=127.0.0.1
2022/12/27 16:18:22 http: panic serving 127.0.0.1:51180: runtime error: invalid memory address or nil pointer dereference
goroutine 13 [running]:
net/http.(*conn).serve.func1()
	/Users/ikawaha/sdk/go1.19.3/src/net/http/server.go:1850 +0xbf
panic({0x152e160, 0x19e6f80})
	/Users/ikawaha/sdk/go1.19.3/src/runtime/panic.go:890 +0x262
goa.design/examples/union/gen/http/pet_hotel/server.ValidatePetOwnerRequestBody(0xc000014170)
	/Users/ikawaha/go/src/github.com/goadesign/examples/union/gen/http/pet_hotel/server/types.go:103 +0x23
goa.design/examples/union/gen/http/pet_hotel/server.ValidateCheckInRequestBody(0xc0000023c0?)
	/Users/ikawaha/go/src/github.com/goadesign/examples/union/gen/http/pet_hotel/server/types.go:93 +0x74
goa.design/examples/union/gen/http/pet_hotel/server.DecodeCheckInRequest.func1(0x16a0740?)
	/Users/ikawaha/go/src/github.com/goadesign/examples/union/gen/http/pet_hotel/server/encode_decode.go:48 +0x136

...snip
```

# Comments

For `Type` and `Value`, it was possible to add a nil check by making requiredValTmpl as follows:

```Go
	requiredValTmpl = `if {{ if .isPointer }}{{ .target }} == nil || {{ end }}{{ $.target }}.{{ .attCtx.Scope.Field $.reqAtt .req true }} == nil {
        err = goa.MergeErrors(err, goa.MissingFieldError("{{ .req }}", {{ printf "%q" $.context }}))
}`
```

However, I could not figure out how to modify the generated code for the enum check. I could not disassemble it because the `target` passed to the template is `body.Pet.Type`.



Pull request:
Properly check for nil in validation
of deep data structures.

Fix #3221 

Patch:
diff --git a/codegen/validation.go b/codegen/validation.go
index 719d6890..bb7b74e0 100644
--- a/codegen/validation.go
+++ b/codegen/validation.go
@@ -187,7 +187,21 @@ func recurseValidationCode(att *expr.AttributeExpr, put expr.UserType, attCtx *A
 func validateAttribute(ctx *AttributeContext, att *expr.AttributeExpr, put expr.UserType, target, context string, req bool) string {
 	ut, isUT := att.Type.(expr.UserType)
 	if !isUT {
-		return recurseValidationCode(att, put, ctx, req, false, target, context, nil).String()
+		code := recurseValidationCode(att, put, ctx, req, false, target, context, nil).String()
+		if code == "" {
+			return ""
+		}
+		if expr.IsArray(att.Type) || expr.IsMap(att.Type) || expr.IsUnion(att.Type) {
+			return code
+		}
+		if !ctx.Pointer && (req || (att.DefaultValue != nil && ctx.UseDefault)) {
+			return code
+		}
+		cond := fmt.Sprintf("if %s != nil {\n", target)
+		if strings.HasPrefix(code, cond) {
+			return code
+		}
+		return fmt.Sprintf("%s%s\n}", cond, code)
 	}
 	if expr.IsAlias(ut) {
 		return recurseValidationCode(ut.Attribute(), put, ctx, req, true, target, context, nil).String()


Test patch:
diff --git a/codegen/testdata/validation_code.go b/codegen/testdata/validation_code.go
index 757648dd..ccce3044 100644
--- a/codegen/testdata/validation_code.go
+++ b/codegen/testdata/validation_code.go
@@ -234,18 +234,12 @@ const (
 `
 
 	AliasTypeValidationCode = `func Validate() (err error) {
-	if target.RequiredAlias != nil {
-		err = goa.MergeErrors(err, goa.ValidatePattern("target.required_alias", string(*target.RequiredAlias), "^[A-z].*[a-z]$"))
+	err = goa.MergeErrors(err, goa.ValidatePattern("target.required_alias", string(target.RequiredAlias), "^[A-z].*[a-z]$"))
+	if utf8.RuneCountInString(string(target.RequiredAlias)) < 1 {
+		err = goa.MergeErrors(err, goa.InvalidLengthError("target.required_alias", string(target.RequiredAlias), utf8.RuneCountInString(string(target.RequiredAlias)), 1, true))
 	}
-	if target.RequiredAlias != nil {
-		if utf8.RuneCountInString(string(*target.RequiredAlias)) < 1 {
-			err = goa.MergeErrors(err, goa.InvalidLengthError("target.required_alias", string(*target.RequiredAlias), utf8.RuneCountInString(string(*target.RequiredAlias)), 1, true))
-		}
-	}
-	if target.RequiredAlias != nil {
-		if utf8.RuneCountInString(string(*target.RequiredAlias)) > 10 {
-			err = goa.MergeErrors(err, goa.InvalidLengthError("target.required_alias", string(*target.RequiredAlias), utf8.RuneCountInString(string(*target.RequiredAlias)), 10, false))
-		}
+	if utf8.RuneCountInString(string(target.RequiredAlias)) > 10 {
+		err = goa.MergeErrors(err, goa.InvalidLengthError("target.required_alias", string(target.RequiredAlias), utf8.RuneCountInString(string(target.RequiredAlias)), 10, false))
 	}
 	if target.Alias != nil {
 		err = goa.MergeErrors(err, goa.ValidatePattern("target.alias", string(*target.Alias), "^[A-z].*[a-z]$"))
@@ -527,5 +521,16 @@ const (
 		}
 	}
 }
+`
+
+	TypeWithEmbeddedTypeValidationCode = `func Validate() (err error) {
+	if target.Deep != nil {
+		if target.Deep.Integer != nil {
+			if err2 := ValidateInteger(target.Deep.Integer); err2 != nil {
+				err = goa.MergeErrors(err, err2)
+			}
+		}
+	}
+}
 `
 )
diff --git a/codegen/testdata/validation_types_dsl.go b/codegen/testdata/validation_types_dsl.go
index 4eddfd0d..92da6ce8 100644
--- a/codegen/testdata/validation_types_dsl.go
+++ b/codegen/testdata/validation_types_dsl.go
@@ -65,7 +65,7 @@ var ValidationTypesDSL = func() {
 		_ = Type("AliasType", func() {
 			Attribute("required_alias", Alias)
 			Attribute("alias", Alias)
-			Required("required_string")
+			Required("required_alias")
 		})
 
 		_ = Type("UserType", func() {
@@ -147,5 +147,11 @@ var ValidationTypesDSL = func() {
 				Attribute("collection", CollectionOf(Result))
 			})
 		})
+
+		_ = Type("Deep", func() {
+			Attribute("deep", func() {
+				Attribute("integer", IntegerT)
+			})
+		})
 	)
 }
diff --git a/codegen/validation_test.go b/codegen/validation_test.go
index a4e095c0..6823675b 100644
--- a/codegen/validation_test.go
+++ b/codegen/validation_test.go
@@ -24,6 +24,7 @@ func TestRecursiveValidationCode(t *testing.T) {
 		rtT      = root.UserType("Result")
 		rtcolT   = root.UserType("Collection")
 		colT     = root.UserType("TypeWithCollection")
+		deepT    = root.UserType("Deep")
 	)
 	cases := []struct {
 		Name       string
@@ -58,6 +59,7 @@ func TestRecursiveValidationCode(t *testing.T) {
 		{"collection-required", rtcolT, true, false, false, testdata.ResultCollectionPointerValidationCode},
 		{"collection-pointer", rtcolT, false, true, false, testdata.ResultCollectionPointerValidationCode},
 		{"type-with-collection-pointer", colT, false, true, false, testdata.TypeWithCollectionPointerValidationCode},
+		{"type-with-embedded-type", deepT, false, true, false, testdata.TypeWithEmbeddedTypeValidationCode},
 	}
 	for _, c := range cases {
 		t.Run(c.Name, func(t *testing.T) {