Fix gRPC interface function to merge configs (#1164)

* Fix gRPC interface function to merge configs

This fix adds the possibility to set empty values with the Merge gRPC
function, previously they would have been ignored.

Because of this change I had also to modify the GetValue() function
since it would first check if the value was set using the
Viper.InConfig() function that wouldn't check for values set with
Viper.Set().

* [skip changelog] Add clearer example to client_example

* [skip changelog] Simplified some code and enhance a test

Author: silvanocerza

client_example/main.go

@@ -83,8 +83,20 @@ func main() {
 	callGetAll(settingsClient)
 
 	// Merge applies multiple settings values at once.
-	log.Println("calling Merge(`{\"foo\": \"bar\", \"daemon\":{\"port\":\"422\"}}`)")
-	callMerge(settingsClient)
+	log.Println("calling Merge()")
+	callMerge(settingsClient, `{"foo": {"value": "bar"}, "daemon":{"port":"422"}, "board_manager": {"additional_urls":["https://example.com"]}}`)
+
+	log.Println("calling GetAll()")
+	callGetAll(settingsClient)
+
+	log.Println("calling Merge()")
+	callMerge(settingsClient, `{"foo": {} }`)
+
+	log.Println("calling GetAll()")
+	callGetAll(settingsClient)
+
+	log.Println("calling Merge()")
+	callMerge(settingsClient, `{"foo": "bar" }`)
 
 	// Get the value of the foo key.
 	log.Println("calling GetValue(foo)")
@@ -225,11 +237,10 @@ func callSetValue(client settings.SettingsClient) {
 	}
 }
 
-func callMerge(client settings.SettingsClient) {
-	bulkSettings := `{"foo": "bar", "daemon":{"port":"422"}}`
+func callMerge(client settings.SettingsClient, jsonData string) {
 	_, err := client.Merge(context.Background(),
 		&settings.RawData{
-			JsonData: bulkSettings,
+			JsonData: jsonData,
 		})
 
 	if err != nil {

commands/daemon/settings.go

@@ -19,6 +19,8 @@ import (
 	"context"
 	"encoding/json"
 	"errors"
+	"fmt"
+	"strings"
 
 	"github.com/arduino/arduino-cli/configuration"
 	rpc "github.com/arduino/arduino-cli/rpc/settings"
@@ -40,15 +42,45 @@ func (s *SettingsService) GetAll(ctx context.Context, req *rpc.GetAllRequest) (*
 	return nil, err
 }
 
+// mapper converts a map of nested maps to a map of scalar values.
+// For example:
+//    {"foo": "bar", "daemon":{"port":"420"}, "sketch": {"always_export_binaries": "true"}}
+// would convert to:
+//    {"foo": "bar", "daemon.port":"420", "sketch.always_export_binaries": "true"}
+func mapper(toMap map[string]interface{}) map[string]interface{} {
+	res := map[string]interface{}{}
+	for k, v := range toMap {
+		switch data := v.(type) {
+		case map[string]interface{}:
+			for mK, mV := range mapper(data) {
+				// Concatenate keys
+				res[fmt.Sprintf("%s.%s", k, mK)] = mV
+			}
+			// This is done to avoid skipping keys containing empty maps
+			if len(data) == 0 {
+				res[k] = map[string]interface{}{}
+			}
+		default:
+			res[k] = v
+		}
+	}
+	return res
+}
+
 // Merge applies multiple settings values at once.
 func (s *SettingsService) Merge(ctx context.Context, req *rpc.RawData) (*rpc.MergeResponse, error) {
 	var toMerge map[string]interface{}
 	if err := json.Unmarshal([]byte(req.GetJsonData()), &toMerge); err != nil {
 		return nil, err
 	}
 
-	if err := configuration.Settings.MergeConfigMap(toMerge); err != nil {
-		return nil, err
+	mapped := mapper(toMerge)
+
+	// Set each value individually.
+	// This is done because Viper ignores empty strings or maps when
+	// using the MergeConfigMap function.
+	for k, v := range mapped {
+		configuration.Settings.Set(k, v)
 	}
 
 	return &rpc.MergeResponse{}, nil
@@ -61,7 +93,17 @@ func (s *SettingsService) GetValue(ctx context.Context, req *rpc.GetValueRequest
 	key := req.GetKey()
 	value := &rpc.Value{}
 
-	if !configuration.Settings.InConfig(key) {
+	// Check if settings key actually existing, we don't use Viper.InConfig()
+	// since that doesn't check for keys formatted like daemon.port or those set
+	// with Viper.Set(). This way we check for all existing settings for sure.
+	keyExists := false
+	for _, k := range configuration.Settings.AllKeys() {
+		if k == key || strings.HasPrefix(k, key) {
+			keyExists = true
+			break
+		}
+	}
+	if !keyExists {
 		return nil, errors.New("key not found in settings")
 	}
 

commands/daemon/settings_test.go

@@ -48,21 +48,71 @@ func TestGetAll(t *testing.T) {
 }
 
 func TestMerge(t *testing.T) {
-	bulkSettings := `{"foo": "bar", "daemon":{"port":"420"}}`
-	_, err := svc.Merge(context.Background(), &rpc.RawData{JsonData: bulkSettings})
-	require.Nil(t, err)
+	// Verify defaults
+	require.Equal(t, "50051", configuration.Settings.GetString("daemon.port"))
+	require.Equal(t, "", configuration.Settings.GetString("foo"))
+	require.Equal(t, false, configuration.Settings.GetBool("sketch.always_export_binaries"))
+
+	bulkSettings := `{"foo": "bar", "daemon":{"port":"420"}, "sketch": {"always_export_binaries": "true"}}`
+	res, err := svc.Merge(context.Background(), &rpc.RawData{JsonData: bulkSettings})
+	require.NotNil(t, res)
+	require.NoError(t, err)
 
 	require.Equal(t, "420", configuration.Settings.GetString("daemon.port"))
 	require.Equal(t, "bar", configuration.Settings.GetString("foo"))
+	require.Equal(t, true, configuration.Settings.GetBool("sketch.always_export_binaries"))
+
+	bulkSettings = `{"foo":"", "daemon": {}, "sketch": {"always_export_binaries": "false"}}`
+	res, err = svc.Merge(context.Background(), &rpc.RawData{JsonData: bulkSettings})
+	require.NotNil(t, res)
+	require.NoError(t, err)
+
+	require.Equal(t, "50051", configuration.Settings.GetString("daemon.port"))
+	require.Equal(t, "", configuration.Settings.GetString("foo"))
+	require.Equal(t, false, configuration.Settings.GetBool("sketch.always_export_binaries"))
+
+	bulkSettings = `{"daemon": {"port":""}}`
+	res, err = svc.Merge(context.Background(), &rpc.RawData{JsonData: bulkSettings})
+	require.NotNil(t, res)
+	require.NoError(t, err)
+
+	require.Equal(t, "", configuration.Settings.GetString("daemon.port"))
+	// Verifies other values are not changed
+	require.Equal(t, "", configuration.Settings.GetString("foo"))
+	require.Equal(t, false, configuration.Settings.GetBool("sketch.always_export_binaries"))
 
 	reset()
 }
 
 func TestGetValue(t *testing.T) {
 	key := &rpc.GetValueRequest{Key: "daemon"}
 	resp, err := svc.GetValue(context.Background(), key)
-	require.Nil(t, err)
+	require.NoError(t, err)
 	require.Equal(t, `{"port":"50051"}`, resp.GetJsonData())
+
+	key = &rpc.GetValueRequest{Key: "daemon.port"}
+	resp, err = svc.GetValue(context.Background(), key)
+	require.NoError(t, err)
+	require.Equal(t, `"50051"`, resp.GetJsonData())
+}
+
+func TestGetMergedValue(t *testing.T) {
+	// Verifies value is not set
+	key := &rpc.GetValueRequest{Key: "foo"}
+	res, err := svc.GetValue(context.Background(), key)
+	require.Nil(t, res)
+	require.Error(t, err, "Error getting settings value")
+
+	// Merge value
+	bulkSettings := `{"foo": "bar"}`
+	_, err = svc.Merge(context.Background(), &rpc.RawData{JsonData: bulkSettings})
+	require.NoError(t, err)
+
+	// Verifies value is correctly returned
+	key = &rpc.GetValueRequest{Key: "foo"}
+	res, err = svc.GetValue(context.Background(), key)
+	require.NoError(t, err)
+	require.Equal(t, `"bar"`, res.GetJsonData())
 }
 
 func TestGetValueNotFound(t *testing.T) {