Repository: caicloud/nirvana
Pull request #414 (2020-12-11 05:50:35)

fix(utils): 确保 params 跟 results 的个数与函数的参数及返回值的个数相同
**What this PR does / why we need it**:

目前如果 parameters 的个数比函数的参数个数多，或者 results 的个数比函数的返回值的个数多的话，那么在执行 `nirvana api` 或 `nirvana client` 的时候 nirvana 就会 panic:
```
panic: runtime error: index out of range [1] with length 1

goroutine 1 [running]:
github.com/caicloud/nirvana/utils/api.NewDefinition(0xc00d6038f0, 0xc00022b8f8, 0x157a5e4, 0x3, 0xc005401f00, 0x0, 0x0)
        /Users/knight/go/pkg/mod/github.com/caicloud/nirvana@v0.3.0-rc.3/utils/api/definitions.go:147 +0xe25
github.com/caicloud/nirvana/utils/api.NewDefinitions(0xc00d6038f0, 0xc000164b40, 0x1, 0x1, 0x157a5e4, 0x3, 0xc005401b00, 0x1, 0x1, 0x0, ...)
        /Users/knight/go/pkg/mod/github.com/caicloud/nirvana@v0.3.0-rc.3/utils/api/definitions.go:175 +0x148
github.com/caicloud/nirvana/utils/api.NewPathDefinitions(0xc00d6038f0, 0xc00d603950, 0x157a5e4, 0x3, 0x0, 0x0, 0x0)
        /Users/knight/go/pkg/mod/github.com/caicloud/nirvana@v0.3.0-rc.3/utils/api/definitions.go:188 +0x10b
github.com/caicloud/nirvana/utils/api.(*Container).Generate(0xc00d61e040, 0x157a5e4, 0x3, 0xc00022bd38, 0x1, 0x1)
        /Users/knight/go/pkg/mod/github.com/caicloud/nirvana@v0.3.0-rc.3/utils/api/api.go:144 +0x179
main.main()
        /Users/knight/go/src/github.com/caicloud/redacted/nirvana-generated196043317/main.go:30 +0x610
exit status 2
FATAL 1211-13:45:34.841+08 api.go:54 | exit status 1
```

这个 PR 通过先校验两边的个数是否相等来避免 panic，同时输出一个比较用户友好的错误信息。

**Which issue(s) this PR is related to** *(optional, link to 3rd issue(s))*:

Fixes #

Reference to #
<!-- 填在 Fixes，PR 合并就会关 issue。填在 Reference to 会关联 issue，不会联动关闭。-->

**Special notes for your reviewer**:

/cc @iawia002 

<!-- Please answer the following questions during the code freeze, and delete this line.
**Code freeze questions**

1. What causes this PR to not be merged before code freeze?
2. Why this PR is absolutely necessary for this version? Paste a screenshot of smoke testing docs if you could.
3. What's the effects after merging it?
4. Is there anyway we can skip this to not affect the overall process?
-->

**Release note**:
<!--  Write your release note:
1. Enter your extended release note in the below block. If the PR requires additional action from users switching to the new release, include the string "action required".
2. If no release note is required, just write "NONE".
-->

```release-note
NONE
```

<!--  Thanks for sending a pull request! Here are some tips:

1. https://github.com/caicloud/engineering/blob/master/guidelines/review_conventions.md      <-- what is the review process looks like
2. https://github.com/caicloud/engineering/blob/master/guidelines/git_commit_conventions.md  <-- how to structure your git commit
3. https://github.com/caicloud/engineering/blob/master/guidelines/caicloud_bot.md            <-- how to work with caicloud bot

Other tips:

If this is your first contribution, read our Getting Started guide https://github.com/caicloud/engineering/blob/master/guidelines/README.md
-->


caicloud-bot: [APPROVALNOTIFIER] This PR is **NOT APPROVED**

This pull-request has been approved by: 
To fully approve this pull request, please assign additional approvers.
We suggest the following additional approver: **ddysher**

Assign the PR to them by writing `/assign @ddysher` in a comment when ready.

The full list of commands accepted by this bot can be found [here](https://github.com/caicloud/engineering/blob/master/guidelines/caicloud_bot.md#bot-commands).

The pull request process is described [here](https://github.com/caicloud/engineering/blob/master/guidelines/caicloud_bot.md#typical-workflow)

<details open>
Needs approval from an approver in each of these files:

- **[OWNERS](https://github.com/caicloud/nirvana/blob/master/OWNERS)**

Approvers can indicate their approval by writing `/approve` in a comment
Approvers can cancel approval by writing `/approve cancel` in a comment
</details>
<!-- META={"approvers":["ddysher"]} -->
iawia002: /lgtm
/cherrypick release-0.3
caicloud-bot: @iawia002: once the present PR merges, I will cherry-pick it on top of release-0.3 in a new PR and assign it to you.

<details>

In response to [this](https://github.com/caicloud/nirvana/pull/414#issuecomment-742999836):

>/lgtm
>/cherrypick release-0.3


Instructions for interacting with me using PR comments are available [here](https://github.com/caicloud/engineering/blob/master/docs/caicloud_bot.md).  If you have questions or suggestions related to my behavior, please file an issue against the [kubernetes/test-infra](https://github.com/kubernetes/test-infra/issues/new?title=Prow%20issue:) repository.
</details>
caicloud-bot: [APPROVALNOTIFIER] This PR is **APPROVED**

This pull-request has been approved by: *<a href="https://github.com/caicloud/nirvana/pull/414#issuecomment-742999836" title="LGTM">iawia002</a>*

The full list of commands accepted by this bot can be found [here](https://github.com/caicloud/engineering/blob/master/guidelines/caicloud_bot.md#bot-commands).

The pull request process is described [here](https://github.com/caicloud/engineering/blob/master/guidelines/caicloud_bot.md#typical-workflow)

<details >
Needs approval from an approver in each of these files:

- ~~[OWNERS](https://github.com/caicloud/nirvana/blob/master/OWNERS)~~ [iawia002]

Approvers can indicate their approval by writing `/approve` in a comment
Approvers can cancel approval by writing `/approve cancel` in a comment
</details>
<!-- META={"approvers":[]} -->

Patch:
diff --git a/utils/api/definitions.go b/utils/api/definitions.go
index 92021a4..2c60e7b 100644
--- a/utils/api/definitions.go
+++ b/utils/api/definitions.go
@@ -18,6 +18,7 @@ package api
 
 import (
 	"encoding/json"
+	"fmt"
 	"net/http"
 
 	"github.com/caicloud/nirvana/definition"
@@ -120,6 +121,12 @@ func NewDefinition(tc *TypeContainer, d *definition.Definition, apiStyle service
 		cd.HTTPCode = http.StatusOK
 	}
 	functionType := tc.Type(cd.Function)
+	if len(functionType.In) != len(d.Parameters) {
+		return nil, fmt.Errorf("the number of parameters and function args are not equal: len(params)=%d, len(funcArgs)=%d", len(d.Parameters), len(functionType.In))
+	}
+	if len(functionType.Out) != len(d.Results) {
+		return nil, fmt.Errorf("the number of results and function return values are not equal: len(results)=%d, len(funcVals)=%d", len(d.Results), len(functionType.Out))
+	}
 	for i, p := range d.Parameters {
 		param := Parameter{
 			Source:      p.Source,
@@ -174,7 +181,7 @@ func NewDefinitions(tc *TypeContainer, definitions []definition.Definition, apiS
 	for i, d := range definitions {
 		cd, err := NewDefinition(tc, &d, apiStyle)
 		if err != nil {
-			return nil, err
+			return nil, fmt.Errorf("func=%s: %w", tc.NameOfInstance(d.Function), err)
 		}
 		result[i] = *cd
 	}
@@ -187,7 +194,7 @@ func NewPathDefinitions(tc *TypeContainer, definitions map[string][]definition.D
 	for path, defs := range definitions {
 		cds, err := NewDefinitions(tc, defs, apiStyle)
 		if err != nil {
-			return nil, err
+			return nil, fmt.Errorf("definitions of path %s: %w", path, err)
 		}
 		result[path] = cds
 


Test patch:
diff --git a/utils/api/definitions_test.go b/utils/api/definitions_test.go
new file mode 100644
index 0000000..99edfff
--- /dev/null
+++ b/utils/api/definitions_test.go
@@ -0,0 +1,114 @@
+/*
+Copyright 2020 Caicloud Authors
+
+Licensed under the Apache License, Version 2.0 (the "License");
+you may not use this file except in compliance with the License.
+You may obtain a copy of the License at
+
+    http://www.apache.org/licenses/LICENSE-2.0
+
+Unless required by applicable law or agreed to in writing, software
+distributed under the License is distributed on an "AS IS" BASIS,
+WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+See the License for the specific language governing permissions and
+limitations under the License.
+*/
+
+package api
+
+import (
+	"strings"
+	"testing"
+
+	"github.com/caicloud/nirvana/definition"
+	"github.com/caicloud/nirvana/service"
+)
+
+func TestNewDefinitionShouldNotPanic(t *testing.T) {
+	testCases := []struct {
+		name      string
+		d         *definition.Definition
+		expectErr string
+	}{
+		{
+			name: "len(params) < len(args)",
+			d: &definition.Definition{
+				Method:     definition.Create,
+				Function:   func(s string) {},
+				Parameters: []definition.Parameter{},
+				Results:    []definition.Result{},
+			},
+			expectErr: "the number of parameters and function args are not equal",
+		},
+		{
+			name: "len(params) > len(args)",
+			d: &definition.Definition{
+				Method:   definition.Create,
+				Function: func() {},
+				Parameters: []definition.Parameter{
+					definition.QueryParameterFor("Action", "action"),
+				},
+				Results: []definition.Result{},
+			},
+			expectErr: "the number of parameters and function args are not equal",
+		},
+		{
+			name: "len(params) = len(args)",
+			d: &definition.Definition{
+				Method:   definition.Create,
+				Function: func(action string) {},
+				Parameters: []definition.Parameter{
+					definition.QueryParameterFor("Action", "action"),
+				},
+				Results: []definition.Result{},
+			},
+		},
+		{
+			name: "len(results) < len(returns)",
+			d: &definition.Definition{
+				Method:     definition.Create,
+				Function:   func() error { return nil },
+				Parameters: []definition.Parameter{},
+				Results:    []definition.Result{},
+			},
+			expectErr: "the number of results and function return values are not equal",
+		},
+		{
+			name: "len(results) > len(returns)",
+			d: &definition.Definition{
+				Method:     definition.Create,
+				Function:   func() {},
+				Parameters: []definition.Parameter{},
+				Results:    []definition.Result{definition.ErrorResult()},
+			},
+			expectErr: "the number of results and function return values are not equal",
+		},
+		{
+			name: "len(results) = len(returns)",
+			d: &definition.Definition{
+				Method:     definition.Create,
+				Function:   func() error { return nil },
+				Parameters: []definition.Parameter{},
+				Results:    []definition.Result{definition.ErrorResult()},
+			},
+		},
+	}
+
+	for _, testCase := range testCases {
+		t.Run(testCase.name, func(t *testing.T) {
+			_, err := NewDefinition(NewTypeContainer(), testCase.d, service.APIStyleRPC)
+			if len(testCase.expectErr) > 0 {
+				if err == nil {
+					t.Fatalf("Unexpected success")
+				}
+				if !strings.Contains(err.Error(), testCase.expectErr) {
+					t.Fatalf("Unexpcted error: expected=%s\ngot=%s", testCase.expectErr, err.Error())
+				}
+				return
+			}
+			if err != nil {
+				t.Fatalf("Unexpected error: %v", err)
+			}
+		})
+	}
+}