Merge pull request #8793 from ipfs/kylehuntsman/fix/repo/omitempty-error

fix(fsrepo): deep merge when setting config

Author: schomatis

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
@@ -54,3 +60,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
+}

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")
+}

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
@@ -559,10 +577,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
@@ -571,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()
@@ -645,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

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},}" \

test/sharness/t0250-files-api.sh

@@ -875,8 +875,7 @@ 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_failure "reset automatic sharding" '
+test_expect_success "reset automatic sharding" '
   ipfs config --json Internal.UnixFSShardingSizeThreshold null
 '