Skip to content

Add MergeUnder #29

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

Merged
merged 1 commit into from
Jul 17, 2020
Merged

Add MergeUnder #29

merged 1 commit into from
Jul 17, 2020

Conversation

0xjac
Copy link
Contributor

@0xjac 0xjac commented Jul 16, 2020

MergeUnder allows to merge a Koanf in another Koanf under the given key
path. For details, see #27

Closes #27

@0xjac 0xjac force-pushed the 27/feat/merge-under branch 2 times, most recently from 1103416 to 098f7a9 Compare July 16, 2020 17:38
@0xjac 0xjac marked this pull request as ready for review July 16, 2020 17:39
@knadh
Copy link
Owner

knadh commented Jul 17, 2020

Hi @0xjac,
I think this logic (splitting keys by "." and flattening them before merging) corrupts nested confmaps.

package main

import (
	"fmt"

	"github.com/knadh/koanf"
	"github.com/knadh/koanf/providers/confmap"
)

func main() {
	k1 := koanf.New(".")
	k1.Load(confmap.Provider(map[string]interface{}{
		"map1key1": "map1val1",
		"map1key2": "map1val2",
	}, "."), nil)
	fmt.Println(k1.Get(""))
	// Prints: map[map1key1:map1val1 map1key2:map1val2]

	k2 := koanf.New(".")
	k2.Load(confmap.Provider(map[string]interface{}{
		"map2key1": "map2val1",
		"map2key2": "map2val2",
		"map2key3": map[string]interface{}{
			"map2key3a": "map2val3a",
		},
	}, "."), nil)
	fmt.Println(k2.Get(""))
	// Prints: map[map2key1:map2val1 map2key2:map2val2 map2key3:map[map2key3a:map2val3a]]
	// Notice that there are no periods in the key names, instead, children are nested (map2key3:map)
	// The delimiter (.) is only used to describe getters and when visualizing with Print().
	// Internally, the confmaps are stored as nested hierarchies.

	k1.MergeUnder(k2, "1")
	fmt.Println(k1.Get(""))
	// Prints: map[1.map2key1:map2val1 1.map2key2:map2val2 1.map2key3.map2key3a:map2val3a map1key1:map1val1 map1key2:map1val2]
	// Notice that the keys now have "." in them and the nested hierarchy (map2key3) is lost.
}

@knadh
Copy link
Owner

knadh commented Jul 17, 2020

This seems to work as intended. Could you please test and amend your PR? Also, I think MergeAt makes better sense than MergeUnder

// MergeAt merges the config map of a given Koanf instance into
// the current instance as a sub map, under the given key path.
// If all or part of the key path is missing, it will be created.
func (ko *Koanf) MergeAt(in *Koanf, path string) {
	// Get the confmap at the given path.
	current := make(map[string]interface{})
	if v, ok := ko.Get(path).(map[string]interface{}); ok {
		current = v
	}

	// Merge the incoming instance and the confmap at the current path
	// into a new instance.
	n := New(ko.delim)
	n.merge(current)
	n.Merge(in)

	// Merge the mix back into the main instance.
	ko.Merge(n)
}

@0xjac
Copy link
Contributor Author

0xjac commented Jul 17, 2020

@knadh Thanks a lot. I'll update it all now

edit:
There is a small issue with the code you provided. If I take your example and use MergeAt, at the end I get:

k1.MergeAt(k2, "1")
fmt.Println(k1.Get(""))
// Prints: map[map1key1:map1val1 map1key2:map1val2 map2key1:map2val1 map2key2:map2val2 map2key3:map[map2key3a:map2val3a]]
// Notice the "1" is gone. You're right it should not be a dot, 
// but I would expect: map[map1key1:map1val1 map1key2:map1val2 1:map[map2key1:map2val1 map2key2:map2val2 map2key3:map[map2key3a:map2val3a]]]
// Notice the 1:map[...]

I also checked my example in #27 to see what it looks like, and I have:

ftpMap := map[string]interface{}{
	"ftp.user": "user",
	"ftp.password": "pass",
	"ftp.addr": "addr",
}

mainKoanf := koanf.New(".")
mainKoanf.Load(confmap.Provider(ftpMap, "koanf"), nil)
fmt.Println(mainKoanf.Get(""))
// Prints: map[ftp.addr:addr ftp.password:pass ftp.user:user]
// Notice the "." are appearing here too...

I thought it was handling nested keys there correctly but it doesn't. I am working on a fix.
edit2: My bad, I put "koanf" instead of "." as a delimiter. With ".", it works as expected.

@0xjac 0xjac force-pushed the 27/feat/merge-under branch from 098f7a9 to 19ab513 Compare July 17, 2020 10:08
@0xjac
Copy link
Contributor Author

0xjac commented Jul 17, 2020

@knadh I've done the update. Namely:

  1. Rename the function as MergeAt (in the code, test, and README)

  2. Updated the test to use Koanf.Get("") instead of Koanf.All() in assertions. (This allows to catch the issue of dots in a key name.)

  3. Rewrote the MergeAt function based on your suggestion. What I do is build a nested config map based on the keys starting from the leaf and going all the way to the root. I then rely on merge to merge everything back together. For your convenience, here is the code of the function:

    func (ko *Koanf) MergeAt(in *Koanf, path string) {
     // No path. Merge the two config maps.
     if path == "" {
     	ko.Merge(in)
     	return
     }
    
     keys := strings.Split(path, ko.delim)
    
     // Wrap the config map in the leaf key from the path
     n := map[string]interface{}{
     	keys[len(keys)-1]: in.Raw(),
     }
    
     // Build the path back to the root as nested config maps
     for i := len(keys) - 2; i >= 0; i-- {
     	n = map[string]interface{}{
     		keys[i]: n,
     	}
     }
     ko.merge(n)
    }

It's a bit more verbose but it seems to work with everything I could think of throwing at it. Let me know if there is anything I can improve further.

@knadh
Copy link
Owner

knadh commented Jul 17, 2020

There's no need to construct the map manually by iterating the path, right? We could just use merge() the API like in the previous example to handle all that with no additional code/logic.

@0xjac
Copy link
Contributor Author

0xjac commented Jul 17, 2020

There's no need to construct the map manually by iterating the path, right? We could just use merge() the API like in the previous example to handle all that with no additional code/logic.

I am not sure I understand what you mean. merge() is used at the end. But it merges two maps at their root. With MergeAt we do not merge at the root but at a lower level. Constructing the map is needed to "move down" the map at the level we want.

I am reconstructing the map because I did not find an existing function which did it already. (Maybe I missed it ?) I am happy to move that logic in a separate function if you want.

If you don't want to construct the map manually, we could just flatten the given config map, append the path to each key, then unflatten the whole thing and merge it. But I felt like this approach was more confusing and more bloated. But I am happy to do it that way if you prefer.

@knadh
Copy link
Owner

knadh commented Jul 17, 2020

Apologies for the confusion. I was referring to the example I shared earlier. It uses the existing Get() + Merge() methods to merge a given instance at the given path. My point was, when that works, there's no need to split the key, traverse, construct a map manually before finally merging in place, right?

@0xjac
Copy link
Contributor Author

0xjac commented Jul 17, 2020

I see. I tried that code, combined with your earlier example and it did not work for me. I got:

func main() {
	k1 := koanf.New(".")
	k1.Load(confmap.Provider(map[string]interface{}{
		"map1key1": "map1val1",
		"map1key2": "map1val2",
	}, "."), nil)
	fmt.Println(k1.Get(""))
	// Prints: map[map1key1:map1val1 map1key2:map1val2]

	k2 := koanf.New(".")
	k2.Load(confmap.Provider(map[string]interface{}{
		"map2key1": "map2val1",
		"map2key2": "map2val2",
		"map2key3": map[string]interface{}{
			"map2key3a": "map2val3a",
		},
	}, "."), nil)
	fmt.Println(k2.Get(""))
	// Prints: map[map2key1:map2val1 map2key2:map2val2 map2key3:map[map2key3a:map2val3a]]

	k1.MergeUnder(k2, "1")
	fmt.Println(k1.Get(""))
	// Prints: map[map1key1:map1val1 map1key2:map1val2 map2key1:map2val1 map2key2:map2val2 map2key3:map[map2key3a:map2val3a]]
}

You can see the last print lost the "1" key. It was not merged under it but under the root of the config map.
The value should be:

map[1:map[map2key1:map2val1 map2key2:map2val2 map2key3:map[map2key3a:map2val3a]] map1key1:map1val1 map1key2:map1val2]

From what I understand, this comes from the fact that when we get the confmap with:

current := make(map[string]interface{})
if v, ok := ko.Get(path).(map[string]interface{}); ok {
	current = v
}

while we do get it if it exists, we loose the information about the path, so we would still need to construct the path manually.

We could do a hybrid approach where we get the confmap as you suggested and then build the path. That would give:

func (ko *Koanf) MergeAt(in *Koanf, path string) {
 // No path. Merge the two config maps.
 if path == "" {
 	ko.Merge(in)
 	return
 }

 keys := strings.Split(path, ko.delim)

// Get the confmap at the given path.
current := make(map[string]interface{})
if v, ok := ko.Get(path).(map[string]interface{}); ok {
	current = v
}
current.merge(in.Raw())

 // Wrap the config map in the leaf key from the path
 n := map[string]interface{}{
 	keys[len(keys)-1]: current,
 }

 // Build the path back to the root as nested config maps
 for i := len(keys) - 2; i >= 0; i-- {
 	n = map[string]interface{}{
 		keys[i]: n,
 	}
 }
 ko.merge(n)
}

But from my understanding, it is not needed as maps.Merge (which is called by koanf.merge) takes care of merging keys recursively.

@knadh
Copy link
Owner

knadh commented Jul 17, 2020

This one liner works I believe 😅.

Whatever path we get, create an empty map with a single key (the path) and the incoming instance as the value. eg: {"a.b.c": incomingKoanf{}}. Then Unflatten() will magically create a nested map out of it {a: { b: { c: incomingKoanf{} } } }, which can now just be merged directly.

// MergeAt merges the config map of a given Koanf instance into
// the current instance as a sub map, under the given key path.
// If all or part of the key path is missing, it will be created.
func (ko *Koanf) MergeAt(in *Koanf, path string) {
	if path == "" {
		ko.Merge(in)
		return
	}
	ko.merge(maps.Unflatten(map[string]interface{}{path: in.Raw()}, ko.delim))
}

@0xjac
Copy link
Contributor Author

0xjac commented Jul 17, 2020

Yes it does 😎 Thanks a lot. However do you mind If I slightly expand it such that it is more readable and maintainable:

// MergeAt merges the config map of a given Koanf instance into
// the current instance as a sub map, at the given key path.
// If all or part of the key path is missing, it will be created.
// If the key path is `""`, this is equivalent to Merge
func (ko *Koanf) MergeAt(in *Koanf, path string) {
	// No path. Merge the two config maps.
	if path == "" {
		ko.Merge(in)
		return
	}

	// Unflatten the config map with the given key path
	n := maps.Unflatten(map[string]interface{}{
		path: in.Raw(),
	}, ko.delim)

	ko.merge(n)
}

It's the exact same code you wrote, just split on a few lines and with comments.

@0xjac 0xjac force-pushed the 27/feat/merge-under branch 2 times, most recently from 52ebfba to 2a0fdba Compare July 17, 2020 13:49
@knadh
Copy link
Owner

knadh commented Jul 17, 2020

Sure. I knew there was a method that would let us get from manually handling paths, but couldn't remember what it was.

MergeAt allows to merge a Koanf in another Koanf at the given key path.
For details, see knadh#27

Unit test and documentation in the README included.

Closes knadh#27
@0xjac 0xjac force-pushed the 27/feat/merge-under branch from 2a0fdba to 9c08d5e Compare July 17, 2020 13:50
@0xjac
Copy link
Contributor Author

0xjac commented Jul 17, 2020

I've updated. Let me know if there is anything else I can do.

Thank you for your assistance!

@knadh knadh merged commit 7f3d1b9 into knadh:master Jul 17, 2020
@0xjac 0xjac deleted the 27/feat/merge-under branch July 17, 2020 14:15
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.

Feature request: MergeUnder (or path argument for all providers)
2 participants