Skip to content
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.

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

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

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

Conversation

hailaz
Copy link
Member

@hailaz hailaz commented Mar 14, 2025

Fixes #4193

@hailaz hailaz requested review from gqcn and wln32 March 14, 2025 07:46
@wln32
Copy link
Member

wln32 commented Mar 18, 2025

@hailaz @gqcn 是否需要给ghttp定义一个单独的converter?

@hailaz
Copy link
Member Author

hailaz commented Mar 18, 2025

@hailaz @gqcn 是否需要给ghttp定义一个单独的converter?

我不知道,我改这个的时候绕了很久,也不知道这样解决是否合理。
本来想在converter里面处理的,但是类型一引用就导致包循环引用了。

@Issues-translate-bot
Copy link

Bot detected the issue body's language is not English, translate it automatically. 👯👭🏻🧑‍🤝‍🧑👫🧑🏿‍🤝‍🧑🏻👩🏾‍🤝‍👨🏿👬🏿


@hailaz @gqcn Do you need to define a separate converter for ghttp?

I don’t know. I went around for a long time when I changed this, and I don’t know if this solution is reasonable.
I originally wanted to handle it in converter, but once a type is referenced, it causes a package to be recycled.

@wln32
Copy link
Member

wln32 commented Mar 18, 2025

@hailaz @gqcn 是否需要给ghttp定义一个单独的converter?

我不知道,我改这个的时候绕了很久,也不知道这样解决是否合理。 本来想在converter里面处理的,但是类型一引用就导致包循环引用了。

可以像orm那样,给ghttp包单独定义一个converter,不和gconv中的共享,可以把ghttp包中目前所有用到gconv的地方,都替换为ghttp中自定义的converter,issue #2913 中提到,目前ghttp对于参数转换错误的,直接返回了默认值,没有返回错误

@Issues-translate-bot
Copy link

Bot detected the issue body's language is not English, translate it automatically. 👯👭🏻🧑‍🤝‍🧑👫🧑🏿‍🤝‍🧑🏻👩🏾‍🤝‍👨🏿👬🏿


@hailaz @gqcn Do you need to define a separate converter for ghttp?

I don’t know, I went around for a long time when I changed this, and I don’t know if this solution is reasonable. I originally wanted to handle it in converter, but once a type is referenced, it causes a package to be recycled.

Like orm, you can define a converter for the ghttp package separately, which is not shared with gconv. You can replace all the places in the ghttp package that are currently used with ghttp with the custom converter in ghttp. It is mentioned in issue #2913 that currently, for parameter conversion errors, ghttp directly returns the default value and does not return an error.

// 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。我刚跟了一下代码,具体问题应该出在这

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。我刚跟了一下代码,具体问题应该出在这

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

@hailaz
Copy link
Member Author

hailaz commented Mar 26, 2025

既然选择了注册自定义类型这个方式,为什么不把框架中非标准库的类型解耦出来,使用注册的方式在自定义类型所在的包进行注册,也能同时简化gconv中的逻辑。

@Issues-translate-bot
Copy link

Bot detected the issue body's language is not English, translate it automatically. 👯👭🏻🧑‍🤝‍🧑👫🧑🏿‍🤝‍🧑🏻👩🏾‍🤝‍👨🏿👬🏿


Since you have chosen to register a custom type, why not decouple the non-standard library types in the framework and register in the package where the custom type is located by registering, which can also simplify the logic in gconv.

@hailaz
Copy link
Member Author

hailaz commented Mar 26, 2025

Bot detected the issue body's language is not English, translate it automatically. 👯👭🏻🧑‍🤝‍🧑👫🧑🏿‍🤝‍🧑🏻👩🏾‍🤝‍👨🏿👬🏿

Since you have chosen to register a custom type, why not decouple the non-standard library types in the framework and register in the package where the custom type is located by registering, which can also simplify the logic in gconv.

这个自动翻译真的很影响交流,现在翻译插件已经很常见了,真的没有必要自动翻译。有心人根本不会因为没有翻译而不交流

@gqcn
Copy link
Member

gqcn commented Mar 26, 2025

@wln32 @hailaz 我没有在这个pr上修改,我新提了一个pr来修复这个问题,大家看看呢 #4226 。其中比较重要一个改动是,如果是结构体转换,那么这里直接返回转换错误,不再进一步执行反射转换:
图片

@houseme houseme requested a review from Copilot March 28, 2025 03:53
Copy link

@Copilot Copilot AI left a comment

Choose a reason for hiding this comment

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

Pull Request Overview

This pull request fixes a panic issue when the file upload parameter is provided as a string rather than the expected file type. It adds a new test case to validate the error response and implements a custom type converter to safely handle string inputs.

  • Added a new unit test (Test_Issue4193) to verify proper error messaging when a file parameter is a string.
  • Registered a custom type converter (stringToUploadFile) and updated the Save method to check for a nil FileHeader.

Reviewed Changes

Copilot reviewed 2 out of 2 changed files in this pull request and generated 1 comment.

File Description
net/ghttp/ghttp_z_unit_issue_test.go Added a new test to ensure proper handling of string-based uploads.
net/ghttp/ghttp_request_param_file.go Registered a type converter and added a nil check for FileHeader in Save.
Comments suppressed due to low confidence (1)

net/ghttp/ghttp_z_unit_issue_test.go:746

  • Using time.Sleep to wait for the server startup may cause flaky tests on slower systems; consider using a synchronization mechanism to ensure the server is ready before sending requests.
time.Sleep(100 * time.Millisecond)

@@ -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.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

net/ghttp: Multipart file upload fails: empty string causes ghttp.UploadFile conversion error
4 participants