From e1d14441a090e893a145112dc2f26a6b20c9cf4b Mon Sep 17 00:00:00 2001 From: Kyle Huntsman <3432646+kylehuntsman@users.noreply.github.com> Date: Wed, 2 Mar 2022 20:26:56 -0700 Subject: [PATCH 1/5] fix(fsrepo): deep merge when setting config --- repo/common/common.go | 30 +++++++ repo/common/common_test.go | 132 +++++++++++++++++++++++++++++++ repo/fsrepo/fsrepo.go | 6 +- test/sharness/t0250-files-api.sh | 2 +- 4 files changed, 165 insertions(+), 5 deletions(-) create mode 100644 repo/common/common_test.go diff --git a/repo/common/common.go b/repo/common/common.go index dc1e7c503..1f1f0cd57 100644 --- a/repo/common/common.go +++ b/repo/common/common.go @@ -54,3 +54,33 @@ func MapSetKV(v map[string]interface{}, key string, value interface{}) error { } return nil } + +// Merges the right map into the left map, recursively traversing child maps +// until a non-map value is found +func MapMergeDeep(left, right map[string]interface{}) map[string]interface{} { + // We want to alter a copy of the map, not the original + result := make(map[string]interface{}) + for k, v := range left { + result[k] = v + } + + for key, rightVal := range right { + // If right value is a map + if rightMap, ok := rightVal.(map[string]interface{}); ok { + // If key is in left + if leftVal, found := result[key]; found { + // If left value is also a map + if leftMap, ok := leftVal.(map[string]interface{}); ok { + // Merge nested map + result[key] = MapMergeDeep(leftMap, rightMap) + continue + } + } + } + + // Otherwise set new value to result + result[key] = rightVal + } + + return result +} diff --git a/repo/common/common_test.go b/repo/common/common_test.go new file mode 100644 index 000000000..5f62b688c --- /dev/null +++ b/repo/common/common_test.go @@ -0,0 +1,132 @@ +package common + +import ( + "testing" + + "github.com/ipfs/go-ipfs/thirdparty/assert" +) + +func TestMapMergeDeepReturnsNew(t *testing.T) { + leftMap := make(map[string]interface{}) + leftMap["A"] = "Hello World" + + rightMap := make(map[string]interface{}) + rightMap["A"] = "Foo" + + MapMergeDeep(leftMap, rightMap) + + assert.True(leftMap["A"] == "Hello World", t, "MapMergeDeep should return a new map instance") +} + +func TestMapMergeDeepNewKey(t *testing.T) { + leftMap := make(map[string]interface{}) + leftMap["A"] = "Hello World" + /* + leftMap + { + A: "Hello World" + } + */ + + rightMap := make(map[string]interface{}) + rightMap["B"] = "Bar" + /* + rightMap + { + B: "Bar" + } + */ + + result := MapMergeDeep(leftMap, rightMap) + /* + expected + { + A: "Hello World" + B: "Bar" + } + */ + + assert.True(result["B"] == "Bar", t, "New keys in right map should exist in resulting map") +} + +func TestMapMergeDeepRecursesOnMaps(t *testing.T) { + leftMapA := make(map[string]interface{}) + leftMapA["B"] = "A value!" + leftMapA["C"] = "Another value!" + + leftMap := make(map[string]interface{}) + leftMap["A"] = leftMapA + /* + leftMap + { + A: { + B: "A value!" + C: "Another value!" + } + } + */ + + rightMapA := make(map[string]interface{}) + rightMapA["C"] = "A different value!" + + rightMap := make(map[string]interface{}) + rightMap["A"] = rightMapA + /* + rightMap + { + A: { + C: "A different value!" + } + } + */ + + result := MapMergeDeep(leftMap, rightMap) + /* + expected + { + A: { + B: "A value!" + C: "A different value!" + } + } + */ + + resultA := result["A"].(map[string]interface{}) + assert.True(resultA["B"] == "A value!", t, "Unaltered values should not change") + assert.True(resultA["C"] == "A different value!", t, "Nested values should be altered") +} + +func TestMapMergeDeepRightNotAMap(t *testing.T) { + leftMapA := make(map[string]interface{}) + leftMapA["B"] = "A value!" + + leftMap := make(map[string]interface{}) + leftMap["A"] = leftMapA + /* + origMap + { + A: { + B: "A value!" + } + } + */ + + rightMap := make(map[string]interface{}) + rightMap["A"] = "Not a map!" + /* + newMap + { + A: "Not a map!" + } + */ + + result := MapMergeDeep(leftMap, rightMap) + /* + expected + { + A: "Not a map!" + } + */ + + assert.True(result["A"] == "Not a map!", t, "Right values that are not a map should be set on the result") +} diff --git a/repo/fsrepo/fsrepo.go b/repo/fsrepo/fsrepo.go index 2aa4a5e46..a4234dc49 100644 --- a/repo/fsrepo/fsrepo.go +++ b/repo/fsrepo/fsrepo.go @@ -559,10 +559,8 @@ func (r *FSRepo) setConfigUnsynced(updated *config.Config) error { if err != nil { return err } - for k, v := range m { - mapconf[k] = v - } - if err := serialize.WriteConfigFile(configFilename, mapconf); err != nil { + mergedMap := common.MapMergeDeep(mapconf, m) + if err := serialize.WriteConfigFile(configFilename, mergedMap); err != nil { return err } // Do not use `*r.config = ...`. This will modify the *shared* config diff --git a/test/sharness/t0250-files-api.sh b/test/sharness/t0250-files-api.sh index c91dd123b..78cfc39a2 100755 --- a/test/sharness/t0250-files-api.sh +++ b/test/sharness/t0250-files-api.sh @@ -876,7 +876,7 @@ test_expect_success "set up automatic sharding/unsharding data" ' ' # TODO: This does not need to report an error https://github.com/ipfs/go-ipfs/issues/8088 -test_expect_failure "reset automatic sharding" ' +test_expect_success "reset automatic sharding" ' ipfs config --json Internal.UnixFSShardingSizeThreshold null ' From a09b6c205eea1ecaaae1e9ba0b45e4a091308694 Mon Sep 17 00:00:00 2001 From: Kyle Huntsman <3432646+kylehuntsman@users.noreply.github.com> Date: Mon, 7 Mar 2022 17:00:49 -0700 Subject: [PATCH 2/5] fix(repo/common): improve MapGetKV not found error --- repo/common/common.go | 8 +++++++- 1 file changed, 7 insertions(+), 1 deletion(-) diff --git a/repo/common/common.go b/repo/common/common.go index 1f1f0cd57..e9c56e65e 100644 --- a/repo/common/common.go +++ b/repo/common/common.go @@ -21,7 +21,13 @@ func MapGetKV(v map[string]interface{}, key string) (interface{}, error) { cursor, ok = mcursor[part] if !ok { - return nil, fmt.Errorf("%s key has no attributes", sofar) + // Construct the current path traversed to print a nice error message + var path string + if len(sofar) > 0 { + path += sofar + "." + } + path += part + return nil, fmt.Errorf("%s not found", path) } } return cursor, nil From 76128272b7fbf431eb3c1daf160cd9921a7a1802 Mon Sep 17 00:00:00 2001 From: Lucas Molas Date: Tue, 15 Mar 2022 18:00:25 -0300 Subject: [PATCH 3/5] unroll setConfigUnsynced --- repo/fsrepo/fsrepo.go | 38 ++++++++++++++++++++++++-------------- 1 file changed, 24 insertions(+), 14 deletions(-) diff --git a/repo/fsrepo/fsrepo.go b/repo/fsrepo/fsrepo.go index a4234dc49..baafb6630 100644 --- a/repo/fsrepo/fsrepo.go +++ b/repo/fsrepo/fsrepo.go @@ -542,8 +542,26 @@ func (r *FSRepo) BackupConfig(prefix string) (string, error) { return orig.Name(), nil } -// setConfigUnsynced is for private use. -func (r *FSRepo) setConfigUnsynced(updated *config.Config) error { +// SetConfig updates the FSRepo's config. The user must not modify the config +// object after calling this method. +// FIXME: There is an inherent contradiction with storing non-user-generated +// Go config.Config structures as user-generated JSON nested maps. This is +// evidenced by the issue of `omitempty` property of fields that aren't defined +// by the user and Go still needs to initialize them to its default (which +// is not reflected in the repo's config file, see +// https://github.com/ipfs/go-ipfs/issues/8088 for more details). +// In general we should call this API with a JSON nested maps as argument +// (`map[string]interface{}`). Many calls to this function are forced to +// synthesize the config.Config struct from their available JSON map just to +// satisfy this (causing incompatibilities like the `omitempty` one above). +// We need to comb SetConfig calls and replace them when possible with a +// JSON map variant. +func (r *FSRepo) SetConfig(updated *config.Config) error { + + // packageLock is held to provide thread-safety. + packageLock.Lock() + defer packageLock.Unlock() + configFilename, err := config.Filename(r.path) if err != nil { return err @@ -569,17 +587,6 @@ func (r *FSRepo) setConfigUnsynced(updated *config.Config) error { return nil } -// SetConfig updates the FSRepo's config. The user must not modify the config -// object after calling this method. -func (r *FSRepo) SetConfig(updated *config.Config) error { - - // packageLock is held to provide thread-safety. - packageLock.Lock() - defer packageLock.Unlock() - - return r.setConfigUnsynced(updated) -} - // GetConfigKey retrieves only the value of a particular key. func (r *FSRepo) GetConfigKey(key string) (interface{}, error) { packageLock.Lock() @@ -643,10 +650,13 @@ func (r *FSRepo) SetConfigKey(key string, value interface{}) error { if err != nil { return err } + r.config = conf + if err := serialize.WriteConfigFile(filename, mapconf); err != nil { return err } - return r.setConfigUnsynced(conf) // TODO roll this into this method + + return nil } // Datastore returns a repo-owned datastore. If FSRepo is Closed, return value From 3c09c260bff4eb40e2d75091c1778fc918508168 Mon Sep 17 00:00:00 2001 From: Lucas Molas Date: Wed, 16 Mar 2022 19:03:03 -0300 Subject: [PATCH 4/5] remove todo --- test/sharness/t0250-files-api.sh | 1 - 1 file changed, 1 deletion(-) diff --git a/test/sharness/t0250-files-api.sh b/test/sharness/t0250-files-api.sh index 78cfc39a2..382758a05 100755 --- a/test/sharness/t0250-files-api.sh +++ b/test/sharness/t0250-files-api.sh @@ -875,7 +875,6 @@ test_expect_success "set up automatic sharding/unsharding data" ' done ' -# TODO: This does not need to report an error https://github.com/ipfs/go-ipfs/issues/8088 test_expect_success "reset automatic sharding" ' ipfs config --json Internal.UnixFSShardingSizeThreshold null ' From f62bd64fdb840809a211984ec3c6499abc62690b Mon Sep 17 00:00:00 2001 From: Lucas Molas Date: Mon, 21 Mar 2022 19:03:37 -0300 Subject: [PATCH 5/5] allow config to fail as we have no empty default --- test/sharness/t0171-peering.sh | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/test/sharness/t0171-peering.sh b/test/sharness/t0171-peering.sh index 9b775cb3c..207b27980 100755 --- a/test/sharness/t0171-peering.sh +++ b/test/sharness/t0171-peering.sh @@ -33,7 +33,7 @@ peer_addrs() { peer() { PEER1="$1" && PEER2="$2" && - PEER_LIST="$(ipfsi "$PEER1" config Peering.Peers)" && + PEER_LIST="$(ipfsi "$PEER1" config Peering.Peers || true)" && { [[ "$PEER_LIST" == "null" ]] || PEER_LIST_INNER="${PEER_LIST:1:-1}"; } && ADDR_INFO="$(printf '[%s{"ID": "%s", "Addrs": %s}]' \ "${PEER_LIST_INNER:+${PEER_LIST_INNER},}" \