Fix: vela show docs can't generate composition type (#5163)

* Fix: vela show can not display or result

Signed-off-by: Jianbo Sun <jianbo.sjb@alibaba-inc.com>

* Fix: vela show docs can't generate composition type

Signed-off-by: Jianbo Sun <jianbo.sjb@alibaba-inc.com>

Signed-off-by: Jianbo Sun <jianbo.sjb@alibaba-inc.com>

references/docgen/def-doc/trait/env.eg.md

@@ -28,4 +28,30 @@ spec:
                 env:
                   key_for_nginx_first: value_first
                   key_for_nginx_second: value_second
+```
+
+```yaml
+apiVersion: core.oam.dev/v1beta1
+kind: Application
+metadata:
+  name: busybox
+spec:
+  components:
+    - name: busybox
+      type: webservice
+      properties:
+        image: busybox
+        cmd: ["sleep", "86400"]
+      traits:
+        - type: sidecar
+          properties:
+            name: sidecar-nginx
+            image: nginx
+        - type: env
+          properties:
+            # you can use env to control one container, if containerName not specified, it will patch on the first index container 
+            containerName: busybox
+            env:
+              key_for_busybox_first: value_first
+              key_for_busybox_second: value_second
 ```

references/docgen/markdown.go

@@ -24,19 +24,15 @@ import (
 	"sort"
 	"strings"
 
+	"github.com/kubevela/workflow/pkg/cue/packages"
+	"github.com/pkg/errors"
 	"golang.org/x/text/cases"
 	"golang.org/x/text/language"
-
-	"github.com/oam-dev/kubevela/pkg/oam/discoverymapper"
-
 	"k8s.io/klog/v2"
 
-	"github.com/pkg/errors"
-
-	"github.com/kubevela/workflow/pkg/cue/packages"
-
 	"github.com/oam-dev/kubevela/apis/types"
 	"github.com/oam-dev/kubevela/pkg/cue"
+	"github.com/oam-dev/kubevela/pkg/oam/discoverymapper"
 	"github.com/oam-dev/kubevela/pkg/utils/common"
 )
 

references/docgen/parser.go

@@ -156,8 +156,66 @@ func (ref *ParseReference) prepareConsoleParameter(tableName string, parameterLi
 	return ConsoleReference{TableName: tableName, TableObject: table}
 }
 
-// parseParameters parses every parameter
-// TODO(wonderflowe2e/plugin/plugin_test.go:122): refactor the code to reduce the complexity
+func cueValue2Ident(val cue.Value) *ast.Ident {
+	var ident *ast.Ident
+	if source, ok := val.Source().(*ast.Ident); ok {
+		ident = source
+	}
+	if source, ok := val.Source().(*ast.Field); ok {
+		if v, ok := source.Value.(*ast.Ident); ok {
+			ident = v
+		}
+	}
+	return ident
+}
+
+func getIndentName(val cue.Value) string {
+	ident := cueValue2Ident(val)
+	if ident != nil && len(ident.Name) != 0 {
+		return strings.TrimPrefix(ident.Name, "#")
+	}
+	return val.IncompleteKind().String()
+}
+
+func getConcreteOrValueType(val cue.Value) string {
+	op, elements := val.Expr()
+	if op != cue.OrOp {
+		return val.IncompleteKind().String()
+	}
+	var printTypes []string
+	for _, ev := range elements {
+		incompleteKind := ev.IncompleteKind().String()
+		if !ev.IsConcrete() {
+			return incompleteKind
+		}
+		ident := cueValue2Ident(ev)
+		if ident != nil && len(ident.Name) != 0 {
+			printTypes = append(printTypes, strings.TrimPrefix(ident.Name, "#"))
+		} else {
+			// only convert string in `or` operator for now
+			opName, err := ev.String()
+			if err != nil {
+				return incompleteKind
+			}
+			opName = `"` + opName + `"`
+			printTypes = append(printTypes, opName)
+		}
+	}
+	return strings.Join(printTypes, " or ")
+}
+
+func getSuffix(capName string, containSuffix bool) (string, string) {
+	var suffixTitle = " (" + capName + ")"
+	var suffixRef = "-" + strings.ToLower(capName)
+	if !containSuffix || capName == "" {
+		suffixTitle = ""
+		suffixRef = ""
+	}
+	return suffixTitle, suffixRef
+}
+
+// parseParameters parses every parameter to docs
+// TODO(wonderflow): refactor the code to reduce the complexity
 // nolint:staticcheck,gocyclo
 func (ref *ParseReference) parseParameters(capName string, paraValue cue.Value, paramKey string, depth int, containSuffix bool) (string, []ConsoleReference, error) {
 	var doc string
@@ -167,19 +225,15 @@ func (ref *ParseReference) parseParameters(capName string, paraValue cue.Value,
 	if !paraValue.Exists() {
 		return "", console, nil
 	}
+	suffixTitle, suffixRef := getSuffix(capName, containSuffix)
 
-	var suffixTitle = " (" + capName + ")"
-	var suffixRef = "-" + strings.ToLower(capName)
-	if !containSuffix || capName == "" {
-		suffixTitle = ""
-		suffixRef = ""
-	}
 	switch paraValue.Kind() {
 	case cue.StructKind:
 		arguments, err := paraValue.Struct()
 		if err != nil {
-			return "", nil, fmt.Errorf("arguments not defined as struct %w", err)
+			return "", nil, fmt.Errorf("field %s not defined as struct %w", paramKey, err)
 		}
+
 		if arguments.Len() == 0 {
 			var param ReferenceParameter
 			param.Name = "\\-"
@@ -200,7 +254,7 @@ func (ref *ParseReference) parseParameters(capName string, paraValue cue.Value,
 				continue
 			}
 			val := fi.Value
-			name := fi.Name
+			name := fi.Selector
 			param.Name = name
 			if def, ok := val.Default(); ok && def.IsConcrete() {
 				param.Default = velacue.GetDefault(def)
@@ -212,19 +266,18 @@ func (ref *ParseReference) parseParameters(capName string, paraValue cue.Value,
 			case cue.StructKind:
 				if subField, err := val.Struct(); err == nil && subField.Len() == 0 { // err cannot be not nil,so ignore it
 					if mapValue, ok := val.Elem(); ok {
-						var ident *ast.Ident
-						if source, ok := mapValue.Source().(*ast.Ident); ok {
-							ident = source
-						}
-						if source, ok := mapValue.Source().(*ast.Field); ok {
-							if v, ok := source.Value.(*ast.Ident); ok {
-								ident = v
+						indentName := getIndentName(mapValue)
+						_, err := mapValue.Fields()
+						if err == nil {
+							subDoc, subConsole, err := ref.parseParameters(capName, mapValue, indentName, depth+1, containSuffix)
+							if err != nil {
+								return "", nil, err
 							}
-						}
-						if ident != nil && len(ident.Name) != 0 {
-							param.PrintableType = fmt.Sprintf("map[string]:%s", ident.Name)
+							param.PrintableType = fmt.Sprintf("map[string]%s(#%s%s)", indentName, strings.ToLower(indentName), suffixRef)
+							doc += subDoc
+							console = append(console, subConsole...)
 						} else {
-							param.PrintableType = fmt.Sprintf("map[string]:%s", mapValue.IncompleteKind().String())
+							param.PrintableType = "map[string]" + mapValue.IncompleteKind().String()
 						}
 					} else {
 						param.PrintableType = val.IncompleteKind().String()
@@ -234,7 +287,10 @@ func (ref *ParseReference) parseParameters(capName string, paraValue cue.Value,
 					if op == cue.OrOp {
 						var printTypes []string
 						for idx, ev := range elements {
-							opName := fmt.Sprintf("%s-option-%d", name, idx)
+							opName := getIndentName(ev)
+							if opName == "struct" {
+								opName = fmt.Sprintf("type-option-%d", idx+1)
+							}
 							subDoc, subConsole, err := ref.parseParameters(capName, ev, opName, depth+1, containSuffix)
 							if err != nil {
 								return "", nil, err
@@ -276,16 +332,35 @@ func (ref *ParseReference) parseParameters(capName string, paraValue cue.Value,
 					param.PrintableType = fmt.Sprintf("[]%s", elem.IncompleteKind().String())
 				}
 			default:
-				param.PrintableType = param.Type.String()
+				param.PrintableType = getConcreteOrValueType(val)
 			}
 			params = append(params, param)
 		}
 	default:
 		var param ReferenceParameter
-		// TODO better composition type handling, see command trait.
-		param.Name = "--"
-		param.Usage = "Composition type"
-		param.PrintableType = extractTypeFromError(paraValue)
+		op, elements := paraValue.Expr()
+		if op == cue.OrOp {
+			var printTypes []string
+			for idx, ev := range elements {
+				opName := getIndentName(ev)
+				if opName == "struct" {
+					opName = fmt.Sprintf("type-option-%d", idx+1)
+				}
+				subDoc, subConsole, err := ref.parseParameters(capName, ev, opName, depth+1, containSuffix)
+				if err != nil {
+					return "", nil, err
+				}
+				printTypes = append(printTypes, fmt.Sprintf("[%s](#%s%s)", opName, strings.ToLower(opName), suffixRef))
+				doc += subDoc
+				console = append(console, subConsole...)
+			}
+			param.PrintableType = strings.Join(printTypes, " or ")
+		} else {
+			// TODO more composition type to be handle here
+			param.Name = "--"
+			param.Usage = "Unsupported Composition Type"
+			param.PrintableType = extractTypeFromError(paraValue)
+		}
 		params = append(params, param)
 	}
 

references/docgen/parser_test.go

@@ -19,7 +19,6 @@ package docgen
 import (
 	"encoding/json"
 	"fmt"
-	"io/ioutil"
 	"os"
 	"path/filepath"
 	"reflect"
@@ -562,34 +561,272 @@ func TestParseLocalFile(t *testing.T) {
 }
 
 func TestExtractParameter(t *testing.T) {
-	ref := &ConsoleReference{}
-	cueTemplate := `
-parameter: {
+	testcases := map[string]struct {
+		cueTemplate string
+		contains    string
+	}{
+		"normal-case": {
+			cueTemplate: `parameter: {
+	str: string
+	itr: int
+	btr: bool
+	ct: cts: string
+}`,
+			contains: `### normal-case
+
+ Name | Description | Type | Required | Default 
+ ---- | ----------- | ---- | -------- | ------- 
+ str |  | string | true |  
+ itr |  | int | true |  
+ btr |  | bool | true |  
+ ct |  | [ct](#ct) | true |  
+
+
+#### ct
+
+ Name | Description | Type | Required | Default 
+ ---- | ----------- | ---- | -------- | ------- 
+ cts |  | string | true |`,
+		},
+		"normal-map-string-string": {
+			cueTemplate: `parameter: {
+	envMappings: [string]: string
+}`,
+			contains: `### normal-map-string-string
+
+ Name | Description | Type | Required | Default 
+ ---- | ----------- | ---- | -------- | ------- 
+ envMappings |  | map[string]string | true |`,
+		},
+		"normal-map-case": {
+			cueTemplate: `parameter: {
 	// +usage=The mapping of environment variables to secret
 	envMappings: [string]: #KeySecret
 }
 #KeySecret: {
 	key?:   string
 	secret: string
-}
-`
-	oldStdout := os.Stdout
-	defer func() {
-		os.Stdout = oldStdout
-	}()
+}`,
+			contains: `### normal-map-case
 
-	r, w, _ := os.Pipe()
-	os.Stdout = w
-	cueValue, _ := common.GetCUEParameterValue(cueTemplate, nil)
-	defaultDepth := 0
-	defaultDisplay := "console"
-	ref.DisplayFormat = defaultDisplay
-	_, console, err := ref.parseParameters("", cueValue, Specification, defaultDepth, false)
-	assert.NoError(t, err)
-	assert.Equal(t, 1, len(console))
-	console[0].TableObject.Render()
-	err = w.Close()
-	assert.NoError(t, err)
-	out, _ := ioutil.ReadAll(r)
-	assert.Contains(t, string(out), "map[string]:#KeySecret")
+ Name | Description | Type | Required | Default 
+ ---- | ----------- | ---- | -------- | ------- 
+ envMappings | The mapping of environment variables to secret. | map[string]KeySecret(#keysecret) | true |  
+
+
+#### KeySecret
+
+ Name | Description | Type | Required | Default 
+ ---- | ----------- | ---- | -------- | ------- 
+ key |  | string | false |  
+ secret |  | string | true |`,
+		},
+		"or-case-with-type": {
+			cueTemplate: `	parameter: {
+	   		orValue:  #KeyConfig | #KeySecret
+	   	}
+
+	   	#KeySecret: {
+	   		key:   "abc"
+	   		secret: string
+	   	}
+
+	   	#KeyConfig: {
+	   		key:   "abc"
+	   		config: string
+	   	}`,
+			contains: `### or-case-with-type
+
+ Name | Description | Type | Required | Default 
+ ---- | ----------- | ---- | -------- | ------- 
+ orValue |  | [KeyConfig](#keyconfig) or [KeySecret](#keysecret) | true |  
+
+
+#### KeyConfig
+
+ Name | Description | Type | Required | Default 
+ ---- | ----------- | ---- | -------- | ------- 
+ key |  | string | true |  
+ config |  | string | true |  
+
+
+#### KeySecret
+
+ Name | Description | Type | Required | Default 
+ ---- | ----------- | ---- | -------- | ------- 
+ key |  | string | true |  
+ secret |  | string | true | `,
+		},
+		"or-type-with-const-str": {
+			cueTemplate: `parameter: {
+	  type: *"configMap" | "secret" | "emptyDir" | "ephemeral"
+}`,
+			contains: `### or-type-with-const-str
+
+ Name | Description | Type | Required | Default 
+ ---- | ----------- | ---- | -------- | ------- 
+ type |  | "configMap" or "secret" or "emptyDir" or "ephemeral" | false | configMap`,
+		},
+		"or-type-with-const-and-string": {
+			cueTemplate: `parameter: {
+	  type: *"configMap" | "secret" | "emptyDir" | string
+}`,
+			contains: `### or-type-with-const-and-string
+
+ Name | Description | Type | Required | Default 
+ ---- | ----------- | ---- | -------- | ------- 
+ type |  | string | false | configMap`,
+		},
+		"var-or-with-struct-var": {
+			cueTemplate: `
+	   	parameter: {
+	   		orValue:  KeyConfig | KeySecret
+	   	}
+
+	   	KeySecret: {
+	   		key:   "abc"
+	   		secret: string
+	   	}
+
+	   	KeyConfig: {
+	   		key:   "abc"
+	   		config: string
+	   	}`,
+			contains: `### var-or-with-struct-var
+
+ Name | Description | Type | Required | Default 
+ ---- | ----------- | ---- | -------- | ------- 
+ orValue |  | [KeyConfig](#keyconfig) or [KeySecret](#keysecret) | true |  
+
+
+#### KeyConfig
+
+ Name | Description | Type | Required | Default 
+ ---- | ----------- | ---- | -------- | ------- 
+ key |  | string | true |  
+ config |  | string | true |  
+
+
+#### KeySecret
+
+ Name | Description | Type | Required | Default 
+ ---- | ----------- | ---- | -------- | ------- 
+ key |  | string | true |  
+ secret |  | string | true | `,
+		},
+	}
+
+	ref := &MarkdownReference{}
+	for key, ca := range testcases {
+		cueValue, _ := common.GetCUEParameterValue(ca.cueTemplate, nil)
+		out, _, err := ref.parseParameters("", cueValue, key, 0, false)
+		assert.NoError(t, err, key)
+		assert.Contains(t, out, ca.contains, key)
+	}
+}
+
+func TestExtractParameterFromFiles(t *testing.T) {
+	ref := &MarkdownReference{}
+	testcases := map[string]struct {
+		path     string
+		contains string
+	}{
+		"env": {
+			path: "testdata/parameter/env.cue",
+			contains: `### env
+
+ Name | Description | Type | Required | Default 
+ ---- | ----------- | ---- | -------- | ------- 
+  |  | [PatchParams](#patchparams) or [type-option-2](#type-option-2) | false |  
+
+
+#### PatchParams
+
+ Name | Description | Type | Required | Default 
+ ---- | ----------- | ---- | -------- | ------- 
+ containerName | Specify the name of the target container, if not set, use the component name. | string | false | empty 
+ replace | Specify if replacing the whole environment settings for the container. | bool | false | false 
+ env | Specify the  environment variables to merge, if key already existing, override its value. | map[string]string | true |  
+ unset | Specify which existing environment variables to unset. | []string | true |  
+
+
+#### type-option-2
+
+ Name | Description | Type | Required | Default 
+ ---- | ----------- | ---- | -------- | ------- 
+ containers | Specify the environment variables for multiple containers. | [[]containers](#containers) | true |  
+
+
+##### containers
+
+ Name | Description | Type | Required | Default 
+ ---- | ----------- | ---- | -------- | ------- 
+ containerName | Specify the name of the target container, if not set, use the component name. | string | false | empty 
+ replace | Specify if replacing the whole environment settings for the container. | bool | false | false 
+ env | Specify the  environment variables to merge, if key already existing, override its value. | map[string]string | true |  
+ unset | Specify which existing environment variables to unset. | []string | true |`,
+		},
+		"command": {
+			path: "testdata/parameter/command.cue",
+			contains: `### command
+
+ Name | Description | Type | Required | Default 
+ ---- | ----------- | ---- | -------- | ------- 
+  |  | [PatchParams](#patchparams) or [type-option-2](#type-option-2) | false |  
+
+
+#### PatchParams
+
+ Name | Description | Type | Required | Default 
+ ---- | ----------- | ---- | -------- | ------- 
+ containerName | Specify the name of the target container, if not set, use the component name. | string | false | empty 
+ command | Specify the command to use in the target container, if not set, it will not be changed. | null | true |  
+ args | Specify the args to use in the target container, if set, it will override existing args. | null | true |  
+ addArgs | Specify the args to add in the target container, existing args will be kept, cannot be used with args. | null | true |  
+ delArgs | Specify the existing args to delete in the target container, cannot be used with args. | null | true |  
+
+
+#### type-option-2
+
+ Name | Description | Type | Required | Default 
+ ---- | ----------- | ---- | -------- | ------- 
+ containers | Specify the commands for multiple containers. | [[]containers](#containers) | true |  
+
+
+##### containers
+
+ Name | Description | Type | Required | Default 
+ ---- | ----------- | ---- | -------- | ------- 
+ containerName | Specify the name of the target container, if not set, use the component name. | string | false | empty 
+ command | Specify the command to use in the target container, if not set, it will not be changed. | null | true |  
+ args | Specify the args to use in the target container, if set, it will override existing args. | null | true |  
+ addArgs | Specify the args to add in the target container, existing args will be kept, cannot be used with args. | null | true |  
+ delArgs | Specify the existing args to delete in the target container, cannot be used with args. | null | true |`,
+		},
+		"condition": {
+			path: "testdata/parameter/condition.cue",
+			contains: `### condition
+
+ Name | Description | Type | Required | Default 
+ ---- | ----------- | ---- | -------- | ------- 
+ volumes |  | [[]volumes](#volumes) | true |  
+
+
+#### volumes
+
+ Name | Description | Type | Required | Default 
+ ---- | ----------- | ---- | -------- | ------- 
+ name |  | string | true |  
+ defaultMode | only works when type equals configmap. | int | false | 420 
+ type |  | "configMap" or "secret" or "emptyDir" or "ephemeral" | false | configMap`,
+		},
+	}
+	for key, ca := range testcases {
+		content, err := os.ReadFile(ca.path)
+		assert.NoError(t, err, ca.path)
+		cueValue, _ := common.GetCUEParameterValue(string(content), nil)
+		out, _, err := ref.parseParameters("", cueValue, key, 0, false)
+		assert.NoError(t, err, key)
+		assert.Contains(t, out, ca.contains, key)
+	}
 }

references/docgen/testdata/parameter/command.cue

@@ -0,0 +1,17 @@
+#PatchParams: {
+	// +usage=Specify the name of the target container, if not set, use the component name
+	containerName: *"" | string
+	// +usage=Specify the command to use in the target container, if not set, it will not be changed
+	command: *null | [...string]
+	// +usage=Specify the args to use in the target container, if set, it will override existing args
+	args: *null | [...string]
+	// +usage=Specify the args to add in the target container, existing args will be kept, cannot be used with args
+	addArgs: *null | [...string]
+	// +usage=Specify the existing args to delete in the target container, cannot be used with args
+	delArgs: *null | [...string]
+}
+
+parameter: *#PatchParams | close({
+	// +usage=Specify the commands for multiple containers
+	containers: [...#PatchParams]
+})

references/docgen/testdata/parameter/condition.cue

@@ -0,0 +1,10 @@
+parameter: {
+	volumes: [...{
+		name: string
+		type: *"configMap" | "secret" | "emptyDir" | "ephemeral"
+		if type == "configMap" {
+			//+usage=only works when type equals configmap
+			defaultMode: *420 | int
+		}},
+	]
+}

references/docgen/testdata/parameter/env.cue

@@ -0,0 +1,14 @@
+#PatchParams: {
+	// +usage=Specify the name of the target container, if not set, use the component name
+	containerName: *"" | string
+	// +usage=Specify if replacing the whole environment settings for the container
+	replace: *false | bool
+	// +usage=Specify the  environment variables to merge, if key already existing, override its value
+	env: [string]: string
+	// +usage=Specify which existing environment variables to unset
+	unset: *[] | [...string]
+}
+parameter: *#PatchParams | close({
+	// +usage=Specify the environment variables for multiple containers
+	containers: [...#PatchParams]
+})