Automatic Parameter Router #2586

eyalpost · 2017-04-22T22:59:05Z

This is a POC for a feature i've been working on that I'd like to get reviewed before I complete the work (to make sure the direction is ok)
The idea is to make Comments Controller even more "magical" by translating request parameters to method parameters.

[See updated information below]

Let me know if the direction looks ok and I will complete the missing parts. Also if you think this is good, please review the changes.
Note that most changes are in new files and there should be no changes to current flows. Changes in existing files are minimal.

astaxie · 2017-04-24T13:16:04Z

That is a good ideas. Could you move the params and response to the context package? I think context packgage is used for that, And also I think we already have parse params to type there

eyalpost · 2017-04-24T15:38:17Z

@astaxie I can move it under context but will it be ok if I keep it as sub folders of context?
The reason is that it makes code shorter and more readable this way e.g.:
param.Bool vs context.BoolParam
or
response.StatusCode(400) vs context.ResponseStatusCode(400)
What do you think?

Also, Can I delete Params map from ControllerComments struct? It doesn't seem to be used anywhere

eyalpost · 2017-04-25T22:31:33Z

@astaxie This is now ready for review. It has an accompanying PR in bee: beego/bee#418 which also add automatic swagger generation for method parameters.

What does it do?

Add parameters to your controller method and the router will convert request parameter (query, form ,body or header) and pass to your function. This works in coordination with the @param comment so the router knows where to look for the parameter
if your function has a result, it will automatically be converted to json and returned in the http response. If you have an error result, it will be translated to an http error (example below)
if parameters are declared as pointers, you will get null if they don't appear in the request. unless if @param defines a parameter as required: then an error will occur if it's missing
all built in types are supported (well.. except Complex)
struct types are marshalled as json (except time.Time which is converted to\from RFC3339 string)
arrays in body are treated as json. in query or formData: as comma separated list

End result:
It allows you to convert code like this:

// @Param	id		path 	string	true	"The id you want to update"
// @Param	task	body 	models.Task	true	"body for Task content"
// @Success 200 {object} Status
// @router /task/:id [put]
func (c *TaskController) PutTaskById() {
	id, err := c.GetInt64(":id")
	if err != nil {
		c.Ctx.Output.SetStatus(400)
		c.Data["json"] = err.Error()
		c.ServeJSON()
		return
	}

	v := models.Task{}
	if err := json.Unmarshal(c.Ctx.Input.RequestBody, &v); err != nil {
		c.Ctx.Output.SetStatus(400)
		c.Data["json"] = Status{Status: err.Error()}
		c.ServeJSON()
		return
	}
	if err := models.UpdateTask(id, &v); err == nil {
		c.Data["json"] = Status{Status: "OK"}
	} else {
		c.Ctx.Output.SetStatus(400)
		c.Data["json"] = Status{Status: err.Error()}
	}
	c.ServeJSON()
}

To this:

// @Param	id		path 	string	true	"The id you want to update"
// @Param	task	body 	models.Task	true	"body for Task content"
// @Success 200 {object} Status
// @router /task/:id [put]
func(c *TaskController) PutTaskById(id int64, task *models.Task) (*Status, error) {
	if err := models.UpdateTask(id, task); err != nil {
		return nil, err
	}
	return &Status{Status: "OK"}, nil
}

Note how all of the boilerplate code is gone and all that is left is pure business logic

astaxie · 2017-04-30T15:42:28Z

@eyalpost This is a great feature. But I will close it as I add golint and ineffassign tool to check the code in the CI. please fix the comments and then send a new PR. Thanks

astaxie · 2017-04-30T15:40:22Z

context/param/conv.go

+	"github.com/astaxie/beego/logs"
+)
+
+func ConvertParams(methodParams []*MethodParam, methodType reflect.Type, ctx *beecontext.Context) (result []reflect.Value) {


please add comments

astaxie · 2017-04-30T15:40:47Z

context/param/methodparams.go

+)
+
+//Keeps param information to be auto passed to controller methods
+type MethodParam struct {


golints need start the comments with MethodParam

eyalpost · 2017-04-30T15:44:29Z

Can you leave this PR open? I will do the fixes and it will run the build again automatically

astaxie · 2017-04-30T15:45:27Z

@eyalpost yea, I can let it open. But it looks will not automaticlly use the latest travis config. let's try

eyalpost · 2017-04-30T15:47:33Z

I can sync my branch so I will have the latest travis

astaxie · 2017-04-30T15:52:10Z

@eyalpost thanks

eyalpost · 2017-05-01T11:25:01Z

@astaxie i've made the golint fixes and updates with the latest travis. build passes

astaxie · 2017-05-01T15:02:01Z

context/response/responses.go

+)
+
+// JSON renders value to the response as JSON
+func JSON(value interface{}, encoding ...bool) Renderer {


It's already have context response, why create a new package response here. That's really confuse

Where there is already package response?

As context.Output already have a lot functions based on response. I think the seperate response package does not make sense here.

This is mainly for code readability
The idea is that a controller method can be written like this:

func(c *TaskController) PutTaskById(id int64, t *Task) (*Status, error) { if /*not found */ { return nil, response.NotFound } else if /* needs redirect */ { return nil, response.Redirect("/somePath") } return &Status{Status: "OK"}, nil }

What do you think?

Yea, I understand the readability. But why not just use the ctx.Output

func(c *TaskController) PutTaskById(id int64, t *Task) (*Status, error) { if /*not found */ { return nil, c.Ctx.StatusNotFound } else if /* needs redirect */ { return nil, c.Ctx.Output.Redirect("/somePath") } return &Status{Status: "OK"}, nil }

StatusNotFound is simply a const - I don't think it should be placed on the context object.
Redirect returns a renderer. It does not changes directly the output - so I don't think it belongs on the Output object itself. also c.Ctx.Output.Redirect feels to verbose IMO

Also, response.JSON can not be put on Output because Output already has JSON. and repsonse JSON again returns a renderer which eventually calls Output.JSON.
The point of renderers was that writing to output is always done after the function is finished and not within the code.

c.Ctx.Output.Redirect feels to verbose IMO

When I design the Ctx, we just name request as input and response as output. we need to keep consistency with the design.

Also, response.JSON can not be put on Output because Output already has JSON. and repsonse JSON again returns a renderer which eventually calls Output.JSON.

If you return the render from the JSON, I would like to rename it to JSONRenderer

OK, I refactored the code a bit and hopefully it will look better now.
One thing I want to keep in mind: I don't want controller code to rely on c.Ctx and want it to be as "stateless" as possible. The reason is that it makes testing the controller method much easier.
No need to setup context to test the method if it simply accepts all parameters as function parameters and returns simple objects
Please review now and let me know if this is better

eyalpost · 2017-05-17T22:41:35Z

@astaxie I updated the code and also added documentation here: beego/beedoc#580

astaxie · 2017-05-18T02:39:00Z

httpResponse/response.go

@@ -0,0 +1,52 @@
+package httpResponse


Could you also move this package to context?

eyalpost · 2017-05-18T04:16:13Z

I think it doesn't belong in context. It's an implementation of the Renderer interface which also a user can do within his own packages

astaxie · 2017-05-18T04:21:35Z

I think it doesn't belong in context. It's an implementation of the Renderer interface which also a user can do within his own packages

I don't think so. httpResponse means response.

eyalpost · 2017-05-18T04:38:54Z

No it's a helper for generating responses. Just like implementing an error interface does not mean it should be in the error package

eyalpost · 2017-05-18T05:52:30Z

And it's similar to all helper method on the controller (ServeJson(), GetInt() etc. ) - they handle request\response but they're not part of the context package.

astaxie · 2017-05-18T06:02:31Z

No it's a helper for generating responses. Just like implementing an error interface does not mean it should be in the error package

yea, they are helper functions. But they just works for response. While response should be in context package. Like the context/param/. While this two helper response are response not other common function can reuse by other functions.

eyalpost · 2017-05-18T06:06:08Z

So move it under context, not inside context? That's ok with me. Moved

astaxie · 2017-05-18T06:07:58Z

So move it under context, not inside context? That's ok with me. Moved

My idea is moving the file to context package. What's your concern?

eyalpost · 2017-05-18T06:14:15Z

We have quite a lot of projects using beego and almost none of our code imports context package. The fact that it will now start importing it means (for me) these helpers are not in the correct place.
Also, as I said before, it's for readability reasons:
return nil, httpResponse.NotFound
instead of
return nil, context.HttpResponseNotFound
If you prefer, I can delete these helpers and implement them in my code. But I think they will be useful for other developers too.

astaxie · 2017-05-18T06:43:06Z

We have quite a lot of projects using beego and almost none of our code imports context package. The fact that it will now start importing it means (for me) these helpers are not in the correct place.

If you already use controller, context package already imported.

Also, as I said before, it's for readability reasons:
return nil, httpResponse.NotFound
instead of
return nil, context.HttpResponseNotFound
If you prefer, I can delete these helpers and implement them in my code. But I think they will be useful for other developers too.

It still confuse for me that httpResponse have a Redirect. But context also have a Redirect. The other statuscode also should be parts of reponse.

I think maybe should be like this:

return nil, context.Output.NotFound

eyalpost · 2017-05-18T06:52:11Z

If you already use controller, context package already imported.

I mean explicitly in code. We don't have explicit import of "github.com/astaxie/beego/context"

It still confuse for me that httpResponse have a Redirect. But context also have a Redirect. The other statuscode also should be parts of response.

So how do you want to call it?

astaxie · 2017-05-18T06:59:03Z

I mean explicitly in code. We don't have explicit import of "github.com/astaxie/beego/context"

that's what I think is you don't need to import that now. and also don't need to import httpResponse. As you can just call these functions like c.Ctx.Redirect or c.Ctx.Output.NotFound

eyalpost · 2017-05-18T07:04:11Z

you can just call these functions like c.Ctx.Redirect or c.Ctx.Output.NotFound

But these functions require an actual Context object while the way that my responses are built is that they don't need a context.
You can call the controller method in a test, and get a response all without needing to setup a context object. If I call c.Redirect in a test then it needs the entire http infrastructure (responseWriter etc.) but in my case it doesn't need anything because it just returns an "instruction" to redirect. The redirection is executed later

astaxie · 2017-05-18T07:15:48Z

But these functions require an actual Context object while the way they are built now is that they don't need a context. You can call the controller method in a test, and get a response all without needing to setup a context object. If I call c.Redirect in a test then it needs the entire http infrastructure (responseWriter etc.) but in my case it doesn't need anything because it just returns an "instruction" to redirect. The redirection is executed later

you can still rename current httpResponse.Redirect to RedirectRender. I think it should be a render while not the Redirect

eyalpost · 2017-05-18T07:20:32Z

maybe return httpResponse.RedirectResult(..) ?

eyalpost · 2017-05-18T07:23:47Z

or RedirectRenderer ?

astaxie · 2017-05-18T07:25:09Z

I like return c.Ctx. RedirectRenderer(..) As we already define many renderer now. What do you think?

eyalpost · 2017-05-18T07:30:59Z

Making it a context method again loses ability to test it. I will delete this helper for now to push this forward

astaxie · 2017-05-18T07:34:03Z

Could you also move context/httpResponse/response.go to context? or remove it

eyalpost · 2017-05-18T08:50:42Z

Done

astaxie · 2017-05-18T10:14:30Z

@eyalpost thanks so much on this PR. Appreciated.

eyalpost added 2 commits April 21, 2017 15:26

phase beego#1

9aedb4d

all types implemented

89e01d1

amrfaissal added the kind/experimental label Apr 23, 2017

Eyal Post added 3 commits April 23, 2017 21:37

slice support

19f4a6a

Merge remote-tracking branch 'upstream/develop' into develop

08ea9b3

mall fixes

864693d

Merge remote-tracking branch 'upstream/develop' into develop

3b29a9c

eyalpost added 3 commits April 25, 2017 16:00

all types working + controller comments generation

9b79437

move under context

cbd8310

small fixes

4cba78a

eyalpost mentioned this pull request Apr 25, 2017

Add support for controller methods with paramaters beego/bee#418

Closed

dont generate comment if router not found

a1bc94e

eyalpost mentioned this pull request Apr 28, 2017

Add support for controller methods with paramaters beego/bee#421

Merged

astaxie requested changes Apr 30, 2017

View reviewed changes

astaxie closed this Apr 30, 2017

astaxie reopened this Apr 30, 2017

eyalpost force-pushed the develop branch from 1130338 to a1bc94e Compare April 30, 2017 15:58

Eyal Post added 3 commits April 30, 2017 18:59

Merge remote-tracking branch 'upstream/develop' into develop

cfb2f68

golint fixes

1b8f05c

Redirect should returns error

d3a16dc

astaxie reviewed May 1, 2017

View reviewed changes

Eyal Post added 2 commits May 17, 2017 20:50

fix gosimple

ee1d8bc

fix gosimple

e32a182

astaxie reviewed May 18, 2017

View reviewed changes

httpResponse/response.go Outdated

@@ -0,0 +1,52 @@

package httpResponse

Copy link

Member

astaxie May 18, 2017

Choose a reason for hiding this comment

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

Could you also move this package to context?

move response

3e51823

remove Redirect to avoid confusion

2513bcf

move to context

11b4bf8

astaxie approved these changes May 18, 2017

View reviewed changes

astaxie merged commit 655484b into beego:develop May 18, 2017

astaxie mentioned this pull request May 18, 2017

v1.8.3 #2651

Merged

Automatic Parameter Router #2586

Automatic Parameter Router #2586

Conversation

eyalpost commented Apr 22, 2017 • edited Loading

astaxie commented Apr 24, 2017

eyalpost commented Apr 24, 2017

eyalpost commented Apr 25, 2017 • edited Loading

astaxie commented Apr 30, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

eyalpost commented Apr 30, 2017

astaxie commented Apr 30, 2017 • edited Loading

eyalpost commented Apr 30, 2017

astaxie commented Apr 30, 2017 • edited Loading

eyalpost commented May 1, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

eyalpost May 11, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

eyalpost May 12, 2017 • edited Loading

Choose a reason for hiding this comment

eyalpost May 12, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

eyalpost commented May 17, 2017

Choose a reason for hiding this comment

eyalpost commented May 18, 2017

astaxie commented May 18, 2017

eyalpost commented May 18, 2017

eyalpost commented May 18, 2017

astaxie commented May 18, 2017

eyalpost commented May 18, 2017

astaxie commented May 18, 2017

eyalpost commented May 18, 2017 • edited Loading

astaxie commented May 18, 2017 • edited Loading

eyalpost commented May 18, 2017 • edited Loading

astaxie commented May 18, 2017

eyalpost commented May 18, 2017 • edited Loading

astaxie commented May 18, 2017

eyalpost commented May 18, 2017

eyalpost commented May 18, 2017

astaxie commented May 18, 2017 • edited Loading

eyalpost commented May 18, 2017

astaxie commented May 18, 2017

eyalpost commented May 18, 2017

astaxie commented May 18, 2017

eyalpost commented Apr 22, 2017 •

edited

Loading

eyalpost commented Apr 25, 2017 •

edited

Loading

astaxie commented Apr 30, 2017 •

edited

Loading

astaxie commented Apr 30, 2017 •

edited

Loading

eyalpost May 11, 2017 •

edited

Loading

eyalpost May 12, 2017 •

edited

Loading

eyalpost May 12, 2017 •

edited

Loading

eyalpost commented May 18, 2017 •

edited

Loading

astaxie commented May 18, 2017 •

edited

Loading

eyalpost commented May 18, 2017 •

edited

Loading

eyalpost commented May 18, 2017 •

edited

Loading

astaxie commented May 18, 2017 •

edited

Loading