interpolation: sort map keys for deterministic error reporting - #895
Closed
mahesh-sadupalli wants to merge 1 commit into
Closed
Conversation
Iterating over a Go map yields keys in a random order. When recursiveInterpolate processes a map-valued config node (e.g. the per-service config containing "environment", "volumes", …) and a missing required variable is encountered, the error reported depends on whichever key happens to be visited first in that run. Sort the map keys alphabetically before iterating in both Interpolate and recursiveInterpolate so that the first error reported is always the same across repeated calls with identical input. This makes `docker compose config --quiet` and `docker compose up` produce a consistent error message when required variables are absent. Add a regression test that invokes Interpolate 100 times with two map keys both referencing a missing required variable and asserts that the reported key is always the alphabetically earlier one. Fixes docker/compose#13712 Signed-off-by: Mahesh Sadupalli <mahesh.sadupalli@gmail.com>
glours
requested changes
Jul 29, 2026
Comment on lines
+131
to
+139
| func sortedKeys(m map[string]interface{}) []string { | ||
| keys := make([]string, 0, len(m)) | ||
| for k := range m { | ||
| keys = append(keys, k) | ||
| } | ||
| sort.Strings(keys) | ||
| return keys | ||
| } | ||
|
|
Collaborator
There was a problem hiding this comment.
This helper already exists in the repo: utils.MapKeys (utils/collectionutils.go) does exactly this, generically, and is already used for the same "iterate map keys deterministically" purpose in types/project.go and graph/cycle.go.
Could you drop sortedKeys (and the sort import) and use it instead?
|
|
||
| var firstErr string | ||
| for range 100 { | ||
| _, err := Interpolate(config, Options{}) |
Collaborator
There was a problem hiding this comment.
Options{} makes LookupValue fall back to os.LookupEnv, so the test silently depends on MISSING not being set in the process environment. Passing Options{LookupValue: defaultMapping} like the other tests in this file makes it hermetic.
Comment on lines
+238
to
+252
| for range 100 { | ||
| _, err := Interpolate(config, Options{}) | ||
| if err == nil { | ||
| t.Fatal("expected an interpolation error, got nil") | ||
| } | ||
| if firstErr == "" { | ||
| firstErr = err.Error() | ||
| } else if err.Error() != firstErr { | ||
| t.Fatalf("non-deterministic error: got %q, want %q", err.Error(), firstErr) | ||
| } | ||
| } | ||
| // Alphabetically first key ("aaa") must appear in the path. | ||
| if !strings.Contains(firstErr, "service.aaa") { | ||
| t.Fatalf("expected error path to contain 'service.aaa', got: %s", firstErr) | ||
| } |
Collaborator
There was a problem hiding this comment.
We can use the assert library for this. Checking the exact error message would also make the second assertion unnecessary, since it would already verify that the alphabetical ordering is correct.
Suggested change
| for range 100 { | |
| _, err := Interpolate(config, Options{}) | |
| if err == nil { | |
| t.Fatal("expected an interpolation error, got nil") | |
| } | |
| if firstErr == "" { | |
| firstErr = err.Error() | |
| } else if err.Error() != firstErr { | |
| t.Fatalf("non-deterministic error: got %q, want %q", err.Error(), firstErr) | |
| } | |
| } | |
| // Alphabetically first key ("aaa") must appear in the path. | |
| if !strings.Contains(firstErr, "service.aaa") { | |
| t.Fatalf("expected error path to contain 'service.aaa', got: %s", firstErr) | |
| } | |
| for range 100 { | |
| _, err := Interpolate(config, Options{LookupValue: defaultMapping}) | |
| assert.Error(t, err, "error while interpolating service.aaa: required variable MISSING is missing a value: required") | |
| } |
Author
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Problem
docker compose config --quiet(anddocker compose up) can report a differentmissing-required-variable error on each run when multiple config sections reference
absent required variables. For example, one run may report:
and the next may report:
Root cause:
recursiveInterpolateandInterpolateiterate overmap[string]interface{}directly. Go randomises map iteration order, so thefirst missing variable encountered varies between runs.
Fix
Sort the map keys alphabetically before iterating in both
InterpolateandrecursiveInterpolate. This adds a smallsortedKeyshelper and changes tworangeloops to range over a sorted key slice instead.The output
map[string]interface{}is unchanged — only the order in which errorsare encountered becomes deterministic. With alphabetical traversal the error now
always identifies the lexicographically first problematic path, giving users
a stable, reproducible message to act on.
Test
A new
TestInterpolateRequiredErrorIsDeterministictest invokesInterpolate100 times on a config with two map keys (
"aaa"and"zzz") both referencinga missing required variable, and asserts that every run reports the same error
and that the path contains
"aaa"(the alphabetically first key).Fixes docker/compose#13712