Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Strict validation when using SQL DB for visibility #3905

Merged
merged 1 commit into from
Feb 7, 2023

Conversation

rodrigozhou
Copy link
Collaborator

What changed?
Implement strict mode to decode/validate search attributes. Strict mode don't allow list of values for the respective types. Only KeywordList is allowed.
It will be enabled by default when using SQL DB for visibility.

Why?
For now, we're still gonna let allow it for ES until the SDK start enforcing as well.
For SQL, it's not supported at all, so adding it to validation.

How did you test it?
Added unit tests.

Potential risks
It's no-op for existing usages.

Is hotfix candidate?
No.

@rodrigozhou rodrigozhou requested a review from a team as a code owner February 4, 2023 01:52
// decodeValueTyped tries to decode to the given type.
// If the input is a list and allowList is false, then it will return only the first element.
// If the input is a list and allowList is true, then it will return the decoded list.
func decodeValueTyped[T any](value *commonpb.Payload, allowList bool) (any, error) {
Copy link
Collaborator Author

@rodrigozhou rodrigozhou Feb 4, 2023

Choose a reason for hiding this comment

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

I refactored and wrote a generic function to avoid code duplication.

common/searchattribute/encode_value.go Outdated Show resolved Hide resolved
common/searchattribute/validator.go Outdated Show resolved Hide resolved
@alexshtin
Copy link
Member

Thanks for addressing my concerns!

@rodrigozhou rodrigozhou merged commit a840d75 into temporalio:master Feb 7, 2023
@rodrigozhou rodrigozhou deleted the strict-validation-sa branch February 7, 2023 19:51
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