Add generic "Overrider"

[everactivetim]

Preflight Checklist

• I have searched the issue tracker for an issue that matches the one I want to file, without success.

Problem Description

We have a use case where we would like to override values found in config files or environment variables with another source. The source cannot be queried for all of the values ahead of time (like the remote providers) and instead needs to make a request at time of lookup (like env).

I

Proposed Solution

would like to propose (and I have a proof-of-concept) that we add a generic Overrider interface. Right now that interface just has a Get method. But if this is an idea that we like I would refactor the automatic env behavior to be an Overrider and extend the interface to support that case, which would benefit all overriders.

Then, in a similar fashion to some of the behavior of remote providers, it would be last Overrider added gets first chance at providing the value and then last-1, etc until all overriders are checked and then it defaults to regular lookup if no value is found.

Alternatives Considered

No response

Additional Information

No response

[github-actions]

👋 Thanks for reporting!

A maintainer will take a look at your issue shortly. 👀

In the meantime: We are working on Viper v2 and we would love to hear your thoughts about what you like or don't like about Viper, so we can improve or fix those issues.

⏰ If you have a couple minutes, please take some time and share your thoughts: https://forms.gle/R6faU74qPRPAzchZ9

📣 If you've already given us your feedback, you can still help by spreading the news,
either by sharing the above link or telling people about this on Twitter:

https://twitter.com/sagikazarmark/status/1306904078967074816

Thank you! ❤️

[everactivetim]

I should also add that the amount of code required to accomplish this is small and the overrall change is small relative to the existing code base.

[sagikazarmark]

Hi @everactivetim

Is this proposal by any chance similar to #1046? #1046 is a comprehensive plan to rewrite all config sources and give more power to users in how they are configured.

Let me know what you think.

[everactivetim]

@sagikazarmark - Well #1046 would probably cover the use case. But seeing as how that is extensive and not yet done it doesn't really help us. This is, as I mentioned a trivial change and just gives you another mechanism that is similar to how environment variables are overloads.

We would like this change sooner than later so we can start using it and would probably opt to maintain a fork. #1046 is a proposal that is over 13 months old. :(

Perhaps there's a path from this to #1046? So that we can at least get the immediate need satisfied?

p.s. - we are love Viper :)

[sagikazarmark]

From a maintenance perspective there are a couple things to consider:

• Making changes to the interface that will later be dropped is not always the best idea
• Viper's interface is already bloated as it is. Adding new things should be carefully considered.

If I'm being completely honest, I'm not sure I completely understand the problem and the proposed solution.

For example: is there a reason why Set is not enough? Values set through Set override everything else.

[everactivetim]

Understandable about maintenance which is why if there's a road forward in a particular direction that could be done partially to support that then I would be in favor.

So the constraints of this provider are that at run-time we do not know what is in the k/v store and there is no way to discover it. So unless every key is known ahead of time (impractical) we cannot use Set. So it's the exact same setup as the env overrides. The way Viper handles that would be perfect if there was an "Environment" provider.

I investigated remote providers as a solution but they too appear to need to know all of the information ahead of time. If I'm missing something please, I'm happy to use an existing behavior. :)

[sagikazarmark]

I think I understand.

So you basically want an additional config provider that's called as a first (or second if we still prioritize manually set values) upon any Get* calls when looking for a specific value. If this overrider returns a value use it, otherwise go to the next source.

With pseudo code:

func Get(key string) interface{} {
    // 1. Check manually set values

    v, ok := overrider.Override(key)
    if ok {
        return v
    }

    // go to the next source
}

Is this roughly what you would like to see?

If so, here is a couple issues I see:

You said knowing each potential key upfront is impractical (I disagree with that statement, but let's put that aside for a moment). How is the overrider going to know which keys should it override? I have a couple ideas, but none of them are prefect.

Also, I'm not sure you actually have to know each key upfront. For example: if overriding happens by reading a separate YAML file, you could just read the file, flatten the keys and call set for each of them.

Another potential issue is what AutomaticEnv suffers from a lot these days: inconsistent behavior between Get* and Unmarshal. Since the keys are not explicitly specified, Unmarshal cannot rely on the overrider at all, so the inconsistency would become worse. It could be an acceptable compromise, but it's also related to the problem outlined in #1277

Currently Viper's internal data representation is quite fragmented: most of the "keys" are actually in the form of maps, while things like flags and env vars are actually flat keys. This is something that we will have to sort out in Viper 2 and I'm leaning towards maps instead of flat keys. (As a result, Get* calls will become secondary citizens (actually, they are kind of already are) to Unmarshal functions).

You said you already have a proof of concept solution. Can I see it?

Honestly, at this point I'm leaning towards declining this feature request based on the information I have (and the assumptions that I made based on said information). If the proposed solution turns out to be something entirely different, I'll of course reevaluate my assumptions.

[everactivetim]

Sure thing, I will get a branch of it up shortly.

As to the behavior, I've tried to stress it before; it's almost identical behavior to the AutomaticEnv behavior. Viper does not try to discover all possible keys out of the environment or build internal maps from them - it just goes looking there if you've told it to. This is the exact same thing.

The datasource we have makes it so the pattern that's enforced is: have key, ask for value. There's no other way to make it do anything different. Right now we use Viper for config loading from file and environment variables.

I agree and understand the problem with unmarshalling. I guess if the stance is we already have one and we don't want to make that generic so that people can choose to add others if they want then that would be that. We would have to do all of the exact same behavior too which is not in the branch yet (like checking for shadowed children, etc).

Thanks for considering! I'll post a comment when I get the branch up.

[everactivetim]

Comparison of changes: https://github.com/spf13/viper/compare/master...everactivetim:snap_config?expand=1

[everactivetim]

Re-iterate that the above was to test an idea, not to submit a PR for this as final code.

[sagikazarmark]

Viper does not try to discover all possible keys out of the environment or build internal maps from them - it just goes looking there if you've told it to.

Yes, this is the current behavior and as I explained above it causes all kinds of trouble, so there is a good chance that this kind of behavior will disappear from Viper entirely. AutomaticEnv might get replaced with something that pulls in all env vars.

Oh, yeah and another thing I missed from the above list: how would overriders work with viper.Sub. Sub is another feature that I think should eventually disappear/be rewritten, but in the meantime, we shouldn't increase the amount of bugs in it unless necessary.

Would you mind providing an overrider implementation as well? Or at least a list of potential implementations to see how this would practically work.

[everactivetim]

I don't have an overrider I want to share at the moment. It's possible I'll have one later but if the feature is going nowhere then there's very little reason to bother.

How do environment variables work now for Sub?

Maybe overrider is SimpleGetterSetter and Sub doesn't yield a value from that? I imagine if you are using one you would be responsible for the implications? Just as there are remote providers and you need to know how they behave and interact.

Maybe the overrider is just a LazyOverrider. It could be framed in some way that it has implications when used.

[sagikazarmark]

Well, having an abstraction is one thing. But if you have no examples how an implementation would look like, how do you want to prove that this is actually useful?

[everactivetim]

Sorry, perhaps I was unclear. I do have an Overrider implementation, it is useful but I'm not in a position to share it at the moment.

[sagikazarmark]

Can you at least describe how it works? I still don't see how this is not an edge case.

[everactivetim]

Sure. I'll do my best.

We have a number of services that start up and load keys/values from the normal sources, config file and environment variables.

There is a REST API that provides values based on keys, that's it. We ask the REST API if there's a value for a key and it tells us the value or gives us an empty string. We have no control over this or its behavior and they can be set for the service at any time during the lifetime of the service. So it could not exist as a value through the REST API one moment and the exist later at a different moment.

So, during the lifetime of the service when we query Viper for values for a given key we need it to do that order:

• check REST API, if not there, continue
• check environment variables, if not there, continue
• check regular config file values, if not there or no default was provided, return empty string

One thing that did occur to me while I was typing is maybe I don't understand Remote Providers and an implementation of that would work but how does Viper and Remote providers handle keys that are added after initial setup? Or is that not handled? Or is it handled and I missed it?

Say during the lifetime of a service a value is added for a key in a remote provider that did not exist there before and some part of the service then calls viper.GetString(.... new key in remote provider ...), will it return the value in the remote provider to the service?

If that's true then I missed that behavior and it's possible I could just make a RemoteProvider implementation, add it as an option and use that?

Thanks!

[andig]

I have a similar requirement in #1548 for sqlite. It's not obvious to me how/if the Overrider is different from the Viper 2 provider mechanism?

[andig]

@everactivetim append will already take are of

if v.overriders == nil {
	v.overriders = []Overrider{}
}