Overriding nested keys

[gabdelab]

Hey viper team, and thanks for the amazing work on viper !
I just encountered an issue while playing around with viper, it seems to me that overriding values in nested keys doesn't update the parent value:

With a simple nested config:

level1:
    level2:
        oldValue

func SampleTest(t *testing.T) {
    config := viper.New()
    config.SetConfigFile("sample-nested-conf.yaml")
    err := config.ReadInConfig()
    if err == nil {
        config.Set("level1.level2", "newValue")
        // Returns map[level2:oldValue]
        fmt.Println(config.Get("level1"))
        // Returns newValue
        fmt.Println(config.Get("level1.level2"))
    }
}

Outputs:

map[level2:oldValue]
newValue

"oldValue" should be overriden at all levels by Set(), and a Get() should return consistent values.

[harnash]

I think the same problem exists with SetDefault()

[benoitmasson]

I'm currently investigating this issue, which seems to be related to mine (issue #190), in order to complete a PR.

The overriding functions (Set, SetDefault, BindFlagValue, etc.) don't handle dots as separator, thus they don't change “deep” values. Correcting this behavior is not too hard, but it raises 2 (related) questions.

Given this config:

level1a:
    value1
level1b:
    level2:
        value2

1. What should happen if one calls Set(level1a.value1, value)? (level1a already being a value, not a map)
2. What should happen if one calls Set(level1b, value)? (level1b already being a map, not a value)

My feeling is that it should raise errors (not yet in the prototype of the function however, but I can add it).

Another option would be to replace the value by the map, or conversely, or to work out something more complex where both would cohabit, but I find this risky for the end-user.

What's your (and @spf13's) opinion on that?

[harnash]

In other languages (Perl for example) you would get exception in first case and second one will override the value which is acceptable I think. If we decide to throw errors in both cases it will be fine too. Only drawback of this is that one would not be able to recover from this situation unless some Delete() function exists.

[spf13]

It's a bit more complex than the Perl suggestion because of the merging of values across various data sources in an ordered way. There's a bunch of scenarios to consider and we need to be very thoughtful and consistent in our approach.

I wrote out a few scenarios.. but then realized really there's just a few fundamental questions that need to be addressed. I'm not certain that this is a complete list, but it's a good start.

What should the correct behavior be when:

1. An overlay layer has a value where a base layer has a key:value?
2. An overlay layer has a key:value where a base layer has a value?
3. An overlay layer has a key:value where a base layer has a map?
4. An overlay layer has a map where a base layer has a map?

My thoughts around these are:

1. It should replace the key:value with a value. The nested key is no longer available.
2. It should replace the value with the key:value (via replacing the value with a map containing that key:value).
3. It should merge the key:value into the map. If the map has an existing matching key it should replace it and follow 1 & 2).
4. It should merge the keys and follow 1, 2 & 3.

I don't think the functions (set, setDefault, etc) should trigger errors in these conditions. These aren't errors, but a normal function of configuration with completely valid usages. If a specific key expects a specifically shaped value then the app should error when that condition isn't met, but that's for the application to determine, not Viper.

I'd love feedback on these guidelines and I think it would be a good next step to come up with practical scenarios and see if they hold.

[harnash]

@spf13 What is the difference between map and key:value? Does map means that there is already a complex structure in place where key:value is just one entry on the same level?

Your proposal sounds good but I have concerns about fourth case. Imagine situation like this:

Base

---
web:
  ports:
    - 8080
    - 9500
    - 9090

Overlay

---
web:
  ports:
    - 80

According to fourth it may end up like this (unless array is treated as value):
Result

---
web:
  ports:
    - 8080
    - 9500
    - 9090
    - 80

So you might treat arrays as simple values too.

Edit: Actually it should be as intuitive as merging two JSON files: https://jsfiddle.net/rhwmvta1/ :)

[benoitmasson]

I understand that you find it reasonable that a user “replaces” anything in a map by anything, that's fine with me.

As for your 4 behaviors:
1&2. OK
3&4. Agree with @harnash, for me, these are particular cases of 2, no need to merge explicitly the maps. If a key path is found in the overlay layer, its value is used, otherwise lower layers are inspected (same behavior as you describe, without dealing with anything specific). This all means that I'd handle maps as values (so we can do the same with arrays).

But that's when you Set() values which were parsed in the config, i.e., maps are distinct. What if you do sequentially:

Set(level1.level2, value1)
Set(level1, value2)

(or the converse)? In this case the same map is modified.
From your comments I understand that in both cases, the new value (or key:value) should replace the old key:value (respectively, value) in the map.

Finally, what should Get(level1.level2) return after the 2 lines above, after it has been invalidated? Currently, my tests show that if level1.level2 is not defined but level1 is a value, then this latter value is returned. That's rather strange, I'd return nil instead (or 0 for GetInt(), and so on).

[spf13]

@harnash yes, that is what I meant by those two terms. Your example is a good one as it highlights an overall issue with this or any approach. I didn't really consider arrays in my above rules.

config.Set("web.ports", "80")
or 
config.Set("web.ports", []int{80})

are clearly different, the first would be a key:value where the second is an array.

We need an additional rule to handle arrays:

5. Arrays will be joined

So if the overlay is

---
web:
  ports:
    - 80

The merge would be

---
web:
  ports:
    - 8080
    - 9500
    - 9090
    - 80

But if the overlay is

---
web:
  ports: 80

The merge would be

---
web:
  ports: 80

This does create a problem though as in some methods these overlay settings are ambiguous... or only permit single values, not arrays.

ENV_WEB__PORTS=80
--web.ports=80

These methods do provide distinct settings for a map/k:v vs value

Base

---
web:
  ports:
    - 80
    - 90

Overlay

---
web:
  path: "/srv/www/"

The merge would be

---
web:
  ports:
    - 80
    - 90
  path: "/srv/www/"

All methods work with maps well.

ENV_WEB__PATH="/srv/www/"
--web.path="/srv/www/"

[spf13]

If a key path is found in the overlay layer, its value is used, otherwise lower layers are inspected (same behavior as you describe, without dealing with anything specific).

I'm not sure I agree with that. Let's use a scenario to make sure we are talking about the same behavior.

Config:

---
web:
  port: 80

Flag:
--web="/srv/www/"

What should Get("web.port") return?
I think the answer should be nil/zero. If I am understanding you correctly you are suggesting it should be 80. That is odd behavior to me. Even though in the implementation the config value is accessible internally, I would think that any value in a parent branch would prevent access to nested values.

Finally, what should Get(level1.level2) return after the 2 lines above, after it has been invalidated?

I think it should be the zero/nil value (depending on type).

[benoitmasson]

What should Get("web.port") return? I think the answer should be nil/zero

I agree with that, that's what I meant. Actually you're right, reasoning on examples will make it easier to understand each other (and will give us plenty of test cases ;-) )

So if I reformulate the previous behaviors as examples, to make sure we agree on everything (excluding the obvious base case where the base value is overridden by a value):

Case 1. (An overlay layer has a value where a base layer has a key:value)

SetDefault(level1.level2, value1)
Set(level1, value2)
fmt.Println(Get(level1), Get(level1.level2))

should display value2 <nil>

We expect the same result for an override in the same layer:

Set(level1.level2, value1)
Set(level1, value2)

Case 2. (An overlay layer has a key:value where a base layer has a value)

SetDefault(level1, value1) // or Set(level1, value1)
Set(level1.level2, value2)
fmt.Println(Get(level1), Get(level1.level2))

should display map[level2:value2] value2

Case 3&4. (An overlay layer has a map where a base layer has a map/a value)

SetDefault(level1.level2a, value1) // or Set(level1.level2a, value1)
Set(level1.level2b, value2)
fmt.Println(Get(level1), Get(level1.level2a), Get(level1.level2b))

should display map[level2a:value1 level2b:value2] value1 value2

But maybe you meant this instead?

Case 3&4b. (An overlay layer has a map where a base layer has a map)

SetDefault(level1.level2a, value1) // or Set(level1.level2a, value1)
Set(level1, map[string]interface{}{"level2b": value2})
fmt.Println(Get(level1), Get(level1.level2a), Get(level1.level2b))

As for me, this should display map[level2b:value2] <nil> value2, i.e., the old map should be replaced. Isn't that the meaning of the “Set” word after all? If one wants to merge maps, he can still perform it manually with a simple merge function:

Set(level1, merge(Get(level1), map[string]interface{}{"level2b": value2}))

If we choose to merge maps inside the Set function as you seem to suggest, then a user would have no easy way to “replace” an entire submap, and besides, this behavior would be rather inconsistent with the way we deal with case 1.

Case 5. (An overlay layer has a value/a map where a base layer has an array)

SetDefault(level1, []int{value1}) // or Set(level1, []int{value1})
Set(level1, value2)
fmt.Println(Get(level1))

should display value2

Case 6. (An overlay layer has an array where a base layer has an array)

SetDefault(level1, []int{value1}) // or Set(level1, []int{value1})
Set(level1, []int{value2})
fmt.Println(Get(level1))

Unlike you, and for the same reasons as in Case 3&4b, I'd replace the old array, thus expecting [value2] to display. If one wants to merge arrays, it suffices to write

Set(level1, append(Get(level1), []int{value2}))

I think this covers everything... we just need to agree on something for cases 3&4b and 6.

[harnash]

@spf13

But if the overlay is

---
web:
  ports: 80

The merge would be

---
web:
  ports: 80

This might be fine when we want to set one value. What if we want to override default array with completely new set of values?

@benoitmasson:
Case 1: ✅
Case 2: ✅
Case 3: ✅
Case 4: ✅
Case 4b: ✅ (it is a valid case I think)
Case 5: ✅
Case 6: ✅

I like the idea of writing cases using code and discuss edge cases based on that. We could use that to write Unit Tests later ;-) Unfortunately github is not a great place to work with such examples since we can't easily reference specific lines... How about throwing those cases into some Google Doc or similar when we could add comments, new cases, resolve questions/comments? Or maybe start PR with just Unit Tests already?

[benoitmasson]

As suggested, I created PR #195 with the above tests, following this global rule: every assignment replaces any existing value (thus, merges have to be explicit).

Feel free to comment (in the PR?) if you (dis)agree on anything…

[benoitmasson]

PR #195 is completed and should have fixed this issue. Let me know if something still goes wrong.