From 7e4583581057ffe75a9ed0f235b622a8c110ad0c Mon Sep 17 00:00:00 2001 From: Silvano Cerza Date: Tue, 2 Feb 2021 14:41:21 +0100 Subject: [PATCH 1/3] 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(). --- client_example/main.go | 7 ++-- commands/daemon/settings.go | 49 ++++++++++++++++++++++++++-- commands/daemon/settings_test.go | 55 +++++++++++++++++++++++++++++--- 3 files changed, 102 insertions(+), 9 deletions(-) diff --git a/client_example/main.go b/client_example/main.go index ab97da9bebc..ec5266b13f5 100644 --- a/client_example/main.go +++ b/client_example/main.go @@ -83,9 +83,12 @@ func main() { callGetAll(settingsClient) // Merge applies multiple settings values at once. - log.Println("calling Merge(`{\"foo\": \"bar\", \"daemon\":{\"port\":\"422\"}}`)") + log.Println("calling Merge()") callMerge(settingsClient) + log.Println("calling GetAll()") + callGetAll(settingsClient) + // Get the value of the foo key. log.Println("calling GetValue(foo)") callGetValue(settingsClient) @@ -263,7 +266,7 @@ func callUnsetProxy(client settings.SettingsClient) { } func callMerge(client settings.SettingsClient) { - bulkSettings := `{"foo": "bar", "daemon":{"port":"422"}}` + bulkSettings := `{"foo": "bar", "daemon":{"port":"422"}, "board_manager": {"additional_urls":["https://example.com"]}}` _, err := client.Merge(context.Background(), &settings.RawData{ JsonData: bulkSettings, diff --git a/commands/daemon/settings.go b/commands/daemon/settings.go index a64f6c2a272..37ac7cc6663 100644 --- a/commands/daemon/settings.go +++ b/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,6 +42,32 @@ 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 v.(type) { + case map[string]interface{}: + data := v.(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{} @@ -47,8 +75,13 @@ func (s *SettingsService) Merge(ctx context.Context, req *rpc.RawData) (*rpc.Mer 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 +94,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") } diff --git a/commands/daemon/settings_test.go b/commands/daemon/settings_test.go index 5941321ecae..e3c8b512d72 100644 --- a/commands/daemon/settings_test.go +++ b/commands/daemon/settings_test.go @@ -48,12 +48,35 @@ 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")) reset() } @@ -61,8 +84,32 @@ func TestMerge(t *testing.T) { 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) { From 85b824c9e3b16a06d38599f5c7c40f117aeb5b29 Mon Sep 17 00:00:00 2001 From: Silvano Cerza Date: Tue, 2 Feb 2021 16:21:51 +0100 Subject: [PATCH 2/3] [skip changelog] Add clearer example to client_example --- client_example/main.go | 16 ++++++++++++---- 1 file changed, 12 insertions(+), 4 deletions(-) diff --git a/client_example/main.go b/client_example/main.go index ec5266b13f5..e564c9627c5 100644 --- a/client_example/main.go +++ b/client_example/main.go @@ -84,11 +84,20 @@ func main() { // Merge applies multiple settings values at once. log.Println("calling Merge()") - callMerge(settingsClient) + 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)") callGetValue(settingsClient) @@ -265,11 +274,10 @@ func callUnsetProxy(client settings.SettingsClient) { } } -func callMerge(client settings.SettingsClient) { - bulkSettings := `{"foo": "bar", "daemon":{"port":"422"}, "board_manager": {"additional_urls":["https://example.com"]}}` +func callMerge(client settings.SettingsClient, jsonData string) { _, err := client.Merge(context.Background(), &settings.RawData{ - JsonData: bulkSettings, + JsonData: jsonData, }) if err != nil { From b59900cfda779ac194029c35e27baff00984477e Mon Sep 17 00:00:00 2001 From: Silvano Cerza Date: Tue, 2 Feb 2021 16:58:06 +0100 Subject: [PATCH 3/3] [skip changelog] Simplified some code and enhance a test --- commands/daemon/settings.go | 3 +-- commands/daemon/settings_test.go | 3 +++ 2 files changed, 4 insertions(+), 2 deletions(-) diff --git a/commands/daemon/settings.go b/commands/daemon/settings.go index 37ac7cc6663..d2e714d5108 100644 --- a/commands/daemon/settings.go +++ b/commands/daemon/settings.go @@ -50,9 +50,8 @@ func (s *SettingsService) GetAll(ctx context.Context, req *rpc.GetAllRequest) (* func mapper(toMap map[string]interface{}) map[string]interface{} { res := map[string]interface{}{} for k, v := range toMap { - switch v.(type) { + switch data := v.(type) { case map[string]interface{}: - data := v.(map[string]interface{}) for mK, mV := range mapper(data) { // Concatenate keys res[fmt.Sprintf("%s.%s", k, mK)] = mV diff --git a/commands/daemon/settings_test.go b/commands/daemon/settings_test.go index e3c8b512d72..98c01653497 100644 --- a/commands/daemon/settings_test.go +++ b/commands/daemon/settings_test.go @@ -77,6 +77,9 @@ func TestMerge(t *testing.T) { 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() }