internal/relui, internal/workflow: add parameter type support

Previously, it was only possible to create workflow parameters
with the implicit "string" type hard-coded. Some workflows we're
creating either require or will benefit from more flexibility
in parameter types (slices of strings, single-line vs multi-line
strings, and so on). It was also not yet possible to associate
metadata to parameters (such as documentation, example values).

This change implements that flexibility for workflow parameters,
and uses it to better document the existing Twitter workflows.
The next change will add a workflow that uses new slice types.

For simplicity and clarity reasons, all parameter information
is contained in one place in the workflow.Parameter struct,
including some fields that control the HTML presentation of
said parameters, instead of trying to factor out HTML bits
into the relui package and creating a bridge between the two.

Also type check in more stages of the workflow processing.

For golang/go#47405.
Fixes golang/go#51191.

Change-Id: Ia805b3b355e65fcbf2397ad21800da448ccb826a
Reviewed-on: https://go-review.googlesource.com/c/build/+/404454
Reviewed-by: Dmitri Shuralyov <dmitshur@google.com>
Run-TryBot: Dmitri Shuralyov <dmitshur@golang.org>
Auto-Submit: Dmitri Shuralyov <dmitshur@golang.org>
Reviewed-by: Heschi Kreinick <heschi@google.com>
TryBot-Result: Gopher Robot <gobot@golang.org>
This commit is contained in:
Dmitri Shuralyov 2022-04-22 17:36:11 -04:00 коммит произвёл Gopher Robot
Родитель 5d48fa5aa7
Коммит fb638e1ffb
10 изменённых файлов: 293 добавлений и 66 удалений

Просмотреть файл

@ -60,7 +60,7 @@ func (l *PGListener) TaskStateChanged(workflowID uuid.UUID, taskName string, sta
}
// WorkflowStarted persists a new workflow execution in the database.
func (l *PGListener) WorkflowStarted(ctx context.Context, workflowID uuid.UUID, name string, params map[string]string) error {
func (l *PGListener) WorkflowStarted(ctx context.Context, workflowID uuid.UUID, name string, params map[string]interface{}) error {
q := db.New(l.db)
m, err := json.Marshal(params)
if err != nil {

Просмотреть файл

@ -17,3 +17,41 @@
registerTaskListExpandListeners(".TaskList-expandableItem")
})()
// addSliceRow creates and appends a row to a slice parameter
// for filling in an element.
//
// container is the parameter container element.
// paramName is the parameter name.
// element is the element tag to create, and inputType is its type attribute if element is "input".
// paramExample is an example value for the parameter.
addSliceRow = (container, paramName, element, inputType, paramExample) => {
/*
Create an input element, a button to remove it, group them in a "parameterRow" div:
<div class="NewWorkflow-parameterRow">
<input name="workflow.params.{{$p.Name}}" placeholder="{{paramExample}}" />
<button title="Remove this row from the slice." onclick="/ * Remove this row. * /">-</button>
</div>
*/
const input = document.createElement(element)
input.name = "workflow.params." + paramName
if (element == "input") {
input.type = inputType
}
input.placeholder = paramExample
const removeButton = document.createElement("button")
removeButton.title = "Remove this row from the slice."
removeButton.addEventListener("click", (e) => {
e.preventDefault()
container.removeChild(div)
})
removeButton.appendChild(document.createTextNode("-"))
const div = document.createElement("div")
div.className = "NewWorkflow-parameterRow";
div.appendChild(input)
div.appendChild(removeButton)
// Add the "parameterRow" div to the document.
container.appendChild(div)
}

Просмотреть файл

@ -111,9 +111,24 @@ h6 {
display: flex;
gap: 0.5rem;
}
.NewWorkflow-parameter input {
.NewWorkflow-parameter--slice {
flex-direction: column;
}
.NewWorkflow-parameterRow {
display: flex;
gap: 0.5rem;
}
.NewWorkflow-parameter--string input {
flex-grow: 1;
}
.NewWorkflow-parameter--slice textarea {
font-family: inherit;
width: 100%;
height: 4rem;
}
.NewWorkflow-parameter--slice button {
font-size: 0.625rem;
}
.NewWorkflow-workflowCreate {
padding-top: 0.5rem;
border-top: 0.0625rem solid #d6d6d6;

Просмотреть файл

@ -23,14 +23,30 @@
{{if .Selected}}
<form action="{{baseLink "/workflows/create"}}" method="post">
<input type="hidden" id="workflow.name" name="workflow.name" value="{{$.Name}}" />
{{range $name := .Selected.ParameterNames}}
<div class="NewWorkflow-parameter">
<label for="workflow.params.{{$name}}">{{$name}}</label>
<input id="workflow.params.{{$name}}" name="workflow.params.{{$name}}" value="" />
</div>
{{range $_, $p := .Selected.Parameters}}
{{if eq $p.Type.String "string"}}
<div class="NewWorkflow-parameter NewWorkflow-parameter--string">
<label for="workflow.params.{{$p.Name}}" title="{{$p.Doc}}">{{$p.Name}}</label>
<input id="workflow.params.{{$p.Name}}" name="workflow.params.{{$p.Name}}"
{{- with $p.HTMLInputType}} type="{{.}}"{{end}}
{{- if $p.RequireNonZero}} required{{end}} placeholder="{{$p.Example}}" />
</div>
{{else if eq $p.Type.String "[]string"}}
<div class="NewWorkflow-parameter NewWorkflow-parameter--slice">
<div class="NewWorkflow-parameterRow">
<label title="{{$p.Doc}}">{{$p.Name}}</label>
<button title="Increment the slice length." onclick="event.preventDefault(); addSliceRow(this.parentElement.parentElement, '{{$p.Name}}', '{{$p.HTMLElement}}', '{{$p.HTMLInputType}}', '{{$p.Example}}');">+</button>
</div>
</div>
{{else}}
<div class="NewWorkflow-parameter">
<label title="{{$p.Doc}}">{{$p.Name}}</label>
<span>unsupported parameter type "{{$p.ParameterType}}"</span>
</div>
{{end}}
{{end}}
<div class="NewWorkflow-workflowCreate">
<input name="workflow.create" type="submit" value="Create" onclick="return confirm('This will create and immediately run this workflow.\n\nReady to proceed?')" />
<input name="workflow.create" type="submit" value="Create" onclick="return this.form.reportValidity() && confirm('This will create and immediately run this workflow.\n\nReady to proceed?')" />
</div>
</form>
{{end}}

Просмотреть файл

@ -220,14 +220,25 @@ func (s *Server) createWorkflowHandler(w http.ResponseWriter, r *http.Request) {
http.Error(w, http.StatusText(http.StatusBadRequest), http.StatusBadRequest)
return
}
params := make(map[string]string)
for _, n := range d.ParameterNames() {
params[n] = r.FormValue(fmt.Sprintf("workflow.params.%s", n))
// TODO(go.dev/issue/51191): Create a better mechanism for storing parameter metadata.
requiredParam := !strings.HasSuffix(n, " (optional)")
if requiredParam && params[n] == "" {
http.Error(w, http.StatusText(http.StatusBadRequest), http.StatusBadRequest)
params := make(map[string]interface{})
for _, p := range d.Parameters() {
switch p.Type.String() {
case "string":
v := r.FormValue(fmt.Sprintf("workflow.params.%s", p.Name))
if p.RequireNonZero() && v == "" {
http.Error(w, fmt.Sprintf("parameter %q must have non-zero value", p.Name), http.StatusBadRequest)
return
}
params[p.Name] = v
case "[]string":
v := r.Form[fmt.Sprintf("workflow.params.%s", p.Name)]
if p.RequireNonZero() && len(v) == 0 {
http.Error(w, fmt.Sprintf("parameter %q must have non-zero value", p.Name), http.StatusBadRequest)
return
}
params[p.Name] = v
default:
http.Error(w, fmt.Sprintf("parameter %q has an unsupported type %q", p.Name, p.Type), http.StatusInternalServerError)
return
}
}

Просмотреть файл

@ -22,7 +22,7 @@ import (
type Listener interface {
workflow.Listener
WorkflowStarted(ctx context.Context, workflowID uuid.UUID, name string, params map[string]string) error
WorkflowStarted(ctx context.Context, workflowID uuid.UUID, name string, params map[string]interface{}) error
WorkflowFinished(ctx context.Context, workflowID uuid.UUID, outputs map[string]interface{}, err error) error
}
@ -84,7 +84,7 @@ func (w *Worker) run(wf *workflow.Workflow) error {
}
// StartWorkflow persists and starts running a workflow.
func (w *Worker) StartWorkflow(ctx context.Context, name string, def *workflow.Definition, params map[string]string) (uuid.UUID, error) {
func (w *Worker) StartWorkflow(ctx context.Context, name string, def *workflow.Definition, params map[string]interface{}) (uuid.UUID, error) {
wf, err := workflow.Start(def, params)
if err != nil {
return uuid.UUID{}, err

Просмотреть файл

@ -34,7 +34,7 @@ func TestWorkerStartWorkflow(t *testing.T) {
wd := newTestEchoWorkflow()
dh.RegisterDefinition(t.Name(), wd)
params := map[string]string{"echo": "greetings"}
params := map[string]interface{}{"echo": "greetings"}
wg.Add(1)
wfid, err := w.StartWorkflow(ctx, t.Name(), wd, params)
@ -217,7 +217,7 @@ func newTestEchoWorkflow() *workflow.Definition {
echo := func(ctx context.Context, arg string) (string, error) {
return arg, nil
}
wd.Output("echo", wd.Task("echo", echo, wd.Parameter("echo")))
wd.Output("echo", wd.Task("echo", echo, wd.Parameter(workflow.Parameter{Name: "echo"})))
return wd
}
@ -256,7 +256,7 @@ func (u *unimplementedListener) Logger(uuid.UUID, string) workflow.Logger {
return log.Default()
}
func (u *unimplementedListener) WorkflowStarted(context.Context, uuid.UUID, string, map[string]string) error {
func (u *unimplementedListener) WorkflowStarted(context.Context, uuid.UUID, string, map[string]interface{}) error {
return errors.New("method WorkflowStarted not implemented")
}

Просмотреть файл

@ -58,32 +58,76 @@ func (h *DefinitionHolder) Definitions() map[string]*workflow.Definition {
// RegisterTweetDefinitions registers workflow definitions involving tweeting
// onto h, using e for the external service configuration.
func RegisterTweetDefinitions(h *DefinitionHolder, e task.ExternalConfig) {
version := workflow.Parameter{
Name: "Version",
Doc: `Version is the Go version that has been released.
The version string must use the same format as Go tags.`,
}
security := workflow.Parameter{
Name: "Security (optional)",
Doc: `Security is an optional sentence describing security fixes included in this release.
The empty string means there are no security fixes to highlight.
Past examples:
"Includes a security fix for crypto/tls (CVE-2021-34558)."
"Includes a security fix for the Wasm port (CVE-2021-38297)."
"Includes security fixes for encoding/pem (CVE-2022-24675), crypto/elliptic (CVE-2022-28327), crypto/x509 (CVE-2022-27536)."`,
}
announcement := workflow.Parameter{
Name: "Announcement",
ParameterType: workflow.URL,
Doc: `Announcement is the announcement URL.
It's applicable to all release types other than major
(since major releases point to release notes instead).`,
Example: "https://groups.google.com/g/golang-announce/c/wB1fph5RpsE/m/ZGwOsStwAwAJ",
}
{
minorVersion := version
minorVersion.Example = "go1.18.2"
secondaryVersion := workflow.Parameter{
Name: "SecondaryVersion",
Doc: `SecondaryVersion is an older Go version that was also released.`,
Example: "go1.17.10",
}
wd := workflow.New()
wd.Output("TweetURL", wd.Task("tweet-minor", func(ctx *workflow.TaskContext, v1, v2, sec, ann string) (string, error) {
return task.TweetMinorRelease(ctx, task.ReleaseTweet{Version: v1, SecondaryVersion: v2, Security: sec, Announcement: ann}, e)
}, wd.Parameter("Version"), wd.Parameter("SecondaryVersion"), wd.Parameter("Security (optional)"), wd.Parameter("Announcement")))
}, wd.Parameter(minorVersion), wd.Parameter(secondaryVersion), wd.Parameter(security), wd.Parameter(announcement)))
h.RegisterDefinition("tweet-minor", wd)
}
{
betaVersion := version
betaVersion.Example = "go1.19beta1"
wd := workflow.New()
wd.Output("TweetURL", wd.Task("tweet-beta", func(ctx *workflow.TaskContext, v, sec, ann string) (string, error) {
return task.TweetBetaRelease(ctx, task.ReleaseTweet{Version: v, Security: sec, Announcement: ann}, e)
}, wd.Parameter("Version"), wd.Parameter("Security (optional)"), wd.Parameter("Announcement")))
}, wd.Parameter(betaVersion), wd.Parameter(security), wd.Parameter(announcement)))
h.RegisterDefinition("tweet-beta", wd)
}
{
rcVersion := version
rcVersion.Example = "go1.19rc1"
wd := workflow.New()
wd.Output("TweetURL", wd.Task("tweet-rc", func(ctx *workflow.TaskContext, v, sec, ann string) (string, error) {
return task.TweetRCRelease(ctx, task.ReleaseTweet{Version: v, Security: sec, Announcement: ann}, e)
}, wd.Parameter("Version"), wd.Parameter("Security (optional)"), wd.Parameter("Announcement")))
}, wd.Parameter(rcVersion), wd.Parameter(security), wd.Parameter(announcement)))
h.RegisterDefinition("tweet-rc", wd)
}
{
majorVersion := version
majorVersion.Example = "go1.19"
wd := workflow.New()
wd.Output("TweetURL", wd.Task("tweet-major", func(ctx *workflow.TaskContext, v, sec string) (string, error) {
return task.TweetMajorRelease(ctx, task.ReleaseTweet{Version: v, Security: sec}, e)
}, wd.Parameter("Version"), wd.Parameter("Security (optional)")))
}, wd.Parameter(majorVersion), wd.Parameter(security)))
h.RegisterDefinition("tweet-major", wd)
}
}
@ -92,8 +136,8 @@ func RegisterTweetDefinitions(h *DefinitionHolder, e task.ExternalConfig) {
// development.
func newEchoWorkflow() *workflow.Definition {
wd := workflow.New()
wd.Output("greeting", wd.Task("greeting", echo, wd.Parameter("greeting")))
wd.Output("farewell", wd.Task("farewell", echo, wd.Parameter("farewell")))
wd.Output("greeting", wd.Task("greeting", echo, wd.Parameter(workflow.Parameter{Name: "greeting"})))
wd.Output("farewell", wd.Task("farewell", echo, wd.Parameter(workflow.Parameter{Name: "farewell"})))
return wd
}

Просмотреть файл

@ -18,7 +18,7 @@
//
// Each task has a set of input Values, and returns a single output Value.
// Calling Task defines a task that will run a Go function when it runs. That
// function must take a *TaskContext or context.Context, followed by arguments
// function must take a context.Context or *TaskContext, followed by arguments
// corresponding to the dynamic type of the Values passed to it. The TaskContext
// can be used as a normal Context, and also supports unstructured logging.
//
@ -32,6 +32,7 @@ import (
"encoding/json"
"fmt"
"reflect"
"strings"
"github.com/google/uuid"
)
@ -39,17 +40,16 @@ import (
// New creates a new workflow definition.
func New() *Definition {
return &Definition{
parameterNames: map[string]struct{}{},
tasks: map[string]*taskDefinition{},
outputs: map[string]*taskResult{},
tasks: make(map[string]*taskDefinition),
outputs: make(map[string]*taskResult),
}
}
// A Definition defines the structure of a workflow.
type Definition struct {
parameterNames map[string]struct{}
tasks map[string]*taskDefinition
outputs map[string]*taskResult
parameters []Parameter // Ordered according to registration, unique parameter names.
tasks map[string]*taskDefinition
outputs map[string]*taskResult
}
// A Value is a piece of data that will be produced or consumed when a task
@ -60,36 +60,93 @@ type Value interface {
deps() []*taskDefinition
}
// Parameter creates a Value that is filled in at workflow creation time.
func (d *Definition) Parameter(name string) Value {
d.parameterNames[name] = struct{}{}
return &workflowParameter{name: name}
// Parameter describes a Value that is filled in at workflow creation time.
//
// It can be registered to a workflow with the Workflow.Parameter method.
type Parameter struct {
Name string // Name identifies the parameter within a workflow. Must be non-empty.
ParameterType // Parameter type. Defaults to BasicString if not specified.
Doc string // Doc documents the parameter. Optional.
Example string // Example is an example value. Optional.
}
// ParameterNames returns the names of all parameters associated with
// the Definition.
func (d *Definition) ParameterNames() []string {
var names []string
for n := range d.parameterNames {
names = append(names, n)
// RequireNonZero reports whether parameter p is required to have a non-zero value.
func (p Parameter) RequireNonZero() bool {
return !strings.HasSuffix(p.Name, " (optional)")
}
// ParameterType defines the type of a workflow parameter.
//
// Since parameters are entered via an HTML form,
// there are some HTML-related knobs available.
type ParameterType struct {
Type reflect.Type // The Go type of the parameter.
// HTMLElement configures the HTML element for entering the parameter value.
// Supported values are "input" and "textarea".
HTMLElement string
// HTMLInputType optionally configures the <input> type attribute when HTMLElement is "input".
// If this attribute is not specified, <input> elements default to type="text".
// See https://developer.mozilla.org/en-US/docs/Web/HTML/Element/input#input_types.
HTMLInputType string
}
var (
// String parameter types.
BasicString = ParameterType{
Type: reflect.TypeOf(""),
HTMLElement: "input",
}
return names
URL = ParameterType{
Type: reflect.TypeOf(""),
HTMLElement: "input",
HTMLInputType: "url",
}
// Slice of string parameter types.
SliceShort = ParameterType{
Type: reflect.TypeOf([]string(nil)),
HTMLElement: "input",
}
SliceLong = ParameterType{
Type: reflect.TypeOf([]string(nil)),
HTMLElement: "textarea",
}
)
// Parameter registers a new parameter p that is filled in at
// workflow creation time and returns the corresponding Value.
// Parameter name must be non-empty and uniquely identify the
// parameter in the workflow definition.
//
// If the parameter type is unspecified, BasicString is used.
func (d *Definition) Parameter(p Parameter) Value {
if p.Name == "" {
panic(fmt.Errorf("parameter name must be non-empty"))
}
if p.ParameterType == (ParameterType{}) {
p.ParameterType = BasicString
}
for _, old := range d.parameters {
if p.Name == old.Name {
panic(fmt.Errorf("parameter with name %q was already registered with this workflow definition", p.Name))
}
}
d.parameters = append(d.parameters, p)
return parameter(p)
}
type workflowParameter struct {
name string
}
// parameter implements Value for a workflow parameter.
type parameter Parameter
func (wp *workflowParameter) typ() reflect.Type {
return reflect.TypeOf("")
}
func (p parameter) typ() reflect.Type { return p.Type }
func (p parameter) value(w *Workflow) reflect.Value { return reflect.ValueOf(w.params[p.Name]) }
func (p parameter) deps() []*taskDefinition { return nil }
func (wp *workflowParameter) value(w *Workflow) reflect.Value {
return reflect.ValueOf(w.params[wp.name])
}
func (wp *workflowParameter) deps() []*taskDefinition {
return nil
// Parameters returns parameters associated with the Definition
// in the same order that they were registered.
func (d *Definition) Parameters() []Parameter {
return d.parameters
}
// Constant creates a Value from an existing object.
@ -158,8 +215,8 @@ func (d *Definition) Output(name string, v Value) {
// Task adds a task to the workflow definition. It can take any number of
// arguments, and returns one output. name must uniquely identify the task in
// the workflow.
// f must be a function that takes a context.Context argument, followed by one
// argument for each of args, corresponding to the Value's dynamic type.
// f must be a function that takes a context.Context or *TaskContext argument,
// followed by one argument for each of args, corresponding to the Value's dynamic type.
// It must return two values, the first of which will be returned as its Value,
// and an error that will be used by the workflow engine. See the package
// documentation for examples.
@ -222,7 +279,7 @@ type TaskState struct {
// WorkflowState contains the shallow state of a running workflow.
type WorkflowState struct {
ID uuid.UUID
Params map[string]string
Params map[string]interface{}
}
// A Logger is a debug logger passed to a task implementation.
@ -256,7 +313,7 @@ func (tr *taskResult) deps() []*taskDefinition {
type Workflow struct {
ID uuid.UUID
def *Definition
params map[string]string
params map[string]interface{}
tasks map[*taskDefinition]*taskState
}
@ -298,7 +355,7 @@ func (t *taskState) toExported() *TaskState {
}
// Start instantiates a workflow with the given parameters.
func Start(def *Definition, params map[string]string) (*Workflow, error) {
func Start(def *Definition, params map[string]interface{}) (*Workflow, error) {
w := &Workflow{
ID: uuid.New(),
def: def,
@ -315,6 +372,7 @@ func Start(def *Definition, params map[string]string) (*Workflow, error) {
}
func (w *Workflow) validate() error {
// Validate tasks.
used := map[*taskDefinition]bool{}
for _, taskDef := range w.def.tasks {
for _, arg := range taskDef.args {
@ -331,6 +389,21 @@ func (w *Workflow) validate() error {
return fmt.Errorf("task %v is not referenced and should be deleted", task.name)
}
}
// Validate parameters.
if got, want := len(w.params), len(w.def.parameters); got != want {
return fmt.Errorf("parameter count mismatch: workflow instance has %d, but definition has %d", got, want)
}
paramDefs := map[string]Value{} // Key is parameter name.
for _, p := range w.def.parameters {
paramDefs[p.Name] = parameter(p)
}
for name, v := range w.params {
if !paramDefs[name].typ().AssignableTo(reflect.TypeOf(v)) {
return fmt.Errorf("parameter type mismatch: value of parameter %q has type %v, but definition specifies %v", name, reflect.TypeOf(v), paramDefs[name].typ())
}
}
return nil
}

Просмотреть файл

@ -117,18 +117,48 @@ func TestParameters(t *testing.T) {
}
wd := workflow.New()
param1 := wd.Parameter("param1")
param2 := wd.Parameter("param2")
param1 := wd.Parameter(workflow.Parameter{Name: "param1"})
param2 := wd.Parameter(workflow.Parameter{Name: "param2"})
out1 := wd.Task("echo 1", echo, param1)
out2 := wd.Task("echo 2", echo, param2)
wd.Output("out1", out1)
wd.Output("out2", out2)
w := startWorkflow(t, wd, map[string]string{"param1": "#1", "param2": "#2"})
wantParams := []workflow.Parameter{
{Name: "param1", ParameterType: workflow.BasicString},
{Name: "param2", ParameterType: workflow.BasicString},
}
if diff := cmp.Diff(wantParams, wd.Parameters(), cmp.Comparer(func(x, y reflect.Type) bool { return x == y })); diff != "" {
t.Errorf("wd.Parameters() mismatch (-want +got):\n%s", diff)
}
w := startWorkflow(t, wd, map[string]interface{}{"param1": "#1", "param2": "#2"})
outputs := runWorkflow(t, w, nil)
if want := map[string]interface{}{"out1": "#1", "out2": "#2"}; !reflect.DeepEqual(outputs, want) {
t.Errorf("outputs = %#v, want %#v", outputs, want)
}
t.Run("CountMismatch", func(t *testing.T) {
_, err := workflow.Start(wd, map[string]interface{}{"param1": "#1"})
if err == nil {
t.Errorf("workflow.Start didn't return an error despite a parameter count mismatch")
}
})
t.Run("TypeMismatch", func(t *testing.T) {
_, err := workflow.Start(wd, map[string]interface{}{"param1": "#1", "param2": 42})
if err == nil {
t.Errorf("workflow.Start didn't return an error despite a parameter type mismatch")
}
})
}
// Test that passing workflow.Parameter{...} directly to Definition.Task would be a build-time error.
// Parameters need to be registered via the Definition.Parameter method.
func TestParameterValue(t *testing.T) {
var p interface{} = workflow.Parameter{}
if _, ok := p.(workflow.Value); ok {
t.Errorf("Parameter unexpectedly implements Value; it intentionally tries not to to reduce possible API misuse")
}
}
func TestLogging(t *testing.T) {
@ -256,7 +286,7 @@ func (l *mapListener) assertState(t *testing.T, w *workflow.Workflow, want map[s
}
}
func startWorkflow(t *testing.T, wd *workflow.Definition, params map[string]string) *workflow.Workflow {
func startWorkflow(t *testing.T, wd *workflow.Definition, params map[string]interface{}) *workflow.Workflow {
t.Helper()
w, err := workflow.Start(wd, params)
if err != nil {