🐛 [Bug]: QueryParser is not parsing comma separated values to slices by default

[lacerdev]

Bug Description

Bug inserted into #2560.

I agree that BodyParser shouldn't be splitting values by comma by default, but I don't agree that the same behavior should be extrapolated to QueryParser. By default query params separated by commas are arrays/slices and I believe that the framework must follow this rule by default and not by enabling a configuration

How to Reproduce

curl --location 'http://localhost:5000/api/alerts?level=warning%2Cerror' 

package main

import "github.com/gofiber/fiber/v2"
import "log"

func main() {
  app := fiber.New()

  // Steps to reproduce
	app.Post("/alerts",func(ctx *fiber.Ctx) error {
		type ReqParams struct {
			Level []string `query:"level"`
		}

		var params ReqParams
		ctx.QueryParser(&params)

	    //incorrect member size 1
          // params.Level[0] = "warning,error" 
        
         // correct members should be:  params.Level[0]="warning"      params.Level[1]="error"
		return nil
	})
  log.Fatal(app.Listen(":5000"))
}

Steps to reproduce the behavior:

1. Go to '....'
2. Click on '....'
3. Do '....'
4. See '....'

Expected Behavior

Query params must be parsed to slices when they came separated with commas

Fiber Version

v2.49.1

Code Snippet (optional)

Checklist:

• I agree to follow Fiber's Code of Conduct.
• I have checked for existing issues that describe my problem prior to opening this one.
• I understand that improperly formatted bug reports may be closed without explanation.

[welcome]

Thanks for opening your first issue here! 🎉 Be sure to follow the issue template! If you need help or want to chat with us, join us on Discord https://gofiber.io/discord

[ReneWerner87]

@rere61 what do you think about it ?

[ReneWerner87]

@rere61 actually meant about the current behavior and the new behavior which is requested in the report

[rere61]

@rere61 actually meant about the current behavior and the new behavior which is requested in the report

Yes, the new behavior is more common in server-side development.
And this issuse is completed solved, please close this issuse. thanks.

[ReneWerner87]

And this issuse is completed solved, please close this issuse. thanks.

@rere61
this is not your original report, but one from someone else
please take your time and read from top to bottom

[rere61]

And this issuse is completed solved, please close this issuse. thanks.

@rere61 this is not your original report, but one from someone else please take your time and read from top to bottom

@ReneWerner87
Okay. I got it.
Please take a look at my original post.
I write many examples and the comparsion of Go Fiber v.s Go standard HTTP v.s Gin.

So, my short answer for this request is "No".
We can not make GoFiber different than Go net/http, Gin, others framework in Go, all frameworks of all programmin languages in the world.

And most imporantly, split string by comma is very easy task, why should this feature needed to be built-in framework?

[lacerdev]

Because that's what frameworks are for. So we don't have to do the "dirty work".

Go Standard HTTP is a raw implementation, so it's understandable that we must do these transformations manually

Comma-separated query parameters are by convention transformed into arrays/lists/slices of values around the web developer community.

Again, in case of BodyParser I agree with this behavior of not parsing comma-separated values into slices, but to the QueryParser it should be the default behavior by convention, since query parameters are usually filter parameters

Or, another solution is decide based on param type, if that field was defined as string type, treat like string, if defined as []string treat like slice and split values by comma.

[rere61]

Or, another solution is decide based on param type, if that field was defined as string type, treat like string, if defined as []string treat like slice and split values by comma

Can you tell me which framework will automatically separate a user's single query value by comma, such as input containing a given name and first name(ex: Hemingway, Ernest Miller ), with commas without causing side effects?

[lacerdev]

In this specific case the query parameters must be encoded and decoded on server side. But, this is an atypical case, in most cases the comma will be used as a separator and not as an integral part of the string.

It is possible turn the method smarter as below

if equalFieldType(out, reflect.Slice, k) {
			values := strings.Split(v, ",")
			for i := 0; i < len(values); i++ {
				data[k] = append(data[k], values[i])
			}
		} else {
			data[k] = append(data[k], v)
		}

It would be not needed to check if strings.Contains(v, ","). This way if the developer would be considering receiving the value as slice, the framework tries to do splitting, if does not contains ",", would just return a slice of 1 position with full string and the code flows naturally

Another option is to enable/disable this behavior as a new parameter in the QueryParser method, instead of a global config.

// enableSplitting is a second param in QueryParser method, or maybe could be exist a options{} second 
// parameter containing some configs, one of them enableSplitting. And by default this value would be true
if equalFieldType(out, reflect.Slice, k) && enableSplitting {
			values := strings.Split(v, ",")
			for i := 0; i < len(values); i++ {
				data[k] = append(data[k], values[i])
			}
		} else {
			data[k] = append(data[k], v)
		}

this way It would not be a breaking change and will not change the behavior for entire application

[joey1123455]

In this specific case the query parameters must be encoded and decoded on server side. But, this is an atypical case, in most cases the comma will be used as a separator and not as an integral part of the string.

It is possible turn the method smarter as below

if equalFieldType(out, reflect.Slice, k) {
			values := strings.Split(v, ",")
			for i := 0; i < len(values); i++ {
				data[k] = append(data[k], values[i])
			}
		} else {
			data[k] = append(data[k], v)
		}

It would be not needed to check if strings.Contains(v, ","). This way if the developer would be considering receiving the value as slice, the framework tries to do splitting, if does not contains ",", would just return a slice of 1 position with full string and the code flows naturally

Another option is to enable/disable this behavior as a new parameter in the QueryParser method, instead of a global config.

// enableSplitting is a second param in QueryParser method, or maybe could be exist a options{} second 
// parameter containing some configs, one of them enableSplitting. And by default this value would be true
if equalFieldType(out, reflect.Slice, k) && enableSplitting {
			values := strings.Split(v, ",")
			for i := 0; i < len(values); i++ {
				data[k] = append(data[k], values[i])
			}
		} else {
			data[k] = append(data[k], v)
		}

this way It would not be a breaking change and will not change the behavior for entire application

This isn't a bad idea actually but it would be nice to have it also take into account the global config

[lacerdev]

And if it has two global configs? One for BodyParser and other for QueryParser? Where the QueryParser config would be true by default. And, besides that, could have this second param too, that would be initialized by the global config value, but that could be changed if user has passed as argument...

[joey1123455]

And if it has two global configs? One for BodyParser and other for QueryParser? Where the QueryParser config would be true by default. And, besides that, could have this second param too, that would be initialized by the global config value, but that could be changed if user has passed as argument...

Hmm or instead it would take the global variable if true by default, if its not set then a local variable takes preference.

[joey1123455]

But then it would require changes to the body parser and also the cookie parser