lib/config: Remove "Invalid" attribute (fixes #2471)

This contains the following behavioral changes:

 - Duplicate folder IDs is now fatal during startup
 - Invalid folder flags in the ClusterConfig is fatal for the connection
   (this will go away soon with the proto changes, as we won't have any
   unknown flags any more then)
 - Empty path is a folder error reported at runtime

GitHub-Pull-Request: https://github.com/syncthing/syncthing/pull/3370

lib/config/config.go

@@ -10,6 +10,7 @@ package config
 import (
 	"encoding/json"
 	"encoding/xml"
+	"fmt"
 	"io"
 	"io/ioutil"
 	"net/url"
@@ -81,11 +82,15 @@ func ReadXML(r io.Reader, myID protocol.DeviceID) (Configuration, error) {
 	util.SetDefaults(&cfg.Options)
 	util.SetDefaults(&cfg.GUI)
 
-	err := xml.NewDecoder(r).Decode(&cfg)
+	if err := xml.NewDecoder(r).Decode(&cfg); err != nil {
+		return Configuration{}, err
+	}
 	cfg.OriginalVersion = cfg.Version
 
-	cfg.prepare(myID)
-	return cfg, err
+	if err := cfg.prepare(myID); err != nil {
+		return Configuration{}, err
+	}
+	return cfg, nil
 }
 
 func ReadJSON(r io.Reader, myID protocol.DeviceID) (Configuration, error) {
@@ -97,14 +102,16 @@ func ReadJSON(r io.Reader, myID protocol.DeviceID) (Configuration, error) {
 
 	bs, err := ioutil.ReadAll(r)
 	if err != nil {
-		return cfg, err
+		return Configuration{}, err
 	}
 
 	err = json.Unmarshal(bs, &cfg)
 	cfg.OriginalVersion = cfg.Version
 
-	cfg.prepare(myID)
-	return cfg, err
+	if err := cfg.prepare(myID); err != nil {
+		return Configuration{}, err
+	}
+	return cfg, nil
 }
 
 type Configuration struct {
@@ -154,7 +161,7 @@ func (cfg *Configuration) WriteXML(w io.Writer) error {
 	return err
 }
 
-func (cfg *Configuration) prepare(myID protocol.DeviceID) {
+func (cfg *Configuration) prepare(myID protocol.DeviceID) error {
 	util.FillNilSlices(&cfg.Options)
 
 	// Initialize any empty slices
@@ -168,19 +175,19 @@ func (cfg *Configuration) prepare(myID protocol.DeviceID) {
 		cfg.Options.AlwaysLocalNets = []string{}
 	}
 
-	// Check for missing, bad or duplicate folder ID:s
-	var seenFolders = map[string]*FolderConfiguration{}
+	// Prepare folders and check for duplicates. Duplicates are bad and
+	// dangerous, can't currently be resolved in the GUI, and shouldn't
+	// happen when configured by the GUI. We return with an error in that
+	// situation.
+	seenFolders := make(map[string]struct{})
 	for i := range cfg.Folders {
 		folder := &cfg.Folders[i]
 		folder.prepare()
 
-		if seen, ok := seenFolders[folder.ID]; ok {
-			l.Warnf("Multiple folders with ID %q; disabling", folder.ID)
-			seen.Invalid = "duplicate folder ID"
-			folder.Invalid = "duplicate folder ID"
-		} else {
-			seenFolders[folder.ID] = folder
+		if _, ok := seenFolders[folder.ID]; ok {
+			return fmt.Errorf("duplicate folder ID %q in configuration", folder.ID)
 		}
+		seenFolders[folder.ID] = struct{}{}
 	}
 
 	cfg.Options.ListenAddresses = util.UniqueStrings(cfg.Options.ListenAddresses)
@@ -257,6 +264,8 @@ func (cfg *Configuration) prepare(myID protocol.DeviceID) {
 	if cfg.GUI.APIKey == "" {
 		cfg.GUI.APIKey = rand.String(32)
 	}
+
+	return nil
 }
 
 func convertV14V15(cfg *Configuration) {

lib/config/config_test.go

@@ -595,22 +595,14 @@ func TestGUIConfigURL(t *testing.T) {
 	}
 }
 
-func TestRemoveDuplicateDevicesFolders(t *testing.T) {
-	wrapper, err := Load("testdata/duplicates.xml", device1)
+func TestDuplicateDevices(t *testing.T) {
+	// Duplicate devices should be removed
+
+	wrapper, err := Load("testdata/dupdevices.xml", device1)
 	if err != nil {
 		t.Fatal(err)
 	}
 
-	// All folders are loaded, but the duplicate ones are disabled.
-	if l := len(wrapper.Raw().Folders); l != 3 {
-		t.Errorf("Incorrect number of folders, %d != 3", l)
-	}
-	for i, f := range wrapper.Raw().Folders {
-		if f.ID == "f1" && f.Invalid == "" {
-			t.Errorf("Folder %d (%q) is not set invalid", i, f.ID)
-		}
-	}
-
 	if l := len(wrapper.Raw().Devices); l != 3 {
 		t.Errorf("Incorrect number of devices, %d != 3", l)
 	}
@@ -621,6 +613,30 @@ func TestRemoveDuplicateDevicesFolders(t *testing.T) {
 	}
 }
 
+func TestDuplicateFolders(t *testing.T) {
+	// Duplicate folders are a loading error
+
+	_, err := Load("testdata/dupfolders.xml", device1)
+	if err == nil || !strings.HasPrefix(err.Error(), "duplicate folder ID") {
+		t.Fatal(`Expected error to mention "duplicate folder ID":`, err)
+	}
+}
+
+func TestEmptyFolderPaths(t *testing.T) {
+	// Empty folder paths are allowed at the loading stage, and should not
+	// get messed up by the prepare steps (e.g., become the current dir or
+	// get a slash added so that it becomes the root directory or similar).
+
+	wrapper, err := Load("testdata/nopath.xml", device1)
+	if err != nil {
+		t.Fatal(err)
+	}
+	folder := wrapper.Folders()["f1"]
+	if folder.Path() != "" {
+		t.Errorf("Expected %q to be empty", folder.Path())
+	}
+}
+
 func TestV14ListenAddressesMigration(t *testing.T) {
 	tcs := [][3][]string{
 

lib/config/folderconfiguration.go

@@ -39,7 +39,6 @@ type FolderConfiguration struct {
 	DisableSparseFiles    bool                        `xml:"disableSparseFiles" json:"disableSparseFiles"`
 	DisableTempIndexes    bool                        `xml:"disableTempIndexes" json:"disableTempIndexes"`
 
-	Invalid    string `xml:"-" json:"invalid"` // Set at runtime when there is an error, not saved
 	cachedPath string
 
 	DeprecatedReadOnly bool `xml:"ro,attr,omitempty" json:"-"`
@@ -70,7 +69,7 @@ func (f FolderConfiguration) Path() string {
 	// This is intentionally not a pointer method, because things like
 	// cfg.Folders["default"].Path() should be valid.
 
-	if f.cachedPath == "" {
+	if f.cachedPath == "" && f.RawPath != "" {
 		l.Infoln("bug: uncached path call (should only happen in tests)")
 		return f.cleanedPath()
 	}
@@ -108,31 +107,24 @@ func (f *FolderConfiguration) DeviceIDs() []protocol.DeviceID {
 }
 
 func (f *FolderConfiguration) prepare() {
-	if len(f.RawPath) == 0 {
-		f.Invalid = "no directory configured"
-		return
-	}
+	if f.RawPath != "" {
+		// The reason it's done like this:
+		// C:          ->  C:\            ->  C:\        (issue that this is trying to fix)
+		// C:\somedir  ->  C:\somedir\    ->  C:\somedir
+		// C:\somedir\ ->  C:\somedir\\   ->  C:\somedir
+		// This way in the tests, we get away without OS specific separators
+		// in the test configs.
+		f.RawPath = filepath.Dir(f.RawPath + string(filepath.Separator))
 
-	// The reason it's done like this:
-	// C:          ->  C:\            ->  C:\        (issue that this is trying to fix)
-	// C:\somedir  ->  C:\somedir\    ->  C:\somedir
-	// C:\somedir\ ->  C:\somedir\\   ->  C:\somedir
-	// This way in the tests, we get away without OS specific separators
-	// in the test configs.
-	f.RawPath = filepath.Dir(f.RawPath + string(filepath.Separator))
-
-	// If we're not on Windows, we want the path to end with a slash to
-	// penetrate symlinks. On Windows, paths must not end with a slash.
-	if runtime.GOOS != "windows" && f.RawPath[len(f.RawPath)-1] != filepath.Separator {
-		f.RawPath = f.RawPath + string(filepath.Separator)
+		// If we're not on Windows, we want the path to end with a slash to
+		// penetrate symlinks. On Windows, paths must not end with a slash.
+		if runtime.GOOS != "windows" && f.RawPath[len(f.RawPath)-1] != filepath.Separator {
+			f.RawPath = f.RawPath + string(filepath.Separator)
+		}
 	}
 
 	f.cachedPath = f.cleanedPath()
 
-	if f.ID == "" {
-		f.ID = "default"
-	}
-
 	if f.RescanIntervalS > MaxRescanIntervalS {
 		f.RescanIntervalS = MaxRescanIntervalS
 	} else if f.RescanIntervalS < 0 {
@@ -145,6 +137,10 @@ func (f *FolderConfiguration) prepare() {
 }
 
 func (f *FolderConfiguration) cleanedPath() string {
+	if f.RawPath == "" {
+		return ""
+	}
+
 	cleaned := f.RawPath
 
 	// Attempt tilde expansion; leave unchanged in case of error

lib/config/testdata/dupdevices.xml

@@ -15,15 +15,6 @@
         <!-- duplicate, will be removed -->
         <address>192.0.2.5</address>
     </device>
-    <folder id="f1" directory="testdata/">
-        <!-- duplicate, will be disabled -->
-        <device id="AIR6LPZ-7K4PTTV-UXQSMUU-CPQ5YWH-OEDFIIQ-JUG777G-2YQXXR5-YD6AWQR"></device>
-        <device id="GYRZZQBIRNPV4T7TC52WEQYJ3TFDQW6MWDFLMU4SSSU6EMFBK2VA"></device>
-    </folder>
-    <folder id="f1" directory="testdata/">
-        <!-- duplicate, will be disabled -->
-        <device id="AIR6LPZ-7K4PTTV-UXQSMUU-CPQ5YWH-OEDFIIQ-JUG777G-2YQXXR5-YD6AWQR"></device>
-    </folder>
     <folder id="f2" directory="testdata/">
         <device id="AIR6LPZ-7K4PTTV-UXQSMUU-CPQ5YWH-OEDFIIQ-JUG777G-2YQXXR5-YD6AWQR"></device>
         <device id="GYRZZQBIRNPV4T7TC52WEQYJ3TFDQW6MWDFLMU4SSSU6EMFBK2VA"></device>

lib/config/testdata/dupfolders.xml

@@ -0,0 +1,6 @@
+<configuration version="15">
+    <folder id="f1" directory="testdata/">
+    </folder>
+    <folder id="f1" directory="testdata/">
+    </folder>
+</configuration>

lib/config/testdata/nopath.xml

@@ -0,0 +1,4 @@
+<configuration version="15">
+    <folder id="f1">
+    </folder>
+</configuration>