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

Parameter-matching is case-sensitive (sink arguments, etc) #111

Closed
josephgardner opened this issue May 9, 2018 · 13 comments
Closed

Parameter-matching is case-sensitive (sink arguments, etc) #111

josephgardner opened this issue May 9, 2018 · 13 comments

Comments

@josephgardner
Copy link

josephgardner commented May 9, 2018

I'm trying to set the formatter using Environment Variables, and nothing I've tried is working.

I was able to set it using an in-memory configuration, so I'm pretty sure the syntax is correct:

var defaultConfig = new Dictionary<string, string>
{
    { "Serilog:WriteTo:0:Name", "Trace" },
    { "Serilog:WriteTo:0:Args:formatter", "Serilog.Formatting.Json.JsonFormatter, Serilog" }
}

var builder = new ConfigurationBuilder()
    .AddInMemoryCollection(defaultConfig)
    .AddEnvironmentVariables("IDP_");

All of my other environment variables are loaded by configuration correctly, just not Serilog.

Any suggestions?

IDP_SERILOG__WRITETO__0__NAME='Trace' 
IDP_SERILOG__WRITETO__0__ARGS__FORMATTER='Serilog.Formatting.Json.JsonFormatter, Serilog' 
IDP_Serilog:WriteTo:0:Name='Trace'
IDP_Serilog:WriteTo:0:Args:formatter='Serilog.Formatting.Json.JsonFormatter, Serilog'

Adding a breakpoint, I'm able to see the values are loaded in the config object correctly
image

@josephgardner
Copy link
Author

josephgardner commented May 9, 2018

I was able to get this working. The environment variable names are case sensitive. I believe environment variable names should be case insensitive?

    environment:
      - IDP_Serilog__MinimumLevel=Debug
      - IDP_Serilog__WriteTo__0__Name=Trace
      - IDP_Serilog__WriteTo__0__Args__formatter=Serilog.Formatting.Json.JsonFormatter, Serilog

@josephgardner josephgardner changed the title Environment Variables can't set formatter Environment Variable names are case sensitive May 9, 2018
@MV10
Copy link
Contributor

MV10 commented May 9, 2018

I noticed a case-sensitivity issue while working on a related project. Combined with the fact that most configuration is moving towards JSON and it has different default casing conventions than this package uses (JSON is normally camel-case, C# and this repo tends towards Pascal-case), this should probably be regarded as a bug.

And I'll probably fix it because it bothers me.

@merbla
Copy link
Contributor

merbla commented May 9, 2018

@josephgardner @MV10 there maybe implications changing case sensitivity on other platforms (*nix, OSX).

What platforms are you targeting?

@MV10
Copy link
Contributor

MV10 commented May 9, 2018

@merbla Microsoft.Extensions.Configuration is case-sensitive irrespective of platform ... but it's my personal opinion that this is a Very Bad Idea. Not because I just don't like it (which is true), but because different configuration sources could have different casing conventions or rules, which directly conflicts with the concept of overlaying settings from different sources.

I did some searching and found a couple tangentially-related MS github discussions about config key case-sensitivity but nothing looked definitive -- no statement akin to, "We decided the rule is XYZ and that's what everyone should expect."

I'm tempted to open a question on the subject in an MS repo.

@josephgardner
Copy link
Author

josephgardner commented May 9, 2018

According to Configuration in ASP.NET Core,

Configuration keys are not case-sensitive.

@MV10
Copy link
Contributor

MV10 commented May 9, 2018

@josephgardner I just came back to post the same thing, but there are some weird decisions within the .NET Core repos: they refer things like Configuration to the ASP.NET Core team, even though their config package isn't specific to ASP.NET Core. (This is true of many things that came from the ASP.NET world but are now stand-alone in Core ... dependency injection is another major example. There is a serious case of web-app-blinders over there right now.)

Regardless, I agree with you, it should be case-insensitive. We know the config system itself is case-sensitive (or we wouldn't be having this conversation) so I'm guessing that's something the ASP.NET Core team does differently (and so far, MS has chosen to ignore documenting config for any other use-case). Hopefully there is an easily-applied fix of some kind. (It would be great if this turns out to be an option you can set in the config-builder stage.)

@merbla
Copy link
Contributor

merbla commented May 9, 2018

Thanks for the notes @josephgardner @MV10, I was referring specifically to the environment variables.

See

Note: On Windows and macOS, environment variables and values are not case sensitive. Linux environment variables and values are case sensitive by default.

@MV10
Copy link
Contributor

MV10 commented May 10, 2018

The ASP.NET Core issue 2012 is that the filename is case-sensitive (I saw that one while searching, too) and the linked doc relates to constructing the filename based on environment variables. Not related to the contents of any configuration vars or files.

Looking at the source, the environment builder's Load method declares a case-insensitive dictionary:

internal void Load(IDictionary envVariables)
{
    Data = new Dictionary<string, string>(StringComparer.OrdinalIgnoreCase);

I guess the real test (and I don't have Linux available) is whether key A from the environment will overwrite key a loaded from JSON after Build runs. Once configuration is built, there is no way to determine the origin of a given key.

Edit: The observation above about the source code also makes it weird that I ran into case-sensitivity issues with one of my other projects (adding MS Config v2 support to the Serilog SQL sink). I definitely want to dig into this one a bit more!

@MV10
Copy link
Contributor

MV10 commented Aug 28, 2018

I finally had time to set up a Linux VM to see how Configuration deals with environment variables, case sensitivity, and overlaying settings (if applicable on an OS with case-sensitive environment variables).

For Windows, I created one environment variable, casesensitive = lowercase and I also created a JSON config with CASESENSITIVE : UPPERCASE. For Linux, I use the same JSON file but I edited my user .profile to export CASEsensitive = upperCASE and casesensitive = lowercase.

The test app builds config twice, querying for all three settings each time -- once with environment variables before JSON, then again with JSON before the environment variables.

On both platforms, the last value wins, regardless of case.

Windows:
image

Linux:
image

It's also worth noting that on Linux if you have vars that differ only by case, it's probably effectively random as to which value wins.

I didn't bother testing on OSX because it's a pain to start up that slug and copy stuff across the network, but I assume it'll be the same. And unfortunately I can't remember what the case-sensitivity issue was that I ran into a few months ago, nor can I explain the OP's issue.

The test:

static void Main(string[] args)
{
    var appConfig = new ConfigurationBuilder()
        .AddEnvironmentVariables()
        .SetBasePath(Directory.GetCurrentDirectory())
        .AddJsonFile(path: "appsettings.json", optional: false, reloadOnChange: true)
        .Build();

    Console.WriteLine($"env first casesensitive {appConfig["casesensitive"]}");
    Console.WriteLine($"env first CASEsensitive {appConfig["CASEsensitive"]}");
    Console.WriteLine($"env first CASESENSITIVE {appConfig["CASESENSITIVE"]}");

    appConfig = new ConfigurationBuilder()
        .SetBasePath(Directory.GetCurrentDirectory())
        .AddJsonFile(path: "appsettings.json", optional: false, reloadOnChange: true)
        .AddEnvironmentVariables()
        .Build();

    Console.WriteLine($"json first casesensitive {appConfig["casesensitive"]}");
    Console.WriteLine($"json first CASEsensitive {appConfig["CASEsensitive"]}");
    Console.WriteLine($"json first CASESENSITIVE {appConfig["CASESENSITIVE"]}");

    Console.WriteLine("Any key...");
    Console.ReadKey();
}

@josephgardner
Copy link
Author

Did you forget to fix the casing in each appConfig["casesensitive"] to match the message?

Console.WriteLine($"json first CASESENSITIVE {appConfig["casesensitive"]}");

Should be

Console.WriteLine($"json first CASESENSITIVE {appConfig["CASESENSITIVE"]}");

?

@MV10
Copy link
Contributor

MV10 commented Aug 28, 2018

@josephgardner Thanks ... but the screenshots were taken after I fixed that. I use one of those multi-page clipboard utils and pasted the wrong version. But code in the post is now updated. No change in conclusion.

@MV10 MV10 changed the title Environment Variable names are case sensitive Parameter-matching is case-sensitive (sink arguments, etc) Aug 29, 2018
@MV10
Copy link
Contributor

MV10 commented Aug 29, 2018

@josephgardner After playing around with some variations, now I see what you're talking about -- it looks like parameter-matching is case-sensitive (the "args" entries, specifically). Going back to your earlier posts, it looks like you realized this but it wasn't clear to me from the description (no big deal, we got there in the end). I took the liberty of editing the issue title. This potentially affects all configuration sources. Now I'm reminded that's also what I ran into working on the SQL sink. (I sometimes wonder how many millions of man-hours have been wasted on case-sensitivity...)

It turns out (fortunately for me) that this can be tested and reproduced in Windows. Even though the name of an environment variable is case-insensitive in the sense that A and a can't co-exist and either will match in a lookup, case is nonethless preserved, so the following works for theme but not Theme or THEME:

image

Of course, namespaces and assembly names (like Serilog.Sinks.Console above) are also case-sensitive. My concern is we'll find both are unavoidable given that c# and the rest of the .NET stack is unrepentantly case-sensitive... but we'll see.

I'll take a look at the project shortly. Technically I'm supposed to be packing for a vacation, but maybe it's an easy fix. If not, I'll work on it next week.

@MV10
Copy link
Contributor

MV10 commented Aug 29, 2018

For argument matching, the culprits are the suppliedArgumentValues checks here. This test is only used to locate a matching method. The parameters themselves are passed to Invoke positionally.

internal static MethodInfo SelectConfigurationMethod(IEnumerable<MethodInfo> candidateMethods, string name, Dictionary<string, IConfigurationArgumentValue> suppliedArgumentValues)
{
    return candidateMethods
        .Where(m => m.Name == name &&
                    m.GetParameters().Skip(1).All(p => p.HasDefaultValue || suppliedArgumentValues.Any(s => s.Key == p.Name)))
        .OrderByDescending(m => m.GetParameters().Count(p => suppliedArgumentValues.Any(s => s.Key == p.Name)))
        .FirstOrDefault();
}

In my earlier reply I questioned whether this is a good idea given everything in .NET is case-sensitive, but after thinking about it more, case-insensitivity is safe here. The following code is not valid in .NET (or at least in c#, though I assume this limitation extends to the CLR), the IDE flags the second one with "Type 'Program' already defines a member called 'Foo' with the same parameter types."

string Foo(string value) => "lowercase arg";
string Foo(string VALUE) => "uppercase arg";

If the CLR can't differentiate and MES doesn't differentiate, that's good enough for me!

@MV10 MV10 closed this as completed in 5cb8e92 Sep 16, 2018
MV10 added a commit that referenced this issue Sep 16, 2018
fixes #111 - case-insensitive method argument-matching
@merbla merbla mentioned this issue Oct 7, 2018
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

3 participants