Skip to content

fix(api): sort label keys in ValidateLabels for deterministic errors - #293

Merged
aojea merged 1 commit into
google:mainfrom
HosniBelfeki:fix/api-labels-deterministic-error
Aug 22, 2026
Merged

fix(api): sort label keys in ValidateLabels for deterministic errors#293
aojea merged 1 commit into
google:mainfrom
HosniBelfeki:fix/api-labels-deterministic-error

Conversation

@HosniBelfeki

Copy link
Copy Markdown
Contributor

Summary

  • api.ValidateLabels iterates the input map[string]string directly and returns on the first invalid key it visits. Because Go map iteration is randomized, the reported key — and therefore the diagnostic surface operators see — varies between runs. Two reproductions of the same bad config could print different keys, and a flaky unit test against a multi-bad-key input would be unreliable.
  • This change collects the keys into a slice, sorts them with sort.Strings, and walks them in that order. The reported key for any given input is now deterministic (the lexicographically smallest invalid one), and the function stays a no-op for valid input. The pattern mirrors the existing one already used for LabelsToFacts in api/datalog.go, so no new abstraction is introduced.
  • Two regression tests are added directly next to the change in api/labels_test.go:
    • TestValidateLabels_ReportsFirstErrorDeterministically calls ValidateLabels 200 times on a five-bad-key input and asserts the error always quotes the lexicographically smallest invalid key. The previous nondeterministic implementation would have leaked a different key on different iterations.
    • TestValidateLabels_AllKeysValidStaysSorted asserts the happy path is unaffected by the sort.

No new module is added to go.mod; only the stdlib sort package is used, in line with the repository's dependency policy.

Tests

$ go test -v -run TestValidateLabels ./api/...
=== RUN   TestValidateLabels
--- PASS: TestValidateLabels (0.00s)
=== RUN   TestValidateLabels_ReportsFirstErrorDeterministically
--- PASS: TestValidateLabels_ReportsFirstErrorDeterministically (0.00s)
=== RUN   TestValidateLabels_AllKeysValidStaysSorted
--- PASS: TestValidateLabels_AllKeysValidStaysSorted (0.00s)
PASS
ok      github.com/google/sam/api  0.836s

$ go test -count=1 ./api/...
ok      github.com/google/sam/api  0.856s

$ go vet ./api/...
(no findings)

$ gofmt -l api/labels.go api/labels_test.go
(clean)

The package-local test result above is the only check I can run locally without gcc/CGO-enabled race; CI on google/sam will run the full gated matrix on top of this.

@gemini-code-assist gemini-code-assist Bot left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Code Review

This pull request updates ValidateLabels to sort map keys lexicographically before validation, ensuring deterministic and stable error reporting across runs. It also adds corresponding unit tests to verify this behavior. The review feedback suggests optimizing ValidateLabels by returning early when the map is empty to avoid unnecessary allocations, and improving the determinism test by adding a second invalid key to ensure the sorting logic is robustly tested.

Comment thread api/labels.go
Comment thread api/labels_test.go
Go map iteration order is randomized, so ValidateLabels returned
whichever bad key the runtime happened to visit first. After this
change the function walks the input in lexicographic order, making
the first reported error stable across runs and easier to assert in
tests. Mirrors the same pattern already used in api/datalog.go LabelsToFacts.

Also short-circuits when the input map is empty or nil so the common
"no labels" case skips the slice allocation entirely.

The regression test now exercises a mix of valid keys and three
independently invalid keys (whitespace, comma, bang) over 200
iterations and asserts the reported error always quotes the
lexicographically smallest invalid key, proving sorting is what
controlled the choice. A second test confirms the happy path is
unaffected.
@HosniBelfeki
HosniBelfeki force-pushed the fix/api-labels-deterministic-error branch from 8652f5c to e5a8338 Compare August 22, 2026 11:41
@HosniBelfeki

HosniBelfeki commented Aug 22, 2026

Copy link
Copy Markdown
Contributor Author

Addressed the two Gemini Code Assist suggestions in the latest force-push (e5a8338):

  • ValidateLabels now short-circuits when the input map is empty or nil, so the common "no labels" case skips the slice allocation entirely.
  • The determinism regression test now exercises a mix of valid keys and three independently invalid keys (a!key, b_ bad, c,key). It still asserts the reported error always quotes the lexicographically smallest invalid key, which is "a!key" in the new input — so the assertion only passes if sort order drives the choice, not luck.

Local validations re-run after the change:

$ go test -v -count=1 -run TestValidateLabels ./api/...
=== RUN   TestValidateLabels
--- PASS: TestValidateLabels (0.00s)
=== RUN   TestValidateLabels_ReportsFirstErrorDeterministically
--- PASS: TestValidateLabels_ReportsFirstErrorDeterministically (0.00s)
=== RUN   TestValidateLabels_AllKeysValidStaysSorted
--- PASS: TestValidateLabels_AllKeysValidStaysSorted (0.00s)
PASS
ok      github.com/google/sam/api  0.776s

$ go vet ./api/...
(no findings)

$ gofmt -l api/labels.go api/labels_test.go
(clean)

@aojea

aojea commented Aug 22, 2026

Copy link
Copy Markdown
Collaborator

Thanks

@aojea
aojea merged commit cdaf83e into google:main Aug 22, 2026
18 checks passed
@HosniBelfeki
HosniBelfeki deleted the fix/api-labels-deterministic-error branch August 22, 2026 17:01
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants