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.

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

Jump to bottom

Respect AppContext.SetData with APP_CONFIG_FILE key #56748

Merged
merged 6 commits into from Aug 5, 2021
Merged

Respect AppContext.SetData with APP_CONFIG_FILE key #56748

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
6 commits
Select commit Hold shift + click to select a range
2d3d474
Respect AppContext.SetData with APP_CONFIG_FILE key
krwq Aug 2, 2021
0a02303
apply feedback (use BaseDirectory and FileCleanupTestBase)
krwq Aug 3, 2021
cdbceb1
avoid using GetFullPath with 2 arguments as it's not available everyw…
krwq Aug 3, 2021
afcf7a5
trim all directory separators rather than single
krwq Aug 5, 2021
79fe28e
cleanupRootDirectory -> tempDirectory
krwq Aug 5, 2021
eec3437
fix absolute paths on linux
krwq Aug 5, 2021
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
18 changes: 17 additions & 1 deletion ...s/System.Configuration.ConfigurationManager/src/System/Configuration/ClientConfigPaths.cs
View file
Open in desktop
Expand Up @@ -92,7 +92,23 @@ private ClientConfigPaths(string exePath, bool includeUserConfig)
}
}

if (!string.IsNullOrEmpty(ApplicationUri))
string externalConfigPath = AppDomain.CurrentDomain.GetData("APP_CONFIG_FILE") as string;
if (!string.IsNullOrEmpty(externalConfigPath))
{
if (Uri.IsWellFormedUriString(externalConfigPath, UriKind.Absolute))
{
Uri externalConfigUri = new Uri(externalConfigPath);
krwq marked this conversation as resolved.
Show resolved Hide resolved
if (externalConfigUri.IsFile)
{
ApplicationConfigUri = externalConfigUri.LocalPath;
}
}
else
{
ApplicationConfigUri = Path.GetFullPath(externalConfigPath);
Copy link
Member

Choose a reason for hiding this comment

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

This is implicitly prefixing with whatever the current directory is at the time the config system is initialized, I guess?

In the desktop code it is using the appbase to prefix (naming.cpp line 3241 .. assuming I'm looking in the right place)

I think in .NET Core that is AppDomain.BaseDirectory, so should this be Path.Combine(AppDomain.CurrentDomain.BaseDirectory, externalConfigPath) ? There is something similar higher up.

Copy link
Member

Choose a reason for hiding this comment

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

You can change the tests to set Environment.CurrentDirectory to something random before reading the value (I'm guessing the test will fail right now)

Copy link
Member

Choose a reason for hiding this comment

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

When you use Path.Combine you need to be careful with paths that begin with \, as you'll get the second path against the root of the current directory. Trim the start of the externalConfigPath for safety.

Copy link
Member Author

Choose a reason for hiding this comment

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

I have used Path.GetFullPath(externalConfigPath, AppDomain.CurrentDomain.BaseDirectory) which seems to be working fine. The tests are now changing CurrentDirectory to validate we are relative to the BaseDirectory and not CurrentDirectory

Copy link
Member Author

Choose a reason for hiding this comment

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

hmm seems that overload is not available everywhere...

}
}
else if (!string.IsNullOrEmpty(ApplicationUri))
{
string applicationPath = ApplicationUri;
if (isSingleFile)
Expand Down
1 change: 1 addition & 0 deletions ...uration.ConfigurationManager/tests/System.Configuration.ConfigurationManager.Tests.csproj
View file
Open in desktop
Expand Up @@ -65,6 +65,7 @@
<Compile Include="System\Configuration\ConfigurationElementCollectionTests.cs" />
<Compile Include="System\Configuration\ConfigurationElementTests.cs" />
<Compile Include="System\Configuration\ConfigurationPropertyAttributeTests.cs" />
<Compile Include="System\Configuration\ConfigurationPathTests.cs" />
<Compile Include="System\Configuration\CustomHostTests.cs" />
<Compile Include="System\Configuration\ConfigurationPropertyTests.cs" />
<Compile Include="System\Configuration\ConfigurationTests.cs" />
Expand Down
92 changes: 92 additions & 0 deletions ...m.Configuration.ConfigurationManager/tests/System/Configuration/ConfigurationPathTests.cs
View file
Open in desktop
@@ -0,0 +1,92 @@
// Licensed to the .NET Foundation under one or more agreements.
// The .NET Foundation licenses this file to you under the MIT license.

using System.Configuration;
using System.IO;
using Microsoft.DotNet.RemoteExecutor;
using Xunit;

namespace System.ConfigurationTests
{
public class ConfigurationPathTests
{
private const string ConfigName = "APP_CONFIG_FILE";
krwq marked this conversation as resolved.
Show resolved Hide resolved

[ConditionalFact(typeof(RemoteExecutor), nameof(RemoteExecutor.IsSupported))]
krwq marked this conversation as resolved.
Show resolved Hide resolved
public void CustomAppConfigIsUsedWhenSpecifiedAsRelativePath()
{
const string SettingName = "test_CustomAppConfigIsUsedWhenSpecified";
string expectedSettingValue = Guid.NewGuid().ToString();
string configFilePath = CreateAppConfigFileWithSetting(SettingName, expectedSettingValue);

RemoteExecutor.Invoke((string configFilePath, string expectedSettingValue) => {
AppDomain.CurrentDomain.SetData(ConfigName, configFilePath);
Assert.Equal(expectedSettingValue, ConfigurationManager.AppSettings[SettingName]);
}, configFilePath, expectedSettingValue).Dispose();
}

[ConditionalFact(typeof(RemoteExecutor), nameof(RemoteExecutor.IsSupported))]
public void CustomAppConfigIsUsedWhenSpecifiedAsAbsolutePath()
{
const string SettingName = "test_CustomAppConfigIsUsedWhenSpecified";
string expectedSettingValue = Guid.NewGuid().ToString();
string configFilePath = Path.GetFullPath(CreateAppConfigFileWithSetting(SettingName, expectedSettingValue));

RemoteExecutor.Invoke((string configFilePath, string expectedSettingValue) => {
AppDomain.CurrentDomain.SetData(ConfigName, configFilePath);
Assert.Equal(expectedSettingValue, ConfigurationManager.AppSettings[SettingName]);
}, configFilePath, expectedSettingValue).Dispose();
}

[ConditionalFact(typeof(RemoteExecutor), nameof(RemoteExecutor.IsSupported))]
public void CustomAppConfigIsUsedWhenSpecifiedAsAbsoluteUri()
{
const string SettingName = "test_CustomAppConfigIsUsedWhenSpecified";
string expectedSettingValue = Guid.NewGuid().ToString();
string configFilePath = new Uri(Path.GetFullPath(CreateAppConfigFileWithSetting(SettingName, expectedSettingValue))).ToString();

RemoteExecutor.Invoke((string configFilePath, string expectedSettingValue) => {
AppDomain.CurrentDomain.SetData(ConfigName, configFilePath);
Assert.Equal(expectedSettingValue, ConfigurationManager.AppSettings[SettingName]);
}, configFilePath, expectedSettingValue).Dispose();
}

[ConditionalFact(typeof(RemoteExecutor), nameof(RemoteExecutor.IsSupported))]
public void NoErrorWhenCustomAppConfigIsSpecifiedAndItDoesNotExist()
{
RemoteExecutor.Invoke(() =>
{
AppDomain.CurrentDomain.SetData(ConfigName, "non-existing-file.config");
Assert.Null(ConfigurationManager.AppSettings["AnySetting"]);
}).Dispose();
}

[ConditionalFact(typeof(RemoteExecutor), nameof(RemoteExecutor.IsSupported))]
public void MalformedAppConfigCausesException()
{
const string SettingName = "AnySetting";

// Following will cause malformed config file
string configFilePath = CreateAppConfigFileWithSetting(SettingName, "\"");

RemoteExecutor.Invoke((string configFilePath) => {
AppDomain.CurrentDomain.SetData(ConfigName, configFilePath);
Assert.Throws<ConfigurationErrorsException>(() => ConfigurationManager.AppSettings[SettingName]);
}, configFilePath).Dispose();
}

private static string CreateAppConfigFileWithSetting(string key, string rawUnquotedValue)
{
string fileName = Path.GetRandomFileName() + ".config";
Copy link
Member

Choose a reason for hiding this comment

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

These files will leak. it might be easiest to derive from FileCleanupTestBase and call GetTestFileName().

Copy link
Member Author

Choose a reason for hiding this comment

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

this gets a bit tricky when I want to test @JeremyKuhne's scenario #56748 (comment)

I'll use that for everything but related test cases

Copy link
Member Author

@krwq krwq Aug 3, 2021 •
edited

Choose a reason for hiding this comment

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

I have ended up modifying constructor of FileCleanupTestBase so that it allows to pickup something else than TempDirectory which in here I chose to be AppDomain.BaseDirectory - TempDirectory doesn't allow me to test relative paths correctly

File.WriteAllText(fileName,
@$"<?xml version=""1.0"" encoding=""utf-8"" ?>
<configuration>
<appSettings>
<add key=""{key}"" value=""{rawUnquotedValue}""/>
</appSettings>
</configuration>");

return fileName;
}
}
}