-
Notifications
You must be signed in to change notification settings - Fork 8.1k
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
binding: add support of multipart multi files (#1878) #1949
binding: add support of multipart multi files (#1878) #1949
Conversation
Codecov Report
@@ Coverage Diff @@
## master #1949 +/- ##
==========================================
+ Coverage 98.75% 98.77% +0.01%
==========================================
Files 38 39 +1
Lines 2173 2198 +25
==========================================
+ Hits 2146 2171 +25
Misses 15 15
Partials 12 12
Continue to review full report at Codecov.
|
binding/form_test.go
Outdated
"github.com/stretchr/testify/assert" | ||
) | ||
|
||
func Test_formMultipartBinding_Bind_OneFile(t *testing.T) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
please use TestFormMul...
style.
binding/form_test.go
Outdated
assertMultipartFileHeader(t, s.ArrayPtrs[0], file) | ||
} | ||
|
||
func Test_formMultipartBinding_Bind_TwoFiles(t *testing.T) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
please use TestFormMultipart...
style.
binding/form.go
Outdated
return setByForm(value, field, r.MultipartForm.Value, key, opt) | ||
} | ||
|
||
var errMultipartUnsupportedFieldType = errors.New("unsupported field type for multipart.FileHeader") |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
maybe we should not use global var, do you think?
binding/form.go
Outdated
@@ -68,22 +69,54 @@ type multipartRequest http.Request | |||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@vkd can move all multipartRequest
codes to form_mapping.go
? thanks!
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I agree with you about moving this code from form.go
file, but I think form_mapping.go
is not good place for that.
I prefer to move this code to new file like multipart_form_mapping.go
. What do you think?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@vkd OK
Fixed |
@@ -39,7 +39,7 @@ func Test_formMultipartBinding_Bind_OneFile(t *testing.T) { | |||
assertMultipartFileHeader(t, s.ArrayPtrs[0], file) | |||
} | |||
|
|||
func Test_formMultipartBinding_Bind_TwoFiles(t *testing.T) { | |||
func TestFormMultipartBinding_Bind_TwoFiles(t *testing.T) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
please use TestFormMultipartBindingBindTwoFiles
.
@@ -68,7 +68,7 @@ func Test_formMultipartBinding_Bind_TwoFiles(t *testing.T) { | |||
} | |||
} | |||
|
|||
func Test_formMultipartBinding_Bind_Error(t *testing.T) { | |||
func TestFormMultipartBinding_Bind_Error(t *testing.T) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
please use TestFormMultipartBindingBindError
binding/multipart_form_mapping.go
Outdated
"reflect" | ||
) | ||
|
||
type multipartRequestSetter http.Request |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
usually, ...er
or ...or
is interface.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
You are right! Usually..., but not always, example: nopCloser
In my case multipartRequestSetter
is a wrapper for the http.Request
over the setter
interface, and previous name (multipartRequest
) said nothing about why it wrapped.
I know about a naming of interfaces, but in this case I don't know a better name. Maybe you can suggest something for that struct?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
thanks, but I think multipartRequest
is better. and waiting @appleboy review the pull request.
6746d26
to
e8a1e2f
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM, thanks!
@vkd Can you also add an example in |
I've updated the |
…onic#1949) * binding: add support of multipart multi files (gin-gonic#1878) * update readme: add multipart file binding
Fix #1878
Add support multi files on bind multipart requests.
Example: