-
Notifications
You must be signed in to change notification settings - Fork 843
Add ReloadOnChange to KeyPerFile configuration provider #2808
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
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -10,6 +10,16 @@ namespace Microsoft.Extensions.Configuration | |
| /// </summary> | ||
| public static class KeyPerFileConfigurationBuilderExtensions | ||
| { | ||
| /// <summary> | ||
| /// Adds configuration using files from a directory. File names are used as the key, | ||
| /// file contents are used as the value. | ||
| /// </summary> | ||
| /// <param name="builder">The <see cref="IConfigurationBuilder"/> to add to.</param> | ||
| /// <param name="directoryPath">The path to the directory.</param> | ||
| /// <returns>The <see cref="IConfigurationBuilder"/>.</returns> | ||
| public static IConfigurationBuilder AddKeyPerFile(this IConfigurationBuilder builder, string directoryPath) | ||
| => builder.AddKeyPerFile(directoryPath, optional: false, reloadOnChange: false); | ||
|
|
||
| /// <summary> | ||
| /// Adds configuration using files from a directory. File names are used as the key, | ||
| /// file contents are used as the value. | ||
|
|
@@ -19,6 +29,18 @@ public static class KeyPerFileConfigurationBuilderExtensions | |
| /// <param name="optional">Whether the directory is optional.</param> | ||
| /// <returns>The <see cref="IConfigurationBuilder"/>.</returns> | ||
| public static IConfigurationBuilder AddKeyPerFile(this IConfigurationBuilder builder, string directoryPath, bool optional) | ||
Kahbazi marked this conversation as resolved.
Show resolved
Hide resolved
|
||
| => builder.AddKeyPerFile(directoryPath, optional, reloadOnChange: false); | ||
|
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. question - do our other config providers do reloading by default? Looks like JSON does not: https://github.com/aspnet/Extensions/blob/master/src/Configuration/Config.Json/src/JsonConfigurationExtensions.cs#L38
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. We should be consistent so it should be off by default.
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Yeah this is expensive and has some unexpected side effects so generally reload should be off by default unless people explicitly make it enabled |
||
|
|
||
| /// <summary> | ||
| /// Adds configuration using files from a directory. File names are used as the key, | ||
| /// file contents are used as the value. | ||
| /// </summary> | ||
| /// <param name="builder">The <see cref="IConfigurationBuilder"/> to add to.</param> | ||
| /// <param name="directoryPath">The path to the directory.</param> | ||
| /// <param name="optional">Whether the directory is optional.</param> | ||
| /// <param name="reloadOnChange">Whether the configuration should be reloaded if the files are changed, added or removed.</param> | ||
| /// <returns>The <see cref="IConfigurationBuilder"/>.</returns> | ||
| public static IConfigurationBuilder AddKeyPerFile(this IConfigurationBuilder builder, string directoryPath, bool optional, bool reloadOnChange) | ||
| => builder.AddKeyPerFile(source => | ||
| { | ||
| // Only try to set the file provider if its not optional or the directory exists | ||
|
|
@@ -27,6 +49,7 @@ public static IConfigurationBuilder AddKeyPerFile(this IConfigurationBuilder bui | |
| source.FileProvider = new PhysicalFileProvider(directoryPath); | ||
| } | ||
| source.Optional = optional; | ||
| source.ReloadOnChange = reloadOnChange; | ||
| }); | ||
|
|
||
| /// <summary> | ||
|
|
||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -217,6 +217,79 @@ void ReloadLoop() | |
| Assert.Equal("Foo", options.Text); | ||
| } | ||
|
|
||
| [Fact] | ||
|
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Can you add a test where nothing exists to start, and the directory/files get created later so its the nothing found to something case as opposed to just modification scenarios?
Member
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. @HaoK I added the test. |
||
| public void ReloadConfigWhenReloadOnChangeIsTrue() | ||
| { | ||
| var testFileProvider = new TestFileProvider( | ||
| new TestFile("Secret1", "SecretValue1"), | ||
| new TestFile("Secret2", "SecretValue2")); | ||
|
|
||
| var config = new ConfigurationBuilder() | ||
| .AddKeyPerFile(o => | ||
| { | ||
| o.FileProvider = testFileProvider; | ||
| o.ReloadOnChange = true; | ||
| }).Build(); | ||
|
|
||
| Assert.Equal("SecretValue1", config["Secret1"]); | ||
| Assert.Equal("SecretValue2", config["Secret2"]); | ||
|
|
||
| testFileProvider.ChangeFiles( | ||
| new TestFile("Secret1", "NewSecretValue1"), | ||
| new TestFile("Secret3", "NewSecretValue3")); | ||
|
|
||
| Assert.Equal("NewSecretValue1", config["Secret1"]); | ||
| Assert.Null(config["NewSecret2"]); | ||
| Assert.Equal("NewSecretValue3", config["Secret3"]); | ||
| } | ||
|
|
||
| [Fact] | ||
| public void SameConfigWhenReloadOnChangeIsFalse() | ||
| { | ||
| var testFileProvider = new TestFileProvider( | ||
| new TestFile("Secret1", "SecretValue1"), | ||
| new TestFile("Secret2", "SecretValue2")); | ||
|
|
||
| var config = new ConfigurationBuilder() | ||
| .AddKeyPerFile(o => | ||
| { | ||
| o.FileProvider = testFileProvider; | ||
| o.ReloadOnChange = false; | ||
| }).Build(); | ||
|
|
||
| Assert.Equal("SecretValue1", config["Secret1"]); | ||
| Assert.Equal("SecretValue2", config["Secret2"]); | ||
|
|
||
| testFileProvider.ChangeFiles( | ||
| new TestFile("Secret1", "NewSecretValue1"), | ||
| new TestFile("Secret3", "NewSecretValue3")); | ||
|
|
||
| Assert.Equal("SecretValue1", config["Secret1"]); | ||
| Assert.Equal("SecretValue2", config["Secret2"]); | ||
| } | ||
|
|
||
| [Fact] | ||
| public void NoFilesReloadWhenAddedFiles() | ||
| { | ||
| var testFileProvider = new TestFileProvider(); | ||
|
|
||
| var config = new ConfigurationBuilder() | ||
| .AddKeyPerFile(o => | ||
| { | ||
| o.FileProvider = testFileProvider; | ||
| o.ReloadOnChange = true; | ||
| }).Build(); | ||
|
|
||
| Assert.Empty(config.AsEnumerable()); | ||
|
|
||
| testFileProvider.ChangeFiles( | ||
| new TestFile("Secret1", "SecretValue1"), | ||
| new TestFile("Secret2", "SecretValue2")); | ||
|
|
||
| Assert.Equal("SecretValue1", config["Secret1"]); | ||
| Assert.Equal("SecretValue2", config["Secret2"]); | ||
| } | ||
|
|
||
| private sealed class MyOptions | ||
| { | ||
| public int Number { get; set; } | ||
|
|
@@ -227,17 +300,56 @@ private sealed class MyOptions | |
| class TestFileProvider : IFileProvider | ||
| { | ||
| IDirectoryContents _contents; | ||
|
|
||
| MockChangeToken _changeToken; | ||
|
|
||
| public TestFileProvider(params IFileInfo[] files) | ||
| { | ||
| _contents = new TestDirectoryContents(files); | ||
| _changeToken = new MockChangeToken(); | ||
| } | ||
|
|
||
| public IDirectoryContents GetDirectoryContents(string subpath) => _contents; | ||
|
|
||
| public IFileInfo GetFileInfo(string subpath) => new TestFile("TestDirectory"); | ||
|
|
||
| public IChangeToken Watch(string filter) => throw new NotImplementedException(); | ||
| public IChangeToken Watch(string filter) => _changeToken; | ||
|
|
||
| internal void ChangeFiles(params IFileInfo[] files) | ||
| { | ||
| _contents = new TestDirectoryContents(files); | ||
| _changeToken.RaiseCallback(); | ||
| } | ||
| } | ||
|
|
||
| class MockChangeToken : IChangeToken | ||
| { | ||
| private Action _callback; | ||
|
|
||
| public bool ActiveChangeCallbacks => true; | ||
|
|
||
| public bool HasChanged => true; | ||
|
|
||
| public IDisposable RegisterChangeCallback(Action<object> callback, object state) | ||
| { | ||
| var disposable = new MockDisposable(); | ||
| _callback = () => callback(state); | ||
| return disposable; | ||
| } | ||
|
|
||
| internal void RaiseCallback() | ||
| { | ||
| _callback?.Invoke(); | ||
| } | ||
| } | ||
|
|
||
| class MockDisposable : IDisposable | ||
| { | ||
| public bool Disposed { get; set; } | ||
|
|
||
| public void Dispose() | ||
| { | ||
| Disposed = true; | ||
| } | ||
| } | ||
|
|
||
| class TestDirectoryContents : IDirectoryContents | ||
|
|
@@ -291,7 +403,7 @@ public TestFile(string name, string contents) | |
|
|
||
| public Stream CreateReadStream() | ||
| { | ||
| if(IsDirectory) | ||
| if (IsDirectory) | ||
| { | ||
| throw new InvalidOperationException("Cannot create stream from directory"); | ||
| } | ||
|
|
||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Will need API review (even if its the same as what other providers do).