diff --git a/override/merge.go b/override/merge.go index 9aed9b6b9..4c27df000 100644 --- a/override/merge.go +++ b/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 { diff --git a/override/merge_dedup_test.go b/override/merge_dedup_test.go new file mode 100644 index 000000000..d1e4aff11 --- /dev/null +++ b/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) +}