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

Add support for clearing ConfigurationManager sources (was possible in version 5) #61675

Closed
Tracked by #64015
fredjeck opened this issue Nov 16, 2021 · 13 comments
Closed
Tracked by #64015
Labels
api-approved API was approved in API review, it can be implemented area-Extensions-Configuration feature-request help wanted [up-for-grabs] Good issue for external contributors
Milestone

Comments

@fredjeck
Copy link
Contributor

Problem description

Prior to version 6.0 with the old style application startup it was possible for an application to define how configuration file should be loaded by clearing up the configuration sources and by adding the configuration items in the desired order.
A use case is for instance a shared library with a baseline configuration which needs to be "injected" into the Configuration before the appsettings.* files. so that developers just need to implement minimum overrides in their appsettings files.

With version 6 it is now impossible to

  • Set the Configuration of a WebApplicationBuilder instance (readonly property)
  • Access the Sources of the ConfigurationManager (IConfigurationBuilder.Sources) property as it is private

Proposal

As it is always possible to add new configuration sources to the ConfigurationManager class, the cleanest approach which would not break the current encapsulation would be to add a clear() method to the ConfigurationManager class.

@davidfowl
Copy link
Member

This is the workaround:

var builder = WebApplication.CreateBuilder(args);

((IConfigurationBuilder)builder.Configuration).Sources.Clear();

Moving this to the runtime

cc @halter73

@davidfowl davidfowl reopened this Nov 16, 2021
@davidfowl davidfowl transferred this issue from dotnet/aspnetcore Nov 16, 2021
@dotnet-issue-labeler dotnet-issue-labeler bot added area-Extensions-Configuration untriaged New issue has not been triaged by the area owner labels Nov 16, 2021
@ghost
Copy link

ghost commented Nov 16, 2021

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

Issue Details

Problem description

Prior to version 6.0 with the old style application startup it was possible for an application to define how configuration file should be loaded by clearing up the configuration sources and by adding the configuration items in the desired order.
A use case is for instance a shared library with a baseline configuration which needs to be "injected" into the Configuration before the appsettings.* files. so that developers just need to implement minimum overrides in their appsettings files.

With version 6 it is now impossible to

  • Set the Configuration of a WebApplicationBuilder instance (readonly property)
  • Access the Sources of the ConfigurationManager (IConfigurationBuilder.Sources) property as it is private

Proposal

As it is always possible to add new configuration sources to the ConfigurationManager class, the cleanest approach which would not break the current encapsulation would be to add a clear() method to the ConfigurationManager class.

Author: fredjeck
Assignees: -
Labels:

untriaged, area-Extensions-Configuration

Milestone: -

@fredjeck
Copy link
Contributor Author

Thanks a lot for the workaround !

@eerhardt
Copy link
Member

eerhardt commented Dec 1, 2021

@halter73 - any thoughts on this?

@halter73
Copy link
Member

halter73 commented Dec 1, 2021

I see three options:

  1. Make ConfigurationManager.Sources public rather than an explicit interface implementation of IConfigurationBuilder.Sources.
  2. Add a new public method like ConfigurationManager.ClearSources(). This could also be an extension method on IConfigurationBuilder, but it doesn't seem that helpful for ConfigurationBuilder so we probably wouldn't do it that way.
  3. Leave things as they are and force people to call ((IConfigurationBuilder)builder.Configuration).Sources.Clear() to clear sources.

I'm leaning towards option 1. The only reason we didn't make Sources public to begin with is that ConfigurationManager also used to read config and the extra visible property could be confusing, but I'm not sure it's any more confusing than a new ConfigurationManager.ClearSources() method which would be less flexible anyway.

The argument for option 3 is that removing sources is far less common than adding them so the vast majority of interaction with the Sources property is done with extension methods like .AddJsonFile(string path). I am somewhat sympathetic to that argument, but I'm not sure I buy that the Sources property is that confusing.

@davidfowl
Copy link
Member

I like 1

@maryamariyan maryamariyan removed the untriaged New issue has not been triaged by the area owner label Dec 2, 2021
@maryamariyan maryamariyan added this to the Future milestone Dec 2, 2021
@maryamariyan maryamariyan added api-suggestion Early API idea and discussion, it is NOT ready for implementation feature-request and removed api-suggestion Early API idea and discussion, it is NOT ready for implementation labels Dec 2, 2021
@halter73 halter73 added the api-suggestion Early API idea and discussion, it is NOT ready for implementation label Dec 3, 2021
@halter73
Copy link
Member

halter73 commented Dec 3, 2021

namespace Microsoft.Extensions.Configuration
{
    public sealed class ConfigurationManager : IConfigurationBuilder, IConfigurationRoot, IDisposable
    {
-         IList<IConfigurationSource> IConfigurationBuilder.Sources { get; }
+         IList<IConfigurationSource> Sources { get; }

@halter73 halter73 modified the milestones: Future, 7.0.0 Dec 3, 2021
@halter73 halter73 added api-ready-for-review API is ready for review, it is NOT ready for implementation and removed api-suggestion Early API idea and discussion, it is NOT ready for implementation labels Dec 3, 2021
@halter73
Copy link
Member

halter73 commented Dec 3, 2021

@maryamariyan I moved this to the 7.0.0 milestone and marked it ready for review.

@terrajobst
Copy link
Member

terrajobst commented Dec 14, 2021

Video

  • Looks good as proposed
    • Should we also expose IConfigurationBuilder.Properties? What happens if Properties aren't cleared? Would there a benefit to add a Clear method that clears both?
namespace Microsoft.Extensions.Configuration
{
    public partial sealed class ConfigurationManager
    {
        // Currently explicit:
        // IList<IConfigurationSource> IConfigurationBuilder.Sources { get; }

        // Now implicit:
        public IList<IConfigurationSource> Sources { get; }
    }
}

@terrajobst terrajobst added api-approved API was approved in API review, it can be implemented and removed api-ready-for-review API is ready for review, it is NOT ready for implementation labels Dec 14, 2021
@halter73
Copy link
Member

Should we also expose IConfigurationBuilder.Properties? What happens if Properties aren't cleared? Would there a benefit to add a Clear method that clears both?

I don't think clearing both is helpful. IConfigurationBuilder.Properties is primarily used by extension methods that need to load config from files with relative paths. If the IConfiguartionBuilder comes from the host, it will usually call SetBasePath for you which sets a property. If you want to use a different base path for sources, you can call SetBasePath yourself to overwrite the property.

It could be used by third-party extension methods to track other state, but I think most people who clear Sources would not want Properties to be cleared because it would make it harder to load new sources from files with relative paths.

@danmoseley
Copy link
Member

@maryamariyan should this be marked up for grabs in case @fredjeck or someone else is interested in doing it before we get to it?

@maryamariyan
Copy link
Member

marked up for grabs

@maryamariyan maryamariyan added the help wanted [up-for-grabs] Good issue for external contributors label Mar 10, 2022
fredjeck added a commit to fredjeck/runtime that referenced this issue Mar 10, 2022
ConfigurationManager.Sources is now a public property rather than
an explicit interface implementation of  ConfigurationBuilder.Sources.

dotnet#61675
fredjeck added a commit to fredjeck/runtime that referenced this issue Mar 13, 2022
eerhardt pushed a commit that referenced this issue Mar 14, 2022
* Make ConfigurationManager.Sources public

ConfigurationManager.Sources is now a public property rather than
an explicit interface implementation of  ConfigurationBuilder.Sources.

#61675
@eerhardt
Copy link
Member

Resolved by #66485

radekdoulik pushed a commit to radekdoulik/runtime that referenced this issue Mar 30, 2022
* Make ConfigurationManager.Sources public

ConfigurationManager.Sources is now a public property rather than
an explicit interface implementation of  ConfigurationBuilder.Sources.

dotnet#61675
@ghost ghost locked as resolved and limited conversation to collaborators Apr 13, 2022
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
api-approved API was approved in API review, it can be implemented area-Extensions-Configuration feature-request help wanted [up-for-grabs] Good issue for external contributors
Projects
None yet
Development

No branches or pull requests

7 participants