Rework MappingValues Sort

lib/common/data/mapping.go

@@ -72,21 +72,6 @@ func (mapping *Mapping) Data() []byte {
 	return bytes
 }
 
-// HasDuplicateKeys returns true if two keys in a mapping are identical.
-func (mapping *Mapping) HasDuplicateKeys() bool {
-	seen_values := make(map[string]bool)
-	values := mapping.Values()
-	for _, pair := range values {
-		key, _ := pair[0].Data()
-		if _, present := seen_values[key]; present {
-			return true
-		} else {
-			seen_values[key] = true
-		}
-	}
-	return false
-}
-
 // GoMapToMapping converts a Go map of unformatted strings to *Mapping.
 func GoMapToMapping(gomap map[string]string) (mapping *Mapping, err error) {
 	map_vals := MappingValues{}

lib/common/data/mapping_test.go

@@ -99,20 +99,12 @@ func TestValuesReturnsValues(t *testing.T) {
 	assert.Equal("b", val, "Values() did not return value in valid data")
 }
 
-func TestHasDuplicateKeysTrueWhenDuplicates(t *testing.T) {
+func TestReadMappingHasDuplicateKeys(t *testing.T) {
 	assert := assert.New(t)
 
-	dups, _, _ := NewMapping([]byte{0x00, 0x0c, 0x01, 0x61, 0x3d, 0x01, 0x62, 0x3b, 0x01, 0x61, 0x3d, 0x01, 0x62, 0x3b})
+	_, _, errs := NewMapping([]byte{0x00, 0x0c, 0x01, 0x61, 0x3d, 0x01, 0x62, 0x3b, 0x01, 0x61, 0x3d, 0x01, 0x62, 0x3b})
 
-	assert.Equal(true, dups.HasDuplicateKeys(), "HasDuplicateKeys() did not report true when duplicate keys present")
-}
-
-func TestHasDuplicateKeysFalseWithoutDuplicates(t *testing.T) {
-	assert := assert.New(t)
-
-	mapping, _, _ := NewMapping([]byte{0x00, 0x06, 0x01, 0x61, 0x3d, 0x01, 0x62, 0x3b})
-
-	assert.Equal(false, mapping.HasDuplicateKeys(), "HasDuplicateKeys() did not report false when no duplicate keys present")
+	assert.Equal("mapping format violation, duplicate key in mapping", errs[0].Error(), "ReadMapping should fail when duplicate keys are present.")
 }
 
 func TestGoMapToMappingProducesCorrectMapping(t *testing.T) {

lib/common/data/mapping_values.go

@@ -30,37 +30,17 @@ func ValuesToMapping(values MappingValues) *Mapping {
 	}
 }
 
-type byValue MappingValues
-
-func (set byValue) Len() int      { return len(set) }
-func (set byValue) Swap(i, j int) { set[i], set[j] = set[j], set[i] }
-func (set byValue) Less(i, j int) bool {
-	data1, _ := set[i][1].Data()
-	data2, _ := set[j][1].Data()
-	return data1 < data2
-}
-
-type byKey MappingValues
-
-func (set byKey) Len() int      { return len(set) }
-func (set byKey) Swap(i, j int) { set[i], set[j] = set[j], set[i] }
-func (set byKey) Less(i, j int) bool {
-	data1, _ := set[i][0].Data()
-	data2, _ := set[j][0].Data()
-	return data1 < data2
-}
-
-//
-// I2P Mappings require consistent order for for cryptographic signing, and sorting
-// by keys.  When new Mappings are created, they are stable sorted first by values
-// than by keys to ensure a consistent order.
-//
-
-// TODO: This sort doesn't appear to work the same as ref implementation.
-// It also appears to be unstable
+// I2P Mappings require consistent order in some cases for cryptographic signing, and sorting
+// by keys. The Mapping is sorted lexographically by keys. Duplicate keys are allowed in general,
+// but in implementations where they must be sorted like I2CP SessionConfig duplicate keys are not allowed.
+// In practice routers do not seem to allow duplicate keys.
 func mappingOrder(values MappingValues) {
-	sort.Stable(byValue(values))
-	sort.Stable(byKey(values))
+	sort.SliceStable(values, func(i, j int) bool {
+		// Lexographic sort on keys only
+		data1, _ := values[i][0].Data()
+		data2, _ := values[j][0].Data()
+		return data1 < data2
+	})
 }
 
 // ReadMappingValues returns *MappingValues from a []byte.
@@ -109,6 +89,7 @@ func ReadMappingValues(remainder []byte) (values *MappingValues, remainder_bytes
 		errs = append(errs, errors.New("warning parsing mapping: mapping length exceeds provided data"))
 	}
 
+	encounteredKeysMap := map[string]bool{}
 	// pop off length bytes before parsing kv pairs
 	remainder = remainder[2:]
 
@@ -140,6 +121,23 @@ func ReadMappingValues(remainder []byte) (values *MappingValues, remainder_bytes
 				//return
 			}
 		}
+
+		// Check if key has already been encountered in this mapping
+		keyBytes, _ := key_str.Data()
+		keyAsString := string(keyBytes)
+		_, ok := encounteredKeysMap[keyAsString]
+		if ok {
+			log.WithFields(log.Fields{
+				"at":     "(Mapping) Values",
+				"reason": "duplicate key in mapping",
+			}).Error("mapping format violation")
+			errs = append(errs, errors.New("mapping format violation, duplicate key in mapping"))
+			// Based on other implementations this does not seem to happen often?
+			// Java throws an exception in this case, the base object is a Hashmap so the value is overwritten and an exception is thrown.
+			// i2pd  as far as I can tell just overwrites the original value
+			return
+		}
+
 		if !beginsWith(remainder, 0x3d) {
 			log.WithFields(log.Fields{
 				"at":     "(Mapping) Values",
@@ -177,6 +175,9 @@ func ReadMappingValues(remainder []byte) (values *MappingValues, remainder_bytes
 		if len(remainder) == 0 {
 			break
 		}
+
+		// Store the encountered key with arbitrary data
+		encounteredKeysMap[keyAsString] = true
 	}
 	values = &map_values
 	return

lib/common/data/mapping_values_test.go

@@ -1,38 +1,46 @@
 package data
 
 import (
+	"fmt"
 	"testing"
 )
 
 func TestMappingOrderSortsValuesThenKeys(t *testing.T) {
 	a, _ := ToI2PString("a")
 	b, _ := ToI2PString("b")
+	aa, _ := ToI2PString("aa")
+	ab, _ := ToI2PString("ab")
+	ac, _ := ToI2PString("ac")
 	values := MappingValues{
 		[2]I2PString{b, b},
-		[2]I2PString{b, a},
-		[2]I2PString{a, b},
+		[2]I2PString{ac, a},
+		[2]I2PString{ab, b},
+		[2]I2PString{aa, a},
 		[2]I2PString{a, a},
 	}
 	mappingOrder(values)
 	for i, pair := range values {
 		key, _ := pair[0].Data()
-		value, _ := pair[1].Data()
 		switch i {
 		case 0:
-			if !(key == "a" && value == "a") {
-				t.Fatal("mappingOrder produced incorrect sort output at", i)
+			if !(key == "a") {
+				t.Fatal(fmt.Sprintf("mappingOrder expected key a, got %s at index", key), i)
 			}
 		case 1:
-			if !(key == "a" && value == "b") {
-				t.Fatal("mappingOrder produced incorrect sort output at", i)
+			if !(key == "aa") {
+				t.Fatal(fmt.Sprintf("mappingOrder expected key aa, got %s at index", key), i)
 			}
 		case 2:
-			if !(key == "b" && value == "a") {
-				t.Fatal("mappingOrder produced incorrect sort output at", i)
+			if !(key == "ab") {
+				t.Fatal(fmt.Sprintf("mappingOrder expected key ab, got %s at index", key), i)
 			}
 		case 3:
-			if !(key == "b" && value == "b") {
-				t.Fatal("mappingOrder produced incorrect sort output at", i)
+			if !(key == "ac") {
+				t.Fatal(fmt.Sprintf("mappingOrder expected key ac, got %s at index", key), i)
+			}
+		case 4:
+			if !(key == "b") {
+				t.Fatal(fmt.Sprintf("mappingOrder expected key b, got %s at index", key), i)
 			}
 		}
 	}