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

🔥 add config to enable splitting by comma in parsers #2560

Merged
merged 3 commits into from
Aug 21, 2023

Conversation

efectn
Copy link
Member

@efectn efectn commented Aug 6, 2023

Description

This PR adds a config field to enable the feature that was implemented in #817. It's disabled by default since it's not common pattern in web frameworks. Also added escape char support to avoid splitting some fields when we want.
Fixes #2557

Benchmarks Before:

Benchmark_Ctx_BodyParser_JSON
Benchmark_Ctx_BodyParser_JSON-16                                                     	 2245416	       533.9 ns/op	     240 B/op	       6 allocs/op
Benchmark_Ctx_BodyParser_XML
Benchmark_Ctx_BodyParser_XML-16                                                      	  626001	      1902 ns/op	    1160 B/op	      24 allocs/op
Benchmark_Ctx_BodyParser_Form
Benchmark_Ctx_BodyParser_Form-16                                                     	 1261950	       953.7 ns/op	     360 B/op	      13 allocs/op
Benchmark_Ctx_BodyParser_MultipartForm
Benchmark_Ctx_BodyParser_MultipartForm-16                                            	 1412667	       836.6 ns/op	     328 B/op	      12 allocs/op
Benchmark_Ctx_QueryParser
Benchmark_Ctx_QueryParser-16                                                         	  461858	      2507 ns/op	     857 B/op	      38 allocs/op
Benchmark_Ctx_parseQuery
Benchmark_Ctx_parseQuery-16                                                          	  550956	      2174 ns/op	     753 B/op	      29 allocs/op
Benchmark_Ctx_QueryParser_Comma
Benchmark_Ctx_QueryParser_Comma-16                                                   	  424041	      2798 ns/op	     929 B/op	      44 allocs/op
Benchmark_Ctx_ReqHeaderParser
Benchmark_Ctx_ReqHeaderParser-16                                                     	  433765	      2757 ns/op	     929 B/op	      44 allocs/op

Benchmarks After:

Benchmark_Ctx_Body_With_Compression
Benchmark_Ctx_Body_With_Compression-16                                               	 2044170	       588.4 ns/op
Benchmark_Ctx_BodyParser_JSON
Benchmark_Ctx_BodyParser_JSON-16                                                     	 2250416	       536.8 ns/op	     240 B/op	       6 allocs/op
Benchmark_Ctx_BodyParser_XML
Benchmark_Ctx_BodyParser_XML-16                                                      	  618874	      1897 ns/op	    1160 B/op	      24 allocs/op
Benchmark_Ctx_BodyParser_Form
Benchmark_Ctx_BodyParser_Form-16                                                     	 1239142	       968.6 ns/op	     360 B/op	      13 allocs/op
Benchmark_Ctx_BodyParser_MultipartForm
Benchmark_Ctx_BodyParser_MultipartForm-16                                            	 1432702	       851.8 ns/op	     328 B/op	      12 allocs/op
Benchmark_Ctx_QueryParser
Benchmark_Ctx_QueryParser-16                                                         	  459550	      2566 ns/op	     856 B/op	      38 allocs/op
Benchmark_Ctx_parseQuery
Benchmark_Ctx_parseQuery-16                                                          	  538406	      2205 ns/op	     753 B/op	      29 allocs/op
Benchmark_Ctx_QueryParser_Comma
Benchmark_Ctx_QueryParser_Comma-16                                                   	  403694	      2885 ns/op	     945 B/op	      45 allocs/op
Benchmark_Ctx_ReqHeaderParser
Benchmark_Ctx_ReqHeaderParser-16                                                     	  403273	      2857 ns/op	     945 B/op	      45 allocs/op

Type of change

  • Breaking change (fix or feature that would cause existing functionality to not work as expected)
  • This change requires a documentation update

Checklist:

  • For new functionalities I follow the inspiration of the express js framework and built them similar in usage
  • I have performed a self-review of my own code
  • I have commented my code, particularly in hard-to-understand areas
  • I have made corresponding changes to the documentation - /docs/ directory for https://docs.gofiber.io/
  • I have added tests that prove my fix is effective or that my feature works
  • New and existing unit tests pass locally with my changes
  • I tried to make my code as fast as possible with as few allocations as possible

Commit formatting:

Use emojis on commit messages so it provides an easy way of identifying the purpose or intention of a commit. Check out the emoji cheatsheet here: https://gitmoji.carloscuesta.me/

🔥 add config to enable splitting by comma in parsers
app.go Outdated Show resolved Hide resolved
Copy link
Member

@gaby gaby left a comment

Choose a reason for hiding this comment

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

Besides the golangci-lint, lgtm

Copy link
Member

@ReneWerner87 ReneWerner87 left a comment

Choose a reason for hiding this comment

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

I will look a second time because of the escaping

ctx.go Outdated Show resolved Hide resolved
ctx.go Outdated Show resolved Hide resolved
ctx.go Outdated Show resolved Hide resolved
optimize if statements, remove escape char support
@ReneWerner87 ReneWerner87 merged commit 1dea615 into master Aug 21, 2023
20 checks passed
@efectn efectn deleted the disable-comma-split branch August 21, 2023 09:06
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.

🐛 [Bug]: BodyParser will fail to parse correctly into the slices of the struct if input data contains comma
3 participants