Consolidated and refactored activity request validations.

[fretz12]

What changed?

Consolidated and refactored activity request validations to the chasm package.
Added additional tests for the validator.

Why?

Existing workflow activities and standalone activities should share the same validation code. Standalone activities also directly process the frontend request and therefore has additional fields to validate compared to embedded activities.

How did you test it?

• built
• run locally and tested manually
• covered by existing tests
• added new unit test(s)
• added new functional test(s)

Potential risks

This refactors the existing stack, but the validation is basically exactly copied over and all relevant tests are passing. The standalone start activity request is now cloned before sanitizing the request attributes to preserve idempotent behavior during retries, but can potentially impact performance if there are large inputs.

[dandavison]

Nice improvement -- the existing validation code was messy and complex and it's good to encapsulate it.

I have some suggestions for improving this further. Here's the API this PR introduces:

Workflow Activity

validator := activity.NewRequestAttributesValidator(...)
err := validator.ValidateAndAdjustTimeouts(runTimeout)
activityOptions := validator.GetActivityOptions()
// use activityOptions to mutate ScheduleActivityTaskCommandAttributes

Standalone Activity

validator := NewRequestAttributesValidator(...)
err = validator.ValidateAndAdjustTimeouts(durationpb.New(0))
modifiedAttributes, err := validator.ValidateStandaloneActivity()
// use modifiedAttributes to mutate StartActivityExecutionRequest

Two issues with this that I'd like to improve are

1. Someone could forget to call ValidateAndAdjustTimeouts before calling GetActivityOptions or ValidateStandaloneActivity
2. We're mutating the timeouts on the passed-in req.Options in the Standalone case; I am not sure this mutation is correct/desirable given that the request is re-used by retries.

My first idea is how about we change the API to something like

type ValidatedActivityRequest struct {
    Options                   *activitypb.ActivityOptions
    RequestID                 string
    SearchAttributesUnaliased *commonpb.SearchAttributes
}

func ValidateActivityRequest(
    activityID string,
    activityType string,
    // ...
    runTimeout *durationpb.Duration,
    standaloneAttrs *StandaloneActivityAttributes,
) (*ValidatedActivityRequest, error)

Then ValidateActivityRequest will take care of cloning input structs when necessary and adjusting the timeouts, and both code paths can use the returned struct of validated data as needed (e.g. update their own request / task attribute structs).

[fretz12]

Nice improvement -- the existing validation code was messy and complex and it's good to encapsulate it.

I have some suggestions for improving this further. Here's the API this PR introduces:

Workflow Activity

validator := activity.NewRequestAttributesValidator(...)
err := validator.ValidateAndAdjustTimeouts(runTimeout)
activityOptions := validator.GetActivityOptions()
// use activityOptions to mutate ScheduleActivityTaskCommandAttributes

Standalone Activity

validator := NewRequestAttributesValidator(...)
err = validator.ValidateAndAdjustTimeouts(durationpb.New(0))
modifiedAttributes, err := validator.ValidateStandaloneActivity()
// use modifiedAttributes to mutate StartActivityExecutionRequest

Two issues with this that I'd like to improve are

1. Someone could forget to call ValidateAndAdjustTimeouts before calling GetActivityOptions or ValidateStandaloneActivity
2. We're mutating the timeouts on the passed-in req.Options in the Standalone case; I am not sure this mutation is correct/desirable given that the request is re-used by retries.

My first idea is how about we change the API to something like

type ValidatedActivityRequest struct {
    Options                   *activitypb.ActivityOptions
    RequestID                 string
    SearchAttributesUnaliased *commonpb.SearchAttributes
}

func ValidateActivityRequest(
    activityID string,
    activityType string,
    // ...
    runTimeout *durationpb.Duration,
    standaloneAttrs *StandaloneActivityAttributes,
) (*ValidatedActivityRequest, error)

Then ValidateActivityRequest will take care of cloning input structs when necessary and adjusting the timeouts, and both code paths can use the returned struct of validated data as needed (e.g. update their own request / task attribute structs).

@dandavison - good questions, and the proto mutation is a thorn that needs improvement. We mutably sanitize the incoming request as that's what the current codebase does.

On your concerns:

1. Someone could forget to call ValidateAndAdjustTimeouts before calling GetActivityOptions or ValidateStandaloneActivity

I'm ok with that, separating out the mutated fields to a new return struct and letting the caller decide what to do. I'd rename ValidatedActivityRequest perhaps NormalizedActivityRequest as to convey the intent of mutation.

2. We're mutating the timeouts on the passed-in req.Options in the Standalone case; I am not sure this mutation is correct/desirable given that the request is re-used by retries.

Yea def a concern with the retries. If you look at StartActivityExecution in frontend.go, the request is essentially forwarded directly over to the history handler.go rpc. If we truly want to not affect retries, we should reallly clone the request with mutated fields before forwarding. The input can potentially be big. Based on your experience, do you think cloning the request will cause performance issues?

UPDATE: per discussion, we'll clone the whole request if anything needs to be mutated. The safe thing to do for predictable retries.

[dandavison]

Not a real review but a few trivial comments here!

[dandavison]

"Modified" doesn't feel like the right word to me in the return types ModifiedActivityRequestAttributes and ModifiedStandaloneActivityRequestAttributes,
partly since the returned values might not actually differ from the input, and partly because there's a mismatch between "validate" and "modified".

I think

func ValidateActivityRequestAttributes(...) (*ValidatedActivityRequestAttributes, error)

could be good.

[fretz12]

I see your point. However, ValidatedActivityRequestAttributes is ambiguous too as it seems to indicate the attributes that have been validated but not necessarily mutated. Since mutation is code critical, I'd like a more active verb. How about NormalizedActivityRequestAttributes or SanitizedActivityRequestAttributes?

[dandavison]

nit (here and elsewhere): I believe the existing codebase formatting style is:

Suggested change

	saProvider searchattribute.Provider,
	visibilityMgr manager.VisibilityManager) FrontendHandler {
	saProvider searchattribute.Provider,
	visibilityMgr manager.VisibilityManager,
	) FrontendHandler {

[fretz12]

I do like the readability, but we should automate this. gofumpt seems to do the job, but do you know of a better way?
We could use this as a prehook commit.

[dandavison]

LGTM. The main thing of course is that we don't change the behavior or performance on the workflow activity code path.

I made a bunch of style suggestions -- hopefully this is appropriate as a way for us to converge on a consistent style seeing as we're both ramping up on the codebase!

[bergundy]

Left a few very minor comments. Overall looks great!