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

Add option to disable Interpolation #93

Conversation

Philipp-Binder
Copy link
Contributor

I've added an option to disable the Interpolation.
Main reasons:

  • the current Interpolation-Implementation stops working if you do not set EnvironmentVariables (see Interpolation stops working with SetEnvVars == false #92) ==> it is good to have the option to disable it completely
  • Interpolation adds complexity which is not needed in every setup ==> good to have the option to reduce complexity

It was way harder to achieve than expected, because the Interpolation is so deep inside the static Parsers.
Changing Assignment to be a method instead of a readonly field seems to be the least intrusive way.
There is also this test in EnvTests-->ParseInterpolationDisabledTest line 224. I think this is not the really desired result, but I do not see a way how to solve that.

@rogusdev
Copy link
Collaborator

rogusdev commented Mar 2, 2024

Please explain why you think interpolation should be disabled. You are afraid that you might use interpolation in your .env when you don't mean to?

SetEnvVar = false does not provide interpolated values that come from the .env file but it should continue to work with env vars that were set in the env itself.

But this further points at: if you don't want interpolation, don't use it. You can escape $ in unquoted or double quoted values, and will not get interpolation in single quoted values ever.

Please elaborate on the use case.

@Philipp-Binder
Copy link
Contributor Author

Yes, the use case is that you just want to have plain env-file to be able to configure a service for local usage and don't bother with any side-effects.

But this further points at: if you don't want interpolation, don't use it. You can escape $ in unquoted or double quoted values, and will not get interpolation in single quoted values ever.

Sure, it is possible to escape $ or use singlequoted.
But then you have to have it in mind and can't just paste your values exactly as they are meant to be.

The main use cases I see are "complicated" values like passwords, where you do not want to check or change anything to enter the password in the env-file.
Passwords may contain ' and $, so depending on the form of the password you have the following options:

  • contains ' --> unquoted (escaping singeQuotes in singleQuoted values is not possible)
  • contains $ --> use singleQuotes or use unquoted and escape $
  • contains both --> use unquoted and escape $

Escaping values in a complicated value is quite a pain, because you create differences between the written and the resulting value. And imho it introduces hard to detect bugs.

==> the idea is to have the possibility to use env-Files "as plain and stupid as possible"
From my point of view that's the thing I want to achieve. All additional features are cool if you really need them, but in general I don't want to bother with additional complexity for a smart and easy local configuration only.

And "as plain and stupid as possible" includes (my opinion):

  • unquoted values
  • quoted values (single/double) to support multiline values
  • comments
  • simple dictionary-output
    And that's it. Interpolation, updating (Process-)EnvironmentVariables etc. are nice to have, but not really necessary. I would not remove them from the solution of course, but the usage should be optional.

@rogusdev
Copy link
Collaborator

rogusdev commented Mar 2, 2024

Anything that should not have interpolation should be single quoted. You haven't said (yet) that you want to disable utf char sequences as well \u1234 etc, or escaped chars \n etc. Why stop with just disabling $s? You should disable all special processing -- oh wait, you can, that's single quoted values. ;)

That said, your PR does seem to me to make the change you desire, with the only minor consideration what I mentioned about a possible RawValue to avoid creating both interpolated and raw versions every time, and instead only the one that is known to be desired.

src/DotNetEnv/IValue.cs Outdated Show resolved Hide resolved
src/DotNetEnv/Parsers.cs Outdated Show resolved Hide resolved
src/DotNetEnv/Parsers.cs Outdated Show resolved Hide resolved
src/DotNetEnv/Parsers.cs Outdated Show resolved Hide resolved
src/DotNetEnv/IValue.cs Outdated Show resolved Hide resolved
) {
return Assignment.Select(tranform).Or(Empty).AtLeastOnce().End()
return Assignment(enableInterpolation).Select(transform).Or(Empty).AtLeastOnce().End()
Copy link
Collaborator

@rogusdev rogusdev Mar 2, 2024

Choose a reason for hiding this comment

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

Thank you for correcting the typo.

I think your approach is possibly fine, if this change is indeed desirable, but I would consider passing the object to use for values. Currently it is Value, but you could have an alternative called RawValue, so that you did not need the logic inside of Value, and most notably the copy of both interpolated and raw values, but just the raw value every time.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Quite some time ago since I made these changes ;)

I'm not really sure what you exactly mean. Somehow I need to "inject" whether we need to Interpolate or not.

What I understand is, that the Parser (somewhere down the InterpolatedParser) should return a "RawValue" (which would be the same as Value I guess) instead of an "InterpolatedValue".
The problem I see (without looking into the code right now) is, that we have to propagate the Parameter enableInterpolation all the way down to the InterpolatedParser.

Not sure if I understand it correctly, maybe you can give me additional advice.
I will have another look as soon as I find some more time for it.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Checked it again now.
Maybe your intention was to do something like that?

from value in interpolationEnabled ? Value : RawValue

I guess that would be possible, but it would introduce a whole new Parsing-Tree, which should behave exactly like Value, except for the Interpolation part of course.
Not sure how to achieve that without making this whole thing really complicated. I guess it would end in passing parameters too, to be able to use the same Parsers. And that would mean to pass the interpolationEnabled Parameter down into the InterpolatedValue-Parsers and return a not Interpolated value from there.
And that's more or less the same than just passing the interpolationEnabled down the whole road without a differently named Parser.

I'm not really sure if that is something we should aim for, sounds harder to understand for me.
Is that the direction you wanted to point me or did I misunderstood something?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

One more word about it:
Long-time goal imho would be to not interpolate "inside" the Parser at all. Or at least pushing the Interpolation completely after the Parsing-Operation.

That would give the possibility to have Parser-Results, which are as near to the original EnvFile as possible. And further interpretation (like Interpolation, clobbering, setEnvVars etc.) could be done completely independent, which would allow more independant tests for this additional functionality, which is not really dependant to the Parsing and vice versa. In my opinion these are different aspects, which should be handled independently too.

@rogusdev
Copy link
Collaborator

rogusdev commented Mar 2, 2024

You also have a failing test, per the CI

@Philipp-Binder
Copy link
Contributor Author

Philipp-Binder commented Mar 2, 2024

That Test is a quite ugly thing... and it is related to the implementation of EnvVar-Usage.

If you execute the Test locally (single test) it works as expected. But if you concurrently execute other tests it will break.
But if you execute them all at once, it fails locally too.

It seems like a "race-condition" with EnvConfigurationTests.Dispose(), which is quite reliable triggered with my additional Test-method if you execute all tests at once.

This has to be fixed of course.
I think I have to find a way how to separate the executions properly, or just duplicate the .env_embedded file and change some variable-names to remove concurrent keys.

edit:
it it not related to EnvConfigurationTests.Dispose() I guess after having a second look.
The Env-Values set by my additional TestMethod are being used within the previously existing method.
It's all about the direct dependencies to EnvironmentVariables. It would be a great thing to be able to mock that for testing purposes, but I do not see a way to achieve that with the current solution.

--> I will search for a way to make the tests work concurrently soon, but that should not affect the other aspects of this MR

@Philipp-Binder Philipp-Binder force-pushed the feature/disableInterpolation branch from 0a161c0 to 96bc616 Compare March 17, 2024 13:09
@Philipp-Binder Philipp-Binder force-pushed the feature/disableInterpolation branch from 96bc616 to 7d9d64a Compare March 17, 2024 13:16
@Philipp-Binder
Copy link
Contributor Author

You also have a failing test, per the CI

Solved now.

@rogusdev
Copy link
Collaborator

Having thought about this more: The way to disable interpolation is, as it is in normal shell scripting and so on, with single quotes. That is the path to use, and adding new options to do the same thing does not have a compelling argument here for me, not to justify this increase in complexity.

If you can find other .env libs in other langs that have such an option, I would consider it for ease of moving between languages for users. For now, no.

@rogusdev rogusdev closed this Jul 30, 2024
@rogusdev
Copy link
Collaborator

rogusdev commented Jul 30, 2024

Also, if this was desirable, the solution would be to use the already purpose built ValueActual rather than adding _raw to ValueInterpolated. This would require having a swap for the Value Parser used

internal static readonly Parser<ValueCalculator> Value =
such that there is the current Value still and then NonInterpolatedValue -- which would in turn go down to the lower parsers and remove the interpolation option from the UnquotedValueContents and DoubleQuotedValueContents -- and in the ParseDotenvFile func, you would replace Assignment with NonInterpolatedAssignment that would use the new form of Value. But then you'd have to ask hard questions about the other interpreted char sequences in those values as well: special chars / escaped utf codepoints, newlines, tabs, etc, etc. And then you've lost the point of double and unquoted values.

The way unquoted and double quoted work at present is how they work in shells, which is what people are accustomed to in other .env libs as well, to my knowledge (I've only used such libs in 2 or 3 other languages, so idk for sure, obv). Making these kinds of changes would be unnecessarily complicating things, to my perspective.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants