override: fix two merge regressions introduced in v2.15.0

cli/options.go

@@ -629,6 +629,7 @@ func (o *ProjectOptions) prepare(ctx context.Context) (*types.ConfigDetails, err
 }
 
 // ProjectFromOptions load a compose project based on command line options
+//
 // Deprecated: use ProjectOptions.LoadProject or ProjectOptions.LoadModel
 func ProjectFromOptions(ctx context.Context, options *ProjectOptions) (*types.Project, error) {
 	return options.LoadProject(ctx)

loader/mapstructure.go

@@ -44,7 +44,7 @@ func decoderHook(from reflect.Value, to reflect.Value) (interface{}, error) {
 		}
 	}
 	// If it is nil and a pointer, create and assign the target value first
-	if to.Type().Kind() == reflect.Ptr && to.IsNil() {
+	if to.Type().Kind() == reflect.Pointer && to.IsNil() {
 		to.Set(reflect.New(to.Type().Elem()))
 		u = to.Interface().(decoder)
 	}

loader/tests/labels_test.go

@@ -92,3 +92,88 @@ jobs:
 	assert.DeepEqual(t, p.Services["foo"].Labels, expected)
 	assert.DeepEqual(t, p.Jobs["foo"].Labels, expected)
 }
+
+// In a KEY=VALUE list the last entry for a key takes effect, so an override
+// file repeating a base entry after another value for the same key must still
+// win, while an entry repeated as is stays a single one. It holds for the
+// labels of every resource, not only of services.
+func TestLabelsLastEntryWinsAcrossFiles(t *testing.T) {
+	const labels = `
+    labels:
+      - mode=release
+      - keep=1
+`
+	overridden := `
+    labels:
+      - mode=debug
+      - mode=release
+      - keep=1
+`
+	base := `
+name: test
+services:
+  foo:
+    image: alpine
+` + labels + `
+jobs:
+  foo:
+    image: alpine
+    triggers:
+      manual: true
+` + labels + `
+networks:
+  foo:
+` + labels + `
+volumes:
+  foo:
+` + labels + `
+secrets:
+  foo:
+    environment: FOO
+` + labels + `
+configs:
+  foo:
+    environment: FOO
+` + labels
+	override := `
+services:
+  foo:` + overridden + `
+jobs:
+  foo:` + overridden + `
+networks:
+  foo:` + overridden + `
+volumes:
+  foo:` + overridden + `
+secrets:
+  foo:` + overridden + `
+configs:
+  foo:` + overridden
+	loadFilesAs(t, []string{base, override}, `
+name: test
+services:
+  foo:
+    image: alpine
+    labels: {mode: release, keep: "1"}
+jobs:
+  foo:
+    name: foo
+    image: alpine
+    triggers:
+      manual: true
+    labels: {mode: release, keep: "1"}
+networks:
+  foo:
+    labels: {mode: release, keep: "1"}
+volumes:
+  foo:
+    labels: {mode: release, keep: "1"}
+secrets:
+  foo:
+    environment: FOO
+    labels: {mode: release, keep: "1"}
+configs:
+  foo:
+    environment: FOO
+    labels: {mode: release, keep: "1"}
+`)
+}

override/merge.go

@@ -41,15 +41,17 @@ var mergeSpecials = map[tree.Path]merger{}
 
 func init() {
 	mergeSpecials["networks.*.ipam.config"] = mergeIPAMConfig
-	mergeSpecials["networks.*.labels"] = mergeToSequence
-	mergeSpecials["volumes.*.labels"] = mergeToSequence
+	mergeSpecials["networks.*.labels"] = mergeKeyValueSequence
+	mergeSpecials["volumes.*.labels"] = mergeKeyValueSequence
+	mergeSpecials["secrets.*.labels"] = mergeKeyValueSequence
+	mergeSpecials["configs.*.labels"] = mergeKeyValueSequence
 	for _, prefix := range []tree.Path{"services", "jobs"} {
-		mergeSpecials[prefix+".*.annotations"] = mergeToSequence
+		mergeSpecials[prefix+".*.annotations"] = mergeKeyValueSequence
 		mergeSpecials[prefix+".*.build"] = mergeBuild
-		mergeSpecials[prefix+".*.build.args"] = mergeToSequence
-		mergeSpecials[prefix+".*.build.additional_contexts"] = mergeToSequence
+		mergeSpecials[prefix+".*.build.args"] = mergeKeyValueSequence
+		mergeSpecials[prefix+".*.build.additional_contexts"] = mergeKeyValueSequence
 		mergeSpecials[prefix+".*.build.extra_hosts"] = mergeToSequence
-		mergeSpecials[prefix+".*.build.labels"] = mergeToSequence
+		mergeSpecials[prefix+".*.build.labels"] = mergeKeyValueSequence
 		mergeSpecials[prefix+".*.command"] = override
 		mergeSpecials[prefix+".*.depends_on"] = mergeDependsOn
 		mergeSpecials[prefix+".*.dns"] = mergeToSequence
@@ -58,19 +60,19 @@ func init() {
 		mergeSpecials[prefix+".*.entrypoint"] = override
 		mergeSpecials[prefix+".*.env_file"] = mergeToSequence
 		mergeSpecials[prefix+".*.label_file"] = mergeToSequence
-		mergeSpecials[prefix+".*.environment"] = mergeToSequence
+		mergeSpecials[prefix+".*.environment"] = mergeKeyValueSequence
 		mergeSpecials[prefix+".*.extra_hosts"] = mergeToSequence
 		mergeSpecials[prefix+".*.healthcheck.test"] = override
-		mergeSpecials[prefix+".*.labels"] = mergeToSequence
-		mergeSpecials[prefix+".*.volumes.*.volume.labels"] = mergeToSequence
+		mergeSpecials[prefix+".*.labels"] = mergeKeyValueSequence
+		mergeSpecials[prefix+".*.volumes.*.volume.labels"] = mergeKeyValueSequence
 		mergeSpecials[prefix+".*.logging"] = mergeLogging
 		mergeSpecials[prefix+".*.models"] = mergeModels
 		mergeSpecials[prefix+".*.networks"] = mergeNetworks
-		mergeSpecials[prefix+".*.sysctls"] = mergeToSequence
+		mergeSpecials[prefix+".*.sysctls"] = mergeKeyValueSequence
 		mergeSpecials[prefix+".*.tmpfs"] = mergeToSequence
 		mergeSpecials[prefix+".*.ulimits.*"] = mergeUlimit
 	}
-	mergeSpecials["services.*.deploy.labels"] = mergeToSequence
+	mergeSpecials["services.*.deploy.labels"] = mergeKeyValueSequence
 }
 
 // MergeYaml merges map[string]any yaml trees handling special rules
@@ -172,11 +174,11 @@ func mergeNetworks(config any, other any, path tree.Path) (any, error) {
 }
 
 func mergeAsMapping(config, other any, defaults map[string]any, path tree.Path) (any, error) {
-	right, err := convertIntoMapping(config, defaults, path)
+	right, err := convertIntoMapping(nullAsEmpty(config), defaults, path)
 	if err != nil {
 		return nil, err
 	}
-	left, err := convertIntoMapping(other, defaults, path)
+	left, err := convertIntoMapping(nullAsEmpty(other), defaults, path)
 	if err != nil {
 		return nil, err
 	}
@@ -207,6 +209,17 @@ func appendWithoutDuplicates(base []any, override []any) []any {
 	return merged
 }
 
+// mergeKeyValueSequence merges KEY=VALUE lists by key: an override entry
+// replaces the base entry of the same key in place, new keys are appended. The
+// last entry for a key takes effect, so an override repeating a base entry
+// after another value for the same key (`[MODE=debug, MODE=release]` over
+// `[MODE=release]`) must end on the repeated one, not drop it as a duplicate.
+// Keys repeated in the base collapse the same way, so that the override never
+// leaves two identical entries behind.
+func mergeKeyValueSequence(config any, other any, path tree.Path) (any, error) {
+	return uniqueEntries(slices.Concat(convertIntoSequence(config), convertIntoSequence(other)), keyValueIndexer, path)
+}
+
 func convertIntoSequence(value any) []any {
 	switch v := value.(type) {
 	case map[string]any:
@@ -295,6 +308,15 @@ func mergeIPAMConfig(config any, other any, path tree.Path) (any, error) {
 	return ipamConfigs, nil
 }
 
+// nullAsEmpty reads a null value as an empty mapping. An empty one rather than
+// nil, as merging into a nil map panics.
+func nullAsEmpty(value any) any {
+	if value == nil {
+		return map[string]any{}
+	}
+	return value
+}
+
 func convertIntoMapping(a any, defaultValue map[string]any, path tree.Path) (map[string]any, error) {
 	switch v := a.(type) {
 	case map[string]any:

override/merge_dedup_test.go

@@ -128,3 +128,142 @@ services:
 	assert.NilError(t, err)
 	assert.DeepEqual(t, once, twice)
 }
+
+// In a KEY=VALUE list the last entry for a key takes effect, so an override
+// that repeats a base entry after another value for the same key must still
+// win: the repeated entry is not a duplicate to drop, it is the final word.
+// Regression of v2.15.0, reported by docker/buildx#4117.
+func Test_mergeYamlKeyValueSequenceLastEntryWins(t *testing.T) {
+	tests := []struct {
+		name string
+		path string
+	}{
+		{name: "environment", path: "environment"},
+		{name: "labels", path: "labels"},
+		{name: "build args", path: "build:\n      args"},
+		{name: "build labels", path: "build:\n      labels"},
+		{name: "annotations", path: "annotations"},
+		{name: "sysctls", path: "sysctls"},
+		{name: "build additional contexts", path: "build:\n      additional_contexts"},
+		{name: "deploy labels", path: "deploy:\n      labels"},
+	}
+	for _, tt := range tests {
+		t.Run(tt.name, func(t *testing.T) {
+			assertMergeYaml(t, `
+services:
+  test:
+    image: foo
+    `+tt.path+`:
+      - MODE=release
+`, `
+services:
+  test:
+    `+tt.path+`:
+      - MODE=debug
+      - MODE=release
+`, `
+services:
+  test:
+    image: foo
+    `+tt.path+`:
+      - MODE=release
+`)
+		})
+	}
+}
+
+// A file may repeat a key with different values (`[MODE=debug, MODE=release]`
+// is a valid list, the last entry takes effect). Merging must compact those
+// repeats before applying the override, otherwise the override replaces only
+// the last one and leaves two identical entries, which the schema rejects.
+// Jobs are used because EnforceUnicity does not cover them and would hide it.
+func Test_mergeYamlKeyValueSequenceCompactsRepeatedBaseKeys(t *testing.T) {
+	assertMergeYaml(t, `
+jobs:
+  test:
+    image: foo
+    labels:
+      - MODE=debug
+      - MODE=release
+`, `
+jobs:
+  test:
+    labels:
+      - MODE=debug
+`, `
+jobs:
+  test:
+    image: foo
+    labels:
+      - MODE=debug
+`)
+}
+
+// Merging two empty KEY=VALUE lists yields an empty list, not a nil one: a nil
+// list is not a valid `labels` value for the schema. Secrets are used because
+// EnforceUnicity does not cover them and would rebuild the list, hiding it.
+func Test_mergeYamlKeyValueSequenceOfEmptyLists(t *testing.T) {
+	tests := []struct {
+		name     string
+		base     string
+		override string
+	}{
+		{name: "empty list over empty list", base: "[]", override: "[]"},
+		{name: "empty mapping over empty mapping", base: "{}", override: "{}"},
+	}
+	for _, tt := range tests {
+		t.Run(tt.name, func(t *testing.T) {
+			assertMergeYaml(t, `
+secrets:
+  test:
+    labels: `+tt.base+`
+`, `
+secrets:
+  test:
+    labels: `+tt.override+`
+`, `
+secrets:
+  test:
+    labels: []
+`)
+		})
+	}
+}
+
+// With interpolation disabled the merge sees raw strings, and keys are read up
+// to the first `=` as EnforceUnicity does: entries with the same text collapse,
+// at the position of the first one. Entries that could only resolve to the same
+// key once interpolated are not reconciled and stay side by side, so which of
+// them wins after interpolation depends on their position, as it already does
+// for services. Jobs are used because EnforceUnicity does not cover them.
+func Test_mergeYamlKeyValueSequenceOfUninterpolatedEntries(t *testing.T) {
+	tests := []struct {
+		name     string
+		base     string
+		override string
+		want     string
+	}{
+		{name: "same variable key", base: `"${K}=1"`, override: `"${K}=2"`, want: `"${K}=2"`},
+		{name: "default containing an equal sign", base: `"${A:-x=1}"`, override: `"${A:-x=2}"`, want: `"${A:-x=2}"`},
+		{name: "different variable keys", base: `"${K1}=1"`, override: `"${K2}=2"`, want: `"${K1}=1", "${K2}=2"`},
+		{name: "entry that is a bare variable", base: `"${ENTRY}"`, override: `"K=2"`, want: `"${ENTRY}", "K=2"`},
+		{name: "replaced at the position of the first entry", base: `"${K1}=1", "${K2}=2"`, override: `"${K1}=3"`, want: `"${K1}=3", "${K2}=2"`},
+	}
+	for _, tt := range tests {
+		t.Run(tt.name, func(t *testing.T) {
+			assertMergeYaml(t, `
+jobs:
+  test:
+    labels: [`+tt.base+`]
+`, `
+jobs:
+  test:
+    labels: [`+tt.override+`]
+`, `
+jobs:
+  test:
+    labels: [`+tt.want+`]
+`)
+		})
+	}
+}