Tolerate errors in invalid config files, and report them as warnings

arduino · cmaglie · May 15, 2024 · Mar 13, 2024 · Mar 13, 2024 · Mar 13, 2024
commit 27e9c3e4dcb31961ca60911032b89bb90c59a642
diff --git a/commands/service_settings.go b/commands/service_settings.go
@@ -22,7 +22,9 @@ import (
 	"reflect"
 
 	"github.com/arduino/arduino-cli/commands/cmderrors"
+	f "github.com/arduino/arduino-cli/internal/algorithms"
 	"github.com/arduino/arduino-cli/internal/cli/configuration"
+	"github.com/arduino/arduino-cli/internal/go-configmap"
 	rpc "github.com/arduino/arduino-cli/rpc/cc/arduino/cli/commands/v1"
 	"google.golang.org/protobuf/proto"
 	"gopkg.in/yaml.v3"
@@ -187,23 +189,29 @@ func (s *arduinoCoreServerImpl) ConfigurationSave(ctx context.Context, req *rpc.
 
 // SettingsReadFromFile read settings from a YAML file and replace the settings currently stored in memory.
 func (s *arduinoCoreServerImpl) ConfigurationOpen(ctx context.Context, req *rpc.ConfigurationOpenRequest) (*rpc.ConfigurationOpenResponse, error) {
+	warnings := []string{}
+
 	switch req.GetSettingsFormat() {
 	case "yaml":
 		err := yaml.Unmarshal([]byte(req.GetEncodedSettings()), s.settings)
-		if err != nil {
+		if errs, ok := err.(*configmap.UnmarshalErrors); ok {
+			warnings = f.Map(errs.WrappedErrors(), (error).Error)
+		} else if err != nil {
 			return nil, fmt.Errorf("error unmarshalling settings: %v", err)
 		}
 	case "json":
 		err := json.Unmarshal([]byte(req.GetEncodedSettings()), s.settings)
-		if err != nil {
+		if errs, ok := err.(*configmap.UnmarshalErrors); ok {
+			warnings = f.Map(errs.WrappedErrors(), (error).Error)
+		} else if err != nil {
 			return nil, fmt.Errorf("error unmarshalling settings: %v", err)
 		}
 	default:
 		return nil, &cmderrors.InvalidArgumentError{Message: fmt.Sprintf("unsupported format: %s", req.GetSettingsFormat())}
 	}
 
 	configuration.InjectEnvVars(s.settings)
-	return &rpc.ConfigurationOpenResponse{}, nil
+	return &rpc.ConfigurationOpenResponse{Warnings: warnings}, nil
 }
 
 // SettingsEnumerate returns the list of all the settings keys.

diff --git a/internal/go-configmap/errors.go b/internal/go-configmap/errors.go
@@ -0,0 +1,54 @@
+// This file is part of arduino-cli.
+//
+// Copyright 2024 ARDUINO SA (http://www.arduino.cc/)
+//
+// This software is released under the GNU General Public License version 3,
+// which covers the main part of arduino-cli.
+// The terms of this license can be found at:
+// https://www.gnu.org/licenses/gpl-3.0.en.html
+//
+// You can be released from the requirements of the above licenses by purchasing
+// a commercial license. Buying such a license is mandatory if you want to
+// modify or otherwise use the software for commercial activities involving the
+// Arduino software without disclosing the source code of your own applications.
+// To purchase a commercial license, send an email to license@arduino.cc.
+
+package configmap
+
+import "strings"
+
+// UnmarshalErrors is a collection of errors that occurred during unmarshalling.
+// Do not return this type directly, but use its result() method instead.
+type UnmarshalErrors struct {
+	wrapped []error
+}
+
+func (e *UnmarshalErrors) append(err error) {
+	e.wrapped = append(e.wrapped, err)
+}
+
+func (e *UnmarshalErrors) result() error {
+	if len(e.wrapped) == 0 {
+		return nil
+	}
+	return e
+}
+
+func (e *UnmarshalErrors) Error() string {
+	if len(e.wrapped) == 1 {
+		return e.wrapped[0].Error()
+	}
+	errs := []string{"multiple errors:"}
+	for _, err := range e.wrapped {
+		errs = append(errs, "- "+err.Error())
+	}
+	return strings.Join(errs, "\n")
+}
+
+// WrappedErrors returns the list of errors that occurred during unmarshalling.
+func (e *UnmarshalErrors) WrappedErrors() []error {
+	if e == nil {
+		return nil
+	}
+	return e.wrapped
+}
diff --git a/internal/go-configmap/yaml.go b/internal/go-configmap/yaml.go
@@ -29,10 +29,11 @@ func (c *Map) UnmarshalYAML(node *yaml.Node) error {
 		return err
 	}
 
+	errs := &UnmarshalErrors{}
 	for k, v := range flattenMap(in) {
 		if err := c.Set(k, v); err != nil {
-			return err
+			errs.append(err)
 		}
 	}
-	return nil
+	return errs.result()
 }
diff --git a/internal/integrationtest/config/config_test.go b/internal/integrationtest/config/config_test.go
@@ -17,6 +17,7 @@ package config_test
 
 import (
 	"path/filepath"
+	"strings"
 	"testing"
 
 	"github.com/arduino/arduino-cli/internal/integrationtest"
@@ -867,3 +868,29 @@ func TestInitializationOrderOfConfigThroughFlagAndEnv(t *testing.T) {
 	require.NoError(t, err)
 	requirejson.Contains(t, stdout, `{"config":{ "locale": "test" }}`)
 }
+
+func TestConfigLoadWithUnknownOrInvalidKeys(t *testing.T) {
+	env, cli := integrationtest.CreateArduinoCLIWithEnvironment(t)
+	defer env.CleanUp()
+
+	tmp := t.TempDir()
+	unkwnownConfig := paths.New(filepath.Join(tmp, "unknown.yaml"))
+	unkwnownConfig.WriteFile([]byte(`
+unk: "test"
+build.unk: 123
+`))
+	t.Cleanup(func() { unkwnownConfig.Remove() })
+
+	// Run "config get" with a configuration containing an unknown key
+	out, _, err := cli.Run("config", "get", "locale", "--config-file", unkwnownConfig.String())
+	require.NoError(t, err)
+	require.Equal(t, "en", strings.TrimSpace(string(out)))
+
+	// Run "config get" with a configuration containing an invalid key
+	invalidConfig := paths.New(filepath.Join(tmp, "invalid.yaml"))
+	invalidConfig.WriteFile([]byte(`locale: 123`))
+	t.Cleanup(func() { invalidConfig.Remove() })
+	out, _, err = cli.Run("config", "get", "locale", "--config-file", invalidConfig.String())
+	require.NoError(t, err)
+	require.Equal(t, "en", strings.TrimSpace(string(out)))
+}
diff --git a/main.go b/main.go
@@ -45,8 +45,12 @@ func main() {
 	} else if !os.IsNotExist(err) {
 		feedback.FatalError(fmt.Errorf("couldn't read configuration file: %w", err), feedback.ErrGeneric)
 	}
-	if _, err := srv.ConfigurationOpen(ctx, openReq); err != nil {
+	if resp, err := srv.ConfigurationOpen(ctx, openReq); err != nil {
 		feedback.FatalError(fmt.Errorf("couldn't load configuration: %w", err), feedback.ErrGeneric)
+	} else if warnings := resp.GetWarnings(); len(warnings) > 0 {
+		for _, warning := range warnings {
+			feedback.Warning(warning)
+		}
 	}
 
 	// Get the current settings from the server

diff --git a/rpc/cc/arduino/cli/commands/v1/settings.pb.go b/rpc/cc/arduino/cli/commands/v1/settings.pb.go
diff --git a/rpc/cc/arduino/cli/commands/v1/settings.proto b/rpc/cc/arduino/cli/commands/v1/settings.proto
@@ -120,7 +120,11 @@ message ConfigurationOpenRequest {
   string settings_format = 2;
 }
 
-message ConfigurationOpenResponse {}
+message ConfigurationOpenResponse {
+  // Warnings that occurred while opening the configuration (e.g. unknown keys,
+  // or invalid values)
+  repeated string warnings = 1;
+}
 
 message SettingsGetValueRequest {
   // The key to get