feat(http): add form parser #40

CermakM · 2025-08-15T08:28:20Z

The form parser is useful for parsing application/x-www-form-urlencoded data.

codecov-commenter · 2025-08-15T08:29:06Z

Codecov Report

❌ Patch coverage is 56.09756% with 18 lines in your changes missing coverage. Please review.
✅ Project coverage is 65.35%. Comparing base (5357683) to head (7c4ba13).

Files with missing lines	Patch %	Lines
http/param/param.go	56.09%	13 Missing and 5 partials ⚠️

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.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:

❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

bafko · 2025-08-15T13:22:55Z

/gemini review

gemini-code-assist

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.

http/param/param.go

http/param/param_test.go

Fazt01 · 2025-08-16T20:16:08Z

http/param/param.go

 type Parser struct {
 	ParamTagResolver TagResolver
 	PathParamFunc    PathParamFunc
+	FormParamFunc    FormParamFunc


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.

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.

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 (ParseForm or now ParseMultipartForm) in the "hardcoded" part, but then allow reading the populated fields/form values in the custom FormParamFunc

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.

Fazt01 · 2025-08-16T20:25:36Z

http/param/param.go

+	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 {


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

Ah, thanks for pointing that out! Opting for the suggested option. 👍

Fazt01 · 2025-08-16T20:32:11Z

http/param/param.go

+	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 {


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

Fazt01 · 2025-08-16T20:36:07Z

http/param/param_test.go

+type structWithFormParams struct {
+	Subject string            `param:"form=subject"`
+	Amount  *int              `param:"form=amount"`
+	Object  *maybeShinyObject `param:"form=object"`


consider also a test where optional (pointer) fields are not present in the request (similar to TestParser_Parse_QueryParam_Pointers - no params

Thanks, added.

TomasKocman · 2025-08-18T11:16:54Z

Prerequisite: #41

The form parser is useful for parsing application/x-www-form-urlencoded data. Signed-off-by: Marek Cermak <marek.cermak@strv.com>

- use FormParamFunc - use PostForm instead of Form - simplify newPath Signed-off-by: Marek Cermak <marek.cermak@strv.com>

Signed-off-by: Marek Cermak <marek.cermak@strv.com>

Fazt01 · 2025-08-27T16:50:53Z

http/param/param.go

+			return fmt.Errorf("parsing multipart form: %w", err)
+		}
+		// Try to parse regular form if not multipart form.
+		if err := r.ParseForm(); err != nil {


this is called from the ParseMultipartForm, so it is unnecesary to have it also here.

CermakM requested review from TomasKocman, bafko and xburda4 as code owners August 15, 2025 08:28

gemini-code-assist bot reviewed Aug 15, 2025

View reviewed changes

http/param/param.go Show resolved Hide resolved

http/param/param.go Outdated Show resolved Hide resolved

http/param/param.go Show resolved Hide resolved

http/param/param_test.go Outdated Show resolved Hide resolved

CermakM force-pushed the feat/add-http-param-form-parser branch from 0d56b8a to ac7a3a3 Compare August 15, 2025 13:45

Fazt01 reviewed Aug 16, 2025

View reviewed changes

CermakM added 4 commits August 27, 2025 09:21

feat(http): add form parser

d36e3e8

The form parser is useful for parsing application/x-www-form-urlencoded data. Signed-off-by: Marek Cermak <marek.cermak@strv.com>

chore: address review comments

4212783

- use FormParamFunc - use PostForm instead of Form - simplify newPath Signed-off-by: Marek Cermak <marek.cermak@strv.com>

chore: use r.PostFormValue as the default form param func

efa90be

Signed-off-by: Marek Cermak <marek.cermak@strv.com>

chore: address review comments

3618505

Signed-off-by: Marek Cermak <marek.cermak@strv.com>

CermakM force-pushed the feat/add-http-param-form-parser branch from dd4f66b to 3618505 Compare August 27, 2025 07:37

chore: fix linter issues

7c4ba13

Signed-off-by: Marek Cermak <marek.cermak@strv.com>

Fazt01 reviewed Aug 27, 2025

View reviewed changes

feat(http): add form parser #40

Are you sure you want to change the base?

feat(http): add form parser #40

Uh oh!

Conversation

CermakM commented Aug 15, 2025

Uh oh!

codecov-commenter commented Aug 15, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Codecov Report

Uh oh!

bafko commented Aug 15, 2025

Uh oh!

gemini-code-assist bot left a comment

Choose a reason for hiding this comment

Code Review

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

TomasKocman commented Aug 18, 2025

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

6 participants

codecov-commenter commented Aug 15, 2025 •

edited

Loading