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 viper.SetKeysCaseSensitive() to disable automatic key lowercasing. #635

Closed
wants to merge 3 commits into from

Conversation

knadh
Copy link

@knadh knadh commented Jan 25, 2019

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
Copy link

CLAassistant commented Jan 25, 2019

CLA assistant check
All committers have signed the CLA.

@knadh
Copy link
Author

knadh commented Jan 25, 2019

@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
Copy link
Collaborator

@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
Copy link

v1k0d3n commented Jan 26, 2019

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. 🤞

Copy link
Collaborator

@sagikazarmark sagikazarmark left a comment

Choose a reason for hiding this comment

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

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. :)

viper.go Show resolved Hide resolved
viper.go Show resolved Hide resolved
viper.go Outdated Show resolved Hide resolved
viper.go Outdated Show resolved Hide resolved
viper.go Outdated Show resolved Hide resolved
viper.go Outdated Show resolved Hide resolved
viper.go Outdated Show resolved Hide resolved
viper.go Outdated Show resolved Hide resolved
viper.go Outdated Show resolved Hide resolved
@knadh
Copy link
Author

knadh commented Jan 27, 2019

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
Copy link
Collaborator

@knadh the mentioned PR is merged.

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

@v1k0d3n
Copy link

v1k0d3n commented Jan 27, 2019

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
Copy link
Collaborator

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.

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 spf13#131, spf13#260, spf13#293, spf13#371, spf13#373

This commit adds a global function `viper.SetKeysCaseSensitive()` 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.
@knadh
Copy link
Author

knadh commented Jan 29, 2019

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.

@knadh knadh changed the title Add viper.SetKeyCaseSensitivity() to disable automatic key lowercasing. Add viper.SetKeysCaseSensitive() to disable automatic key lowercasing. Jan 29, 2019
@v1k0d3n
Copy link

v1k0d3n commented Jan 30, 2019

Awesome! Would love to see this get in.

@knadh
Copy link
Author

knadh commented Feb 1, 2019

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

@knadh
Copy link
Author

knadh commented Feb 9, 2019

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

@v1k0d3n
Copy link

v1k0d3n commented Feb 12, 2019

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

donkey

@mr-karan
Copy link

mr-karan commented Mar 5, 2019

@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
Copy link
Contributor

terev commented Mar 10, 2019

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

@bep
Copy link
Collaborator

bep commented Mar 10, 2019

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

@prymitive
Copy link

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

@knadh
Copy link
Author

knadh commented Mar 15, 2019

I've updated the PR to incorporate 9e56dac.

@v1k0d3n
Copy link

v1k0d3n commented Mar 21, 2019

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

babygoats

demond2 pushed a commit to demond2/yamlpack that referenced this pull request Mar 22, 2019
spf13/viper#635

YAML keys are case sensitive according to the specification, Viper has historically forced lowercase to make lookups more predictable. This becomes a problem when the YAML is then exported again and the consumer is case sensitive.
The Viper PR to which this sets the module to seems to resolve this.
@v1k0d3n
Copy link

v1k0d3n commented Apr 5, 2019

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
Copy link

lz-dc commented Jan 1, 2020

Any idea as to when will the fix go in?

@stephencheng
Copy link

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

@knadh
Copy link
Author

knadh commented Jan 31, 2020

@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.

image

@sagikazarmark
Copy link
Collaborator

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
Copy link

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
Copy link
Collaborator

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
Copy link

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

@gbunt
Copy link

gbunt commented Jan 31, 2020

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
Copy link

This PR's case is very sensitive

@wfernandes
Copy link

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
Copy link

gauravv7 commented May 20, 2020

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

synfinatic added a commit to synfinatic/vpnexiter that referenced this pull request Jun 19, 2020
- Viper apparently has no interest in merging:
    spf13/viper#635
    and I need that so locations in the Select Exit menu are capitalized
    correct.
- Switched to https://github.com/knadh/koanf
- Config parsing now done in vpnexiter/config.go rather than in main
- Basic code test, seems to work
@hypergig
Copy link

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

@gauravv7
Copy link

covid vaccine is nearly out for production use

@knadh
Copy link
Author

knadh commented Aug 27, 2020

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
Copy link

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
Copy link
Author

knadh commented Aug 27, 2020

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
Copy link

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

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.