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

binding: add support of multipart multi files (#1878) #1949

Merged
merged 2 commits into from
Jun 18, 2019

Conversation

vkd
Copy link
Contributor

@vkd vkd commented Jun 12, 2019

Fix #1878
Add support multi files on bind multipart requests.

Example:

type s struct {
	Files []*multipart.FileHeader `form:"files"`
}

@codecov
Copy link

codecov bot commented Jun 12, 2019

Codecov Report

Merging #1949 into master will increase coverage by 0.01%.
The diff coverage is 100%.

Impacted file tree graph

@@            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
Impacted Files Coverage Δ
binding/form.go 100% <ø> (ø) ⬆️
binding/multipart_form_mapping.go 100% <100%> (ø)

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 75b9d2b...1463acc. Read the comment docs.

"github.com/stretchr/testify/assert"
)

func Test_formMultipartBinding_Bind_OneFile(t *testing.T) {
Copy link
Member

Choose a reason for hiding this comment

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

please use TestFormMul... style.

assertMultipartFileHeader(t, s.ArrayPtrs[0], file)
}

func Test_formMultipartBinding_Bind_TwoFiles(t *testing.T) {
Copy link
Member

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")
Copy link
Member

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?

@thinkerou thinkerou added this to the 1.5 milestone Jun 13, 2019
binding/form.go Outdated
@@ -68,22 +69,54 @@ type multipartRequest http.Request

Copy link
Member

@thinkerou thinkerou Jun 13, 2019

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!

Copy link
Contributor Author

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?

Copy link
Member

Choose a reason for hiding this comment

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

@vkd OK

@vkd
Copy link
Contributor Author

vkd commented Jun 13, 2019

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) {
Copy link
Member

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) {
Copy link
Member

Choose a reason for hiding this comment

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

please use TestFormMultipartBindingBindError

"reflect"
)

type multipartRequestSetter http.Request
Copy link
Member

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.

Copy link
Contributor Author

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?

Copy link
Member

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.

@vkd vkd force-pushed the add_support_multipart_file_array branch from 6746d26 to e8a1e2f Compare June 14, 2019 05:23
thinkerou
thinkerou previously approved these changes Jun 14, 2019
Copy link
Member

@thinkerou thinkerou left a comment

Choose a reason for hiding this comment

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

LGTM, thanks!

appleboy
appleboy previously approved these changes Jun 18, 2019
@appleboy
Copy link
Member

@vkd Can you also add an example in README.md?

@vkd vkd dismissed stale reviews from appleboy and thinkerou via 1463acc June 18, 2019 10:42
@vkd
Copy link
Contributor Author

vkd commented Jun 18, 2019

I've updated the README.md the Multipart/Urlencoded binding part.

@thinkerou thinkerou merged commit 09a3650 into gin-gonic:master Jun 18, 2019
@vkd vkd deleted the add_support_multipart_file_array branch June 18, 2019 12:54
ThomasObenaus pushed a commit to ThomasObenaus/gin that referenced this pull request Feb 19, 2020
…onic#1949)

* binding: add support of multipart multi files (gin-gonic#1878)

* update readme: add multipart file binding
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

how to binding for multi files upload
3 participants