Merge pull request #36 from ivixvi/refactor-with-copilot

Refactor

adder.go

@@ -11,98 +11,92 @@ var adderInstance *adder
 func (r *adder) Direct(scopedMap map[string]interface{}, scopedAttr string, value interface{}) (map[string]interface{}, bool) {
 	switch newValue := value.(type) {
 	case []map[string]interface{}:
-		oldSlice, ok := scopedMap[scopedAttr]
-		if !ok {
-			scopedMap[scopedAttr] = newValue
-			return scopedMap, true
-		}
-		oldMaps, ok := areEveryItemsMap(oldSlice)
-		if !ok {
-			// WARN: unexpected current value
-			scopedMap[scopedAttr] = newValue
-			return scopedMap, true
-		}
-		changed := false
-		for _, newMap := range newValue {
-			found := false
-			for _, oldMap := range oldMaps {
-				if eqMap(newMap, oldMap) {
-					found = true
-					break
-				}
-			}
-			if !found {
-				oldMaps = append(oldMaps, newMap)
-				changed = true
-			}
-		}
-		if changed {
-			scopedMap[scopedAttr] = oldMaps
-		}
-		return scopedMap, changed
+		return r.addMapSlice(scopedMap, scopedAttr, newValue)
 	case map[string]interface{}:
-		oldMap, ok := scopedMap[scopedAttr].(map[string]interface{})
-		if ok {
-			changed := false
-			scopedMap[scopedAttr], changed = mergeMap(oldMap, newValue)
-			return scopedMap, changed
-		}
-		scopedMap[scopedAttr] = value
-		return scopedMap, true
+		return r.addMap(scopedMap, scopedAttr, newValue)
 	case []interface{}:
-		oldSlice, ok := scopedMap[scopedAttr].([]interface{})
-		if !ok {
-			scopedMap[scopedAttr] = newValue
-			return scopedMap, true
+		return r.addSlice(scopedMap, scopedAttr, newValue)
+	case interface{}:
+		return r.addValue(scopedMap, scopedAttr, newValue)
+	}
+	return scopedMap, false
+}
+
+func (r *adder) addMapSlice(scopedMap map[string]interface{}, scopedAttr string, newValue []map[string]interface{}) (map[string]interface{}, bool) {
+	oldSlice, ok := scopedMap[scopedAttr]
+	if !ok {
+		scopedMap[scopedAttr] = newValue
+		return scopedMap, true
+	}
+	oldMaps, ok := areEveryItemsMap(oldSlice)
+	if !ok {
+		// WARN: unexpected current value
+		scopedMap[scopedAttr] = newValue
+		return scopedMap, true
+	}
+	changed := false
+	for _, newMap := range newValue {
+		if !containsMap(oldMaps, newMap) {
+			oldMaps = append(oldMaps, newMap)
+			changed = true
 		}
+	}
+	if changed {
+		scopedMap[scopedAttr] = oldMaps
+	}
+	return scopedMap, changed
+}
+
+func (r *adder) addMap(scopedMap map[string]interface{}, scopedAttr string, newValue map[string]interface{}) (map[string]interface{}, bool) {
+	oldMap, ok := scopedMap[scopedAttr].(map[string]interface{})
+	if ok {
 		changed := false
-		if oldMaps, ok := areEveryItemsMap(oldSlice); ok {
-			if newMaps, ok := areEveryItemsMap(newValue); ok {
-				for _, newMap := range newMaps {
-					found := false
-					for _, oldMap := range oldMaps {
-						if eqMap(newMap, oldMap) {
-							found = true
-							break
-						}
-					}
-					if !found {
-						oldMaps = append(oldMaps, newMap)
-						changed = true
-					}
-				}
-				if changed {
-					scopedMap[scopedAttr] = oldMaps
-				}
+		scopedMap[scopedAttr], changed = mergeMap(oldMap, newValue)
+		return scopedMap, changed
+	}
+	scopedMap[scopedAttr] = newValue
+	return scopedMap, true
+}
 
-				return scopedMap, changed
-			}
-		} else {
-			for _, newItem := range newValue {
-				found := false
-				for _, oldItem := range oldSlice {
-					if newItem == oldItem {
-						found = true
-						break
-					}
-				}
-				if !found {
+func (r *adder) addSlice(scopedMap map[string]interface{}, scopedAttr string, newValue []interface{}) (map[string]interface{}, bool) {
+	oldSlice, ok := scopedMap[scopedAttr].([]interface{})
+	if !ok {
+		scopedMap[scopedAttr] = newValue
+		return scopedMap, true
+	}
+	changed := false
+	if oldMaps, ok := areEveryItemsMap(oldSlice); ok {
+		if newMaps, ok := areEveryItemsMap(newValue); ok {
+			for _, newMap := range newMaps {
+				if !containsMap(oldMaps, newMap) {
+					oldMaps = append(oldMaps, newMap)
 					changed = true
-					oldSlice = append(oldSlice, newItem)
 				}
 			}
 			if changed {
-				scopedMap[scopedAttr] = oldSlice
+				scopedMap[scopedAttr] = oldMaps
 			}
+			return scopedMap, changed
 		}
-		return scopedMap, changed
-	case interface{}:
-		if oldValue, ok := scopedMap[scopedAttr]; !ok || oldValue != newValue {
-			scopedMap[scopedAttr] = value
-			return scopedMap, true
+	} else {
+		for _, newItem := range newValue {
+			if !containsItem(oldSlice, newItem) {
+				oldSlice = append(oldSlice, newItem)
+				changed = true
+			}
+		}
+		if changed {
+			scopedMap[scopedAttr] = oldSlice
 		}
 	}
+	return scopedMap, changed
+}
 
+func (r *adder) addValue(scopedMap map[string]interface{}, scopedAttr string, newValue interface{}) (map[string]interface{}, bool) {
+	if oldValue, ok := scopedMap[scopedAttr]; !ok || oldValue != newValue {
+		scopedMap[scopedAttr] = newValue
+		return scopedMap, true
+	}
 	return scopedMap, false
 }
 

export_test.go

@@ -15,3 +15,13 @@ func EqMap(m1 map[string]interface{}, m2 map[string]interface{}) bool {
 func MergeMap(m1 map[string]interface{}, m2 map[string]interface{}) (map[string]interface{}, bool) {
 	return mergeMap(m1, m2)
 }
+
+// ContainsMap is export containsMap for testing
+func ContainsMap(slice []map[string]interface{}, item map[string]interface{}) bool {
+	return containsMap(slice, item)
+}
+
+// ContainsItem is export containsItem for testing
+func ContainsItem(slice []interface{}, item interface{}) bool {
+	return containsItem(slice, item)
+}

path_specified_replace_test.go

@@ -25,13 +25,13 @@ func TestPathSpecifiedReplace(t *testing.T) {
 			op: scim.PatchOperation{
 				Op:    "replace",
 				Path:  path(`externalId`),
-				Value: "galice",
+				Value: "g-alice",
 			},
 			data: map[string]interface{}{
-				"externalId": "gbob",
+				"externalId": "g-bob",
 			},
 			expected: map[string]interface{}{
-				"externalId": "galice",
+				"externalId": "g-alice",
 			},
 			expectedChanged: true,
 		},

remover.go

@@ -47,14 +47,11 @@ func (r *remover) ByValueExpressionForAttribute(scopedMaps []map[string]interfac
 		if !isMatchExpression(oldValue, expr) {
 			newValues = append(newValues, oldValue)
 		} else {
-			_, ok := oldValue[subAttr]
-			if ok {
+			if _, ok := oldValue[subAttr]; ok {
 				changed = true
 				delete(oldValue, subAttr)
-				newValues = append(newValues, oldValue)
-			} else {
-				newValues = append(newValues, oldValue)
 			}
+			newValues = append(newValues, oldValue)
 		}
 	}
 	return newValues, changed

replacer.go

@@ -11,91 +11,78 @@ var replacerInstance *replacer
 func (r *replacer) Direct(scopedMap map[string]interface{}, scopedAttr string, value interface{}) (map[string]interface{}, bool) {
 	switch newValue := value.(type) {
 	case []map[string]interface{}:
-		oldSlice, ok := scopedMap[scopedAttr]
-		if !ok {
-			scopedMap[scopedAttr] = newValue
-			return scopedMap, true
-		}
-		oldMaps, ok := areEveryItemsMap(oldSlice)
-		if !ok {
-			// WARN: unexpected current value
-			scopedMap[scopedAttr] = newValue
-			return scopedMap, true
-		}
-		if len(oldMaps) != len(newValue) {
-			scopedMap[scopedAttr] = newValue
-			return scopedMap, true
-		}
-		for _, newMap := range newValue {
-			found := false
-			for _, oldMap := range oldMaps {
-				if eqMap(newMap, oldMap) {
-					found = true
-					break
-				}
-			}
-			if !found {
-				scopedMap[scopedAttr] = newValue
-				return scopedMap, true
-			}
-		}
-		return scopedMap, false
+		return r.replaceMapSlice(scopedMap, scopedAttr, newValue)
 	case map[string]interface{}:
-		oldMap, ok := scopedMap[scopedAttr].(map[string]interface{})
-		if ok && eqMap(newValue, oldMap) {
-			return scopedMap, false
-		}
-		scopedMap[scopedAttr] = value
-		return scopedMap, true
+		return r.replaceMap(scopedMap, scopedAttr, newValue)
 	case []interface{}:
-		oldSlice, ok := scopedMap[scopedAttr].([]interface{})
-		if !ok {
-			scopedMap[scopedAttr] = newValue
-			return scopedMap, true
-		}
-		if len(oldSlice) != len(newValue) {
+		return r.replaceSlice(scopedMap, scopedAttr, newValue)
+	case interface{}:
+		return r.replaceValue(scopedMap, scopedAttr, newValue)
+	}
+	return scopedMap, false
+}
+
+func (r *replacer) replaceMapSlice(scopedMap map[string]interface{}, scopedAttr string, newValue []map[string]interface{}) (map[string]interface{}, bool) {
+	oldSlice, ok := scopedMap[scopedAttr]
+	if !ok {
+		scopedMap[scopedAttr] = newValue
+		return scopedMap, true
+	}
+	oldMaps, ok := areEveryItemsMap(oldSlice)
+	// WARN: !ok -> unexpected current value
+	if !ok || len(oldMaps) != len(newValue) {
+		scopedMap[scopedAttr] = newValue
+		return scopedMap, true
+	}
+	for _, newMap := range newValue {
+		if !containsMap(oldMaps, newMap) {
 			scopedMap[scopedAttr] = newValue
 			return scopedMap, true
 		}
-		if oldMaps, ok := areEveryItemsMap(oldSlice); ok {
-			if newMaps, ok := areEveryItemsMap(newValue); ok {
-				for _, newMap := range newMaps {
-					found := false
-					for _, oldMap := range oldMaps {
-						if eqMap(newMap, oldMap) {
-							found = true
-							break
-						}
-					}
-					if !found {
-						scopedMap[scopedAttr] = newValue
-						return scopedMap, true
-					}
-				}
-			}
-		} else {
-			for _, newItem := range newValue {
-				found := false
-				for _, oldItem := range oldSlice {
-					if newItem == oldItem {
-						found = true
-						break
-					}
-				}
-				if !found {
+	}
+	return scopedMap, false
+}
+
+func (r *replacer) replaceMap(scopedMap map[string]interface{}, scopedAttr string, newValue map[string]interface{}) (map[string]interface{}, bool) {
+	oldMap, ok := scopedMap[scopedAttr].(map[string]interface{})
+	if ok && eqMap(newValue, oldMap) {
+		return scopedMap, false
+	}
+	scopedMap[scopedAttr] = newValue
+	return scopedMap, true
+}
+
+func (r *replacer) replaceSlice(scopedMap map[string]interface{}, scopedAttr string, newValue []interface{}) (map[string]interface{}, bool) {
+	oldSlice, ok := scopedMap[scopedAttr].([]interface{})
+	if !ok || len(oldSlice) != len(newValue) {
+		scopedMap[scopedAttr] = newValue
+		return scopedMap, true
+	}
+	if oldMaps, ok := areEveryItemsMap(oldSlice); ok {
+		if newMaps, ok := areEveryItemsMap(newValue); ok {
+			for _, newMap := range newMaps {
+				if !containsMap(oldMaps, newMap) {
 					scopedMap[scopedAttr] = newValue
 					return scopedMap, true
 				}
 			}
 		}
-		return scopedMap, false
-	case interface{}:
-		if oldValue, ok := scopedMap[scopedAttr]; !ok || oldValue != newValue {
-			scopedMap[scopedAttr] = value
-			return scopedMap, true
+	} else {
+		for _, newItem := range newValue {
+			if !containsItem(oldSlice, newItem) {
+				scopedMap[scopedAttr] = newValue
+				return scopedMap, true
+			}
 		}
 	}
+	return scopedMap, false
+}
 
+func (r *replacer) replaceValue(scopedMap map[string]interface{}, scopedAttr string, newValue interface{}) (map[string]interface{}, bool) {
+	if oldValue, ok := scopedMap[scopedAttr]; !ok || oldValue != newValue {
+		scopedMap[scopedAttr] = newValue
+		return scopedMap, true
+	}
 	return scopedMap, false
 }
 

scimpatch.go

@@ -204,5 +204,4 @@ func (p *Patcher) pathUnspecifiedOperate(
 		// unexpected input
 		return data, false, nil
 	}
-
 }

utils.go

@@ -63,3 +63,21 @@ func eqMap(m1 map[string]interface{}, m2 map[string]interface{}) bool {
 	}
 	return true
 }
+
+func containsMap(slice []map[string]interface{}, item map[string]interface{}) bool {
+	for _, v := range slice {
+		if eqMap(v, item) {
+			return true
+		}
+	}
+	return false
+}
+
+func containsItem(slice []interface{}, item interface{}) bool {
+	for _, v := range slice {
+		if v == item {
+			return true
+		}
+	}
+	return false
+}