Repository: honeydipper/honeydipper (go)

Issue:
Function export is broken for subsystem calls
A `system` can have subsystems in Honeydipper.  It is the way that the private system data can be exposed to other systems. When calling a function belong to a subsystem, the parent system data should be added to the the child system and accessed through key `parent`. This should also be done when interpolating the function exports, because the parameters are also used for interpolation.

The function export feature introduced in `1.5.0` has a bug that the subsystem data was not arranged properly.

Pull request:
fix(config): function export should arrange parent subsystem data properly
#### Description
* rewrite the `ExportFunctionContext` func to ensure the parent/subsystem data are arranged
* allow merging and deep copying nil
* more tests for the the changes

#### This PR fixes the following issues

Fixes #269 


Patch:
diff --git a/internal/config/function_export.go b/internal/config/function_export.go
index 19f20d3..accadfc 100644
--- a/internal/config/function_export.go
+++ b/internal/config/function_export.go
@@ -13,7 +13,7 @@ import (
 )
 
 // ExportFunctionContext create a context data structure based on the collapsed function exports
-func ExportFunctionContext(s *System, f *Function, envData map[string]interface{}, cfg *Config) map[string]interface{} {
+func ExportFunctionContext(f *Function, envData map[string]interface{}, cfg *Config) map[string]interface{} {
 	var exported map[string]interface{}
 	status := "success"
 	if labelsData, ok := envData["labels"]; ok {
@@ -25,24 +25,21 @@ func ExportFunctionContext(s *System, f *Function, envData map[string]interface{
 	}
 
 	if status != "error" {
-		if len(f.Parameters) > 0 {
-
-			// the parameters need to be squashed before interpolation
-			// we will also need to provide the squashed sysData for interpolation
-			// the outter parameters should override inner parameters, but
-			// the inner sysData should override outter sysData
-			outterParam, _ := envData["params"]
-			envData["params"] = dipper.MergeMap(dipper.MustDeepCopyMap(f.Parameters), outterParam)
-			var outterSysData map[string]interface{}
-			if d, ok := envData["sysData"]; ok && d != nil {
-				outterSysData = d.(map[string]interface{})
-			}
-			if s != nil && s.Data != nil {
-				envData["sysData"] = dipper.MergeMap(outterSysData, dipper.MustDeepCopyMap(s.Data))
+		var sysData map[string]interface{}
+		if d, ok := envData["sysData"]; ok {
+			sysData, ok = d.(map[string]interface{})
+			if !ok {
+				dipper.Logger.Panicf("[operator] sysData is not a map")
 			}
 		}
 
-		var sysData map[string]interface{}
+		// the parameters need to be squashed before interpolation
+		// we will also need to provide the squashed sysData for interpolation
+		// the outer parameters should override inner parameters, but
+		// the inner sysData should override outer sysData
+		outerParam, _ := envData["params"]
+		envData["params"] = dipper.MergeMap(dipper.MustDeepCopyMap(f.Parameters), outerParam)
+
 		if f.Target.System != "" {
 			childSystem, ok := cfg.DataSet.Systems[f.Target.System]
 			if !ok {
@@ -52,25 +49,27 @@ func ExportFunctionContext(s *System, f *Function, envData map[string]interface{
 			if !ok {
 				dipper.Logger.Panicf("[operator] function not defined %s.%s", f.Target.System, f.Target.Function)
 			}
-			exported = ExportFunctionContext(&childSystem, &childFunction, envData, cfg)
+
+			// merging system data
+			mergedSysData := dipper.MergeMap(dipper.MustDeepCopyMap(sysData), dipper.MustDeepCopy(childSystem.Data))
 
 			// split subsystem from system
-			if data, ok := envData["sysData"]; ok {
-				sysData = data.(map[string]interface{})
-			}
 			subsystems := strings.Split(f.Target.Function, ".")
 			for _, subsystem := range subsystems[:len(subsystems)-1] {
-				parent := sysData
-				sysData = parent[subsystem].(map[string]interface{})
-				sysData["parent"] = parent
+				parent := mergedSysData
+				mergedSysData = parent[subsystem].(map[string]interface{})
+				mergedSysData["parent"] = parent
 			}
+
+			envData["sysData"] = mergedSysData
+			exported = ExportFunctionContext(&childFunction, envData, cfg)
 		} else {
 			if len(f.Target.System) > 0 {
 				dipper.Logger.Panicf("[operator] function cannot have both driver and target %s.%s %s", f.Target.System, f.Target.Function, f.Driver)
 			}
 
 			// here comes the interpolation for the squashed parameters. This interpolation only happens
-			// once, in the inner most function. After that, the parameters can be used for export in all outter
+			// once, in the inner most function. After that, the parameters can be used for export in all outer
 			// layers.
 			squashedParams, _ := envData["params"]
 			envData["params"] = dipper.Interpolate(squashedParams, envData)
@@ -78,9 +77,6 @@ func ExportFunctionContext(s *System, f *Function, envData map[string]interface{
 
 		// here we abandon the squashed sysData after it is consumed, and use a clean sysData for
 		// interpolating the exported data.
-		if s != nil && s.Data != nil {
-			sysData = dipper.MergeMap(sysData, dipper.MustDeepCopyMap(s.Data))
-		}
 		envData["sysData"] = sysData
 		var newCtx map[string]interface{}
 
diff --git a/internal/workflow/continue.go b/internal/workflow/continue.go
index d0de679..3a5b63e 100644
--- a/internal/workflow/continue.go
+++ b/internal/workflow/continue.go
@@ -117,7 +117,7 @@ func (w *Session) processExport(msg *dipper.Message) {
 		status := msg.Labels["status"]
 
 		if w.inFlyFunction != nil && status != SessionStatusError {
-			export := config.ExportFunctionContext(nil, w.inFlyFunction, envData, w.store.Helper.GetConfig())
+			export := config.ExportFunctionContext(w.inFlyFunction, envData, w.store.Helper.GetConfig())
 			delete(envData, "sysData")
 			w.processNoExport(export)
 			if len(export) > 0 {
diff --git a/pkg/dipper/mapUtils.go b/pkg/dipper/mapUtils.go
index 872d639..7c1dd0b 100644
--- a/pkg/dipper/mapUtils.go
+++ b/pkg/dipper/mapUtils.go
@@ -245,6 +245,9 @@ func LockCheckDeleteMap(lock *sync.Mutex, resource interface{}, key interface{},
 
 // DeepCopyMap : performs a deep copy of the given map m.
 func DeepCopyMap(m map[string]interface{}) (map[string]interface{}, error) {
+	if m == nil {
+		return nil, nil
+	}
 	ret, err := DeepCopy(m)
 	if err != nil {
 		return nil, err


Test patch:
diff --git a/internal/config/function_export_test.go b/internal/config/function_export_test.go
index 3ecc447..8f066cd 100644
--- a/internal/config/function_export_test.go
+++ b/internal/config/function_export_test.go
@@ -21,7 +21,7 @@ func TestFunctionExport(t *testing.T) {
 				"test": System{
 					Data: map[string]interface{}{
 						"key1": "value1",
-						"key2": map[string]string{
+						"key2": map[string]interface{}{
 							"key3": "value3",
 						},
 					},
@@ -40,7 +40,6 @@ func TestFunctionExport(t *testing.T) {
 	}
 
 	result := ExportFunctionContext(
-		nil,
 		&Function{Target: Action{System: "test", Function: "testFunc1"}},
 		map[string]interface{}{
 			"ctx": map[string]interface{}{"path": "test/path1"},
@@ -53,15 +52,17 @@ func TestFunctionExport(t *testing.T) {
 	newSys1 := &System{
 		Data: map[string]interface{}{
 			"key1": "value11",
-			"key2": "value22",
+			"key2": map[string]interface{}{
+				"key3": "should_not_be_used",
+			},
 		},
 	}
 
 	result = ExportFunctionContext(
-		newSys1,
 		&Function{Target: Action{System: "test", Function: "testFunc1"}},
 		map[string]interface{}{
-			"ctx": map[string]interface{}{"path": "test/path1"},
+			"ctx":     map[string]interface{}{"path": "test/path1"},
+			"sysData": newSys1.Data,
 		},
 		&cfg,
 	)
@@ -99,7 +100,6 @@ func TestFunctionExportWithParameters(t *testing.T) {
 	}
 
 	result := ExportFunctionContext(
-		nil,
 		&Function{Target: Action{System: "test", Function: "testFunc1"}},
 		map[string]interface{}{
 			"ctx": map[string]interface{}{"path": "test/path1"},
@@ -110,7 +110,6 @@ func TestFunctionExportWithParameters(t *testing.T) {
 	assert.Equal(t, "http://value1.value3/test/path1?q1=param1", result["url"])
 
 	result = ExportFunctionContext(
-		nil,
 		&Function{
 			Target: Action{System: "test", Function: "testFunc1"},
 			Parameters: map[string]interface{}{
@@ -176,7 +175,6 @@ func TestFunctionExportWithSquashedParameters(t *testing.T) {
 	}
 
 	result := ExportFunctionContext(
-		nil,
 		&Function{Target: Action{System: "test2", Function: "testFunc2"}},
 		map[string]interface{}{
 			"ctx": map[string]interface{}{"path": "test/path1"},
@@ -230,7 +228,6 @@ func TestFunctionExportWithSquashedSysData(t *testing.T) {
 	}
 
 	result := ExportFunctionContext(
-		nil,
 		&Function{Target: Action{System: "test2", Function: "testFunc2"}},
 		map[string]interface{}{
 			"ctx": map[string]interface{}{"path": "test/path1"},
@@ -240,3 +237,147 @@ func TestFunctionExportWithSquashedSysData(t *testing.T) {
 
 	assert.Equal(t, "http://value1.value3/test/path1?q1=value4", result["url"])
 }
+
+func TestFunctionExportWithSubsystem(t *testing.T) {
+	cfg := Config{
+		DataSet: &DataSet{
+			Systems: map[string]System{
+				"outter": System{
+					Extends: []string{
+						"inner=test",
+					},
+					Data: map[string]interface{}{
+						"key1": "should-not-be-used",
+					},
+				},
+				"test": System{
+					Data: map[string]interface{}{
+						"key1": "value1",
+						"key2": map[string]interface{}{
+							"key3": "value3",
+						},
+					},
+					Functions: map[string]Function{
+						"testFunc1": Function{
+							Driver:    "testDriver",
+							RawAction: "action",
+							Export: map[string]interface{}{
+								"url": "http://{{ .sysData.key1 }}.{{ .sysData.key2.key3}}/{{ .ctx.path }}",
+							},
+						},
+					},
+				},
+			},
+		},
+	}
+
+	(&cfg).extendAllSystems()
+
+	result := ExportFunctionContext(
+		&Function{Target: Action{System: "outter", Function: "inner.testFunc1"}},
+		map[string]interface{}{
+			"ctx": map[string]interface{}{"path": "test/path1"},
+		},
+		&cfg,
+	)
+
+	assert.Equal(t, "http://value1.value3/test/path1", result["url"])
+}
+
+func TestFunctionExportWithSubsystemParameters(t *testing.T) {
+	cfg := Config{
+		DataSet: &DataSet{
+			Systems: map[string]System{
+				"outter": System{
+					Extends: []string{
+						"inner=test",
+					},
+					Data: map[string]interface{}{
+						"key1": "should-not-be-used",
+					},
+				},
+				"test": System{
+					Data: map[string]interface{}{
+						"key1": "value1",
+						"key2": map[string]interface{}{
+							"key3": "value3",
+						},
+						"key5": "internal-data",
+					},
+					Functions: map[string]Function{
+						"testFunc1": Function{
+							Driver:    "testDriver",
+							RawAction: "action",
+							Parameters: map[string]interface{}{
+								"param1": "$sysData.key5",
+							},
+							Export: map[string]interface{}{
+								"url": "http://{{ .sysData.key1 }}.{{ .sysData.key2.key3}}/{{ .ctx.path }}?{{ .params.param1 }}",
+							},
+						},
+					},
+				},
+			},
+		},
+	}
+
+	(&cfg).extendAllSystems()
+
+	result := ExportFunctionContext(
+		&Function{Target: Action{System: "outter", Function: "inner.testFunc1"}},
+		map[string]interface{}{
+			"ctx": map[string]interface{}{"path": "test/path1"},
+		},
+		&cfg,
+	)
+
+	assert.Equal(t, "http://value1.value3/test/path1?internal-data", result["url"])
+}
+
+func TestFunctionExportWithSubsystemParentData(t *testing.T) {
+	cfg := Config{
+		DataSet: &DataSet{
+			Systems: map[string]System{
+				"outter": System{
+					Extends: []string{
+						"inner=test",
+					},
+					Data: map[string]interface{}{
+						"key1": "should-not-be-used",
+						"key2": "belong-to-parent",
+					},
+				},
+				"test": System{
+					Data: map[string]interface{}{
+						"key1": "value1",
+						"key2": map[string]interface{}{
+							"key3": "value3",
+						},
+						"key5": "internal-data",
+					},
+					Functions: map[string]Function{
+						"testFunc1": Function{
+							Driver:    "testDriver",
+							RawAction: "action",
+							Export: map[string]interface{}{
+								"url": "http://{{ .sysData.key1 }}.{{ .sysData.key2.key3}}/{{ .ctx.path }}?{{ .sysData.parent.key2 }}",
+							},
+						},
+					},
+				},
+			},
+		},
+	}
+
+	(&cfg).extendAllSystems()
+
+	result := ExportFunctionContext(
+		&Function{Target: Action{System: "outter", Function: "inner.testFunc1"}},
+		map[string]interface{}{
+			"ctx": map[string]interface{}{"path": "test/path1"},
+		},
+		&cfg,
+	)
+
+	assert.Equal(t, "http://value1.value3/test/path1?belong-to-parent", result["url"])
+}
diff --git a/pkg/dipper/mapUtils_test.go b/pkg/dipper/mapUtils_test.go
index 4252982..6de4471 100644
--- a/pkg/dipper/mapUtils_test.go
+++ b/pkg/dipper/mapUtils_test.go
@@ -95,3 +95,34 @@ func TestRecursive(t *testing.T) {
 		assert.Equal(t, testExpects[i], testValue, "recursive test case %v failed", i)
 	}
 }
+
+func TestDeepCopyNil(t *testing.T) {
+	var ret interface{}
+	var err error
+	assert.NotPanics(t, func() { ret, err = DeepCopy(nil) })
+	assert.Nil(t, ret)
+	assert.Nil(t, err)
+}
+
+func TestDeepCopyNilMap(t *testing.T) {
+	var ret interface{}
+	var err error
+	assert.NotPanics(t, func() { ret, err = DeepCopyMap(nil) })
+	assert.Nil(t, ret)
+	assert.Nil(t, err)
+}
+
+func TestDeepCopy(t *testing.T) {
+	var ret interface{}
+	var err error
+	src := map[string]interface{}{
+		"key1": "value1",
+		"key2": 2,
+		"key3": true,
+	}
+	assert.NotPanics(t, func() { ret, err = DeepCopy(src) })
+	assert.Equal(t, src, ret)
+	src["key3"] = false
+	assert.NotEqual(t, src, ret)
+	assert.Nil(t, err)
+}