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

Property display name set via DisplayAttribute in validation messages are not used when app is native AOT published but are when trimmed #84324

Closed
DamianEdwards opened this issue Apr 4, 2023 · 3 comments · Fixed by #84326

Comments

@DamianEdwards
Copy link
Member

DamianEdwards commented Apr 4, 2023

Description

In investigating the requirements to make #85475 native AOT friendly, I encountered differing behavior WRT to a console app that is publish trimmed (TrimMode=full) vs. publish native AOT, whereby validation error messages use the property display name specified via the attribute [Display(Name = "Property display name")] or [Display(Name = "ResourceKey", ResourceType = typeof(MyResources))] when the app is published trimmed, but not when the app is published native AOT.

The trimming warning from calling new ValidationContext(instance) is being suppressed in the repro as this investigation is looking at what's possible WRT to a potential source generator implementation.

Should the behavior not be the same in this case when publishing trimmed vs. publishing native AOT?

Reproduction Steps

using System.ComponentModel.DataAnnotations;
using System.Diagnostics.CodeAnalysis;

// Title is blank
var todo = new Todo { Title = "" };
var validationContext = CreateValidationContext(todo, nameof(todo.Title));
Console.WriteLine("Empty Todo.Title:");
ValidateDirect(todo.Title, validationContext);
ValidateValidator(todo.Title, validationContext);
Console.WriteLine();

// Title is too long
Console.WriteLine("Long Todo.Title:");
todo.Title = "123456789";
validationContext = CreateValidationContext(todo, nameof(todo.Title));
ValidateDirect(todo.Title, validationContext);
ValidateValidator(todo.Title, validationContext);
Console.WriteLine();

static void ValidateDirect(object? value, ValidationContext context)
{
    var attributes = GetValidationAttributes();

    foreach (var attribute in attributes)
    {
        var directResult = attribute.GetValidationResult(value, context);
        Console.WriteLine($"{attribute.GetType().Name}.GetValidationResult: ");
        Console.WriteLine(directResult?.ErrorMessage is null ? "Valid" : directResult.ErrorMessage);
    }
}

static bool ValidateValidator(object? value, ValidationContext context)
{
    var attributes = GetValidationAttributes();
    var validationResults = new List<ValidationResult>();
    var validatorResult = Validator.TryValidateValue(value!, context, validationResults, attributes);
    Console.WriteLine("Validator.TryValidateValue: ");
    Console.WriteLine(validatorResult ? "Valid" : "Invalid");
    if (!validatorResult)
    {
        foreach (var item in validationResults)
        {
            Console.WriteLine(item.ErrorMessage);
        }
    }
    return validatorResult;
}

[UnconditionalSuppressMessage("Trimming", "IL2026:Members annotated with 'RequiresUnreferencedCodeAttribute' require dynamic access otherwise can break functionality when trimming application code", Justification = "<Pending>")]
static ValidationContext CreateValidationContext<T>(T instance, string? memberName = null, string? displayName = null)
    where T : notnull
{
    var context = new ValidationContext(instance) { MemberName = memberName };

    if (!string.IsNullOrEmpty(displayName))
    {
        context.DisplayName = displayName;
    }

    return context;
}

static ValidationAttribute[] GetValidationAttributes()
    => new ValidationAttribute[]
    {
        // This must match what is declared on the Todo.Title property below
        new RequiredAttribute(),
        new StringLengthAttribute(5) { MinimumLength = 2 }
    };

public class Todo
{
    public int Id { get; set; }

    [Required]
    [StringLength(5, MinimumLength = 2)]
    //[Display(Name = "TitleDisplayName", ResourceType = typeof(ValidationMessages))]
    [Display(Name = "Title Display Name")]
    public string? Title { get; set; }
}

Expected behavior

The behavior of Validator.TryValidateValues and ValidationAttribute.GetValidationResult WRT to getting the display name for the member indicated by ValidationContext.MemberName when that property is annotated with DisplayAttribute, is the same when publishing trimmed and publishing native AOT.

Actual behavior

After publishing trimmed, the validation messages contain the property display names specified in the DisplayAttribute instance, but when publishing native AOT, the validation messages just use the property name.

Output of app after publish trimmed:

Empty Todo.Title:
RequiredAttribute.GetValidationResult:
The Title Display Name field is required.
StringLengthAttribute.GetValidationResult:
The field Title Display Name must be a string with a minimum length of 2 and a maximum length of 5.
Validator.TryValidateValue:
Invalid
The Title Display Name field is required.

Long Todo.Title:
RequiredAttribute.GetValidationResult:
Valid
StringLengthAttribute.GetValidationResult:
The field Title Display Name must be a string with a minimum length of 2 and a maximum length of 5.
Validator.TryValidateValue:
Invalid
The field Title Display Name must be a string with a minimum length of 2 and a maximum length of 5.

Output of app after publish native AOT:

Empty Todo.Title:
RequiredAttribute.GetValidationResult:
The Title field is required.
StringLengthAttribute.GetValidationResult:
The field Title must be a string with a minimum length of 2 and a maximum length of 5.
Validator.TryValidateValue:
Invalid
The Title field is required.

Long Todo.Title:
RequiredAttribute.GetValidationResult:
Valid
StringLengthAttribute.GetValidationResult:
The field Title must be a string with a minimum length of 2 and a maximum length of 5.
Validator.TryValidateValue:
Invalid
The field Title must be a string with a minimum length of 2 and a maximum length of 5.

Regression?

No response

Known Workarounds

No response

Configuration

SDK: 8.0.100-preview.3.23178.7
Runtime: 8.0.0-preview.3.23174.8

Windows 11 x64

Other information

No response

@ghost ghost added the untriaged New issue has not been triaged by the area owner label Apr 4, 2023
@ghost
Copy link

ghost commented Apr 4, 2023

Tagging subscribers to this area: @dotnet/area-system-componentmodel-dataannotations
See info in area-owners.md if you want to be subscribed.

Issue Details

Description

In investigating the requirements to make #85475 native AOT friendly, I encountered differing behavior WRT to a console app that is publish trimmed (TrimMode=full) vs. publish native AOT, whereby validation error messages use the property display name specified via the attribute [Display(Name = "Property display name")] or [Display(Name = "ResourceKey", ResourceType = typeof(MyResources))] when the app is published trimmed, but not when the app is published native AOT.

The trimming warning from calling new ValidationContext(instance) is being suppressed in the repro as this investigation is looking at what's possible WRT to a potential source generator implementation.

Should the behavior not be the same in this case when publishing trimmed vs. publishing native AOT?

Reproduction Steps

using System.ComponentModel.DataAnnotations;
using System.Diagnostics.CodeAnalysis;
using ValidationNativeAot;

// Title is blank
var todo = new Todo { Title = "" };
var validationContext = CreateValidationContext(todo, nameof(todo.Title));
Console.WriteLine("Empty Todo.Title:");
ValidateDirect(todo.Title, validationContext);
ValidateValidator(todo.Title, validationContext);
Console.WriteLine();

// Title is too long
Console.WriteLine("Long Todo.Title:");
todo.Title = "123456789";
validationContext = CreateValidationContext(todo, nameof(todo.Title));
ValidateDirect(todo.Title, validationContext);
ValidateValidator(todo.Title, validationContext);
Console.WriteLine();

static void ValidateDirect(object? value, ValidationContext context)
{
    var attributes = GetValidationAttributes();

    foreach (var attribute in attributes)
    {
        var directResult = attribute.GetValidationResult(value, context);
        Console.WriteLine($"{attribute.GetType().Name}.GetValidationResult: ");
        Console.WriteLine(directResult?.ErrorMessage is null ? "Valid" : directResult.ErrorMessage);
    }
}

static bool ValidateValidator(object? value, ValidationContext context)
{
    var attributes = GetValidationAttributes();
    var validationResults = new List<ValidationResult>();
    var validatorResult = Validator.TryValidateValue(value!, context, validationResults, attributes);
    Console.WriteLine("Validator.TryValidateValue: ");
    Console.WriteLine(validatorResult ? "Valid" : "Invalid");
    if (!validatorResult)
    {
        foreach (var item in validationResults)
        {
            Console.WriteLine(item.ErrorMessage);
        }
    }
    return validatorResult;
}

[UnconditionalSuppressMessage("Trimming", "IL2026:Members annotated with 'RequiresUnreferencedCodeAttribute' require dynamic access otherwise can break functionality when trimming application code", Justification = "<Pending>")]
static ValidationContext CreateValidationContext<T>(T instance, string? memberName = null, string? displayName = null)
    where T : notnull
{
    var context = new ValidationContext(instance) { MemberName = memberName };

    if (!string.IsNullOrEmpty(displayName))
    {
        context.DisplayName = displayName;
    }

    return context;
}

static ValidationAttribute[] GetValidationAttributes()
    => new ValidationAttribute[]
    {
        // This must match what is declared on the Todo.Title property below
        new RequiredAttribute(),
        new StringLengthAttribute(5) { MinimumLength = 2 }
    };

public class Todo
{
    public int Id { get; set; }

    [Required]
    [StringLength(5, MinimumLength = 2)]
    //[Display(Name = "TitleDisplayName", ResourceType = typeof(ValidationMessages))]
    [Display(Name = "Title Display Name")]
    public string? Title { get; set; }
}

Expected behavior

The behavior of Validator.TryValidateValues and ValidationAttribute.GetValidationResult WRT to getting the display name for the member indicated by ValidationContext.MemberName when that property is annotated with DisplayAttribute, is the same when publishing trimmed and publishing native AOT.

Actual behavior

After publishing trimmed, the validation messages contain the property display names specified in the DisplayAttribute instance, but when publishing native AOT, the validation messages just use the property name.

Regression?

No response

Known Workarounds

No response

Configuration

SDK: 8.0.100-preview.3.23178.7
Runtime: 8.0.0-preview.3.23174.8

Windows 11 x64

Other information

No response

Author: DamianEdwards
Assignees: -
Labels:

area-System.ComponentModel.DataAnnotations

Milestone: -

@eerhardt
Copy link
Member

eerhardt commented Apr 4, 2023

Taking a look at this, the reason the behavior happens is because the Native AOT compiler is trimming the property metadata from the Todo class. There is a warning that this could happen, which is being suppressed in the repro code. I've opened #84326 to make this warning message better so devs know why new ValidationContext(object) is unsafe.

There is a difference here between PublishTrimmed and PublishAot. When PublishTrimmed=true, the property metadata for properties that are actually referenced in code aren't trimmed today. However, PublishAot doesn't follow the same rules, and if it doesn't see someone reflecting against a Type, it will trim the property metadata.

The above app can be made to work by changing:

-static ValidationContext CreateValidationContext<T>(T instance, string? memberName = null, string? displayName = null)
+static ValidationContext CreateValidationContext<[DynamicallyAccessedMembers(DynamicallyAccessedMemberTypes.PublicProperties)] T>(T instance, string? memberName = null, string? displayName = null)

FYI - @MichalStrehovsky @vitek-karas @agocke

eerhardt added a commit that referenced this issue Apr 4, 2023
@ghost ghost added the in-pr There is an active PR which will close this issue when it is merged label Apr 4, 2023
@tarekgh tarekgh added this to the 8.0.0 milestone Apr 5, 2023
@ghost ghost removed the untriaged New issue has not been triaged by the area owner label Apr 5, 2023
@MichalStrehovsky
Copy link
Member

In general it's expected and fine to have behavioral differences when there's warnings. The warning was suppressed incorrectly, but there's nothing we can do about that (besides being very explicit with "Thou shall not suppress a warning"). ILLink could be more aggressive with stripping properties that are not targets of reflection in the future. This is a piece of relatively low hanging fruit - once ILLink does that, the behaviors will align. But it's not the goal - some of the behaviors will stay different because the optimization are different. It should only be observable if there are warnings.

We document this in https://learn.microsoft.com/en-us/dotnet/core/deploying/trimming/prepare-libraries-for-trimming#unconditionalsuppressmessage ("It is important to underline that it is only valid to suppress a warning if there are annotations or code that ensure the reflected-on members are visible targets of reflection. It is not sufficient that the member was simply a target of a call, field or property access.")

eerhardt added a commit that referenced this issue Apr 6, 2023
* Use better trimming message in ValidationContext

Add more details about why ValidationContext requires unreferenced code.

Fix #84324

* Exclude RequiresUnreferencedCodeAttribute from baseline API compat
@ghost ghost removed the in-pr There is an active PR which will close this issue when it is merged label Apr 6, 2023
@ghost ghost locked as resolved and limited conversation to collaborators May 27, 2023
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

Successfully merging a pull request may close this issue.

4 participants