-
Couldn't load subscription status.
- Fork 1.2k
Add CHASM Visibility registration and task processing #8491
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
base: main
Are you sure you want to change the base?
Conversation
4a8af63 to
c07d41e
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Started reviewing but I think we still don't agree on the API, so let's discuss that before and agree before implementing anything.
560562e to
c103f5f
Compare
acf6008 to
cfccb46
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Overall looks great.
chasm/registrable_component.go
Outdated
| } | ||
|
|
||
| // GetAlias returns the search attribute alias for the given field name. | ||
| func (rc *RegistrableComponent) GetAlias(field string) (string, error) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
| func (rc *RegistrableComponent) GetAlias(field string) (string, error) { | |
| func (rc *RegistrableComponent) SearchAttributeAlias(field string) (string, error) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Also replied in the other comment, I think the only reason to have the SearchAttribute prefix in this function name is because it's making RegistrableComponent implement the VisibilitySearchAttributesMapper interface, which I'm not sure I like this approach.
I'd rather if RegistrableComponent had an actual var for the mapper. In fact, I'm wondering if the interface is even needed.
chasm/visibility.go
Outdated
| GetAlias(field string) (string, error) | ||
| GetField(alias string) (string, error) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
| GetAlias(field string) (string, error) | |
| GetField(alias string) (string, error) | |
| SearchAttributeAlias(field string) (string, error) | |
| SearchAttributeField(alias string) (string, error) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Isn't it redundant to have functions here prefixed with SearchAttribute since the interface is already called VisibilitySearchAttributesMapper? I think GetAlias/GetFieldName are in this case.
chasm/registrable_component.go
Outdated
| for _, sa := range searchAttributes { | ||
| alias := sa.getAlias() | ||
| field := sa.getField() | ||
| valueType := sa.getValueType() | ||
|
|
||
| rc.aliasToField[alias] = field | ||
| rc.fieldToAlias[field] = alias | ||
| rc.saTypeMap[field] = valueType | ||
| } |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
You cannot create these maps before validating. It's possible to create an invalid mapping that passes the validation. Example: Foo -> Field1, Foo -> Field2, Bar -> Field1
chasm/visibility.go
Outdated
| GetAlias(field string) (string, error) | ||
| GetField(alias string) (string, error) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Isn't it redundant to have functions here prefixed with SearchAttribute since the interface is already called VisibilitySearchAttributesMapper? I think GetAlias/GetFieldName are in this case.
chasm/registrable_component.go
Outdated
| } | ||
|
|
||
| // GetAlias returns the search attribute alias for the given field name. | ||
| func (rc *RegistrableComponent) GetAlias(field string) (string, error) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Also replied in the other comment, I think the only reason to have the SearchAttribute prefix in this function name is because it's making RegistrableComponent implement the VisibilitySearchAttributesMapper interface, which I'm not sure I like this approach.
I'd rather if RegistrableComponent had an actual var for the mapper. In fact, I'm wondering if the interface is even needed.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is already very close.
Please document all exported fields from the chasm package.
| func (v *VisibilitySearchAttributesMapper) GetAlias(field string) (string, error) { | ||
| if v == nil { | ||
| return "", serviceerror.NewInvalidArgument("visibility search attributes mapper is not registered") | ||
| } | ||
| alias, ok := v.aliasToField[field] | ||
| if !ok { | ||
| return "", serviceerror.NewInvalidArgument(fmt.Sprintf("visibility search attributes mapper has no registered field %s", field)) | ||
| } | ||
| return alias, nil | ||
| } | ||
|
|
||
| func (v *VisibilitySearchAttributesMapper) GetField(alias string) (string, error) { | ||
| if v == nil { | ||
| return "", serviceerror.NewInvalidArgument("visibility search attributes mapper is not registered") | ||
| } | ||
| field, ok := v.fieldToAlias[alias] | ||
| if !ok { | ||
| return "", serviceerror.NewInvalidArgument(fmt.Sprintf("visibility search attributes mapper has no registered alias %s", alias)) | ||
| } | ||
| return field, nil | ||
| } | ||
|
|
||
| func (v *VisibilitySearchAttributesMapper) GetFieldValueType(field string) (enumspb.IndexedValueType, error) { | ||
| if v == nil { | ||
| return enumspb.INDEXED_VALUE_TYPE_UNSPECIFIED, serviceerror.NewInvalidArgument("visibility search attributes mapper is not registered") | ||
| } | ||
| saType, ok := v.saTypeMap[field] | ||
| if !ok { | ||
| return enumspb.INDEXED_VALUE_TYPE_UNSPECIFIED, serviceerror.NewInvalidArgument(fmt.Sprintf("visibility search attributes mapper has no registered field %s", field)) | ||
| } | ||
| return saType, nil | ||
| } |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why do you need to export these methods? And please don't use Get as a prefix for getters in Go.
| _ SearchAttribute = (*SearchAttributeKeywordList)(nil) | ||
| ) | ||
|
|
||
| type ( |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
nit: don't group all of the types together. The practice in Go is to put the type definition above its constructor followed by all of the methods.
| // alias refers to the user defined name of the search attribute | ||
| Alias string | ||
| // field refers to a fully formed schema field, which is either a Predefined or CHASM search attribute | ||
| Field string | ||
| ValueType enumspb.IndexedValueType |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Do these fields need to be exported? They're defined on a struct that isn't exported.
| ) | ||
|
|
||
| var ( | ||
| TestKeywordSearchAttribute = chasm.NewSearchAttributeKeyword(TestKeywordSAFieldName, chasm.SearchAttributeFieldKeyword01) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We need another search attribute to test aliasing.
Also, don't use TemporalScheduledById as the mapped attribute.
You're going to want:
- Define
SearchAttributeTemporalScheduledById = newSearchAttributeKeywordByField(searchattribute.TemporalScheduledById)inchasm/search_attribute.go - Define a separate search attribute that will map to
SearchAttributeFieldKeyword01for testing aliasing - Register both with the component
- Emit both from the component
- Test both in functional tests
What changed?
Adds CHASM search attribute provider and mapper implementation. To use CHASM search attributes, embed your component with ComponentSearchAttributesProvider and register the search attributes to the CHASM Registry.
To define a component search attribute, define a SearchAttribute as a package/global scoped variable (CHASM search attributes should reference exported variables like
SearchAttributeFieldInt01,SearchAttributeFieldDateTime01:To update SearchAttributes during an execution of a CHASM archetype, implement the provider
SearchAttributesin the root component, and call theNewValuemethod on a SearchAttribute within the Provider method to generate a new SearchAttributeValue. On SearchAttribute updates during CHASM transactions, the framework will detect changes and submit a Visibility task for persistence.To register the search attribute with the CHASM Registry,
WithSearchAttributesmust be passed as a RegistrableComponentOption to the root component of the library. This is required to support Visibility queries.Why?
Required to support CHASM Search Attributes.
How did you test it?
Potential risks
TBD