Fix bug to query header search attributes correctly in visibility #6163
Conversation
shijiesheng
requested review from
Shaddoll,
neil-xie,
davidporter-id-au,
Groxx,
agautam478,
jakobht,
3vilhamster,
sankari165,
dkrotx,
taylanisikdemir and
demirkayaender
as code owners
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files
... and 9 files with indirect coverage changes Continue to review full report in Codecov by Sentry.
|
| @@ -50,6 +51,8 @@ const ( | |||
| defaultDomainName = "defaultDomainName" | |||
| ) | |||
|
|
|||
| var nonAlphanumericRegex = regexp.MustCompile(`[^a-zA-Z0-9]+`) | |||
There was a problem hiding this comment.
it's better to not spread these visibility store constraints across the codebase. can you move sanitizedHeaderKey to common folder or somewhere in visibility folders?
There was a problem hiding this comment.
make sense. Now it's in the common folder
tools/cli/admin_cluster_commands.go
Outdated
| @@ -36,7 +36,7 @@ import ( | |||
|
|
|||
| // An indirection for the prompt function so that it can be mocked in the unit tests | |||
| var promptFn = prompt | |||
| var validSearchAttributeKey = regexp.MustCompile(`^[a-zA-Z][a-zA-Z_.-0-9]*$`) | |||
| var validSearchAttributeKey = regexp.MustCompile(`^[a-zA-Z][a-zA-Z_0-9]*$`) | |||
There was a problem hiding this comment.
same here. this regex should live close to other search attribute handling code in a central place.
| @@ -30,6 +30,7 @@ import ( | |||
| "github.com/uber/cadence/common" | |||
| "github.com/uber/cadence/common/backoff" | |||
| "github.com/uber/cadence/common/definition" | |||
| "github.com/uber/cadence/common/elasticsearch/validator" | |||
There was a problem hiding this comment.
elasticseach is a plugin. rest of the codebase shouldn't directly depend on this package. same search attribute constrationts apply to Pinot and other future visibility stores as well. so let's move this validator to common/visibility or somewhere generic like that
What changed?
Why?
.and this assumption exists in multiple places in the code-in the column name in where clause.How did you test it?
unit test
Potential risks
This feature only exists in pre-releases and internally it's not rolled out. Additionally, this feature is also behind a feature flag so it's very safe.
Release notes
Documentation Changes