feat(http): add form parser #40
Conversation
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## master #40 +/- ##
==========================================
- Coverage 66.19% 65.35% -0.84%
==========================================
Files 12 12
Lines 704 739 +35
==========================================
+ Hits 466 483 +17
- Misses 227 240 +13
- Partials 11 16 +5 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
|
/gemini review |
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request adds a form parser to the http/param package, allowing parsing of application/x-www-form-urlencoded data into structs. The changes are generally in the right direction, introducing new constants, types, and functions to handle form parameters, along with a test case.
However, there are a few issues to address. The most critical one is that the new FormParamFunc for customizing form parsing is defined but never used, making part of the new public API non-functional. Another important issue is that the parser uses r.Form which can lead to query parameters being parsed as form data, instead of r.PostForm which would correctly limit parsing to the request body. I've also included a suggestion to improve code maintainability by reducing duplication.
Once these points are addressed, the feature will be a solid addition.
CermakM
force-pushed
the
feat/add-http-param-form-parser
branch
from
0d56b8a to
ac7a3a3
Compare
| @@ -43,6 +51,7 @@ type PathParamFunc func(r *http.Request, key string) string | |||
| type Parser struct { | |||
| ParamTagResolver TagResolver | |||
| PathParamFunc PathParamFunc | |||
| FormParamFunc FormParamFunc | |||
There was a problem hiding this comment.
remove
I don't think it makes sense to have any customisation possible here - the default go std parser should be always called. In contrast, the PathParam func is not handled by go std lib, and the PathParamFunc allows caller to configure to their particular path routing library (e.g. chi) to extract the path placeholder/variable.
There was a problem hiding this comment.
Thanks for the note. I agree that likely every func would use just the default, but some might still benefit from interecepting the parsing somehow, for example with a logging mechanism, prefixing of the key, etc. It would perhaps also make sense to make these functions return error for additional validation, but that's out of the scope of this PR.
Anyway, I don't see any harm in having this option here so I don't see a reason for removal, especially with the default being also implemented in the lib, which somewhat benefits the dev experience.
There was a problem hiding this comment.
- I would lean towards keeping the library simple and not providing options that are not obvious what they are for
- the implementation in this PR is a bit weird how it does some part (
ParseFormor nowParseMultipartForm) in the "hardcoded" part, but then allow reading the populated fields/form values in the customFormParamFunc - adding the func for form but not for query is inconsistent, maybe there could be a more general way that handles all types of "param", if it is needed for something at all.
- I'd argue that logging should happen on different layer, instead of this library providing a way to completely change the behaviour of one of the param types. Or as in above bullet point, the library should provide a more obvious way to intercept the parsed params (of all types if possible), without possibility to "break" the core functionality of this library
Yes, there is a possibility to have everything somehow customisable/configurable. I think we should not attempt to create such configurability without a need in some projects using this library. And if this need arises, I think it would make more sense to handle the issue in a thought out, thorough approach, rather than a side effect of one of the implementation of one of the param types.
That being said it is somewhat minor issue, PR looks good to me either way, after the minor comment I added.
http/param/param.go
Outdated
| if r.Method != http.MethodPost && r.Method != http.MethodPut && r.Method != http.MethodPatch { | ||
| return fmt.Errorf("struct's field was tagged for parsing the form parameter (%s) but request method is not POST, PUT or PATCH", paramName) | ||
| } | ||
| if err := r.ParseForm(); err != nil { |
There was a problem hiding this comment.
testing with curl, naively googling how to do form requests with it, curl -F name=value sends a different format - multipart/form-data. The ParseForm handles application/x-www-form-urlencoded and application/octet-stream.
(my suggestion) Changing this to ParseMultipartForm would handle all content types as before + the multipart/form-data.
Removing this (and still calling FormParamFunc or PostFormValue below - which itself calls ParseMultipartForm on first call) would also handle all content types as before + the multipart/form-data, but would't allow handling errors
There was a problem hiding this comment.
Ah, thanks for pointing that out! Opting for the suggested option. 👍
http/param/param.go
Outdated
| if r.Method != http.MethodPost && r.Method != http.MethodPut && r.Method != http.MethodPatch { | ||
| return fmt.Errorf("struct's field was tagged for parsing the form parameter (%s) but request method is not POST, PUT or PATCH", paramName) | ||
| } | ||
| if err := r.ParseForm(); err != nil { |
There was a problem hiding this comment.
(thought, but no change needed in the end)
initially, I thought it would be nice (or even needed, as request body likely cannot be read multiple times) to parse body only once (only if there is at least one field tagged as form parameter), and not for each form parameter. But looks like both ParseForm and ParseMultipartForm already do both "check this is only called once per request (idempotent)" and "save parse result elsewhere (req.PostForm and req.Form)", so probably no change needed here.
| type structWithFormParams struct { | ||
| Subject string `param:"form=subject"` | ||
| Amount *int `param:"form=amount"` | ||
| Object *maybeShinyObject `param:"form=object"` |
There was a problem hiding this comment.
consider also a test where optional (pointer) fields are not present in the request (similar to TestParser_Parse_QueryParam_Pointers - no params
|
Prerequisite: #41 |
The form parser is useful for parsing application/x-www-form-urlencoded data. Signed-off-by: Marek Cermak <[email protected]>
- use FormParamFunc - use PostForm instead of Form - simplify newPath Signed-off-by: Marek Cermak <[email protected]>
Signed-off-by: Marek Cermak <[email protected]>
Signed-off-by: Marek Cermak <[email protected]>
CermakM
force-pushed
the
feat/add-http-param-form-parser
branch
from
dd4f66b to
3618505
Compare
Signed-off-by: Marek Cermak <[email protected]>
| return fmt.Errorf("parsing multipart form: %w", err) | ||
| } | ||
| // Try to parse regular form if not multipart form. | ||
| if err := r.ParseForm(); err != nil { |
There was a problem hiding this comment.
this is called from the ParseMultipartForm, so it is unnecesary to have it also here.
The form parser is useful for parsing application/x-www-form-urlencoded data.