Skip to content

Shared physical file provider with base directory as root #63538

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.

Sign up for GitHub

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

Closed
mapogolions wants to merge 2 commits into from
Closed

Shared physical file provider with base directory as root #63538

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
1c0cc62
Shared physical file provider is used by configuration sources when i…
mapogolions Jan 8, 2022
81386de
test shared physical file provider
mapogolions Jan 8, 2022
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
5 changes: 3 additions & 2 deletions ...ries/Microsoft.Extensions.Configuration.FileExtensions/src/FileConfigurationExtensions.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -47,8 +47,9 @@ public static IFileProvider GetFileProvider(this IConfigurationBuilder builder)
{
return provider as IFileProvider;
}

return new PhysicalFileProvider(AppContext.BaseDirectory ?? string.Empty);
var fileProvider = new PhysicalFileProvider(AppContext.BaseDirectory ?? string.Empty);
builder.Properties[FileProviderKey] = fileProvider;
Copy link
Member

@eerhardt eerhardt Jan 10, 2022

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm not certain this is a valid change. Thinking about #63540, the PhysicalFileProvider is IDisposable. Sticking it in the builder.Properties means people may be able to get at it after it has been disposed.

Can you add a reasoning/scenario (maybe in an issue?) for why this change is necessary?

Copy link
Contributor Author

@mapogolions mapogolions Jan 10, 2022 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

As I understand from code, the Properties field of the IConfigurationBuilder instance is used like a temporary bag to store some shared data that can be used during construction the IConfiguration . Sticking the PhysicalFileProvider in the Properties of the ConfigurationBuilder also happens in case when we use another extension method the SetBasePath. Could you please explain to me what the difference between these cases? I just thought it would be great to create and use only one PhysicalFileProvider
At the moment, each configuration source calls EnsureDefaults and create own file provider with base directory as root. If we create shared physical file provider then lines of code below will have the same semantic

var builder = new ConfigurationBuilder().AddJson().AddIni().AddAnotherFile()...Build()
var builder = new ConfigurationBuilder().SetBasePath(AppContext.BaseDirectory).AddJson().AddInit().AddAnotherFIle()...Build()

Copy link
Member

@eerhardt eerhardt Jan 14, 2022

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Could you please explain to me what the difference between these cases?

The difference is that calling SetBasePath is an explicit action to stick the file provider into the Properties collection. You can use a single IConfigurationBuilder to build multiple configurations.

If someone disposed the PhysicalFileProvider while it was being held on to the ICollectionBuilder, you can get an ObjectDisposedException if you tried using the ICollectionBuilder again. It should be an explicit action to cache the file provider into the ICollectionBuilder.

return fileProvider;
}

/// <summary>
Expand Down
21 changes: 21 additions & 0 deletions ...t.Extensions.Configuration.FileExtensions/tests/FileConfigurationBuilderExtensionsTest.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -50,6 +50,27 @@ public void GetFileProvider_ReturnPhysicalProviderWithBaseDirectoryIfNotSet()
Assert.Equal(EnsureTrailingSlash(expectedPath), physicalProvider.Root);
}

[Fact]
public void GetFileProvider_ReturnTheSamePhysicalFileProviderIfNotSet()
{
var configurationBuilder = new ConfigurationBuilder();
Assert.Same(configurationBuilder.GetFileProvider(), configurationBuilder.GetFileProvider());
}

[Fact]
public void EnsureDefault_CreateSharedPhysicalFileProviderWithBaseDirectoryIfNotSet()
{
var configurationBuilder = new ConfigurationBuilder();
var source1 = new FileConfigurationSourceImpl();
var source2 = new FileConfigurationSourceImpl();

source1.EnsureDefaults(configurationBuilder);
source2.EnsureDefaults(configurationBuilder);

Assert.Same(configurationBuilder.Properties["FileProvider"], source1.FileProvider);
Assert.Same(configurationBuilder.Properties["FileProvider"], source2.FileProvider);
}

private static string EnsureTrailingSlash(string path)
{
if (!string.IsNullOrEmpty(path) &&
Expand Down
70 changes: 35 additions & 35 deletions .../Microsoft.Extensions.Configuration.FileExtensions/tests/FileConfigurationProviderTest.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -111,47 +111,47 @@ public void ProviderThrowsDirectoryNotFoundExceptionWhenNotFound(string physical
var exception = Assert.Throws<DirectoryNotFoundException>(() => provider.Load());
Assert.Contains(physicalPath, exception.Message);
}
}

public class FileInfoImpl : IFileInfo
{
public FileInfoImpl(string physicalPath, bool exists = true) =>
(PhysicalPath, Exists) = (physicalPath, exists);

public Stream CreateReadStream() => new MemoryStream();
public bool Exists { get; set; }
public bool IsDirectory => false;
public DateTimeOffset LastModified => default;
public long Length => default;
public string Name => default;
public string PhysicalPath { get; }
}

public class FileInfoImpl : IFileInfo
{
public FileInfoImpl(string physicalPath, bool exists = true) =>
(PhysicalPath, Exists) = (physicalPath, exists);

public Stream CreateReadStream() => new MemoryStream();
public bool Exists { get; set; }
public bool IsDirectory => false;
public DateTimeOffset LastModified => default;
public long Length => default;
public string Name => default;
public string PhysicalPath { get; }
}

public class FileConfigurationProviderImpl : FileConfigurationProvider
{
public FileConfigurationProviderImpl(FileConfigurationSource source)
: base(source)
{ }
public class FileConfigurationProviderImpl : FileConfigurationProvider
{
public FileConfigurationProviderImpl(FileConfigurationSource source)
: base(source)
{ }

public override void Load(Stream stream)
{ }
}
public override void Load(Stream stream)
{ }
}

public class ThrowOnLoadFileConfigurationProviderImpl : FileConfigurationProvider
{
public ThrowOnLoadFileConfigurationProviderImpl(FileConfigurationSource source)
: base(source)
{ }
public class ThrowOnLoadFileConfigurationProviderImpl : FileConfigurationProvider
{
public ThrowOnLoadFileConfigurationProviderImpl(FileConfigurationSource source)
: base(source)
{ }

public override void Load(Stream stream) => throw new Exception("This is a test exception.");
}
public override void Load(Stream stream) => throw new Exception("This is a test exception.");
}

public class FileConfigurationSourceImpl : FileConfigurationSource
public class FileConfigurationSourceImpl : FileConfigurationSource
{
public override IConfigurationProvider Build(IConfigurationBuilder builder)
{
public override IConfigurationProvider Build(IConfigurationBuilder builder)
{
EnsureDefaults(builder);
return new FileConfigurationProviderImpl(this);
}
EnsureDefaults(builder);
return new FileConfigurationProviderImpl(this);
}
}
}