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

ConfigurationBinder Source Generator doesn't call setters in same way reflection based ConfigurationBinder does #96873

Closed
eerhardt opened this issue Jan 11, 2024 · 2 comments · Fixed by #107057
Assignees
Labels
area-Extensions-Configuration in-pr There is an active PR which will close this issue when it is merged Priority:1 Work that is critical for the release, but we could probably ship without source-generator Indicates an issue with a source generator feature
Milestone

Comments

@eerhardt
Copy link
Member

eerhardt commented Jan 11, 2024

Description

When a configuration doesn't contain anything for a collection property (in the below case the ResponseHeadersDataClasses collection), the ConfigurationBinder reflection based implementation will still call the setter for the collection. But switching to the ConfigurationBinder source generator causes the setter to no longer be invoked.

Reproduction Steps

csproj:

<Project Sdk="Microsoft.NET.Sdk.Worker">
  <PropertyGroup>
    <TargetFramework>net8.0</TargetFramework>
    <Nullable>enable</Nullable>
    <ImplicitUsings>enable</ImplicitUsings>
    <EnableConfigurationBindingGenerator>true</EnableConfigurationBindingGenerator>
  </PropertyGroup>

  <ItemGroup>
    <PackageReference Include="Microsoft.Extensions.Hosting" Version="8.0.0" />
    <PackageReference Include="Microsoft.Extensions.Configuration.Binder" Version="8.0.1" />
  </ItemGroup>
</Project>

appsettings.json:

{
  "HttpLogging": {
    "ExcludePathStartsWith": [
      "path"
      ]
  }
}

Program.cs:

var builder = Host.CreateApplicationBuilder(args);
builder.Configuration.GetSection("HttpLogging").Get<LoggingRedactionOptions>();
Console.WriteLine("Done");

public class LoggingRedactionOptions
{
    private IDictionary<string, DataClassification> _responseHeadersDataClasses = new Dictionary<string, DataClassification>(StringComparer.OrdinalIgnoreCase);

    public IDictionary<string, DataClassification> ResponseHeadersDataClasses
    {
        get { return _responseHeadersDataClasses; }
        set
        {
            Console.WriteLine("ResponseHeadersDataClasses setter called!");
            _responseHeadersDataClasses = value;
        }
    }

    public ISet<string> ExcludePathStartsWith { get; set; } = new HashSet<string>(StringComparer.OrdinalIgnoreCase);
}

public readonly struct DataClassification : IEquatable<DataClassification>
{
    public string TaxonomyName { get; }
    public string Value { get; }

    public DataClassification(string taxonomyName, string value)
    {
        TaxonomyName = taxonomyName;
        Value = value;
    }

    public override bool Equals(object? obj) => (obj is DataClassification dc) && Equals(dc);
    public bool Equals(DataClassification other) => other.TaxonomyName == TaxonomyName && other.Value == Value;
    public override int GetHashCode() => HashCode.Combine(TaxonomyName, Value);

    public static bool operator ==(DataClassification left, DataClassification right)
    {
        return left.Equals(right);
    }
    public static bool operator !=(DataClassification left, DataClassification right)
    {
        return !left.Equals(right);
    }
    public override string ToString() => string.IsNullOrWhiteSpace(TaxonomyName) ? Value : $"{TaxonomyName}:{Value}";
}

Toggle the EnableConfigurationBindingGenerator setting between true and false.

Expected behavior

The behavior of the app should be the same whether you are setting EnableConfigurationBindingGenerator to false or true. It should print ResponseHeadersDataClasses setter called! in both cases.

Actual behavior

When the Source Generator is not used:

ResponseHeadersDataClasses setter called!
Done

When the Source Generator is used:

Done

Other information

See this comment (and #80438) why the setter is being called in the non-SG case.

// For property binding, there are some cases when HasNewValue is not set in BindingPoint while a non-null Value inside that object can be retrieved from the property getter.
// As example, when binding a property which not having a configuration entry matching this property and the getter can initialize the Value.
// It is important to call the property setter as the setters can have a logic adjusting the Value.
if (!propertyBindingPoint.IsReadOnly && propertyBindingPoint.Value is not null)
{
property.SetValue(instance, propertyBindingPoint.Value);

In the source generator case, the generated code looks like:

            if (AsConfigWithChildren(configuration.GetSection("ResponseHeadersDataClasses")) is IConfigurationSection section2)
            {
                global::System.Collections.Generic.IDictionary<string, global::DataClassification>? temp4 = instance.ResponseHeadersDataClasses;
                temp4 ??= (global::System.Collections.Generic.IDictionary<string, global::DataClassification>)new Dictionary<string, global::DataClassification>();
                BindCore(section2, ref temp4, defaultValueIfNotFound: false, binderOptions);
                instance.ResponseHeadersDataClasses = temp4;
            }

So the setter is only ever called when a ResponseHeadersDataClasses section with children is present in the configuration.

@ghost ghost added the untriaged New issue has not been triaged by the area owner label Jan 11, 2024
@ghost
Copy link

ghost commented Jan 11, 2024

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

Issue Details

Description

Reproduction Steps

csproj:

<Project Sdk="Microsoft.NET.Sdk.Worker">
  <PropertyGroup>
    <TargetFramework>net8.0</TargetFramework>
    <Nullable>enable</Nullable>
    <ImplicitUsings>enable</ImplicitUsings>
    <EnableConfigurationBindingGenerator>true</EnableConfigurationBindingGenerator>
  </PropertyGroup>

  <ItemGroup>
    <PackageReference Include="Microsoft.Extensions.Hosting" Version="8.0.0" />
    <PackageReference Include="Microsoft.Extensions.Configuration.Binder" Version="8.0.1" />
  </ItemGroup>
</Project>
Author: eerhardt
Assignees: -
Labels:

area-Extensions-Configuration

Milestone: -

@tarekgh tarekgh added this to the 9.0.0 milestone Jan 11, 2024
@ghost ghost removed the untriaged New issue has not been triaged by the area owner label Jan 11, 2024
eerhardt added a commit to eerhardt/extensions that referenced this issue Jan 11, 2024
- Change ArgumentNullException to normal ArgumentException
- Add tests to LoggingRedactionOptions collection setters to keep 100% code coverage. dotnet/runtime#96873 causes these setters to no longer be called in the existing tests.
eerhardt added a commit to dotnet/extensions that referenced this issue Jan 12, 2024
* Enable AOT compatibility for all libraries (#4625)

* Enable AOT compatibility for all libraries. Fix warnings.

- Enable configuration binder source generator
- The only library that can't use the ConfigBinder SG is the HeaderParsing library.
  - Blocked by dotnet/runtime#94547

* Fix Compliance Redaction.

* Explicitly reference Microsoft.Extensions.Configuration.Binder

For all libraries that use the Configuration.Binder source generator, explicitly reference the NuGet package. This ensures we get the latest version (8.0.1), which has all the source generator fixes.

* Respond to PR feedback.

- Change ArgumentNullException to normal ArgumentException
- Add tests to LoggingRedactionOptions collection setters to keep 100% code coverage. dotnet/runtime#96873 causes these setters to no longer be called in the existing tests.
@eerhardt eerhardt added the source-generator Indicates an issue with a source generator feature label Jan 12, 2024
@ArXen42
Copy link

ArXen42 commented May 24, 2024

Somewhat related difference between source generated implementation and reflection: source generator will silently skip generating binding code for init only properties, while reflection based binder will set them just fine.

Since this can lead to difficult to detect bugs when enabling trimming for application or otherwise migrating to source generator (especially if the property does not have [Required] validation attribute), perhaps a warning for such case would be a good solution?

@ericstj ericstj added the Priority:1 Work that is critical for the release, but we could probably ship without label Aug 1, 2024
@tarekgh tarekgh self-assigned this Aug 16, 2024
@dotnet-policy-service dotnet-policy-service bot added the in-pr There is an active PR which will close this issue when it is merged label Aug 27, 2024
@github-actions github-actions bot locked and limited conversation to collaborators Oct 4, 2024
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
area-Extensions-Configuration in-pr There is an active PR which will close this issue when it is merged Priority:1 Work that is critical for the release, but we could probably ship without source-generator Indicates an issue with a source generator feature
Projects
None yet
Development

Successfully merging a pull request may close this issue.

4 participants