Skip to content

Commit

Permalink
bugfix: bad dynamicconfig filter/string mapping (cadence-workflow#6151)
Browse files Browse the repository at this point in the history
This logic desperately needs to be refactored, it's incredibly error-prone :\
We should probably just use enumer's codegen tbh.  Or something similar.

Previously `String()` missed both `workflowType` and `ratelimitKey`.
The `String()` impl is now rewritten so it won't be missed with future additions, and there's a test to check it too.
  • Loading branch information
Groxx authored Jul 2, 2024
1 parent b5a79d2 commit ca824a0
Show file tree
Hide file tree
Showing 2 changed files with 14 additions and 3 deletions.
11 changes: 11 additions & 0 deletions common/dynamicconfig/config_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -346,6 +346,17 @@ func TestDynamicConfigFilterTypeIsParseable(t *testing.T) {
}
}

func TestDynamicConfigFilterStringsCorrectly(t *testing.T) {
for _, filterString := range filters {
// filter-string-parsing and the resulting filter's String() must match
parsed := ParseFilter(filterString)
assert.Equal(t, filterString, parsed.String(), "filters need to String() correctly as some impls rely on it")
}
// should not be possible normally, but improper casting could trigger it
badFilter := Filter(len(filters))
assert.Equal(t, UnknownFilter.String(), badFilter.String(), "filters with indexes outside the list of known strings should String() to the unknown filter type")
}

func BenchmarkLogValue(b *testing.B) {
keys := []Key{
HistorySizeLimitError,
Expand Down
6 changes: 3 additions & 3 deletions common/dynamicconfig/filter.go
Original file line number Diff line number Diff line change
Expand Up @@ -31,10 +31,10 @@ import (
type Filter int

func (f Filter) String() string {
if f <= UnknownFilter || f > WorkflowID {
return filters[UnknownFilter]
if f > UnknownFilter && int(f) < len(filters) {
return filters[f]
}
return filters[f]
return filters[UnknownFilter]
}

func ParseFilter(filterName string) Filter {
Expand Down

0 comments on commit ca824a0

Please sign in to comment.