Skip to content

fix(net/ghttp): 修复当上传文件参数的类型为string时panic的问题 #4203

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Open
wants to merge 2 commits into
base: master
Choose a base branch
from
Open

fix(net/ghttp): 修复当上传文件参数的类型为string时panic的问题 #4203

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
9c174c1
fix(net/ghttp): add type converter for UploadFile and improve file va…
hailaz Mar 14, 2025
937a827
fix(net/ghttp): handle error from RegisterTypeConverterFunc in init f…
hailaz Mar 14, 2025
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
13 changes: 12 additions & 1 deletion net/ghttp/ghttp_request_param_file.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -19,9 +19,15 @@ import (
"github.com/gogf/gf/v2/internal/json"
"github.com/gogf/gf/v2/os/gfile"
"github.com/gogf/gf/v2/os/gtime"
"github.com/gogf/gf/v2/util/gconv"
"github.com/gogf/gf/v2/util/grand"
)

// init initializes the type converters for *UploadFile.
func init() {
_ = gconv.RegisterTypeConverterFunc(stringToUploadFile)
}
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@hailaz @wln32 这里不应该注册全局的转换函数，并且目前针对基础类型的转换函数也只能在结构体属性中生效。我建议是修改gconv组件中相关的转换函数，避免panic，而是返回error。我刚跟了一下代码，具体问题应该出在这

gf/util/gconv/internal/converter/converter_convert.go

Line 459 in bc1e101

convertedValue = reflect.ValueOf(result).Convert(referReflectValue.Type()).Interface()
，但是解决方案的话，还需要进一步看看。

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@hailaz @wln32 这里不应该注册全局的转换函数，并且目前针对基础类型的转换函数也只能在结构体属性中生效。我建议是修改gconv组件中相关的转换函数，避免panic，而是返回error。我刚跟了一下代码，具体问题应该出在这

gf/util/gconv/internal/converter/converter_convert.go

Line 459 in bc1e101

convertedValue = reflect.ValueOf(result).Convert(referReflectValue.Type()).Interface()

，但是解决方案的话，还需要进一步看看。

避免panic，而是返回error

不能认同这样的处理逻辑，返回error，会丢掉原来的信息，使得问题定位困难，最佳解决方案应是在ghttp包定义一个converter，ghttp中所有的转换都调用converter来做，而不是直接调用gconv，然后在针对string=>*UploadFile 注册一个转换函数即可

另gconv中有几处defer完全是多余的，本来转换出错了，立马panic即可，完全没必要再包装error，你硬是在defer中又处理了一次，给人看上去的感觉就是非要转换成功不可，这样的做法有时会成功，有时却会失败，而且这样会丧失掉原来的错误信息，我可以举个例子

package main

import (
	"encoding/json"
	"fmt"

	"github.com/gogf/gf/v2/util/gconv"
)

func stringToInt(src string) (dst *[]int, err error) {
	panic("stringToInt")
}

type Value struct {
	V []int
}

func main() {
	err := gconv.RegisterTypeConverterFunc(stringToInt)
	if err != nil {
		panic(err)
	}
	var dst Value
	err = gconv.Scan(map[string]any{
		"V": "[1,2,3]",
	}, &dst)
	fmt.Println(err)
	fmt.Println(dst)
}

输出
nil
{[1 2 3]}

上面代码中的逻辑，即使stringToInt中panic，也能转换成功，看起来匪夷所思，上面代码中转换也不是走正常的逻辑，而是bindVarToStructField中的defer，里面又调用了bindVarToReflectValue，而"[1,2,3]"符合json格式字符串，所以走的是json.Unmarshal这样的逻辑，如果我换一个不符合json格式的，那就会失败，同时也不会有error


// UploadFile wraps the multipart uploading file with more and convenient features.
type UploadFile struct {
*multipart.FileHeader `json:"-"`
Expand All @@ -36,13 +42,18 @@ func (f UploadFile) MarshalJSON() ([]byte, error) {
// UploadFiles is an array type of *UploadFile.
type UploadFiles []*UploadFile

// stringToUploadFile is a custom type converter for converting string to *ghttp.UploadFile.
func stringToUploadFile(in string) (*UploadFile, error) {
return &UploadFile{}, nil
Copy link
Preview

Copilot AI Mar 28, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The 'stringToUploadFile' converter always returns an empty UploadFile without validating the input, which could lead to unexpected behavior. Consider validating the input string and returning an error for invalid values.

Suggested change
return &UploadFile{}, nil
if in == "" {
return nil, gerror.NewCode(gcode.CodeInvalidParameter, "input string is empty")
}
fileHeader := &multipart.FileHeader{
Filename: in,
}
return &UploadFile{FileHeader: fileHeader}, nil

Copilot is powered by AI, so mistakes are possible. Review output carefully before use.

}

// Save saves the single uploading file to directory path and returns the saved file name.
//
// The parameter `dirPath` should be a directory path, or it returns error.
//
// Note that it will OVERWRITE the target file if there's already a same name file exist.
func (f *UploadFile) Save(dirPath string, randomlyRename ...bool) (filename string, err error) {
if f == nil {
if f == nil || f.FileHeader == nil {
return "", gerror.NewCode(
gcode.CodeMissingParameter,
"file is empty, maybe you retrieve it from invalid field name or form enctype",
Expand Down
28 changes: 28 additions & 0 deletions net/ghttp/ghttp_z_unit_issue_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -19,6 +19,7 @@ import (
"github.com/gogf/gf/v2/net/ghttp"
"github.com/gogf/gf/v2/test/gtest"
"github.com/gogf/gf/v2/text/gstr"
"github.com/gogf/gf/v2/util/gmeta"
"github.com/gogf/gf/v2/util/gtag"
"github.com/gogf/gf/v2/util/guid"
)
Expand Down Expand Up @@ -726,3 +727,30 @@ func Test_Issue4093(t *testing.T) {
t.Assert(client.PostContent(ctx, "/test"), `{"page":1,"pageSize":10,"pagination":true,"name":"john","number":1}`)
})
}

// https://github.com/gogf/gf/issues/4193
func Test_Issue4193(t *testing.T) {
type Req struct {
gmeta.Meta `method:"post" mime:"multipart/form-data"`
File ghttp.UploadFile `v:"required" type:"file"`
}
type Res struct{}
s := g.Server(guid.S())
s.BindMiddlewareDefault(ghttp.MiddlewareHandlerResponse)
s.BindHandler("/upload/single", func(ctx context.Context, req *Req) (res *Res, err error) {
return
})
s.SetDumpRouterMap(false)
s.Start()
defer s.Shutdown()
time.Sleep(100 * time.Millisecond)
// normal name
gtest.C(t, func(t *gtest.T) {
client := g.Client()
client.SetPrefix(fmt.Sprintf("http://127.0.0.1:%d", s.GetListenedPort()))
content := client.PostContent(ctx, "/upload/single", g.Map{
"file": "",
})
t.Assert(content, "{\"code\":51,\"message\":\"The File field is required\",\"data\":null}")
})
}
Loading