Respect AppContext.SetData with APP_CONFIG_FILE key #56748

krwq · 2021-08-02T19:30:33Z

Fixes: #931

Full framework works with following path types:

relative path, i.e.: myconfig.config
absolute path
file URI, i.e.: file:///c:/myconfig.config

additionally:

when file doesn't exists there is no error and all settings read as null
when file is malformed you get an error

...ries/System.Configuration.ConfigurationManager/src/System/Configuration/ClientConfigPaths.cs

...stem.Configuration.ConfigurationManager/tests/System/Configuration/ConfigurationPathTests.cs

danmoseley · 2021-08-03T01:17:21Z

...ries/System.Configuration.ConfigurationManager/src/System/Configuration/ClientConfigPaths.cs

+                }
+                else
+                {
+                    ApplicationConfigUri = Path.GetFullPath(externalConfigPath);


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.

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)

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.

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

hmm seems that overload is not available everywhere...

danmoseley · 2021-08-03T01:18:04Z

@JeremyKuhne we don't have much familiarity with the config system -- does this seem reasonable based on your recollection of things?

danmoseley · 2021-08-03T01:21:41Z

...stem.Configuration.ConfigurationManager/tests/System/Configuration/ConfigurationPathTests.cs

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


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

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

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

...stem.Configuration.ConfigurationManager/tests/System/Configuration/ConfigurationPathTests.cs

JeremyKuhne · 2021-08-03T02:00:22Z

@JeremyKuhne we don't have much familiarity with the config system -- does this seem reasonable based on your recollection of things?

@danmoseley Yes, this seems right.

krwq · 2021-08-03T11:59:30Z

Please re-review since a bit changed here

…here

danmoseley · 2021-08-03T14:25:15Z

...ries/System.Configuration.ConfigurationManager/src/System/Configuration/ClientConfigPaths.cs

+                {
+                    // Uri constructor would throw for relative paths but full framework doesn't.
+                    // We will mimic full framework behavior here.
+                    if (RuntimeInformation.IsOSPlatform(OSPlatform.Windows) && externalConfigPath.Length >= 2 && externalConfigPath.StartsWith('\\') && externalConfigPath[1] != '\\')


Is there some way to avoid the OS check here? We don't normally do those. What if on Unix it started with '/' ... should it trim whatever the Path.DirectorySeparatorChar is?

Just always trim any amount of leading Path.DirectorySeparatorChar and you should be good.

I think for Unix we will never go into this code anyway since that would mean absolute path which should be covered by the first check. I'll go ahead and trim all separators on the left - that I think this should cover 99% of the cases

src/libraries/Common/tests/TestUtilities/System/IO/FileCleanupTestBase.cs

danmoseley

thanks for adding this

danmoseley · 2021-08-03T19:27:48Z

(signed off assuming you can fix the OS check as above)

jkotas · 2021-08-04T04:20:38Z

...ries/System.Configuration.ConfigurationManager/src/System/Configuration/ClientConfigPaths.cs

+                    if (RuntimeInformation.IsOSPlatform(OSPlatform.Windows) && externalConfigPath.Length >= 2 && externalConfigPath.StartsWith('\\') && externalConfigPath[1] != '\\')
+                    {
+                        // if path starts with a single backslash Path.Combine would combine with a root of BaseDirectory so we trim that character to avoid that
+                        externalConfigPath = externalConfigPath.Substring(0, externalConfigPath.Length - 1);


Just wondering - does this match the .NET Framework behavior for paths starting with a single slash?

using System; using System.Reflection; class Program { static void Main(string[] args) { AppDomain.CurrentDomain.SetData("APP_CONFIG_FILE", @"\abc"); Console.WriteLine(AppDomain.CurrentDomain.SetupInformation.ConfigurationFile); } }

It prints c:\abc, without taking BaseDirectory into account.

To keep things simple, I would make this just:

if (!string.IsNullOrEmpty(externalConfigPath)) { if (!Path.IsPathRooted(externalConfigPath)) { path = Path.Combine(AppDomain.CurrentDomain.BaseDirectory, externalConfigPath); } path = Path.GetFullPath(path); } else if (!string.IsNullOrEmpty(ApplicationUri))

I'll go ahead with solution suggested by @JeremyKuhne (#56748 (comment)), it's probably not covering all corner cases but it should be close enough. For the backslash starting directories I do not have any strong feeling we should support that as it's hard to describe the behavior in any docs especially since now we also work on Unix

I'll go ahead with solution suggested by @JeremyKuhne (#56748 (comment))

Is there any other existing place in dotnet/runtime that is normalizing non-fully qualified paths using this algorithm? I think it does not have very intuitive behavior, e.g. it does not match what Path.GetFullPath(string path, string basePath)
does.

it should be close enough

I would pick the simplest solution, among the "close enough" ones. It was the gist of my suggestion.

Sadly Path.IsPathRooted will get you "relative" paths. Path.IsPathFullyQualified (new) is the one that gives you accurate indication of whether or not the current working directory will impact the path.

It is unlikely that customers would actually desire paths that start with \ to be rooted to the current working directory drive. I think that is enough to be helpful here, other weird scenarios like C:foo\ don't need addressed I think.

jkotas · 2021-08-04T04:52:30Z

Full framework works with following path types:

I do not think it is necessary to match all .NET Framework behaviors to keep things simple. It is unlikely that you would be able to match the behavior for all corner cases anyway.

The only case that is required to make the customers successful is the absolute path. The rest is optional and can be omitted.

jkotas · 2021-08-05T12:43:05Z

...ries/System.Configuration.ConfigurationManager/src/System/Configuration/ClientConfigPaths.cs

+                    // We will mimic full framework behavior here.
+
+                    // If path starts with directory separator Path.Combine would combine with a root of BaseDirectory so we trim that character to avoid that
+                    // For absolute path we should already be covered by the other code path


I do not think that this is a correct statement for Linux.

Uri.IsWellFormedUriString("/home/jkotas/myconfig.xml", UriKind.Absolute) returns false on Linux.

you're right, I've done some combination of your solution but which supports URIs as well

krwq requested review from jkotas and JeremyKuhne August 2, 2021 19:30

dotnet-issue-labeler bot added the area-System.Configuration label Aug 2, 2021

krwq commented Aug 2, 2021

View reviewed changes

...ries/System.Configuration.ConfigurationManager/src/System/Configuration/ClientConfigPaths.cs Outdated Show resolved Hide resolved

Respect AppContext.SetData with APP_CONFIG_FILE key

2d3d474

krwq force-pushed the app-config-file branch from bcdcee1 to 2d3d474 Compare August 2, 2021 19:36

dotnet deleted a comment from azure-pipelines bot Aug 2, 2021

jeffhandley approved these changes Aug 2, 2021

View reviewed changes

...ries/System.Configuration.ConfigurationManager/src/System/Configuration/ClientConfigPaths.cs Outdated Show resolved Hide resolved

danmoseley reviewed Aug 2, 2021

View reviewed changes

...stem.Configuration.ConfigurationManager/tests/System/Configuration/ConfigurationPathTests.cs Show resolved Hide resolved

danmoseley reviewed Aug 3, 2021

View reviewed changes

...stem.Configuration.ConfigurationManager/tests/System/Configuration/ConfigurationPathTests.cs Outdated Show resolved Hide resolved

krwq closed this Aug 3, 2021

krwq reopened this Aug 3, 2021

apply feedback (use BaseDirectory and FileCleanupTestBase)

0a02303

avoid using GetFullPath with 2 arguments as it's not available everyw…

cdbceb1

…here

danmoseley reviewed Aug 3, 2021

View reviewed changes

src/libraries/Common/tests/TestUtilities/System/IO/FileCleanupTestBase.cs Show resolved Hide resolved

danmoseley approved these changes Aug 3, 2021

View reviewed changes

jkotas reviewed Aug 4, 2021

View reviewed changes

jeffhandley added this to the 6.0.0 milestone Aug 4, 2021

krwq added 2 commits August 5, 2021 14:29

trim all directory separators rather than single

afcf7a5

cleanupRootDirectory -> tempDirectory

79fe28e

jkotas requested changes Aug 5, 2021

View reviewed changes

fix absolute paths on linux

eec3437

jkotas approved these changes Aug 5, 2021

View reviewed changes

krwq merged commit d329350 into dotnet:main Aug 5, 2021

ghost locked as resolved and limited conversation to collaborators Sep 7, 2021

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Respect AppContext.SetData with APP_CONFIG_FILE key #56748

Respect AppContext.SetData with APP_CONFIG_FILE key #56748

krwq commented Aug 2, 2021 •

edited

Loading

danmoseley Aug 3, 2021

danmoseley Aug 3, 2021

JeremyKuhne Aug 3, 2021

krwq Aug 3, 2021

krwq Aug 3, 2021

danmoseley commented Aug 3, 2021

danmoseley Aug 3, 2021

krwq Aug 3, 2021

krwq Aug 3, 2021 •

edited

Loading

JeremyKuhne commented Aug 3, 2021

krwq commented Aug 3, 2021

danmoseley Aug 3, 2021

JeremyKuhne Aug 3, 2021

krwq Aug 5, 2021

danmoseley left a comment

danmoseley commented Aug 3, 2021

jkotas Aug 4, 2021

jkotas Aug 4, 2021

jkotas Aug 4, 2021

krwq Aug 5, 2021

jkotas Aug 5, 2021

JeremyKuhne Aug 5, 2021

jkotas commented Aug 4, 2021

jkotas Aug 5, 2021

krwq Aug 5, 2021

Respect AppContext.SetData with APP_CONFIG_FILE key #56748

Respect AppContext.SetData with APP_CONFIG_FILE key #56748

Conversation

krwq commented Aug 2, 2021 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

danmoseley commented Aug 3, 2021

Choose a reason for hiding this comment

Choose a reason for hiding this comment

krwq Aug 3, 2021 • edited Loading

Choose a reason for hiding this comment

JeremyKuhne commented Aug 3, 2021

krwq commented Aug 3, 2021

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

danmoseley left a comment

Choose a reason for hiding this comment

danmoseley commented Aug 3, 2021

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jkotas commented Aug 4, 2021

Choose a reason for hiding this comment

Choose a reason for hiding this comment

krwq commented Aug 2, 2021 •

edited

Loading

krwq Aug 3, 2021 •

edited

Loading