fix "NoClobber" option in combination with EnvConfigurationProvider

[Philipp-Binder]

Problem Description

I'm not 100% sure about this one, because I'm not sure where and in which way NoClobber should be effective. My understading is as follows:

If we set the NoClobber option, we want the configuration to return the initial Environment-Variable if present. And if the key is duplicated within the EnvFiles, we want to have the first occurrence (this is already working).

This is all working fine if we use the option SetEnvVars and let the configuration rely on the default EnvironmentVariablesConfigurationSource only. This way we do not use the new EnvConfigurationSource at all. One really poor thing about this: when reading the EnvFile takes place after building the Configuration, the updated EnvVars will not be used, because they do not reload by default in the EnvironmentVariablesConfigurationProvider. So you are forced to read EnvFile first, or reload the ConfigurationManager manually after reading the EnvFiles/updating the EnvironmentVariables.

With the EnvConfigurationConfigurationSource in place we get rid of the reload problem, because we do not care about the EnvironmentVariablesConfigurationSource (except if someone adds the EnvConfigurationSource before the default EnvironmentVariablesConfigurationSource, which should be a rare scenario I guess). But we add the problem, that we now "hide/clobber" EnvironmentVariables while the option "NoClobber" is set.

Solution

I see three possible solutions to this problem (all 3 only for NoClobber is set of course):

1. insert EnvConfigurationSource before default EnvironmentVariablesConfigurationSource
2. remove corresponding entries from EnvConfiguration-collection
3. update values in the EnvConfiguration-collection with the actual EnvironmentVariable-Value

The current PR solves the problem with the 3rd option, which brings the most "independence" from other circumstances I think.

Additional Info about the solutions

First I solved it via option 2, which leads to some unexpected (while "correct") cases, where you miss specific values if you rely on EnvConfigurationSource only. (have a look at the first commit if interested, the problematic but correct behavior is shown in a test-case)

Solution 1 and 2 both rely directly on the order of providers to implement the "NoClobber"-option correctly. Solution 3 in contrast to this solves the NoClobber problem by "embedding" the resulting values directly in the provided values, which removes the direct dependency to the order of providers. Of course there is still a dependency to the order of providers, but that is the "default" order dependency, and by having the new provider after the default ones (the default case), we get the correct results. If someone changes this order on purpose (and it is near to impossible to change that order by fault) he is self-responsible for the correct order.

[rogusdev]

No one has complained about this functionality. So, there are 2 cases where this might be interesting:

1. It is a rare/obscure bug that no one has yet encountered, if so, please point at the test that failed with the old code, but now passes
2. It is because of recent changes made to this library, that only went live recently, so people have not yet had the chance to encounter this bug, and we need to clean up our own mess. If so, please point at a test, same as above, and also point at the specific changes (lines in a PR diff) made that caused the problem.

Thanks.

[rogusdev]

Basically at this point, I'm looking for you to explain why the existing logic does not already do what you want (I believe you that it does not, but we should be clear as to why), and then an explanation of why this logic should live here rather than there.

Re-reading your PR description sounds like it lives here because this is specific to the configuration provider for reloads, and thus this may be my own unfamiliarity with that pattern, so please explain the reload case a bit more simply, as this is not something I have ever used before. Thanks.

[Philipp-Binder]

Tried to answer some threads above, but I guess first of all we need to clarify which effect NoClobber should have. That's the point why first of all I wrote I'm not sure about this one ;) I will answer the other Threads/Topics after we get to a common understanding of NoClobber

Possible answer to this question (my thoughts, please add or comment to them)

1. it says, that DotEnvValues are ignored, if we already have an existing value (EnvVar, or previous DotEnvVar e.g. due to multiple files)
   • this is the interpretation I follow atm
   • "earlier" definitions are always preffered over "later" definitions (first EnvVars, then DotEnvVars in reading order)
   • this follows most of the current implementation imho, because we use existing EnvVars and preceding DotEnvVars with higher priority for Interpolation
   • there are some scenarios, where this pattern gets ignored (elaboration about this comes after we agree on the definition)
2. it is just about setting or not setting (Process-)EnvVars
   • DotEnvVars still "override" EnvVars, but we do not reflect that change in the (Process-)EnvVars
   • this does not match with the way we handle interpolation, because if we have "SetEnvVars==false" configured, we still use the EnvVars for interpolation, but the DotEnvVars should override the values for Interpolation too
   • this also does not match the behaviour of the current EnvConfigurationProvider: with NoClobber it takes the first value instead of the last, so it does not override
   • this is kinda senseless, because it is only about the InProcess-EnvVariables --> it does not Clobber the EnvVars on system-level anyway, stop the program and they are gone... --> if it does not "invert" the order of relevance, there is no real use-case I think (except for being as pure as possible, meaning do not change anything which does not need to be changed)

We already have that comment Since the Load method does not take care of cloberring, We have to check it here! in place, which already mentioned the problem.

In theory I would say you are right, we could solve the problem at a different level. Imho that would lead to the solution, that with NoClobber we

1. either adjust the dotEnvKvps (Env.Load()) to contain the EnvVar-Value instead of the DotEnvFile-Value (regardless of the SetEnvVar Option)
2. or adjust the dotEnvKvps (Env.Load()) to have no entry for already existing EnvVars at all --> both would be really bad and breaking changes, and contradicting to the principle, that the dotEnvKvps contains an exact list of all DotEnvFile-Entries (which is imho a really good principle and should not change).

Short summary:

• the DotEnvKvps (Env.Load()) should always contains a "full list" of all DotEnvVar-Entries
• that means, it cannot handle Clobber/NoClobber on it's own (this is a thing about "order" in the resulting kvp-list afterwards)
• actually Load() handles some part of Clobber/NoClobber, because it decides about Interpolation while Parsing
  • EnvConfigurationProvider should follow the ParserLogic for the aspects of NoClobber the Parser should not handle on it's own
  • I would recommend to handle Interpolation at a different stage at all (after paring, not while parsing), but that's a huge different topic --> should be discussed in a different place

[rogusdev]

At this point I would defer to @cdroulers who wrote this original configuration provider code, and which I have very limited understanding of. The changes look possibly fine to me, and certainly limited in scope, but I'd like to have a better sense of what did not pass in the tests before the changes, and why that was not what was expected.

[cdroulers]

@rogusdev Hi there! After re-reading my own PR and looking a bit into this PR, I think I can give a bit of insight.

I think we misunderstood the "no-clobber" option when we wrote it, assuming it only meant if multiple files are loaded with the same variable, only the first is taken into account. Reading deeper in the Env.Load method, it refers to overriding actual environment variables!

The assumed usage with the configuration provider was absolutely to never set environment variables, instead always relying on the other providers for the ConfigurationPRovider, notably AddEnvironmentVariables from Microsoft.

Using providers for configuration is usually readonly (e.g. load configuration from a list of sources, in order, usually the appsettings.json (multiple versions), then env vars, then command line args). The way we used it was always with LoadOptions.NoEnvVars() and I would probably suggest that the API for this provider just not expose these options at all to follow the same API design as the rest of the providers. The behaviour for any provider is "last value wins", and the order is user-controlled.

I don't know which use-case found the bug here! I can make a PR later next week to remove options for the configuration provider that don't make sense!

[rogusdev]

@cdroulers if you want to review the code and make more tweaks, you are welcome to it :)

That said, at this point, I don't think anything strictly needs to be done, unless you believe there are more bugs, or ways for people to use this that would introduce bugs.

[cdroulers]

No bugs that I can see, the clean up would mostly be to remove the options unless I can understand why they are used!

I think I'd also update the readme section about using the configuration provider to explain the "expected" way to use it (E.g. NoEnvVars() always). I'm on a laptop on vacation, but will check next week!

On Wed, 7 Aug 2024 at 16:27, Chris Rogus @.***> wrote:

@cdroulers https://github.com/cdroulers if you want to review the code and make more tweaks, you are welcome to it :)

That said, at this point, I don't think anything strictly needs to be done, unless you believe there are more bugs, or ways for people to use this that would introduce bugs.

— Reply to this email directly, view it on GitHub https://github.com/tonerdo/dotnet-env/pull/90#issuecomment-2274289466, or unsubscribe https://github.com/notifications/unsubscribe-auth/AAFNBPWPSC2Y23ZVYGAAFLDZQJ7JTAVCNFSM6AAAAABLWQVHBWVHI2DSMVQWIX3LMV43OSLTON2WKQ3PNVWWK3TUHMZDENZUGI4DSNBWGY . You are receiving this because you were mentioned.Message ID: @.***>