From 2086ed68a2deaad1afe1ee0a0be4c5dca5b96ac9 Mon Sep 17 00:00:00 2001 From: Nicolas De Loof Date: Mon, 31 Aug 2026 10:03:12 +0200 Subject: [PATCH] feat(override): sequence merge ignores strictly-identical duplicate entries MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Merging sequences appended blindly: the same env_file declared in the base file and an override was applied twice, an identical mount or device entry was duplicated and rejected by the engine at create time. Two strictly identical entries never carry more meaning than one. Both merge regimes now drop an override entry deeply equal to one already present — the registered sequence merges and the default list merge — which also folds mergeExtraHosts (the one merger that already deduplicated) into mergeToSequence. Entries that differ in form (short vs long syntax of the same declaration) are not equal and are kept: the rule is strict identity, never equivalence. Consequence pinned by test: merging a value over an already-merged result is idempotent, so a resolved model can be reloaded and merged again without duplicating accumulated entries. Signed-off-by: Nicolas De Loof --- override/merge.go | 41 ++++++----- override/merge_dedup_test.go | 130 +++++++++++++++++++++++++++++++++++ 2 files changed, 152 insertions(+), 19 deletions(-) create mode 100644 override/merge_dedup_test.go 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) +}