override: sequence merge ignores strictly-identical duplicate entries

override/merge.go

@@ -19,6 +19,7 @@ package override
 import (
 	"cmp"
 	"fmt"
+	"reflect"
 	"slices"
 
 	"github.com/compose-spec/compose-go/v2/tree"
@@ -46,7 +47,7 @@ func init() {
 	mergeSpecials["services.*.build"] = mergeBuild
 	mergeSpecials["services.*.build.args"] = mergeToSequence
 	mergeSpecials["services.*.build.additional_contexts"] = mergeToSequence
-	mergeSpecials["services.*.build.extra_hosts"] = mergeExtraHosts
+	mergeSpecials["services.*.build.extra_hosts"] = mergeToSequence
 	mergeSpecials["services.*.build.labels"] = mergeToSequence
 	mergeSpecials["services.*.command"] = override
 	mergeSpecials["services.*.depends_on"] = mergeDependsOn
@@ -58,7 +59,7 @@ func init() {
 	mergeSpecials["services.*.env_file"] = mergeToSequence
 	mergeSpecials["services.*.label_file"] = mergeToSequence
 	mergeSpecials["services.*.environment"] = mergeToSequence
-	mergeSpecials["services.*.extra_hosts"] = mergeExtraHosts
+	mergeSpecials["services.*.extra_hosts"] = mergeToSequence
 	mergeSpecials["services.*.healthcheck.test"] = override
 	mergeSpecials["services.*.labels"] = mergeToSequence
 	mergeSpecials["services.*.volumes.*.volume.labels"] = mergeToSequence
@@ -96,7 +97,7 @@ func MergeYaml(e any, o any, p tree.Path) (any, error) {
 		if !ok {
 			return nil, fmt.Errorf("cannot override %s", p)
 		}
-		return append(value, other...), nil
+		return appendWithoutDuplicates(value, other), nil
 	default:
 		return o, nil
 	}
@@ -180,26 +181,28 @@ func mergeAsMapping(config, other any, defaults map[string]any, path tree.Path)
 	return mergeMappings(right, left, path)
 }
 
-func mergeExtraHosts(config any, other any, _ tree.Path) (any, error) {
+func mergeToSequence(config any, other any, _ tree.Path) (any, error) {
 	right := convertIntoSequence(config)
 	left := convertIntoSequence(other)
-	// Rewrite content of left slice to remove duplicate elements
-	i := 0
-	for _, v := range left {
-		if !slices.Contains(right, v) {
-			left[i] = v
-			i++
-		}
-	}
-	// keep only not duplicated elements from left slice
-	left = left[:i]
-	return append(right, left...), nil
+	return appendWithoutDuplicates(right, left), nil
 }
 
-func mergeToSequence(config any, other any, _ tree.Path) (any, error) {
-	right := convertIntoSequence(config)
-	left := convertIntoSequence(other)
-	return append(right, left...), nil
+// appendWithoutDuplicates appends override entries to the base sequence,
+// ignoring entries strictly identical (deep equality) to one already
+// present. Two identical entries never carry more meaning than one, while
+// they routinely break things — the same env_file applied twice, a
+// duplicate mount rejected by the engine — and dropping them makes merging
+// a value over an already-merged result idempotent. Entries that differ in
+// form (short vs long syntax of the same thing) are not equal and are kept:
+// the rule is strict identity, not equivalence.
+func appendWithoutDuplicates(base []any, override []any) []any {
+	merged := base
+	for _, v := range override {
+		if !slices.ContainsFunc(merged, func(existing any) bool { return reflect.DeepEqual(existing, v) }) {
+			merged = append(merged, v)
+		}
+	}
+	return merged
 }
 
 func convertIntoSequence(value any) []any {

override/merge_dedup_test.go

@@ -0,0 +1,130 @@
+/*
+   Copyright 2020 The Compose Specification Authors.
+
+   Licensed under the Apache License, Version 2.0 (the "License");
+   you may not use this file except in compliance with the License.
+   You may obtain a copy of the License at
+
+       http://www.apache.org/licenses/LICENSE-2.0
+
+   Unless required by applicable law or agreed to in writing, software
+   distributed under the License is distributed on an "AS IS" BASIS,
+   WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+   See the License for the specific language governing permissions and
+   limitations under the License.
+*/
+
+package override
+
+import (
+	"testing"
+
+	"gotest.tools/v3/assert"
+)
+
+// Merging sequences ignores entries strictly identical to an already-present
+// one: two identical entries never carry more meaning than one, and dropping
+// them makes merging a value over an already-merged result idempotent.
+func Test_mergeYamlSequenceDropsIdenticalDuplicates(t *testing.T) {
+	right := `
+services:
+  test:
+    image: foo
+    dns:
+      - 8.8.8.8
+    cap_add:
+      - NET_ADMIN
+    volumes:
+      - type: volume
+        source: data
+        target: /data
+`
+	left := `
+services:
+  test:
+    dns:
+      - 8.8.8.8
+      - 9.9.9.9
+    cap_add:
+      - NET_ADMIN
+      - SYS_PTRACE
+    volumes:
+      - type: volume
+        source: data
+        target: /data
+`
+	expected := `
+services:
+  test:
+    image: foo
+    dns:
+      - 8.8.8.8
+      - 9.9.9.9
+    cap_add:
+      - NET_ADMIN
+      - SYS_PTRACE
+    volumes:
+      - type: volume
+        source: data
+        target: /data
+`
+	got, err := Merge(unmarshal(t, right), unmarshal(t, left))
+	assert.NilError(t, err)
+	assert.DeepEqual(t, got, unmarshal(t, expected))
+}
+
+// Strict identity, not equivalence: the same declaration under a different
+// form (short vs long syntax) is not deduplicated — the rule never guesses.
+func Test_mergeYamlSequenceKeepsEquivalentButDistinctForms(t *testing.T) {
+	right := `
+services:
+  test:
+    volumes:
+      - data:/data
+`
+	left := `
+services:
+  test:
+    volumes:
+      - type: volume
+        source: data
+        target: /data
+`
+	got, err := Merge(unmarshal(t, right), unmarshal(t, left))
+	assert.NilError(t, err)
+	volumes := got["services"].(map[string]any)["test"].(map[string]any)["volumes"].([]any)
+	assert.Equal(t, len(volumes), 2)
+}
+
+// Merging an already-merged result with the same override again must be a
+// no-op — the property that lets a resolved model be reloaded and re-merged
+// without duplicating accumulated entries.
+func Test_mergeYamlSequenceIsIdempotent(t *testing.T) {
+	base := `
+services:
+  test:
+    dns:
+      - 8.8.8.8
+    env_file:
+      - common.env
+    extra_hosts:
+      - "host1:127.0.0.1"
+`
+	override := `
+services:
+  test:
+    dns:
+      - 9.9.9.9
+    env_file:
+      - common.env
+      - test.env
+    extra_hosts:
+      - "host1:127.0.0.1"
+      - "host2:127.0.0.2"
+`
+	once, err := Merge(unmarshal(t, base), unmarshal(t, override))
+	assert.NilError(t, err)
+	twice, err := Merge(once, unmarshal(t, override))
+	assert.NilError(t, err)
+	assert.DeepEqual(t, once, twice)
+}