Add the attribute from the request to at-least-one validator's error message

[ChihweiLHBird]

In the schema:

var resourceSchema = schema.Schema{
	Attributes: map[string]schema.Attribute{
		// ... many other attributes
		"attribute1": schema.Int64Attribute{
			Validators: []validator.Int64{
				int64validator.AtLeastOneOf(path.MatchRoot("attribute2")),
			},
			Optional:    true,
			Computed:    true,
		},
	},
}

Let's don't put both attributes in the Terraform config file.

Previous error message:

│ Error: Invalid Attribute Combination
│ 
│   with my_reource.my-resource-instance,
│   on main.tf line 22, in resource "my_reource" "my-resource-instance":
│   22: resource "my_reource" "my-resource-instance" {
│ 
│ At least one attribute out of [attribute2] must be specified

This message above is misleading because it actually requires one of attribute1 and attribute2, but attribute1 is not in the error message.

Now with the change in this PR:

│ Error: Invalid Attribute Combination
│ 
│   with my_reource.my-resource-instance,
│   on main.tf line 22, in resource "my_reource" "my-resource-instance":
│   22: resource "my_reource" "my-resource-instance" {
│ 
│ At least one attribute out of [attribute1,attribute2] must be specified

We can correctly inform the user what attributes are required.

[bendbennett]

Hi @ChihweiLHBird 👋

Thank you for this submission.

Could I ask that you add test coverage to verify the behaviour of this change?
Perhaps a new test to confirm that both the attribute upon which the AtLeastOneOf validator is declared, and all of the attributes supplied in the path.Expression args appear in the diagnostic.

Could you also add a change log entry as described in the New Pull Request section of the doc on contributing?

Thanks again.

[ChihweiLHBird]

Hi @bendbennett, thanks for letting me know! I just added them.

[bendbennett]

Thank you for adding the change log entry, and updating the tests.

How would you feel about the suggested changes to the tests, in which expErrors int, and expDiagMsgSegs []string are dropped in favour of using expected *schemavalidator.AtLeastOneOfValidatorResponse?

With the suggested changes, the following would also need to be updated:

testCases := map[string]testCase{
		"base": {
			/* ... */
			expected: &schemavalidator.AtLeastOneOfValidatorResponse{},
		},
		"self-is-null": {
			/* ... */
			expected: &schemavalidator.AtLeastOneOfValidatorResponse{},
		},
		"error_none-set": {
			/* ... */
			expected: &schemavalidator.AtLeastOneOfValidatorResponse{
				Diagnostics: diag.Diagnostics{
					diag.WithPath(
						path.Root("bar"),
						diag.NewErrorDiagnostic(
							"Invalid Attribute Combination",
							"At least one attribute out of [foo,baz,bar] must be specified",
						),
					),
				},
			},
		},
		"multiple-set": {
			/* ... */
			expected: &schemavalidator.AtLeastOneOfValidatorResponse{},
		},
		"allow-duplicate-input": {
			/* ... */
			expected: &schemavalidator.AtLeastOneOfValidatorResponse{},
		},
		"unknowns": {
			/* ... */
			expected: &schemavalidator.AtLeastOneOfValidatorResponse{},
		},
		"matches-no-attribute-in-schema": {
			/* ... */
			expected: &schemavalidator.AtLeastOneOfValidatorResponse{
				Diagnostics: diag.Diagnostics{
					diag.NewErrorDiagnostic(
						"Invalid Path Expression for Schema",
						"The Terraform Provider unexpectedly provided a path expression that does not match the current schema. "+
							"This can happen if the path expression does not correctly follow the schema in structure or types. "+
							"Please report this to the provider developers.\n\nPath Expression: fooz",
					),
					diag.WithPath(
						path.Root("bar"),
						diag.NewErrorDiagnostic(
							"Invalid Attribute Combination",
							"At least one attribute out of [fooz,bar] must be specified",
						),
					),
				},
			},
		},
	}

	for name, test := range testCases {
		name, test := name, test
		t.Run(name, func(t *testing.T) {
			t.Parallel()
			res := &schemavalidator.AtLeastOneOfValidatorResponse{}

			schemavalidator.AtLeastOneOfValidator{
				PathExpressions: test.in,
			}.Validate(context.TODO(), test.req, res)

			if diff := cmp.Diff(test.expected, res); diff != "" {
				t.Errorf("unexpected diff: %s", diff)
			}
		})
	}

Interested to hear your thoughts.

[ChihweiLHBird]

Hi @bflad, thanks for the suggestion! I think this suggestion is fine, but one downside is that in the future, if any error message is changed in the validator, the tests will need to be adjusted accordingly. This is somehow against the design principle of SSOT and might lead to more effort of maintenance required in the future IMO. However, they are only few error messages, thus the impact is pretty minimum and I think it would be fine. If you think it's okay, I can proceed to this way.

[bendbennett]

Hi @ChihweiLHBird,

Thank you for updating the PR.

The point you make about interrogating the contents of the diagnostics is a good one, but I think in this instance it is warranted as we want to be sure of the contents and structure of the diagnostic error message. This pattern has been used elsewhere in this repository, such as in TestConflictingValidatorValidate.

[bendbennett]

@ChihweiLHBird thank you very much for your PR. This looks good to me.

[github-actions]

I'm going to lock this pull request because it has been closed for 30 days ⏳. This helps our maintainers find and focus on the active contributions.
If you have found a problem that seems related to this change, please open a new issue and complete the issue template so we can capture all the details necessary to investigate further.