Address comments

Signed-off-by: byhsu <[email protected]>
flyteorg · Oct 17, 2023 · fa23167 · fa23167
1 parent 849e7c8
commit fa23167
Show file tree

Hide file tree

Showing 2 changed files with 14 additions and 12 deletions.
diff --git a/flytepropeller/pkg/compiler/validators/bindings.go b/flytepropeller/pkg/compiler/validators/bindings.go
@@ -124,11 +124,12 @@ func validateBinding(w c.WorkflowBuilder, nodeID c.NodeID, nodeParam string, bin
 						sourceType = cType
 					}
 				}
-				var exist bool
-				var tmpType *flyte.LiteralType
 
 				// If the variable has an attribute path. Extract the type of the last attribute.
 				for _, attr := range val.Promise.AttrPath {
+					var tmpType *flyte.LiteralType
+					var exist bool
+
 					if sourceType.GetCollectionType() != nil {
 						sourceType = sourceType.GetCollectionType()
 					} else if sourceType.GetMapValueType() != nil {
@@ -138,6 +139,7 @@ func validateBinding(w c.WorkflowBuilder, nodeID c.NodeID, nodeParam string, bin
 						tmpType, exist = sourceType.GetStructure().GetDataclassType()[attr.GetStringValue()]
 
 						if !exist {
+							// the error should output the sourceType instead of tmpType because tmpType is nil
 							errs.Collect(errors.NewFieldNotFoundErr(nodeID, val.Promise.Var, sourceType.String(), attr.GetStringValue()))
 							return nil, nil, !errs.HasErrors()
 						} else {

diff --git a/flytepropeller/pkg/controller/nodes/attr_path_resolver_test.go b/flytepropeller/pkg/controller/nodes/attr_path_resolver_test.go
@@ -31,23 +31,14 @@ func NewStructFromMap(m map[string]interface{}) *structpb.Struct {
 }
 
 func TestResolveAttrPathIn(t *testing.T) {
-	// - map {"foo": "bar"}
-	// - collection ["foo", "bar"]
-	// - struct1 {"foo": "bar"}
-	// - struct2 {"foo": ["bar1", "bar2"]}
-	// - nested list struct {"foo": [["bar1", "bar2"]]}
-	// - map+collection+struct {"foo": [{"bar": "car"}]}
-	// - exception key error with map
-	// - exception out of range with collection
-	// - exception key error with struct
-	// - exception out of range with struct
 
 	args := []struct {
 		literal  *core.Literal
 		path     []*core.PromiseAttribute
 		expected *core.Literal
 		hasError bool
 	}{
+		// - map {"foo": "bar"}
 		{
 			literal: &core.Literal{
 				Value: &core.Literal_Map{
@@ -68,6 +59,7 @@ func TestResolveAttrPathIn(t *testing.T) {
 			expected: NewScalarLiteral("bar"),
 			hasError: false,
 		},
+		// - collection ["foo", "bar"]
 		{
 			literal: &core.Literal{
 				Value: &core.Literal_Collection{
@@ -89,6 +81,7 @@ func TestResolveAttrPathIn(t *testing.T) {
 			expected: NewScalarLiteral("bar"),
 			hasError: false,
 		},
+		// - struct1 {"foo": "bar"}
 		{
 			literal: &core.Literal{
 				Value: &core.Literal_Scalar{
@@ -109,6 +102,7 @@ func TestResolveAttrPathIn(t *testing.T) {
 			expected: NewScalarLiteral("bar"),
 			hasError: false,
 		},
+		// - struct2 {"foo": ["bar1", "bar2"]}
 		{
 			literal: &core.Literal{
 				Value: &core.Literal_Scalar{
@@ -138,6 +132,7 @@ func TestResolveAttrPathIn(t *testing.T) {
 			expected: NewScalarLiteral("bar2"),
 			hasError: false,
 		},
+		// - nested list struct {"foo": [["bar1", "bar2"]]}
 		{
 			literal: &core.Literal{
 				Value: &core.Literal_Scalar{
@@ -179,6 +174,7 @@ func TestResolveAttrPathIn(t *testing.T) {
 			},
 			hasError: false,
 		},
+		// - map+collection+struct {"foo": [{"bar": "car"}]}
 		{
 			literal: &core.Literal{
 				Value: &core.Literal_Map{
@@ -225,6 +221,7 @@ func TestResolveAttrPathIn(t *testing.T) {
 			expected: NewScalarLiteral("car"),
 			hasError: false,
 		},
+		// - exception key error with map
 		{
 			literal: &core.Literal{
 				Value: &core.Literal_Map{
@@ -245,6 +242,7 @@ func TestResolveAttrPathIn(t *testing.T) {
 			expected: &core.Literal{},
 			hasError: true,
 		},
+		// - exception out of range with collection
 		{
 			literal: &core.Literal{
 				Value: &core.Literal_Collection{
@@ -266,6 +264,7 @@ func TestResolveAttrPathIn(t *testing.T) {
 			expected: &core.Literal{},
 			hasError: true,
 		},
+		// - exception key error with struct
 		{
 			literal: &core.Literal{
 				Value: &core.Literal_Scalar{
@@ -286,6 +285,7 @@ func TestResolveAttrPathIn(t *testing.T) {
 			expected: &core.Literal{},
 			hasError: true,
 		},
+		// - exception out of range with struct
 		{
 			literal: &core.Literal{
 				Value: &core.Literal_Scalar{