Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

core: Check rule error message expressions #30613

Merged
merged 4 commits into from Mar 7, 2022
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
82 changes: 9 additions & 73 deletions internal/configs/checks.go
Expand Up @@ -2,10 +2,8 @@ package configs

import (
"fmt"
"unicode"

"github.com/hashicorp/hcl/v2"
"github.com/hashicorp/hcl/v2/gohcl"
"github.com/hashicorp/terraform/internal/addrs"
"github.com/hashicorp/terraform/internal/lang"
)
Expand All @@ -24,12 +22,15 @@ type CheckRule struct {
// input variables can only refer to the variable that is being validated.
Condition hcl.Expression

// ErrorMessage is one or more full sentences, which would need to be in
// ErrorMessage should be one or more full sentences, which should be in
// English for consistency with the rest of the error message output but
// can in practice be in any language as long as it ends with a period.
// The message should describe what is required for the condition to return
// true in a way that would make sense to a caller of the module.
ErrorMessage string
// can in practice be in any language. The message should describe what is
// required for the condition to return true in a way that would make sense
// to a caller of the module.
//
// The error message expression has the same variables available for
// interpolation as the corresponding condition.
ErrorMessage hcl.Expression

DeclRange hcl.Range
}
Expand Down Expand Up @@ -111,77 +112,12 @@ func decodeCheckRuleBlock(block *hcl.Block, override bool) (*CheckRule, hcl.Diag
}

if attr, exists := content.Attributes["error_message"]; exists {
moreDiags := gohcl.DecodeExpression(attr.Expr, nil, &cr.ErrorMessage)
diags = append(diags, moreDiags...)
if !moreDiags.HasErrors() {
const errSummary = "Invalid validation error message"
switch {
case cr.ErrorMessage == "":
diags = diags.Append(&hcl.Diagnostic{
Severity: hcl.DiagError,
Summary: errSummary,
Detail: "An empty string is not a valid nor useful error message.",
Subject: attr.Expr.Range().Ptr(),
})
case !looksLikeSentences(cr.ErrorMessage):
// Because we're going to include this string verbatim as part
// of a bigger error message written in our usual style in
// English, we'll require the given error message to conform
// to that. We might relax this in future if e.g. we start
// presenting these error messages in a different way, or if
// Terraform starts supporting producing error messages in
// other human languages, etc.
// For pragmatism we also allow sentences ending with
// exclamation points, but we don't mention it explicitly here
// because that's not really consistent with the Terraform UI
// writing style.
diags = diags.Append(&hcl.Diagnostic{
Severity: hcl.DiagError,
Summary: errSummary,
Detail: "The validation error message must be at least one full sentence starting with an uppercase letter and ending with a period or question mark.\n\nYour given message will be included as part of a larger Terraform error message, written as English prose. For broadly-shared modules we suggest using a similar writing style so that the overall result will be consistent.",
Subject: attr.Expr.Range().Ptr(),
})
}
}
cr.ErrorMessage = attr.Expr
}

return cr, diags
}

// looksLikeSentence is a simple heuristic that encourages writing error
// messages that will be presentable when included as part of a larger
// Terraform error diagnostic whose other text is written in the Terraform
// UI writing style.
//
// This is intentionally not a very strong validation since we're assuming
// that module authors want to write good messages and might just need a nudge
// about Terraform's specific style, rather than that they are going to try
// to work around these rules to write a lower-quality message.
func looksLikeSentences(s string) bool {
if len(s) < 1 {
return false
}
runes := []rune(s) // HCL guarantees that all strings are valid UTF-8
first := runes[0]
last := runes[len(runes)-1]

// If the first rune is a letter then it must be an uppercase letter.
// (This will only see the first rune in a multi-rune combining sequence,
// but the first rune is generally the letter if any are, and if not then
// we'll just ignore it because we're primarily expecting English messages
// right now anyway, for consistency with all of Terraform's other output.)
if unicode.IsLetter(first) && !unicode.IsUpper(first) {
return false
}

// The string must be at least one full sentence, which implies having
// sentence-ending punctuation.
// (This assumes that if a sentence ends with quotes then the period
// will be outside the quotes, which is consistent with Terraform's UI
// writing style.)
return last == '.' || last == '?' || last == '!'
}

var checkRuleBlockSchema = &hcl.BodySchema{
Attributes: []hcl.AttributeSchema{
{
Expand Down
25 changes: 25 additions & 0 deletions internal/configs/named_values.go
Expand Up @@ -345,6 +345,31 @@ func decodeVariableValidationBlock(varName string, block *hcl.Block, override bo
}
}

if vv.ErrorMessage != nil {
// The same applies to the validation error message, except that
// references are not required. A string literal is a valid error
// message.
goodRefs := 0
for _, traversal := range vv.ErrorMessage.Variables() {
ref, moreDiags := addrs.ParseRef(traversal)
if !moreDiags.HasErrors() {
if addr, ok := ref.Subject.(addrs.InputVariable); ok {
if addr.Name == varName {
goodRefs++
continue // Reference is valid
}
}
}
// If we fall out here then the reference is invalid.
diags = diags.Append(&hcl.Diagnostic{
Severity: hcl.DiagError,
Summary: "Invalid reference in variable validation",
Detail: fmt.Sprintf("The error message for variable %q can only refer to the variable itself, using var.%s.", varName, varName),
Subject: traversal.SourceRange().Ptr(),
})
}
}

return vv, diags
}

Expand Down
33 changes: 0 additions & 33 deletions internal/configs/named_values_test.go

This file was deleted.

This file was deleted.

Expand Up @@ -9,3 +9,10 @@ variable "validation" {
error_message = "Must be five."
}
}

variable "validation_error_expression" {
validation {
condition = var.validation_error_expression != 1
error_message = "Cannot equal ${local.foo}." # ERROR: Invalid reference in variable validation
}
}
16 changes: 16 additions & 0 deletions internal/configs/testdata/valid-files/variable_validation.tf
Expand Up @@ -4,3 +4,19 @@ variable "validation" {
error_message = "Must be five."
}
}

variable "validation_function" {
type = list(string)
validation {
condition = length(var.validation_function) > 0
error_message = "Must not be empty."
}
}

variable "validation_error_expression" {
type = list(string)
validation {
condition = length(var.validation_error_expression) < 10
error_message = "Too long (${length(var.validation_error_expression)} is greater than 10)."
}
}
2 changes: 1 addition & 1 deletion internal/terraform/context_plan2_test.go
Expand Up @@ -2561,7 +2561,7 @@ func TestContext2Plan_preconditionErrors(t *testing.T) {
{
"test_resource.c.value",
"Invalid condition result",
"Invalid validation condition result value: a bool is required",
"Invalid condition result value: a bool is required",
},
}

Expand Down