Skip to content

Commit fa478dd

Browse files
silvanocerzasilvanocerza
authored
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
1 parent b3fb43f commit fa478dd

File tree

3 files changed

+115
-12
lines changed

3 files changed

+115
-12
lines changed

Diff for: ‎client_example/main.go

+16-5
Original file line numberDiff line numberDiff line change
@@ -83,8 +83,20 @@ func main() {
8383
callGetAll(settingsClient)
8484

8585
// Merge applies multiple settings values at once.
86-
log.Println("calling Merge(`{\"foo\": \"bar\", \"daemon\":{\"port\":\"422\"}}`)")
87-
callMerge(settingsClient)
86+
log.Println("calling Merge()")
87+
callMerge(settingsClient, `{"foo": {"value": "bar"}, "daemon":{"port":"422"}, "board_manager": {"additional_urls":["https://example.com"]}}`)
88+
89+
log.Println("calling GetAll()")
90+
callGetAll(settingsClient)
91+
92+
log.Println("calling Merge()")
93+
callMerge(settingsClient, `{"foo": {} }`)
94+
95+
log.Println("calling GetAll()")
96+
callGetAll(settingsClient)
97+
98+
log.Println("calling Merge()")
99+
callMerge(settingsClient, `{"foo": "bar" }`)
88100

89101
// Get the value of the foo key.
90102
log.Println("calling GetValue(foo)")
@@ -262,11 +274,10 @@ func callUnsetProxy(client settings.SettingsClient) {
262274
}
263275
}
264276

265-
func callMerge(client settings.SettingsClient) {
266-
bulkSettings := `{"foo": "bar", "daemon":{"port":"422"}}`
277+
func callMerge(client settings.SettingsClient, jsonData string) {
267278
_, err := client.Merge(context.Background(),
268279
&settings.RawData{
269-
JsonData: bulkSettings,
280+
JsonData: jsonData,
270281
})
271282

272283
if err != nil {

Diff for: ‎commands/daemon/settings.go

+45-3
Original file line numberDiff line numberDiff line change
@@ -19,6 +19,8 @@ import (
1919
"context"
2020
"encoding/json"
2121
"errors"
22+
"fmt"
23+
"strings"
2224

2325
"github.com/arduino/arduino-cli/configuration"
2426
rpc "github.com/arduino/arduino-cli/rpc/settings"
@@ -40,15 +42,45 @@ func (s *SettingsService) GetAll(ctx context.Context, req *rpc.GetAllRequest) (*
4042
return nil, err
4143
}
4244

45+
// mapper converts a map of nested maps to a map of scalar values.
46+
// For example:
47+
// {"foo": "bar", "daemon":{"port":"420"}, "sketch": {"always_export_binaries": "true"}}
48+
// would convert to:
49+
// {"foo": "bar", "daemon.port":"420", "sketch.always_export_binaries": "true"}
50+
func mapper(toMap map[string]interface{}) map[string]interface{} {
51+
res := map[string]interface{}{}
52+
for k, v := range toMap {
53+
switch data := v.(type) {
54+
case map[string]interface{}:
55+
for mK, mV := range mapper(data) {
56+
// Concatenate keys
57+
res[fmt.Sprintf("%s.%s", k, mK)] = mV
58+
}
59+
// This is done to avoid skipping keys containing empty maps
60+
if len(data) == 0 {
61+
res[k] = map[string]interface{}{}
62+
}
63+
default:
64+
res[k] = v
65+
}
66+
}
67+
return res
68+
}
69+
4370
// Merge applies multiple settings values at once.
4471
func (s *SettingsService) Merge(ctx context.Context, req *rpc.RawData) (*rpc.MergeResponse, error) {
4572
var toMerge map[string]interface{}
4673
if err := json.Unmarshal([]byte(req.GetJsonData()), &toMerge); err != nil {
4774
return nil, err
4875
}
4976

50-
if err := configuration.Settings.MergeConfigMap(toMerge); err != nil {
51-
return nil, err
77+
mapped := mapper(toMerge)
78+
79+
// Set each value individually.
80+
// This is done because Viper ignores empty strings or maps when
81+
// using the MergeConfigMap function.
82+
for k, v := range mapped {
83+
configuration.Settings.Set(k, v)
5284
}
5385

5486
return &rpc.MergeResponse{}, nil
@@ -61,7 +93,17 @@ func (s *SettingsService) GetValue(ctx context.Context, req *rpc.GetValueRequest
6193
key := req.GetKey()
6294
value := &rpc.Value{}
6395

64-
if !configuration.Settings.InConfig(key) {
96+
// Check if settings key actually existing, we don't use Viper.InConfig()
97+
// since that doesn't check for keys formatted like daemon.port or those set
98+
// with Viper.Set(). This way we check for all existing settings for sure.
99+
keyExists := false
100+
for _, k := range configuration.Settings.AllKeys() {
101+
if k == key || strings.HasPrefix(k, key) {
102+
keyExists = true
103+
break
104+
}
105+
}
106+
if !keyExists {
65107
return nil, errors.New("key not found in settings")
66108
}
67109

Diff for: ‎commands/daemon/settings_test.go

+54-4
Original file line numberDiff line numberDiff line change
@@ -48,21 +48,71 @@ func TestGetAll(t *testing.T) {
4848
}
4949

5050
func TestMerge(t *testing.T) {
51-
bulkSettings := `{"foo": "bar", "daemon":{"port":"420"}}`
52-
_, err := svc.Merge(context.Background(), &rpc.RawData{JsonData: bulkSettings})
53-
require.Nil(t, err)
51+
// Verify defaults
52+
require.Equal(t, "50051", configuration.Settings.GetString("daemon.port"))
53+
require.Equal(t, "", configuration.Settings.GetString("foo"))
54+
require.Equal(t, false, configuration.Settings.GetBool("sketch.always_export_binaries"))
55+
56+
bulkSettings := `{"foo": "bar", "daemon":{"port":"420"}, "sketch": {"always_export_binaries": "true"}}`
57+
res, err := svc.Merge(context.Background(), &rpc.RawData{JsonData: bulkSettings})
58+
require.NotNil(t, res)
59+
require.NoError(t, err)
5460

5561
require.Equal(t, "420", configuration.Settings.GetString("daemon.port"))
5662
require.Equal(t, "bar", configuration.Settings.GetString("foo"))
63+
require.Equal(t, true, configuration.Settings.GetBool("sketch.always_export_binaries"))
64+
65+
bulkSettings = `{"foo":"", "daemon": {}, "sketch": {"always_export_binaries": "false"}}`
66+
res, err = svc.Merge(context.Background(), &rpc.RawData{JsonData: bulkSettings})
67+
require.NotNil(t, res)
68+
require.NoError(t, err)
69+
70+
require.Equal(t, "50051", configuration.Settings.GetString("daemon.port"))
71+
require.Equal(t, "", configuration.Settings.GetString("foo"))
72+
require.Equal(t, false, configuration.Settings.GetBool("sketch.always_export_binaries"))
73+
74+
bulkSettings = `{"daemon": {"port":""}}`
75+
res, err = svc.Merge(context.Background(), &rpc.RawData{JsonData: bulkSettings})
76+
require.NotNil(t, res)
77+
require.NoError(t, err)
78+
79+
require.Equal(t, "", configuration.Settings.GetString("daemon.port"))
80+
// Verifies other values are not changed
81+
require.Equal(t, "", configuration.Settings.GetString("foo"))
82+
require.Equal(t, false, configuration.Settings.GetBool("sketch.always_export_binaries"))
5783

5884
reset()
5985
}
6086

6187
func TestGetValue(t *testing.T) {
6288
key := &rpc.GetValueRequest{Key: "daemon"}
6389
resp, err := svc.GetValue(context.Background(), key)
64-
require.Nil(t, err)
90+
require.NoError(t, err)
6591
require.Equal(t, `{"port":"50051"}`, resp.GetJsonData())
92+
93+
key = &rpc.GetValueRequest{Key: "daemon.port"}
94+
resp, err = svc.GetValue(context.Background(), key)
95+
require.NoError(t, err)
96+
require.Equal(t, `"50051"`, resp.GetJsonData())
97+
}
98+
99+
func TestGetMergedValue(t *testing.T) {
100+
// Verifies value is not set
101+
key := &rpc.GetValueRequest{Key: "foo"}
102+
res, err := svc.GetValue(context.Background(), key)
103+
require.Nil(t, res)
104+
require.Error(t, err, "Error getting settings value")
105+
106+
// Merge value
107+
bulkSettings := `{"foo": "bar"}`
108+
_, err = svc.Merge(context.Background(), &rpc.RawData{JsonData: bulkSettings})
109+
require.NoError(t, err)
110+
111+
// Verifies value is correctly returned
112+
key = &rpc.GetValueRequest{Key: "foo"}
113+
res, err = svc.GetValue(context.Background(), key)
114+
require.NoError(t, err)
115+
require.Equal(t, `"bar"`, res.GetJsonData())
66116
}
67117

68118
func TestGetValueNotFound(t *testing.T) {

0 commit comments

Comments
 (0)