Repository: goadesign/goa (go)

Issue:
Goa attribute validation "ExclusiveMinimum" is not exclusive
Hello, 

Thank you guys for the awesome work, we appreciate it. While working with GOA I have recently stumbled upon a bug concerning the exclusive minimum when it comes to attribute validation. More specifically, the exclusive minimum behaves as an inclusive one - the value provided is still validated as if desired. The following DSL validates 0 as a proper value: 

var ExchangeRate = Type("ExchangeRate", func() {
	Attribute("rate", Float64, "Exchange rate for the currency.", func() {
		ExclusiveMinimum(0)
	})
})

Upon a request with a negative value, the message displayed also hints at non-exclusivity: "rate must be greater **or equal** than 0 but got value -0.1" e.g.

Thank you for your time.

Pull request:
Fix generate code for exclusive min and max
Add validations for validations :)

Fix #3160 

Patch:
diff --git a/codegen/validation.go b/codegen/validation.go
index 60480da8..719d6890 100644
--- a/codegen/validation.go
+++ b/codegen/validation.go
@@ -287,7 +287,6 @@ func validationCode(att *expr.AttributeExpr, attCtx *AttributeContext, req, alia
 	if exclMin := validation.ExclusiveMinimum; exclMin != nil {
 		data["exclMin"] = *exclMin
 		data["isExclMin"] = true
-		delete(data, "exclMax")
 		if val := runTemplate(exclMinMaxValT, data); val != "" {
 			res = append(res, val)
 		}
@@ -295,7 +294,6 @@ func validationCode(att *expr.AttributeExpr, attCtx *AttributeContext, req, alia
 	if min := validation.Minimum; min != nil {
 		data["min"] = *min
 		data["isMin"] = true
-		delete(data, "max")
 		if val := runTemplate(minMaxValT, data); val != "" {
 			res = append(res, val)
 		}
@@ -303,7 +301,6 @@ func validationCode(att *expr.AttributeExpr, attCtx *AttributeContext, req, alia
 	if exclMax := validation.ExclusiveMaximum; exclMax != nil {
 		data["exclMax"] = *exclMax
 		data["isExclMax"] = true
-		delete(data, "exclMax")
 		if val := runTemplate(exclMinMaxValT, data); val != "" {
 			res = append(res, val)
 		}
@@ -311,7 +308,6 @@ func validationCode(att *expr.AttributeExpr, attCtx *AttributeContext, req, alia
 	if max := validation.Maximum; max != nil {
 		data["max"] = *max
 		data["isMin"] = false
-		delete(data, "min")
 		if val := runTemplate(minMaxValT, data); val != "" {
 			res = append(res, val)
 		}
@@ -530,7 +526,7 @@ if !({{ oneof .targetVal .values }}) {
 
 	exclMinMaxValTmpl = `{{ if .isPointer }}if {{ .target }} != nil {
 {{ end -}}
-        if {{ .targetVal }} {{ if .isExclMin }}<{{ else }}>{{ end }} {{ if .isExclMin }}{{ .exclMin }}{{ else }}{{ .exclMax }}{{ end }} {
+        if {{ .targetVal }} {{ if .isExclMin }}<={{ else }}>={{ end }} {{ if .isExclMin }}{{ .exclMin }}{{ else }}{{ .exclMax }}{{ end }} {
         err = goa.MergeErrors(err, goa.InvalidRangeError({{ printf "%q" .context }}, {{ .targetVal }}, {{ if .isExclMin }}{{ .exclMin }}, true{{ else }}{{ .exclMax }}, false{{ end }}))
 {{ if .isPointer -}}
 }
diff --git a/expr/attribute.go b/expr/attribute.go
index 5c911fcf..8cdab01b 100644
--- a/expr/attribute.go
+++ b/expr/attribute.go
@@ -198,6 +198,9 @@ func (a *AttributeExpr) Validate(ctx string, parent eval.Expression) *eval.Valid
 		ctx += " - "
 	}
 	verr.Merge(a.validateEnumDefault(ctx, parent))
+	if v := a.Validation; v != nil {
+		verr.Merge(v.Validate(ctx, parent))
+	}
 	if o := AsObject(a.Type); o != nil {
 		for _, n := range a.AllRequired() {
 			if a.Find(n) == nil {
@@ -738,6 +741,35 @@ func (a *ExampleExpr) EvalName() string {
 	return `example "` + a.Summary + `"`
 }
 
+// Validate validates the validation expression.
+func (v *ValidationExpr) Validate(ctx string, parent eval.Expression) *eval.ValidationErrors {
+	verr := new(eval.ValidationErrors)
+	hasMin, hasMax := v.Minimum != nil, v.Maximum != nil
+	hasExclusiveMin, hasExclusiveMax := v.ExclusiveMinimum != nil, v.ExclusiveMaximum != nil
+	if hasMin && hasExclusiveMin {
+		verr.Add(parent, "%sboth minimum and exclusive minimum are defined", ctx)
+	}
+	if hasMax && hasExclusiveMax {
+		verr.Add(parent, "%sboth maximum and exclusive maximum are defined", ctx)
+	}
+	if hasMin && hasMax && *v.Minimum > *v.Maximum {
+		verr.Add(parent, "%sminimum is greater than maximum", ctx)
+	}
+	if hasMin && hasExclusiveMax && *v.Minimum >= *v.ExclusiveMaximum {
+		verr.Add(parent, "%sminimum is greater than or equal to exclusive maximum", ctx)
+	}
+	if hasExclusiveMin && hasExclusiveMax && *v.ExclusiveMinimum > *v.ExclusiveMaximum {
+		verr.Add(parent, "%sexclusive minimum is greater than exclusive maximum", ctx)
+	}
+	if hasExclusiveMin && hasMax && *v.ExclusiveMinimum >= *v.Maximum {
+		verr.Add(parent, "%sexclusive minimum is greater than or equal to maximum", ctx)
+	}
+	if v.MinLength != nil && v.MaxLength != nil && *v.MinLength > *v.MaxLength {
+		verr.Add(parent, "%smin length is greater than max length", ctx)
+	}
+	return verr
+}
+
 // Merge merges other into v.
 func (v *ValidationExpr) Merge(other *ValidationExpr) {
 	if v.Values == nil {


Test patch:
diff --git a/codegen/testdata/validation_code.go b/codegen/testdata/validation_code.go
index 7e76edf1..757648dd 100644
--- a/codegen/testdata/validation_code.go
+++ b/codegen/testdata/validation_code.go
@@ -16,12 +16,12 @@ const (
 		}
 	}
 	if target.ExclusiveInteger != nil {
-		if *target.ExclusiveInteger < 1 {
+		if *target.ExclusiveInteger <= 1 {
 			err = goa.MergeErrors(err, goa.InvalidRangeError("target.exclusive_integer", *target.ExclusiveInteger, 1, true))
 		}
 	}
 	if target.ExclusiveInteger != nil {
-		if *target.ExclusiveInteger < 1 {
+		if *target.ExclusiveInteger <= 1 {
 			err = goa.MergeErrors(err, goa.InvalidRangeError("target.exclusive_integer", *target.ExclusiveInteger, 1, true))
 		}
 	}
@@ -48,12 +48,12 @@ const (
 		}
 	}
 	if target.ExclusiveInteger != nil {
-		if *target.ExclusiveInteger < 1 {
+		if *target.ExclusiveInteger <= 1 {
 			err = goa.MergeErrors(err, goa.InvalidRangeError("target.exclusive_integer", *target.ExclusiveInteger, 1, true))
 		}
 	}
 	if target.ExclusiveInteger != nil {
-		if *target.ExclusiveInteger < 1 {
+		if *target.ExclusiveInteger <= 1 {
 			err = goa.MergeErrors(err, goa.InvalidRangeError("target.exclusive_integer", *target.ExclusiveInteger, 1, true))
 		}
 	}
@@ -73,12 +73,12 @@ const (
 		}
 	}
 	if target.ExclusiveInteger != nil {
-		if *target.ExclusiveInteger < 1 {
+		if *target.ExclusiveInteger <= 1 {
 			err = goa.MergeErrors(err, goa.InvalidRangeError("target.exclusive_integer", *target.ExclusiveInteger, 1, true))
 		}
 	}
 	if target.ExclusiveInteger != nil {
-		if *target.ExclusiveInteger < 1 {
+		if *target.ExclusiveInteger <= 1 {
 			err = goa.MergeErrors(err, goa.InvalidRangeError("target.exclusive_integer", *target.ExclusiveInteger, 1, true))
 		}
 	}
@@ -100,12 +100,12 @@ const (
 		}
 	}
 	if target.ExclusiveFloat64 != nil {
-		if *target.ExclusiveFloat64 < 1 {
+		if *target.ExclusiveFloat64 <= 1 {
 			err = goa.MergeErrors(err, goa.InvalidRangeError("target.exclusive_float64", *target.ExclusiveFloat64, 1, true))
 		}
 	}
 	if target.ExclusiveFloat64 != nil {
-		if *target.ExclusiveFloat64 < 1 {
+		if *target.ExclusiveFloat64 <= 1 {
 			err = goa.MergeErrors(err, goa.InvalidRangeError("target.exclusive_float64", *target.ExclusiveFloat64, 1, true))
 		}
 	}
@@ -132,12 +132,12 @@ const (
 		}
 	}
 	if target.ExclusiveFloat64 != nil {
-		if *target.ExclusiveFloat64 < 1 {
+		if *target.ExclusiveFloat64 <= 1 {
 			err = goa.MergeErrors(err, goa.InvalidRangeError("target.exclusive_float64", *target.ExclusiveFloat64, 1, true))
 		}
 	}
 	if target.ExclusiveFloat64 != nil {
-		if *target.ExclusiveFloat64 < 1 {
+		if *target.ExclusiveFloat64 <= 1 {
 			err = goa.MergeErrors(err, goa.InvalidRangeError("target.exclusive_float64", *target.ExclusiveFloat64, 1, true))
 		}
 	}
@@ -157,12 +157,12 @@ const (
 		}
 	}
 	if target.ExclusiveFloat64 != nil {
-		if *target.ExclusiveFloat64 < 1 {
+		if *target.ExclusiveFloat64 <= 1 {
 			err = goa.MergeErrors(err, goa.InvalidRangeError("target.exclusive_float64", *target.ExclusiveFloat64, 1, true))
 		}
 	}
 	if target.ExclusiveFloat64 != nil {
-		if *target.ExclusiveFloat64 < 1 {
+		if *target.ExclusiveFloat64 <= 1 {
 			err = goa.MergeErrors(err, goa.InvalidRangeError("target.exclusive_float64", *target.ExclusiveFloat64, 1, true))
 		}
 	}
diff --git a/expr/attribute_test.go b/expr/attribute_test.go
index 9f53b5ae..a8e33ed2 100644
--- a/expr/attribute_test.go
+++ b/expr/attribute_test.go
@@ -825,3 +825,44 @@ func TestAttributeExprEvalName(t *testing.T) {
 	}
 
 }
+
+func TestAttributeExprValidationValidate(t *testing.T) {
+	var (
+		max       = 1.0
+		exclMax   = 2.0
+		min       = 3.0
+		ExclMin   = 4.0
+		MaxLength = 5
+		MinLength = 6
+		parent    = &UserTypeExpr{
+			AttributeExpr: &AttributeExpr{Type: String},
+			TypeName:      "Parent",
+		}
+	)
+	cases := map[string]struct {
+		min, max, exclMin, exclMax *float64
+		minLen, maxLen             *int
+		expected                   string
+	}{
+		"min and max":         {min: &min, max: &max, expected: "attribute: minimum is greater than maximum"},
+		"min and exclMax":     {min: &min, exclMax: &exclMax, expected: "attribute: minimum is greater than or equal to exclusive maximum"},
+		"exclMin and max":     {exclMin: &ExclMin, max: &max, expected: "attribute: exclusive minimum is greater than or equal to maximum"},
+		"exclMin and exclMax": {exclMin: &ExclMin, exclMax: &exclMax, expected: "attribute: exclusive minimum is greater than exclusive maximum"},
+		"max and exclMax":     {max: &max, exclMax: &exclMax, expected: "attribute: both maximum and exclusive maximum are defined"},
+		"min and exclMin":     {min: &min, exclMin: &ExclMin, expected: "attribute: both minimum and exclusive minimum are defined"},
+		"minLen and maxLen":   {minLen: &MinLength, maxLen: &MaxLength, expected: "attribute: min length is greater than max length"},
+	}
+	for k, tc := range cases {
+		validation := &ValidationExpr{
+			Minimum:          tc.min,
+			Maximum:          tc.max,
+			ExclusiveMinimum: tc.exclMin,
+			ExclusiveMaximum: tc.exclMax,
+			MinLength:        tc.minLen,
+			MaxLength:        tc.maxLen,
+		}
+		if actual := validation.Validate("", parent); actual.Error() != tc.expected {
+			t.Errorf("%s: got %#v, expected %#v", k, actual.Error(), tc.expected)
+		}
+	}
+}