Repository: cloudwego/hertz
Pull request #1119 (2024-05-17 05:31:31)

optimize: optimize the error message of the `required` constraint for binding
#### What type of PR is this?
<!--
Add one of the following kinds:

build: Changes that affect the build system or external dependencies (example scopes: gulp, broccoli, npm)
ci: Changes to our CI configuration files and scripts (example scopes: Travis, Circle, BrowserStack, SauceLabs)
docs: Documentation only changes
feat: A new feature
optimize: A new optimization
fix: A bug fix
perf: A code change that improves performance
refactor: A code change that neither fixes a bug nor adds a feature
style: Changes that do not affect the meaning of the code (white space, formatting, missing semi-colons, etc)
test: Adding missing tests or correcting existing tests
chore: Changes to the build process or auxiliary tools and libraries such as documentation generation
-->
optimize
#### Check the PR title.
<!--
The description of the title will be attached in Release Notes, 
so please describe it from user-oriented, what this PR does / why we need it.
Please check your PR title with the below requirements:
-->
- [x] This PR title match the format: \<type\>(optional scope): \<description\>
- [x] The description of this PR title is user-oriented and clear enough for others to understand.
- [ ] Attach the PR updating the user documentation if the current PR requires user awareness at the usage level. [User docs repo](https://github.com/cloudwego/cloudwego.github.io)


#### (Optional) Translate the PR title into Chinese.


#### (Optional) More detailed description for this PR(en: English/zh: Chinese).
<!--
Provide more detailed info for review(e.g., it's recommended to provide perf data if this is a perf type PR).
-->
en:

If we have the binding or params definition:

```go

type Params struct {
	AccId string `query:"accountId,required"
}
```

The current error message will be:

```
"'AccId' field is a 'required' parameter, but the request does not have this parameter"
```

This will be confused for the API caller.

The error message will be
```
"'accountId' field is a 'required' parameter, but the request does not have this parameter"
```
with this change.

zh(optional):

#### (Optional) Which issue(s) this PR fixes:
<!--
Automatically closes linked issue when PR is merged.
Eg: `Fixes #<issue number>`, or `Fixes (paste link of issue)`.
-->

#### (Optional) The PR that updates user documentation:
<!--
If the current PR requires user awareness at the usage level, please submit a PR to update user docs. [User docs repo](https://github.com/cloudwego/cloudwego.github.io)
-->

FGYFFFF: 可以看看其他地方的 Required 校验是否也有类似的 case，可以一并修改
haoxins: > 可以看看其他地方的 Required 校验是否也有类似的 case，可以一并修改

done
haoxins: I also created an issue https://github.com/bytedance/go-tagexpr/issues/81 to discuss the possibility of enhancing the validation error message.

Patch:
diff --git a/pkg/app/server/binding/internal/decoder/base_type_decoder.go b/pkg/app/server/binding/internal/decoder/base_type_decoder.go
index 9c5cb12..bf26c9c 100644
--- a/pkg/app/server/binding/internal/decoder/base_type_decoder.go
+++ b/pkg/app/server/binding/internal/decoder/base_type_decoder.go
@@ -75,7 +75,7 @@ func (d *baseTypeFieldTextDecoder) Decode(req *protocol.Request, params param.Pa
 				if found {
 					err = nil
 				} else {
-					err = fmt.Errorf("'%s' field is a 'required' parameter, but the request body does not have this parameter '%s'", d.fieldName, tagInfo.JSONName)
+					err = fmt.Errorf("'%s' field is a 'required' parameter, but the request body does not have this parameter '%s'", tagInfo.Value, tagInfo.JSONName)
 				}
 				if len(tagInfo.Default) != 0 && keyExist(req, tagInfo) {
 					defaultValue = ""
@@ -90,7 +90,7 @@ func (d *baseTypeFieldTextDecoder) Decode(req *protocol.Request, params param.Pa
 			break
 		}
 		if tagInfo.Required {
-			err = fmt.Errorf("'%s' field is a 'required' parameter, but the request does not have this parameter", d.fieldName)
+			err = fmt.Errorf("'%s' field is a 'required' parameter, but the request does not have this parameter", tagInfo.Value)
 		}
 	}
 	if err != nil {
diff --git a/pkg/app/server/binding/internal/decoder/map_type_decoder.go b/pkg/app/server/binding/internal/decoder/map_type_decoder.go
index 59fed77..18e15c9 100644
--- a/pkg/app/server/binding/internal/decoder/map_type_decoder.go
+++ b/pkg/app/server/binding/internal/decoder/map_type_decoder.go
@@ -67,7 +67,7 @@ func (d *mapTypeFieldTextDecoder) Decode(req *protocol.Request, params param.Par
 				if found {
 					err = nil
 				} else {
-					err = fmt.Errorf("'%s' field is a 'required' parameter, but the request does not have this parameter", d.fieldName)
+					err = fmt.Errorf("'%s' field is a 'required' parameter, but the request does not have this parameter", tagInfo.Value)
 				}
 				if len(tagInfo.Default) != 0 && keyExist(req, tagInfo) {
 					defaultValue = ""
@@ -82,7 +82,7 @@ func (d *mapTypeFieldTextDecoder) Decode(req *protocol.Request, params param.Par
 			break
 		}
 		if tagInfo.Required {
-			err = fmt.Errorf("'%s' field is a 'required' parameter, but the request does not have this parameter", d.fieldName)
+			err = fmt.Errorf("'%s' field is a 'required' parameter, but the request does not have this parameter", tagInfo.Value)
 		}
 	}
 	if err != nil {
diff --git a/pkg/app/server/binding/internal/decoder/slice_type_decoder.go b/pkg/app/server/binding/internal/decoder/slice_type_decoder.go
index c2887d1..7b6c591 100644
--- a/pkg/app/server/binding/internal/decoder/slice_type_decoder.go
+++ b/pkg/app/server/binding/internal/decoder/slice_type_decoder.go
@@ -70,7 +70,7 @@ func (d *sliceTypeFieldTextDecoder) Decode(req *protocol.Request, params param.P
 				if found {
 					err = nil
 				} else {
-					err = fmt.Errorf("'%s' field is a 'required' parameter, but the request does not have this parameter", d.fieldName)
+					err = fmt.Errorf("'%s' field is a 'required' parameter, but the request does not have this parameter", tagInfo.Value)
 				}
 				if len(tagInfo.Default) != 0 && keyExist(req, tagInfo) { //
 					defaultValue = ""
@@ -88,7 +88,7 @@ func (d *sliceTypeFieldTextDecoder) Decode(req *protocol.Request, params param.P
 			break
 		}
 		if tagInfo.Required {
-			err = fmt.Errorf("'%s' field is a 'required' parameter, but the request does not have this parameter", d.fieldName)
+			err = fmt.Errorf("'%s' field is a 'required' parameter, but the request does not have this parameter", tagInfo.Value)
 		}
 	}
 	if err != nil {
diff --git a/pkg/app/server/binding/internal/decoder/struct_type_decoder.go b/pkg/app/server/binding/internal/decoder/struct_type_decoder.go
index 75f3ae4..c2af2c0 100644
--- a/pkg/app/server/binding/internal/decoder/struct_type_decoder.go
+++ b/pkg/app/server/binding/internal/decoder/struct_type_decoder.go
@@ -44,7 +44,7 @@ func (d *structTypeFieldTextDecoder) Decode(req *protocol.Request, params param.
 				if found {
 					err = nil
 				} else {
-					err = fmt.Errorf("'%s' field is a 'required' parameter, but the request does not have this parameter", d.fieldName)
+					err = fmt.Errorf("'%s' field is a 'required' parameter, but the request does not have this parameter", tagInfo.Value)
 				}
 				if len(tagInfo.Default) != 0 && keyExist(req, tagInfo) {
 					defaultValue = ""
@@ -59,7 +59,7 @@ func (d *structTypeFieldTextDecoder) Decode(req *protocol.Request, params param.
 			break
 		}
 		if tagInfo.Required {
-			err = fmt.Errorf("'%s' field is a 'required' parameter, but the request does not have this parameter", d.fieldName)
+			err = fmt.Errorf("'%s' field is a 'required' parameter, but the request does not have this parameter", tagInfo.Value)
 		}
 	}
 	if err != nil {


Test patch:
diff --git a/pkg/app/server/binding/binder_test.go b/pkg/app/server/binding/binder_test.go
index 225b8bd..ec962da 100644
--- a/pkg/app/server/binding/binder_test.go
+++ b/pkg/app/server/binding/binder_test.go
@@ -466,10 +466,14 @@ func TestBind_RequiredBind(t *testing.T) {
 		A int `query:"a,required"`
 	}
 	req := newMockRequest().
+		SetRequestURI("http://foobar.com")
+	err := DefaultBinder().Bind(req.Req, &s, nil)
+	assert.DeepEqual(t, "'a' field is a 'required' parameter, but the request does not have this parameter", err.Error())
+
+	req = newMockRequest().
 		SetRequestURI("http://foobar.com").
 		SetHeader("A", "1")
-
-	err := DefaultBinder().Bind(req.Req, &s, nil)
+	err = DefaultBinder().Bind(req.Req, &s, nil)
 	if err == nil {
 		t.Fatal("expected error")
 	}
@@ -904,6 +908,7 @@ func TestBind_JSONRequiredField(t *testing.T) {
 	if err == nil {
 		t.Errorf("expected an error, but get nil")
 	}
+	assert.DeepEqual(t, "'c' field is a 'required' parameter, but the request body does not have this parameter 'n.n2.c'", err.Error())
 	assert.DeepEqual(t, 1, result.N.A)
 	assert.DeepEqual(t, 2, result.N.B)
 	assert.DeepEqual(t, 0, result.N.N2.C)
@@ -1492,6 +1497,7 @@ func Test_ValidatorErrorFactory(t *testing.T) {
 	if err == nil {
 		t.Fatalf("unexpected nil, expected an error")
 	}
+	assert.DeepEqual(t, "'a' field is a 'required' parameter, but the request does not have this parameter", err.Error())
 
 	type TestValidate struct {
 		B int `query:"b" vd:"$>100"`
diff --git a/pkg/app/server/binding/tagexpr_bind_test.go b/pkg/app/server/binding/tagexpr_bind_test.go
index 1533b5a..1d331e1 100644
--- a/pkg/app/server/binding/tagexpr_bind_test.go
+++ b/pkg/app/server/binding/tagexpr_bind_test.go
@@ -121,7 +121,7 @@ func TestGetBody(t *testing.T) {
 	if err == nil {
 		t.Fatalf("expected an error, but get nil")
 	}
-	assert.DeepEqual(t, err.Error(), "'E' field is a 'required' parameter, but the request body does not have this parameter 'X.e'")
+	assert.DeepEqual(t, err.Error(), "'e' field is a 'required' parameter, but the request body does not have this parameter 'X.e'")
 }
 
 func TestQueryNum(t *testing.T) {
@@ -431,7 +431,7 @@ func TestJSON(t *testing.T) {
 	if err == nil {
 		t.Error("expected an error, but get nil")
 	}
-	assert.DeepEqual(t, err.Error(), "'Y' field is a 'required' parameter, but the request body does not have this parameter 'y'")
+	assert.DeepEqual(t, err.Error(), "'y' field is a 'required' parameter, but the request body does not have this parameter 'y'")
 	assert.DeepEqual(t, []string{"a1", "a2"}, (**recv.X).A)
 	assert.DeepEqual(t, int32(21), (**recv.X).B)
 	assert.DeepEqual(t, &[]uint16{31, 32}, (**recv.X).C)
@@ -753,7 +753,7 @@ func TestOption(t *testing.T) {
 	req = newRequest("", header, nil, bodyReader)
 	recv = new(Recv)
 	err = DefaultBinder().Bind(req.Req, recv, nil)
-	assert.DeepEqual(t, err.Error(), "'C' field is a 'required' parameter, but the request body does not have this parameter 'X.c'")
+	assert.DeepEqual(t, err.Error(), "'c' field is a 'required' parameter, but the request body does not have this parameter 'X.c'")
 	assert.DeepEqual(t, 0, recv.X.C)
 	assert.DeepEqual(t, 0, recv.X.D)
 	assert.DeepEqual(t, "y1", recv.Y)