dotnet / runtime

.NET is a cross-platform runtime for cloud, mobile, desktop, and IoT apps.
https://docs.microsoft.com/dotnet/core/
MIT License
15.27k stars 4.73k forks source link

Regression introduced in the https://github.com/dotnet/runtime/pull/51772 #64505

Open VladimirKhvostov opened 2 years ago

VladimirKhvostov commented 2 years ago

Description

I noticed that regression was introduced in the https://github.com/dotnet/runtime/pull/51772

See example in a Reproduction Steps. An error is reported when this code runs on netcoreapp22, netcoreapp31, and net5.0, but no errors on net6.0.

Notice that Text property is public, but has an internal getter (same behavior when getter is private or protected)

An error is reported when this code runs on netcoreapp22, netcoreapp31, and net5.0, but no errors on net6.0. The regression is in the GetPropertyValues method

Before:

            var properties = instance.GetType().GetRuntimeProperties()
                                .Where(p => ValidationAttributeStore.IsPublic(p) && !p.GetIndexParameters().Any());
            var items = new List<KeyValuePair<ValidationContext, object?>>(properties.Count());
            foreach (var property in properties)

After:

            var properties = TypeDescriptor.GetProperties(instance);
            var items = new List<KeyValuePair<ValidationContext, object?>>(properties.Count);
            foreach (PropertyDescriptor property in properties)

TypeDescriptor.GetProperties method does not return properties which do not have public getter.

I should mention that behavior matches net4.8 behavior and scenario is kind of weird, but it is still a regression, which can cause problems for people upgrading from .NET Core 3.1 or .NET 5.0 to .NET 6.0.

I am glad we had a unit test for this scenario. It took me couple hours to figure out the root of the issue, but if we did not have that unit test, we would have a regression in production.

Reproduction Steps

using System;
using System.Collections.Generic;
using System.ComponentModel;
using System.ComponentModel.DataAnnotations;
using System.Linq;
using System.Reflection;

internal class Settings
{
    [StringLength(4, ErrorMessage = "The Text value cannot exceed 4 characters.")]
    public string Text { internal get; set; } = "Hello, World!";
}

public class Program
{
    public static void Main()
    {
        Settings settings = new Settings();

        PropertyInfo[] properties = settings.GetType().GetRuntimeProperties().ToArray();
        PropertyDescriptorCollection propertyCollection = TypeDescriptor.GetProperties(settings);

        // Console.WriteLine($"properties.Length: {properties.Length}, propertyCollection.Count: {propertyCollection.Count}");

        var validationContext = new ValidationContext(settings);
        List<ValidationResult> validationResults = new List<ValidationResult>();
        Validator.TryValidateObject(settings, validationContext, validationResults, validateAllProperties: true);

        if (validationResults.Count > 0)
        {
            Console.WriteLine(validationResults[0].ErrorMessage);
        }
        else
        {
            Console.WriteLine("No errors!");
        }
    }
}

Expected behavior

Sample program should report an error when running on .NET 6.0 (same as .NET Core 3.1 and .NET 5.0)

Actual behavior

Program does not report any errors.

Regression?

Yes, regression from netcoreapp3.1 and net5.0

Known Workarounds

N/A

Configuration

No response

Other information

No response

dotnet-issue-labeler[bot] commented 2 years ago

I couldn't figure out the best area label to add to this issue. If you have write-permissions please help me learn by adding exactly one area label.

ghost commented 2 years ago

Tagging subscribers to this area: @ajcvickers, @bricelam, @roji See info in area-owners.md if you want to be subscribed.

Issue Details
### Description I noticed that regression was introduced in the https://github.com/dotnet/runtime/pull/51772 See example in a Reproduction Steps. An error is reported when this code runs on netcoreapp22, netcoreapp31, and net5.0, but no errors on net6.0. Notice that Text property is public, but has an internal getter (same behavior when getter is private or protected) An error is reported when this code runs on netcoreapp22, netcoreapp31, and net5.0, but no errors on net6.0. The regression is in the [GetPropertyValues](https://github.com/dotnet/runtime/pull/51772/files#diff-7d9d37023d10b7d3491eacd94e0864b8f1c26e125a16ee3b5edba59131698e4bR517) method Before: ```CSharp var properties = instance.GetType().GetRuntimeProperties() .Where(p => ValidationAttributeStore.IsPublic(p) && !p.GetIndexParameters().Any()); var items = new List>(properties.Count()); foreach (var property in properties) ``` After: ``` var properties = TypeDescriptor.GetProperties(instance); var items = new List>(properties.Count); foreach (PropertyDescriptor property in properties) ``` TypeDescriptor.GetProperties method does not return properties which do not have public getter. I should mention that behavior matches net4.8 behavior and scenario is kind of weird, but it is still a regression, which can cause problems for people upgrading from .NET Core 3.1 or .NET 5.0 to .NET 6.0. I am glad we had a unit test for this scenario. It took me couple hours to figure out the root of the issue, but if we did not have that unit test, we would have a regression in production. ### Reproduction Steps ```CSharp using System; using System.Collections.Generic; using System.ComponentModel; using System.ComponentModel.DataAnnotations; using System.Linq; using System.Reflection; internal class Settings { [StringLength(4, ErrorMessage = "The Text value cannot exceed 4 characters.")] public string Text { internal get; set; } = "Hello, World!"; } public class Program { public static void Main() { Settings settings = new Settings(); PropertyInfo[] properties = settings.GetType().GetRuntimeProperties().ToArray(); PropertyDescriptorCollection propertyCollection = TypeDescriptor.GetProperties(settings); // Console.WriteLine($"properties.Length: {properties.Length}, propertyCollection.Count: {propertyCollection.Count}"); var validationContext = new ValidationContext(settings); List validationResults = new List(); Validator.TryValidateObject(settings, validationContext, validationResults, validateAllProperties: true); if (validationResults.Count > 0) { Console.WriteLine(validationResults[0].ErrorMessage); } else { Console.WriteLine("No errors!"); } } } ``` ### Expected behavior Sample program should report an error when running on .NET 6.0 (same as .NET Core 3.1 and .NET 5.0) ### Actual behavior Program does not report any errors. ### Regression? Yes, regression from netcoreapp3.1 and net5.0 ### Known Workarounds N/A ### Configuration _No response_ ### Other information _No response_
Author: VladimirKhvostov
Assignees: -
Labels: `area-System.ComponentModel.DataAnnotations`, `untriaged`
Milestone: -