DisallowAllDefaultValues behaves unexpectedly for null

[Varorbc]

RequiredAttribute.DisallowAllDefaultValues

The RequiredAttribute now allows validating that structs do not equal their default values. For example:

[Required(DisallowAllDefaultValues = true)]
public Guid MyGuidValue { get; set; }

will fail validation if its value equals Guid.Empty.

If the property is null, it does not work well. Because the default value of the nullable property is null, and the RequiredAttribute itself is validation null, DisallowAllDefaultValues will meaningless. I expect validation to be successful when the value is Guid.Empty.

Originally posted by @eiriktsarpalis in dotnet/core#8134 (comment)

[ghost]

Tagging subscribers to this area: @jeffhandley
See info in area-owners.md if you want to be subscribed.

Issue Details

---

          ## System.ComponentModel.DataAnnotations Extensions https://github.com/dotnet/runtime/pull/82311

We're introducing extensions to the built-in validation attributes in System.ComponentModel.DataAnnotations:

RequiredAttribute.DisallowAllDefaultValues

The RequiredAttribute now allows validating that structs do not equal their default values. For example:

[Required(DisallowAllDefaultValues = true)]
public Guid MyGuidValue { get; set; }

will fail validation if its value equals Guid.Empty.

RangeAttribute exclusive bounds

Users can now specify exclusive bounds in their range validation:

[Range(0d, 1d, IsLowerBoundExclusive = false, IsUpperBoundExclusive = false)]
public double Sample { get; set; }

This accepts any values in the open $(0, 1)$ interval but rejects the boundary values 0 and 1 themselves.

LengthAttribute

The LengthAttribute can now be used to set both lower and upper bounds for strings or collections:

[Length(10, 20)] // Require at least 10 elements and at most 20 elements.
public ICollection<int> Values { get; set; }

AllowedValuesAttribute and DeniedValuesAttribute

These attributes can be used to specify allow lists and deny lists for validating a property:

[AllowedValues("apple", "banana", "mango")]
public string Fruit { get; set; }

[DeniedValues("pineapple", "anchovy", "broccoli")]
public string PizzaTopping { get; set; }

Base64StringAttribute

As the name suggests, this attribute validates that a given string is a valid Base64 representation.

Originally posted by @eiriktsarpalis in dotnet/core#8134 (comment)

Author:	Varorbc
Assignees:	-
Labels:	area-System.ComponentModel.DataAnnotations
Milestone:	-

[jeffhandley]

Hi @Varorbc, to make sure your issue is understood, can you confirm that the scenario you're concerned about is the following?

public class Entity_DisallowDefault
{
    [Required(DisallowAllDefaultValues = true)]
    public Guid? EntityId { get; set; }
}

[Fact]
public void DisallowDefault_Null()
{
    Entity_DisallowDefault entity = new();
    List<ValidationResult> results = new();
    ValidationContext context = new(entity);
    bool isValid = Validator.TryValidateObject(entity, context, results);

    Assert.False(isValid);
}

[Fact]
public void DisallowDefault_Empty()
{
    Entity_DisallowDefault entity = new() { EntityId = Guid.Empty };
    List<ValidationResult> results = new();
    ValidationContext context = new(entity);
    bool isValid = Validator.TryValidateObject(entity, context, results);

    Assert.True(isValid); // YOU ARE EXPECTING THIS TO BE INVALID, BUT IT IS VALID
}

[Varorbc]

[Fact]
public void DisallowDefault_Empty()
{
Entity_DisallowDefault entity = new() { EntityId = Guid.Empty };
List results = new();
ValidationContext context = new(entity);
bool isValid = Validator.TryValidateObject(entity, context, results);

Assert.True(isValid); // YOU ARE EXPECTING THIS TO BE INVALID, BUT IT IS VALID

}

@jeffhandley Yes, I expect this to be invalid

[pinkfloydx33]

I can sort of see both sides of this. Compare it to strings with AllowEmptyStrings=false; The attribute itself prevents null and the additional argument prevents the empty string. In this case it feels like it should do something similar: the attribute's base behavior prevents null, and setting DisallowAllDefaultValues=true prevents Guid.Empty.

Of course nullable structs are legitimate types unlikes then difference between string and string? so it's not quite the same thing. The default value in this case is null so the attribute is doing what it claims to. Plus, if it's meant to be required why bother making it nullable in the first place?

If this is being used in MVC, a null/absent Guid in the request should result in Guid.Empty for a non-nullable property, and DisallowAllDefaultValues would take care of the rest. If using the STJ model binder, JsonRequired would result in different errors for null (in both scenarios) that don't even make it to data annotations.

On the flip side, keeping the property as nullable could have some value depending on your use case. For example, null might be a valid value whereas Guid.Empty isn't, like if you need to distinguish between the absent (null) and default/empty cases. In this scenario you'd probably want to allow null but not Guid.Empty but there's no combination of settings for the attribute that can achieve that. This is why we made our own DisallowEmpty attribute that allows null but fails for Guid.Empty and collections of size=0; we fallback to RequiredAtrribute for everything else.

TLDR: I think the attribute is doing the "right"--or least surprising--thing for the situation. Perhaps the current binary nature of the property needs to be changed to an enum with various knobs? Otherwise a custom validation attribute could help

[eiriktsarpalis]

I can see why you might expect this behaviour, but I think it's important to consider what RequiredAttribute is meant to support and what motivated the inclusion of DisallowAllDefaultValues: generally speaking it is meant to detect the absence of a value being set by the underlying deserializer. For unbound properties of type Guid you would expect a value of default(Guid) and for unbound properties of type Guid? you would expect a value of null. If the value is (Guid?)default(Guid) instead, that is almost always an indication that this value has been set explicitly by a deserializer and therefore a RequiredAttribute should accept it as valid.

As @pinkfloydx33 already points out, the DisallowAllDefaultValues property does what it says on the tin, and extending it to reject (T?)default(T) values would require a different name, and I would say lies beyond the scope of what RequiredAttribute is meant to handle.

[Varorbc]

In this case it feels like it should do something similar: the attribute's base behavior prevents null, and setting DisallowAllDefaultValues=true prevents Guid.Empty.

The case you mentioned is for non-nullable property, and I think there is no problem. But if it is an nullable property and this is being used in MVC,if I send null data, as you said, the attribute's base behavior prevents null, and setting DisallowAllDefaultValues=true will prevents null, they do the same thing. Is this DisallowAllDefaultValues still meaningful?

if it's meant to be required why bother making it nullable in the first place?
If this is being used in MVC, a null/absent Guid in the request should result in Guid.Empty for a non-nullable property, and DisallowAllDefaultValues would take care of the rest.

Because if I mark it as a non-nullable property, it won't work very well if I send null data. So, all I can do for now is mark it as a nullable property and expect the data that prevents sending to be Guid. Empty
#83706

[jeffhandley]

I'm going to reopen this. I think we should reconsider this design given the immediate feedback we've received (now from multiple people).

[eiriktsarpalis]

I'm going to reopen this. I think we should reconsider this design given the immediate feedback we've received (now from multiple people).

I'm conflicted about this. If we did change the behaviour to include (TStruct?)default(TStruct), I anticipate that an equal amount would be surprised by that behaviour in different contexts (e.g. validation failing for int? properties that are set to 0). My recommendation is to not make this change for the following reasons:

1. It would necessitate a name change for the property to reflect the new semantics.
2. RequiredAttribute works as intended for nullable types without need for the new flag, which was specifically introduced to account for non-nullable structs.
3. A property containing (TStruct?)default(TStruct) typically indicates that this value was explicitly set by a deserializer, and is not a indication that no value or a null value was provided.

cc @geeknoid

[jeffhandley]

Another approach on this could be to remove this functionality from RequiredAttribute and instead use a different validator for this logic. Too bad we can't use DeniedValuesAttribute for it, but something targeted for this scenario might be better anyway.