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

feat: allow arbitrary fields in MultipartFormFiles #706

Open
wants to merge 5 commits into
base: main
Choose a base branch
from

Conversation

b-kamphorst
Copy link
Contributor

@danielgtaylor I started working on #694. The first part of the work is a major refactoring that I submitted separately in #705. The second part is a feature where MultipartFormFiles also parses and validates multipart.Form.Value into the input struct. The diff is probably clearer in this PR.

At time of writing, the added feature only parses fields that have a form tag, mostly because I copied the approach for parsing and validating other request parameters and I was not quite sure how to do that differently. Before I continue, I would much like to hear your thoughts:

  1. Would you consider this feature for Huma?
  2. If so, do you have any feedback on how to continue with this PR?

Closes #694.

@b-kamphorst b-kamphorst force-pushed the feat-allow-arbitrary-fields-in-multipartformfiles branch from 85801d2 to 14b6613 Compare January 13, 2025 21:11
Copy link

codecov bot commented Jan 13, 2025

Codecov Report

Attention: Patch coverage is 92.22222% with 7 lines in your changes missing coverage. Please review.

Project coverage is 92.84%. Comparing base (1c3924e) to head (c71c254).

Files with missing lines Patch % Lines
huma.go 90.90% 4 Missing and 3 partials ⚠️
Additional details and impacted files
@@            Coverage Diff             @@
##             main     #706      +/-   ##
==========================================
- Coverage   92.90%   92.84%   -0.07%     
==========================================
  Files          22       22              
  Lines        4963     5014      +51     
==========================================
+ Hits         4611     4655      +44     
- Misses        305      309       +4     
- Partials       47       50       +3     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

@danielgtaylor danielgtaylor requested a review from Copilot January 20, 2025 19:05
Copy link
Contributor

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

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

Comments suppressed due to low confidence (2)

formdata.go:125

  • Potential nil pointer dereference. Check if formValueParser is nil before calling it.
formValueParser(value)

formdata.go:178

  • [nitpick] Error message 'Failed to open file' is not descriptive. Consider including more context about the failure.
return FormFile{}, &ErrorDetail{Message: "Failed to open file", Location: location}

formdata.go Show resolved Hide resolved
@danielgtaylor
Copy link
Owner

@b-kamphorst I think this looks good. Do you mind adding some additional user-facing documentation for form handling and the new feature? Thanks!

@b-kamphorst
Copy link
Contributor Author

Thank you for the initial review. I will update on the documentation shortly.

Question: is it acceptable that only struct fields with a form tag are passed? This deviaties from the parsing of non-form request data, but I did it so that I could easily (/ knew how to) reuse validation logic.

@b-kamphorst
Copy link
Contributor Author

Oh and I appreciate your merging effort :)

@b-kamphorst
Copy link
Contributor Author

@danielgtaylor I think this extention to the docs suffices, but please let me know what you think.

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.

Allow non-FormFile types in multipart forms
2 participants