From 5e967f7146a9c1383d94097e0c4f641187189aeb Mon Sep 17 00:00:00 2001 From: Haytham Abuelfutuh Date: Thu, 17 Jun 2021 13:00:01 -0700 Subject: [PATCH] Only pass literals to execution if they users provided values (#105) * Only pass literals to execution if they users provided values Signed-off-by: Haytham Abuelfutuh * Adding unit tests and a bit of refactoring Signed-off-by: Haytham Abuelfutuh --- flytectl/cmd/create/serialization_utils.go | 49 +++++- .../cmd/create/serialization_utils_test.go | 148 ++++++++++++++++++ 2 files changed, 189 insertions(+), 8 deletions(-) create mode 100644 flytectl/cmd/create/serialization_utils_test.go diff --git a/flytectl/cmd/create/serialization_utils.go b/flytectl/cmd/create/serialization_utils.go index b9759ca7c1..53045d2c9e 100644 --- a/flytectl/cmd/create/serialization_utils.go +++ b/flytectl/cmd/create/serialization_utils.go @@ -1,29 +1,62 @@ package create import ( + "fmt" + "github.com/flyteorg/flyteidl/clients/go/coreutils" "github.com/flyteorg/flyteidl/gen/pb-go/flyteidl/core" ) // TODO: Move all functions to flyteidl +// MakeLiteralForVariables builds a map of literals for the provided serialized values. If a provided value does not have +// a corresponding variable or if that variable is invalid (e.g. doesn't have Type property populated), it returns an +// error. func MakeLiteralForVariables(serialize map[string]interface{}, variables map[string]*core.Variable) (map[string]*core.Literal, error) { - result := make(map[string]*core.Literal) - var err error + types := make(map[string]*core.LiteralType) for k, v := range variables { - if result[k], err = coreutils.MakeLiteralForType(v.Type, serialize[k]); err != nil { - return nil, err + t := v.GetType() + if t == nil { + return nil, fmt.Errorf("variable [%v] has nil type", k) } + + types[k] = t } - return result, nil + + return MakeLiteralForTypes(serialize, types) } +// MakeLiteralForParams builds a map of literals for the provided serialized values. If a provided value does not have +// a corresponding parameter or if that parameter is invalid (e.g. doesn't have Type property populated), it returns an +// error. func MakeLiteralForParams(serialize map[string]interface{}, parameters map[string]*core.Parameter) (map[string]*core.Literal, error) { + types := make(map[string]*core.LiteralType) + for k, v := range parameters { + if variable := v.GetVar(); variable == nil { + return nil, fmt.Errorf("parameter [%v] has nil Variable", k) + } else if t := variable.GetType(); t == nil { + return nil, fmt.Errorf("parameter [%v] has nil variable type", k) + } else { + types[k] = t + } + } + + return MakeLiteralForTypes(serialize, types) +} + +// MakeLiteralForTypes builds a map of literals for the provided serialized values. If a provided value does not have +// a corresponding type or if it fails to create a literal for the given type and value, it returns an error. +func MakeLiteralForTypes(serialize map[string]interface{}, types map[string]*core.LiteralType) (map[string]*core.Literal, error) { result := make(map[string]*core.Literal) var err error - for k, v := range parameters { - if result[k], err = coreutils.MakeLiteralForType(v.GetVar().Type, serialize[k]); err != nil { - return nil, err + for k, v := range serialize { + if t, typeFound := types[k]; typeFound { + if result[k], err = coreutils.MakeLiteralForType(t, v); err != nil { + return nil, err + } + } else { + return nil, fmt.Errorf("no matching type for [%v]", k) } } + return result, nil } diff --git a/flytectl/cmd/create/serialization_utils_test.go b/flytectl/cmd/create/serialization_utils_test.go new file mode 100644 index 0000000000..5c7326493d --- /dev/null +++ b/flytectl/cmd/create/serialization_utils_test.go @@ -0,0 +1,148 @@ +package create + +import ( + "testing" + + "github.com/flyteorg/flyteidl/gen/pb-go/flyteidl/core" + "github.com/stretchr/testify/assert" +) + +func TestMakeLiteralForTypes(t *testing.T) { + inputTypes := map[string]*core.LiteralType{ + "a": { + Type: &core.LiteralType_Simple{ + Simple: core.SimpleType_INTEGER, + }, + }, + "x": { + Type: &core.LiteralType_Simple{ + Simple: core.SimpleType_INTEGER, + }, + }, + "b": { + Type: &core.LiteralType_Simple{ + Simple: core.SimpleType_STRING, + }, + }, + "y": { + Type: &core.LiteralType_Simple{ + Simple: core.SimpleType_STRING, + }, + }, + } + + t.Run("Happy path", func(t *testing.T) { + inputValues := map[string]interface{}{ + "a": 5, + "b": "hello", + } + + m, err := MakeLiteralForTypes(inputValues, inputTypes) + assert.NoError(t, err) + assert.Len(t, m, len(inputValues)) + }) + + t.Run("Type not found", func(t *testing.T) { + inputValues := map[string]interface{}{ + "notfound": 5, + } + + _, err := MakeLiteralForTypes(inputValues, inputTypes) + assert.Error(t, err) + }) + + t.Run("Invalid value", func(t *testing.T) { + inputValues := map[string]interface{}{ + "a": "hello", + } + + _, err := MakeLiteralForTypes(inputValues, inputTypes) + assert.Error(t, err) + }) +} + +func TestMakeLiteralForParams(t *testing.T) { + inputValues := map[string]interface{}{ + "a": "hello", + } + + t.Run("Happy path", func(t *testing.T) { + inputParams := map[string]*core.Parameter{ + "a": { + Var: &core.Variable{ + Type: &core.LiteralType{ + Type: &core.LiteralType_Simple{ + Simple: core.SimpleType_STRING, + }, + }, + }, + }, + } + + m, err := MakeLiteralForParams(inputValues, inputParams) + assert.NoError(t, err) + assert.Len(t, m, len(inputValues)) + }) + + t.Run("Invalid Param", func(t *testing.T) { + inputParams := map[string]*core.Parameter{ + "a": nil, + } + + _, err := MakeLiteralForParams(inputValues, inputParams) + assert.Error(t, err) + }) + + t.Run("Invalid Type", func(t *testing.T) { + inputParams := map[string]*core.Parameter{ + "a": { + Var: &core.Variable{}, + }, + } + + _, err := MakeLiteralForParams(inputValues, inputParams) + assert.Error(t, err) + }) +} + +func TestMakeLiteralForVariables(t *testing.T) { + inputValues := map[string]interface{}{ + "a": "hello", + } + + t.Run("Happy path", func(t *testing.T) { + inputVariables := map[string]*core.Variable{ + "a": { + Type: &core.LiteralType{ + Type: &core.LiteralType_Simple{ + Simple: core.SimpleType_STRING, + }, + }, + }, + } + + m, err := MakeLiteralForVariables(inputValues, inputVariables) + assert.NoError(t, err) + assert.Len(t, m, len(inputValues)) + }) + + t.Run("Invalid Variable", func(t *testing.T) { + inputVariables := map[string]*core.Variable{ + "a": nil, + } + + _, err := MakeLiteralForVariables(inputValues, inputVariables) + assert.Error(t, err) + }) + + t.Run("Invalid Type", func(t *testing.T) { + inputVariables := map[string]*core.Variable{ + "a": { + Type: nil, + }, + } + + _, err := MakeLiteralForVariables(inputValues, inputVariables) + assert.Error(t, err) + }) +}