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

New input/query API #247

Closed
manucorporat opened this issue Mar 26, 2015 · 9 comments
Closed

New input/query API #247

manucorporat opened this issue Mar 26, 2015 · 9 comments

Comments

@manucorporat
Copy link
Contributor

I want to provide a better API to get the input parameters from the path, POSTForm and GETForm.

Let's say, we register this route:

router.POST("/user/:username", handler)

and we expect to receive requests like this:

POST /user/manu?page=10 HTTP/1.1
Content-Type: application/x-www-form-urlencoded
Host: localhost:8080

email=manu.mtza%40gmail.com
  • Includes a parameter in the path (:username)
  • Includes a GET form parameter (page)
  • Includes a POST form parameter (email)

Right now, you would have to do this in order to get the parameters:

func handler(c *gin.Context) {
    c.Request.ParseForm()

    name := c.Params.ByName("username")
    page := c.Request.Form.Get("page")
    email := c.Request.PostForm.Get("email")
}
  • Inconsistent
  • Low level (c.Request.ParseForm())
  • Difficult to learn

The new proposal:

func handler(c *gin.Context) {
    name := c.Input.FromPath("username")
    page := c.Input.FromGET("page")
    email := c.Input.FromPOST("email")
}

Also, c.Input will implement an intelligent Get() method that takes the value from the proper source. This feature is implemented in popular frameworks like laravel.

router.GET("/account/:id", func(c *gin.Context) {
        // same than  c.Input.FromPath("id") in this context
    id := c.Input.Get("id")

    // ... read account with id
}

GET /list?page=2
router.GET("/list", func(c *gin.Context) {
        // same than  c.Input.FromGET("page") in this context
    page := c.Input.Get("page") // page = 2
}

One more thing, c.Input can be printed directly with fmt.Print() providing a useful debugging tool.

fmt.Println(c.Input)
@itsjamie
Copy link
Contributor

@manucorporat, I'd like to make the request that ParseForm is lazily called on the first call that requires it.

@manucorporat
Copy link
Contributor Author

@itsjamie of course! ParseForm would be lazily called

@itsjamie
Copy link
Contributor

@manucorporat This definitely reduces the amount you need to directly call on the net/http methods.

If you don't mind me asking, what do you feel the cost to the framework is for introducing this functionality?

What is the benefit?

I really like how Gin currently focuses on providing a nice implementation of handling middleware, and provides some convenience method for oft-used functionality that has a slightly nicer API than the underlying tech (like httprouter). I just want to make sure that any code you decide to add to part of core is following the vision you want the project to take.

@itsjamie
Copy link
Contributor

Personally, I feel this is right in line with what I feel Gin should accomplish (btw).

@manucorporat
Copy link
Contributor Author

@itsjamieI totally agree with you.

If you don't mind me asking, what do you feel the cost to the framework is for introducing this functionality?

This new API will have 0 overhead, otherwise I would not include it!

  • Request handing and router path can not slowed down
  • 0 memory allocation policy

What is the benefit?

Gin focuses in a good balance of performance and convenience. A typical request in httpRouter can be handled in 220ns (in my machine), Gin only adds 30ns of overhead in average.

But as you know, Gin provides an much nicer environment: middlewares, content negotiation, parsing, validation....

I want to continue optimizing the most common use cases such us the user input through a form, and I think we need a better API for that.

@leedstyh
Copy link

Will the intelligent one c.Input.Get() cause some secure issue like XSS?

@manucorporat
Copy link
Contributor Author

@leedstyh I dropped this feature because it introduces very complex situations.

@thinkerou
Copy link
Member

As @manucorporat say, we should close the issue, right? @appleboy

@appleboy
Copy link
Member

@thinkerou OK.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

5 participants