Add viper.SetKeysCaseSensitive() to disable automatic key lowercasing.

[knadh]

YAML, TOML, and JSON dictate keys to be case-sensitive. Viper's default
behaviour of lowercasing the keys for key insensitivity is incompatible
with these standards and has the side effect of making it difficult for
use cases such as case sensitive API credentials in configuration.
For eg: MyApiKey=MySecret (in TOML).

See #131, #260, #293, #371, #373

This commit adds a global function viper.SetKeyCaseSensitivity() that
enables this behaviour to be turned off, after which, all keys, irrespective
of nesting, retain their cases. This respects all configuration operations
including getting, setting, and merging.

[CLAassistant]

All committers have signed the CLA.

[knadh]

@sagikazarmark The existing tests and the new tests that were added with this commit all pass locally. Travis is failing at a certain go get. Not sure what's happening there.

[sagikazarmark]

@knadh it seems to be a go modules issue. I sent a PR fixing it: #636

Once it's merged, you can just rebase your branch.

[v1k0d3n]

we really need this for our use case (exactly what you mentioned about case sensitive API creds). it would be awesome to see this merged in. 🤞

[sagikazarmark]

I had a few comments that I think would make the PR better.

My concern regarding this change is that it introduces a strange side effect: what if I set viper to case sensitive mode, add a few values and set it back to case insensitive mode? I think with the current implementation it won't recognize the added keys (I might be wrong, I quickly went through the code).

Maybe it would be better to save keys case sensitively all the time and insensitivise on demand when necessary. If this is already the case, then feel free to ignore my comment. :)

[knadh]

I had a few comments that I think would make the PR better.

My concern regarding this change is that it introduces a strange side effect: what if I set viper to case sensitive mode, add a few values and set it back to case insensitive mode? I think with the current implementation it won't recognize the added keys (I might be wrong, I quickly went through the code).

Maybe it would be better to save keys case sensitively all the time and insensitivise on demand when necessary. If this is already the case, then feel free to ignore my comment. :)

@sagikazarmark You are right. Should've documented this. Preserving key casing internally and then converting on demand leads to more complexities though. For instance:

sensitivity=on
set Abc 1
set abc 2

sensitivity=off
set aBc 3

get ABC = 1, 2, or 3?

This gets trickier with map merges. A feature like this should ideally a one-time flag set during initialisation, although that's not an option here. I think case sensitivity and insensitivity cannot coexist without conflicts. It may be better to document this behaviour explicitly and leave it to the user.

PS: Thanks for the inline comments on the PR :) Will amend.

[sagikazarmark]

@knadh the mentioned PR is merged.

Also, I found this: #592 it seems to be a duplicate, it's probably worth checking.

[v1k0d3n]

I was thinking the same when I saw this PR @sagikazarmark, but it appears that #592 just seemed to fall flat on the floor for some reason (reviews weren't addressed). Hopefully, we can see this PR through to the end.

[sagikazarmark]

Yeah, I just mentioned it because I think it still makes sense to compare the two solutions, but ultimately I think this one should be merged eventually.

[knadh]

Thanks for the merge, the tests now pass. I've incorporated the changes you posted.

#592 follows the same approach, a separate that function that handles key casing. Just that it's incomplete--no tests and no semantic changes to comments or local variables.

[v1k0d3n]

Awesome! Would love to see this get in.

[knadh]

@bep Have you had a chance to review the PR? Thanks.

[knadh]

@bep It'd be great if you could take a look at the PR. Thanks.

[v1k0d3n]

would a cute animal gif help? we could really use this PR, if there are no more hangups. pretty please?

[mr-karan]

@bep Firstly, thanks for all your work. Would be really awesome if this PR is reviewed. This feature could really help us, as well 👍

[terev]

@spf13 Can we get a review on this? This a pretty crucial option.

[bep]

I don't have time to review this at the moment. Maybe @spf13 can chime in.

[prymitive]

What is status of this project? Is viper still actively maintained? Can we assume this will get merged in the near future?

[knadh]

I've updated the PR to incorporate 9e56dac.

[v1k0d3n]

ok, how many animal pics do we need? i can do this all day long folks...

[v1k0d3n]

Ok, so we’re at a crossroads with this PR. I see this as the submitter has done everything to keep this PR fresh, several other folks have expressed interest/need for this PR, and reasons for not pulling it in seem a little trivial or can be addressed rather easily.

Based on this, we need to either find another solution (if the viper maintainers have an extreme objection to this PR), or fork (which we’d rather not do). Can the maintainers give us some feedback, please? Right now, this seems like it’s in limbo, and I’m kind of at a loss. I’ve even reached out to a few folks on gopher slack, in good faith, but I haven’t gotten any response.

Since this has been out there for a couple of months now; what are we recommending here? Anyone who’s following this, and requesting it as well, what are your thoughts or how are you thinking of addressing this? Can we discuss openly what our options are here?

Thanks for any feedback and consideration. I’m getting concerned with all of the outstanding PR’s. We can help, but not if there’s much feedback or any additional requests from the maintainers.

[lz-dc]

Any idea as to when will the fix go in?

[stephencheng]

+1 Any idea as to when will the fix go in??

[knadh]

@stephencheng I opened this PR 1 year and 1 month ago ^_^ and waited for 6 months for a merge before ditching Viper for good and writing koanf 😄. You can give it a shot. Addresses this, and several other issues.

[sagikazarmark]

Unfortunately this change affects Viper at its core and we have to make sure it's aligned with the future of Viper and does not break existing behavior. Believe me, I've given this PR a considerable amount of time to find a way to integrate it and meet the above requirements.

[prymitive]

If this PR is not going to get merged then could it be declined, so everyone gets clarity on the situation here?
It's perfectly fine if viper mainteners say they won't support case sensitive keys, especially that there are alternative solutions, it's just that both the issue and the PR being open for so long puts the whole problem in a limbo.

[sagikazarmark]

We plan to add support for case sensitivity. Whether it will go in a Viper 2 or can be integrated into Viper 1 is a question though. If you need this feature now though, Viper is probably not the best choice at the moment and probably isn't worth waiting for it. That's the best answer I have right now.

[vividvilla]

@sagikazarmark PR doesn't seems to be affecting exisiting behavior though as you mentioned above. Was there any edge case missed?

[gbunt]

fwiw i've merged the changes from @knadh into a fork here: https://github.com/gbunt/viper as i had to also pin mapstructure to v1.0.0. Any mapstructure version above that and i ran into issues with validator.v9 not handling function omitempty correctly on lists and thus, we weren't able to use validation + empty lists. Still have to debug what the issue is exactly but for now, it's working quite nicely for us. Glad to see support for case sensitivity is seriously being looked at 👍

[gauravv7]

This PR's case is very sensitive

[wfernandes]

It seems that this is the discussion thread for case sensitivity. Could we add this issue to the README?

Important: Viper configuration keys are case insensitive. There are ongoing discussions about making that optional.

[gauravv7]

Important: Viper configuration keys are case insensitive. There are ongoing discussions about making that optional.

I am trying to make a sequence engine which uses json config files, to pass query filters as user-defined configs e.g. mongo query filters. Now the keys are fieldName of my documents. Mongodb is just one supported db, there could be many likewise and other algorithmic supports.
Please see if this can count as a factor towards approving this PR? let me know, kinda will help me understand the global standards as well and thought process :)
-with a good heart

[hypergig]

If you merge this in today... I will literally drop 100 bucks in any donation / paypal / venmo account you tell me to

[gauravv7]

covid vaccine is nearly out for production use

[knadh]

If you merge this in today..

Too old and stale to merge at this point @hypergig. The maintainers have clarified this on this thread that it's not happening.

[hypergig]

If you merge this in today..

Too old and stale to merge at this point @hypergig. The maintainers have clarified this on this thread that it's not happening.

😢 🐼

Maybe we can close the PR so it doesn't continue to provide false hope?

[knadh]

Maybe we can close the PR so it doesn't continue to provide false hope?

Yeah, I'd left it open hoping it'll get merged at some point, but makes sense to close now.

[cocktail828]

oops! It's 2023 now, and the issue still not fixed...