Refactor node ID handling, use check digits (fixes #269)

New node ID:s contain four Luhn check digits and are grouped
differently. Code uses NodeID type instead of string, so it's formatted
homogenously everywhere.

2 files changed, 56 insertions, 125 deletions

diff --git a/config/config.go b/config/config.go
index 77c86d4783..7102016548 100644
--- a/config/config.go
+++ b/config/config.go
@@ -14,10 +14,10 @@ import (
 	"regexp"
 	"sort"
 	"strconv"
-	"strings"
 
 	"code.google.com/p/go.crypto/bcrypt"
 	"github.com/calmh/syncthing/logger"
+	"github.com/calmh/syncthing/protocol"
 	"github.com/calmh/syncthing/scanner"
 )
 
@@ -69,7 +69,7 @@ type RepositoryConfiguration struct {
 	Versioning        VersioningConfiguration `xml:"versioning"`
 	SyncOrderPatterns []SyncOrderPattern      `xml:"syncorder>pattern"`
 
-	nodeIDs []string
+	nodeIDs []protocol.NodeID
 }
 
 type VersioningConfiguration struct {
@@ -113,7 +113,7 @@ func (c *VersioningConfiguration) UnmarshalXML(d *xml.Decoder, start xml.StartEl
 	return nil
 }
 
-func (r *RepositoryConfiguration) NodeIDs() []string {
+func (r *RepositoryConfiguration) NodeIDs() []protocol.NodeID {
 	if r.nodeIDs == nil {
 		for _, n := range r.Nodes {
 			r.nodeIDs = append(r.nodeIDs, n.NodeID)
@@ -138,9 +138,9 @@ func (r RepositoryConfiguration) FileRanker() func(scanner.File) int {
 }
 
 type NodeConfiguration struct {
-	NodeID    string   `xml:"id,attr"`
-	Name      string   `xml:"name,attr,omitempty"`
-	Addresses []string `xml:"address,omitempty"`
+	NodeID    protocol.NodeID `xml:"id,attr"`
+	Name      string          `xml:"name,attr,omitempty"`
+	Addresses []string        `xml:"address,omitempty"`
 }
 
 type OptionsConfiguration struct {
@@ -174,8 +174,8 @@ type GUIConfiguration struct {
 	APIKey   string `xml:"apikey,omitempty"`
 }
 
-func (cfg *Configuration) NodeMap() map[string]NodeConfiguration {
-	m := make(map[string]NodeConfiguration, len(cfg.Nodes))
+func (cfg *Configuration) NodeMap() map[protocol.NodeID]NodeConfiguration {
+	m := make(map[protocol.NodeID]NodeConfiguration, len(cfg.Nodes))
 	for _, n := range cfg.Nodes {
 		m[n.NodeID] = n
 	}
@@ -276,7 +276,7 @@ func uniqueStrings(ss []string) []string {
 	return us
 }
 
-func Load(rd io.Reader, myID string) (Configuration, error) {
+func Load(rd io.Reader, myID protocol.NodeID) (Configuration, error) {
 	var cfg Configuration
 
 	setDefaults(&cfg)
@@ -297,15 +297,6 @@ func Load(rd io.Reader, myID string) (Configuration, error) {
 		cfg.Repositories = []RepositoryConfiguration{}
 	}
 
-	// Sanitize node IDs
-	for i := range cfg.Nodes {
-		node := &cfg.Nodes[i]
-		// Strip spaces and dashes
-		node.NodeID = strings.Replace(node.NodeID, "-", "", -1)
-		node.NodeID = strings.Replace(node.NodeID, " ", "", -1)
-		node.NodeID = strings.ToUpper(node.NodeID)
-	}
-
 	// Check for missing, bad or duplicate repository ID:s
 	var seenRepos = map[string]*RepositoryConfiguration{}
 	var uniqueCounter int
@@ -321,13 +312,6 @@ func Load(rd io.Reader, myID string) (Configuration, error) {
 			repo.ID = "default"
 		}
 
-		for i := range repo.Nodes {
-			node := &repo.Nodes[i]
-			// Strip spaces and dashes
-			node.NodeID = strings.Replace(node.NodeID, "-", "", -1)
-			node.NodeID = strings.Replace(node.NodeID, " ", "", -1)
-		}
-
 		if seen, ok := seenRepos[repo.ID]; ok {
 			l.Warnf("Multiple repositories with ID %q; disabling", repo.ID)
 
@@ -390,8 +374,9 @@ func convertV1V2(cfg *Configuration) {
 	for i, repo := range cfg.Repositories {
 		cfg.Repositories[i].ReadOnly = cfg.Options.Deprecated_ReadOnly
 		for j, node := range repo.Nodes {
-			if _, ok := nodes[node.NodeID]; !ok {
-				nodes[node.NodeID] = node
+			id := node.NodeID.String()
+			if _, ok := nodes[id]; !ok {
+				nodes[id] = node
 			}
 			cfg.Repositories[i].Nodes[j] = NodeConfiguration{NodeID: node.NodeID}
 		}
@@ -416,7 +401,7 @@ func convertV1V2(cfg *Configuration) {
 type NodeConfigurationList []NodeConfiguration
 
 func (l NodeConfigurationList) Less(a, b int) bool {
-	return l[a].NodeID < l[b].NodeID
+	return l[a].NodeID.Compare(l[b].NodeID) == -1
 }
 func (l NodeConfigurationList) Swap(a, b int) {
 	l[a], l[b] = l[b], l[a]
@@ -425,10 +410,10 @@ func (l NodeConfigurationList) Len() int {
 	return len(l)
 }
 
-func ensureNodePresent(nodes []NodeConfiguration, myID string) []NodeConfiguration {
+func ensureNodePresent(nodes []NodeConfiguration, myID protocol.NodeID) []NodeConfiguration {
 	var myIDExists bool
 	for _, node := range nodes {
-		if node.NodeID == myID {
+		if node.NodeID.Equals(myID) {
 			myIDExists = true
 			break
 		}
diff --git a/config/config_test.go b/config/config_test.go
index 58f364a2dd..cacb45fe7f 100644
--- a/config/config_test.go
+++ b/config/config_test.go
@@ -12,9 +12,19 @@ import (
 	"testing"
 
 	"github.com/calmh/syncthing/files"
+	"github.com/calmh/syncthing/protocol"
 	"github.com/calmh/syncthing/scanner"
 )
 
+var node1, node2, node3, node4 protocol.NodeID
+
+func init() {
+	node1, _ = protocol.NodeIDFromString("AIR6LPZ7K4PTTUXQSMUUCPQ5YWOEDFIIQJUG7772YQXXR5YD6AWQ")
+	node2, _ = protocol.NodeIDFromString("GYRZZQB-IRNPV4Z-T7TC52W-EQYJ3TT-FDQW6MW-DFLMU42-SSSU6EM-FBK2VAY")
+	node3, _ = protocol.NodeIDFromString("LGFPDIT-7SKNNJL-VJZA4FC-7QNCRKA-CE753K7-2BW5QDK-2FOZ7FR-FEP57QJ")
+	node4, _ = protocol.NodeIDFromString("P56IOI7-MZJNU2Y-IQGDREY-DM2MGTI-MGL3BXN-PQ6W5BM-TBBZ4TJ-XZWICQ2")
+}
+
 func TestDefaultValues(t *testing.T) {
 	expected := OptionsConfiguration{
 		ListenAddress:      []string{"0.0.0.0:22000"},
@@ -31,7 +41,7 @@ func TestDefaultValues(t *testing.T) {
 		UPnPEnabled:        true,
 	}
 
-	cfg, err := Load(bytes.NewReader(nil), "nodeID")
+	cfg, err := Load(bytes.NewReader(nil), node1)
 	if err != io.EOF {
 		t.Error(err)
 	}
@@ -45,10 +55,10 @@ func TestNodeConfig(t *testing.T) {
 	v1data := []byte(`
 <configuration version="1">
     <repository id="test" directory="~/Sync">
-        <node id="NODE1" name="node one">
+        <node id="AIR6LPZ7K4PTTUXQSMUUCPQ5YWOEDFIIQJUG7772YQXXR5YD6AWQ" name="node one">
             <address>a</address>
         </node>
-        <node id="NODE2" name="node two">
+        <node id="P56IOI7MZJNU2IQGDREYDM2MGTMGL3BXNPQ6W5BTBBZ4TJXZWICQ" name="node two">
             <address>b</address>
         </node>
     </repository>
@@ -61,20 +71,20 @@ func TestNodeConfig(t *testing.T) {
 	v2data := []byte(`
 <configuration version="2">
     <repository id="test" directory="~/Sync" ro="true">
-        <node id="NODE1"/>
-        <node id="NODE2"/>
+        <node id="AIR6LPZ7K4PTTUXQSMUUCPQ5YWOEDFIIQJUG7772YQXXR5YD6AWQ"/>
+        <node id="P56IOI7MZJNU2IQGDREYDM2MGTMGL3BXNPQ6W5BTBBZ4TJXZWICQ"/>
     </repository>
-    <node id="NODE1" name="node one">
+    <node id="AIR6LPZ7K4PTTUXQSMUUCPQ5YWOEDFIIQJUG7772YQXXR5YD6AWQ" name="node one">
         <address>a</address>
     </node>
-    <node id="NODE2" name="node two">
+    <node id="P56IOI7MZJNU2IQGDREYDM2MGTMGL3BXNPQ6W5BTBBZ4TJXZWICQ" name="node two">
         <address>b</address>
     </node>
 </configuration>
 `)
 
 	for i, data := range [][]byte{v1data, v2data} {
-		cfg, err := Load(bytes.NewReader(data), "NODE1")
+		cfg, err := Load(bytes.NewReader(data), node1)
 		if err != nil {
 			t.Error(err)
 		}
@@ -83,23 +93,23 @@ func TestNodeConfig(t *testing.T) {
 			{
 				ID:        "test",
 				Directory: "~/Sync",
-				Nodes:     []NodeConfiguration{{NodeID: "NODE1"}, {NodeID: "NODE2"}},
+				Nodes:     []NodeConfiguration{{NodeID: node1}, {NodeID: node4}},
 				ReadOnly:  true,
 			},
 		}
 		expectedNodes := []NodeConfiguration{
 			{
-				NodeID:    "NODE1",
+				NodeID:    node1,
 				Name:      "node one",
 				Addresses: []string{"a"},
 			},
 			{
-				NodeID:    "NODE2",
+				NodeID:    node4,
 				Name:      "node two",
 				Addresses: []string{"b"},
 			},
 		}
-		expectedNodeIDs := []string{"NODE1", "NODE2"}
+		expectedNodeIDs := []protocol.NodeID{node1, node4}
 
 		if cfg.Version != 2 {
 			t.Errorf("%d: Incorrect version %d != 2", i, cfg.Version)
@@ -118,18 +128,13 @@ func TestNodeConfig(t *testing.T) {
 
 func TestNoListenAddress(t *testing.T) {
 	data := []byte(`<configuration version="1">
-    <repository directory="~/Sync">
-        <node id="..." name="...">
-            <address>dynamic</address>
-        </node>
-    </repository>
     <options>
         <listenAddress></listenAddress>
     </options>
 </configuration>
 `)
 
-	cfg, err := Load(bytes.NewReader(data), "nodeID")
+	cfg, err := Load(bytes.NewReader(data), node1)
 	if err != nil {
 		t.Error(err)
 	}
@@ -142,11 +147,6 @@ func TestNoListenAddress(t *testing.T) {
 
 func TestOverriddenValues(t *testing.T) {
 	data := []byte(`<configuration version="2">
-    <repository directory="~/Sync">
-        <node id="..." name="...">
-            <address>dynamic</address>
-        </node>
-    </repository>
     <options>
        <listenAddress>:23000</listenAddress>
         <allowDelete>false</allowDelete>
@@ -180,7 +180,7 @@ func TestOverriddenValues(t *testing.T) {
 		UPnPEnabled:        false,
 	}
 
-	cfg, err := Load(bytes.NewReader(data), "nodeID")
+	cfg, err := Load(bytes.NewReader(data), node1)
 	if err != nil {
 		t.Error(err)
 	}
@@ -193,13 +193,13 @@ func TestOverriddenValues(t *testing.T) {
 func TestNodeAddresses(t *testing.T) {
 	data := []byte(`
 <configuration version="2">
-    <node id="n1">
-        <address>dynamic</address>
-    </node>
-    <node id="n2">
+    <node id="AIR6LPZ7K4PTTUXQSMUUCPQ5YWOEDFIIQJUG7772YQXXR5YD6AWQ">
         <address></address>
     </node>
-    <node id="n3">
+    <node id="GYRZZQBIRNPV4T7TC52WEQYJ3TFDQW6MWDFLMU4SSSU6EMFBK2VA">
+    </node>
+    <node id="LGFPDIT7SKNNJVJZA4FC7QNCRKCE753K72BW5QD2FOZ7FRFEP57Q">
+        <address>dynamic</address>
     </node>
 </configuration>
 `)
@@ -207,25 +207,25 @@ func TestNodeAddresses(t *testing.T) {
 	name, _ := os.Hostname()
 	expected := []NodeConfiguration{
 		{
-			NodeID:    "N1",
+			NodeID:    node1,
 			Addresses: []string{"dynamic"},
 		},
 		{
-			NodeID:    "N2",
+			NodeID:    node2,
 			Addresses: []string{"dynamic"},
 		},
 		{
-			NodeID:    "N3",
+			NodeID:    node3,
 			Addresses: []string{"dynamic"},
 		},
 		{
-			NodeID:    "N4",
+			NodeID:    node4,
 			Name:      name, // Set when auto created
 			Addresses: []string{"dynamic"},
 		},
 	}
 
-	cfg, err := Load(bytes.NewReader(data), "N4")
+	cfg, err := Load(bytes.NewReader(data), node4)
 	if err != nil {
 		t.Error(err)
 	}
@@ -235,86 +235,32 @@ func TestNodeAddresses(t *testing.T) {
 	}
 }
 
-func TestStripNodeIs(t *testing.T) {
-	data := []byte(`
-<configuration version="2">
-    <node id="AAAA-BBBB-CCCC">
-        <address>dynamic</address>
-    </node>
-    <node id="AAAA BBBB DDDD">
-        <address></address>
-    </node>
-    <node id="AAAABBBBEEEE">
-        <address></address>
-    </node>
-    <repository directory="~/Sync">
-        <node id="AAA ABBB-BCC CC" name=""></node>
-        <node id="AA-AAB BBBD-DDD" name=""></node>
-        <node id="AAA AB-BBB EEE-E" name=""></node>
-    </repository>
-</configuration>
-`)
-
-	expected := []NodeConfiguration{
-		{
-			NodeID:    "AAAABBBBCCCC",
-			Addresses: []string{"dynamic"},
-		},
-		{
-			NodeID:    "AAAABBBBDDDD",
-			Addresses: []string{"dynamic"},
-		},
-		{
-			NodeID:    "AAAABBBBEEEE",
-			Addresses: []string{"dynamic"},
-		},
-	}
-
-	cfg, err := Load(bytes.NewReader(data), "n4")
-	if err != nil {
-		t.Error(err)
-	}
-
-	for i := range expected {
-		if !reflect.DeepEqual(cfg.Nodes[i], expected[i]) {
-			t.Errorf("Nodes[%d] differ;\n  E: %#v\n  A: %#v", i, expected[i], cfg.Nodes[i])
-		}
-		if cfg.Repositories[0].Nodes[i].NodeID != expected[i].NodeID {
-			t.Errorf("Repo nodes[%d] differ;\n  E: %#v\n  A: %#v", i, expected[i].NodeID, cfg.Repositories[0].Nodes[i].NodeID)
-		}
-	}
-}
-
 func TestSyncOrders(t *testing.T) {
 	data := []byte(`
 <configuration version="2">
-    <node id="AAAA-BBBB-CCCC">
-        <address>dynamic</address>
-    </node>
     <repository directory="~/Sync">
         <syncorder>
             <pattern pattern="\.jpg$" priority="1" />
         </syncorder>
-        <node id="AAAA-BBBB-CCCC" name=""></node>
     </repository>
 </configuration>
 `)
 
 	expected := []SyncOrderPattern{
 		{
-			Pattern: "\\.jpg$",
-			Priority:  1,
+			Pattern:  "\\.jpg$",
+			Priority: 1,
 		},
 	}
 
-	cfg, err := Load(bytes.NewReader(data), "n4")
+	cfg, err := Load(bytes.NewReader(data), node1)
 	if err != nil {
 		t.Error(err)
 	}
 
 	for i := range expected {
 		if !reflect.DeepEqual(cfg.Repositories[0].SyncOrderPatterns[i], expected[i]) {
-			t.Errorf("Nodes[%d] differ;\n  E: %#v\n  A: %#v", i, expected[i], cfg.Repositories[0].SyncOrderPatterns[i])
+			t.Errorf("Patterns[%d] differ;\n  E: %#v\n  A: %#v", i, expected[i], cfg.Repositories[0].SyncOrderPatterns[i])
 		}
 	}
 }
@@ -361,9 +307,9 @@ func TestFileSorter(t *testing.T) {
 	if !reflect.DeepEqual(f, expected) {
 		t.Errorf(
 			"\n\nexpected:\n" +
-			formatFiles(expected) + "\n" +
-			"got:\n" +
-			formatFiles(f) + "\n\n",
+				formatFiles(expected) + "\n" +
+				"got:\n" +
+				formatFiles(f) + "\n\n",
 		)
 	}
 }