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

Ideas for Go-Json-Rest v3.0.0 (RFC) #110

Closed
ant0ine opened this Issue Dec 27, 2014 · 25 comments

Comments

Projects
None yet
6 participants
@ant0ine
Copy link
Owner

ant0ine commented Dec 27, 2014

Ideas for Go-Json-Rest v3.0.0 (RFC)

V3 is the opportunity for API changes and improvements.

In the past two years Go-Json-Rest has changed a lot. It got the notion of middleware, bringing basic features like Auth, CORS, JSONP that were missing at the beginning. Over time the list of options and settings has doubled. This is reflected in the main object ResourceHandler that has become a long list of options. It works, but it could be more convenient to use. The goal of the v3 is to provide a replacement for ResourceHandler that is simpler to use and more flexible.

The ResourceHandler does not do much, given the settings, it instantiates the middlewares and the router, and wraps all that in order to produce a net/http Handler. The middlewares and the router are in fact the low-level api of go-json-rest. The idea of v3 is to open it, make it public.

Proposed new API

1) Make public the existing private Middlewares

So in addition the following, already public, middlewares:

  • AuthBasicMiddleware
  • CorsMiddleware
  • JsonpMiddleware

We will get the following ones:

  • AccessLogApacheMiddleware
  • AccessLogJsonMiddleware
  • TimerMiddleware
  • RecorderMiddleware
  • GzipMiddleware
  • RecoverMiddleware
  • ContentTypeCheckerMiddleware
  • JsonIndentMiddleware
  • PoweredByMiddleware
  • StatusMiddleware

2) Propose some predefined stacks of Middlewares

Precise lists and options to be determined, but for instance:

var DefaultDevStack = []Middleware{
        &AccessLogApacheMiddleware{},
        &TimerMiddleware{},
        &RecorderMiddleware{},
        &JsonIndentMiddleware{},
        &PoweredByMiddleware{},
        &RecoverMiddleware{
                EnableResponseStackTrace: true,
        },
}

var DefaultProdStack = []Middleware{
        &AccessLogApacheMiddleware{},
        &TimerMiddleware{},
        &RecorderMiddleware{},
        &GzipMiddleware{},
        &PoweredByMiddleware{},
        &RecoverMiddleware{},
        &CheckContentTypeMiddleware{},
}

Most of the options and settings of the current ResourceHandler end up being options of the middlewares, or just including of not including a middleware.
Example:

MyStack := []Middleware{
        &AccessLogApacheMiddleware{
               Logger: &myLogger,
               Format: rest.CombinedLogFormat,
        },
        &TimerMiddleware{},
        &RecorderMiddleware{},
        &GzipMiddleware{},
        &RecoverMiddleware{
               Logger: &myLogger,
        },
        &CheckContentTypeMiddleware{},
}

3) A new App interface

Go-Json-Rest already defines this interface:

    type HandlerFunc func(ResponseWriter, *Request)

    type Middleware interface {
        MiddlewareFunc(handler HandlerFunc) HandlerFunc
    }

In v3, it will be completed by this new:

    type App interface {
        AppFunc() HandlerFunc
    }

v3 will also offer the two following adapter types. Convenient to write simple Apps or Middlewares without defining types. (Unit tests are written this way, for instance)

type MiddlewareSimple func(handler HandlerFunc) HandlerFunc
func (ms MiddlewareSimple) MiddlewareFunc(handler HandlerFunc) HandlerFunc {
}

type AppSimple HandlerFunc
func (as AppSimple) AppFunc() HandlerFunc {
}

It allows to write

        api.SetApp(AppSimple(func(w ResponseWriter, r *Request) {
                ...
        }))

        api.Use(MiddlewareSimple(func(handler HandlerFunc) HandlerFunc {
                return func(w ResponseWriter, r *Request) {
                        ...
                }
        }))

4) Explicitly build the router as an App

The router already implements the App interface by providing the AppFunc method. The following function will be added to explicitly build it:

func MakeRouter(routes ...Routes) (App, error) {
}

Building a Go-Json-Rest api now consists in assembling a stack of Middlewares and putting an App on top of it. It also open the door to interchangeable routers by allowing third party routers to be wrapped inside an App.

5) Finally, this new Api object that provides the syntactic sugar

type Api struct {
}

func NewApi() *Api {
}

func SetApp(app App) {
}

func (api *Api) Use(middlewares ...Middleware) {
}

func (api *Api) MakeHandler() http.Handler {
}

The new "Hello World!" example

"Hello World!" with JSONP support:

func main() {

        api := rest.NewApi()

        // the Middleware stack
        api.Use(rest.DefaultDevStack...)
        api.Use(&rest.JsonpMiddleware{
                CallbackNameKey: "cb",
        })

        // build the App, here the rest Router
        router, err := rest.MakeRouter(
                &Route{"GET", "/message", func(w rest.ResponseWriter, req *rest.Request) {
                        w.WriteJson(map[string]string{"Body": "Hello World!"})
                }},
        )
        if err != nil {
                log.Fatal(err)
        }
        api.SetApp(router)

        // build and run the handler
        log.Fatal(http.ListenAndServe(
                ":8080",
                api.MakeHandler(),
        ))
}

Compared to the previous version:

This:

func main() {
        handler := rest.ResourceHandler{
                PreRoutingMiddlewares: []rest.Middleware{
                        &rest.JsonpMiddleware{
                                CallbackNameKey: "cb",
                        },
                },
        }
        err := handler.SetRoutes(
                &rest.Route{"GET", "/message", func(w rest.ResponseWriter, req *rest.Request) {
                        w.WriteJson(map[string]string{"Body": "Hello World!"})
                }},
        )
        if err != nil {
                log.Fatal(err)
        }
        log.Fatal(http.ListenAndServe(":8080", &handler))
}

Semver and backward compatibility

Go-Json-Rest follows Semver, breaking API changes are introduced only with major version changes. In this particular case, even if the API changes are important, it is possible to maintain the backward compatibility and just mark the old API as deprecated.

The objects will be marked as deprecated, and after a few months, can be removed from the package.

@ant0ine ant0ine self-assigned this Dec 27, 2014

@ant0ine ant0ine changed the title Ideas for Go-Json-Rest v3.0.0 Ideas for Go-Json-Rest v3.0.0 (RFC) Dec 27, 2014

@missedone

This comment has been minimized.

Copy link

missedone commented Dec 28, 2014

Hi ant0ine

When i routes URIs contains '.', i have to use placeholder notation #paramName rather than :paramName, i think it's the nonintuitive part
as this is good opportunity to introduce api change in v3, i'm wondering would you make it consistent so that notation :paramName can handle both case (w/o '.' in the URI)
thanks

@ant0ine

This comment has been minimized.

Copy link
Owner

ant0ine commented Dec 28, 2014

Right, the Go-Json-Rest framework supports :param, #param and *param. It's different than some other frameworks (like ExpressJS for instance), but it's the same as some others (like http://mojolicio.us/ for instance).
So depending on who you ask, it's confusing, or not :-)

I'd like to keep the router work outside of v3 for now, hopefully a standard will emerge.

@nerdatmath

This comment has been minimized.

Copy link

nerdatmath commented Dec 28, 2014

I would like to see a way to create a route directly from a function, using reflection. Like this:

type Message struct{Body string}
func HelloWorld() (Message, error) {
    return Message{Body: "hello, world!"}, nil
}
router, err := rest.MakeRouter(
     rest.NewRoute("GET", "/message", HelloWorld),
)

For a POST or PUT, the input would be a single parameter to the function. If err is returned then the data parameter would be ignored and rest.Error would be called. If params exist in the route, those would be the first parameters to the function, in the order they appear in the pattern. Only simple types would be allowed for params, like int and string.

Bound methods would also be supported, of course.

Reflection would only be needed while building the Router, so performance should not take a hit.

If you like the idea, I can code it up and submit a pull request.

@ant0ine

This comment has been minimized.

Copy link
Owner

ant0ine commented Dec 28, 2014

Hi @mattharden !
Thanks for the feedback! I like the idea.

Essentially, this is auto-generating the Routes for the CRUD operations of a given Go object. There are many ways to do this, and many reasons to auto-generate the Routes. For instance I'm thinking about implementing Swagger on top of this framework. (http://swagger.io/)

These tools to generate Routes, from an API specification language (swagger) or from Go objects (using reflection) are very interesting, but I think this should be done in a higher layer than this framework. Built on top, rather than inside. I think this is key to keep this project light and simple, and offer to the users many ways to generate the Routes.

@nerdatmath

This comment has been minimized.

Copy link

nerdatmath commented Dec 30, 2014

Yes, I agree with that. Something that works with this framework but isn't tied to it would be even better. A code generator that produces Swagger schema and REST implementation from Go struct definitions would be nice.

@yannk

This comment has been minimized.

Copy link
Contributor

yannk commented Dec 31, 2014

re: swagger/other tool: Seems like a job for the new go generate

@yannk

This comment has been minimized.

Copy link
Contributor

yannk commented Dec 31, 2014

What's the reasoning for making Api.Stack and App mutable? Could you just provide it when the struct is instantiated? Relatedly Stack as a name maybe leaks unimportant implementation details (esp. if you remove PushStack and UnshiftStack if deemed unnecessary per my previous question), maybe a better name would be Middlewares -- or middlewares and app if a NewApi(a App, m []Middleware) (Api, error) is provided.

@yannk

This comment has been minimized.

Copy link
Contributor

yannk commented Dec 31, 2014

Is there any specific meaning to Make in MakeRouter() and MakeHandler()? Maybe Router() and Handler() better matches the convention used in net/http

@ant0ine

This comment has been minimized.

Copy link
Owner

ant0ine commented Jan 3, 2015

@yannk Good point about Api.Stack and Api.App being mutable. I like NewApi(a App, m []Middleware) (Api, error) in fact I had a draft with MakeHandler(a App, m []Middleware) (http.Handler, error). I think it's an elegant way of writing it. But I'm left with one problem: All the options/settings must be options/settings of the Middleware objects or the App object. Which is almost true, but not completely. I'm left with a few options that I have to put somewhere. Here are the 3 options that I have to find a place for in the API: https://github.com/ant0ine/go-json-rest/blob/master/rest/adapter.go#L11

@ant0ine

This comment has been minimized.

Copy link
Owner

ant0ine commented Jan 4, 2015

Update: I removed from the spec the idea of a dedicated package for the middlewares. This creates circular import problems. So, middlewares will be objects of the rest package. I'm keeping the "Middleware" suffix as a way to quickly identify them, even if I don't really like it.

@ant0ine

This comment has been minimized.

Copy link
Owner

ant0ine commented Jan 4, 2015

@yannk I'm thinking about way to move the adapter options into other middlewares, which will make the NewApi or MakeHandler idea possible.

Also, I think there is an even better signature for this method:
func MakeHandler(a App, m ...Middleware) (*http.Handler, error)

That way, no need to play with append, it can be conveniently used like this:

handler, err := rest.MakeHandler(
        app,
        rest.DefaultStack...
        rest.JsonpMiddleware{},
)
@nerdatmath

This comment has been minimized.

Copy link

nerdatmath commented Jan 5, 2015

Can you share details on the circular import problem? I think it should be fixable in a less "distasteful" way than keeping the Middleware suffix.

@ant0ine

This comment has been minimized.

Copy link
Owner

ant0ine commented Jan 6, 2015

@mattharden Sure.
Moving the middlewares to a second package (called "middlewares" for instance) is technically possible. "middlewares" will depend on "rest" obviously, and I think it's possible to make "rest" NOT depend on "middlewares" (by moving all the unit tests depending on the middlewares to the "middlewares" package).

But that's the theory :-)

While prototyping that I realized that I could not maintain compatibility with the previous API. For instance, in order to keep ResourceHandler working, it needs to be implemented in terms of the "middlewares" package, creating this circular import.

My current thinking is to keep a single package, keep the ugly suffixes, and maintain the backward compatibility. ResourceHandler will be marked as deprecated (maybe even logging a "deprecated" error message to encourage people to make use of the new API)

Then, once ResourceHandler is gone, a few months from now, moving the middlewares to their own package will be easier. Essentially, just a search and replace, something like that:
s/rest.(\w+)Middleware/middlewares.$1/g

It is kind of a slow path, and I don't like the suffixes, but it seems safe and predictable.

@Quantisan

This comment has been minimized.

Copy link
Contributor

Quantisan commented Jan 6, 2015

This looks exciting. Although I am unable to follow everything. I want to add that when you want a hand in making it happen, feel free to ping me @ant0ine

@ant0ine

This comment has been minimized.

Copy link
Owner

ant0ine commented Jan 7, 2015

Thanks @Quantisan ! It's making progress, slowly but surely!

@nerdatmath

This comment has been minimized.

Copy link

nerdatmath commented Jan 8, 2015

@ant0ine You could take advantage of Go 1.4's internal packages by moving the middleware implementations that are needed by ResourceHandler into rest/internal, and importing that in both rest and rest/middlewares (btw. I suggest the singular middleware instead). Then re-export the stuff in rest/middlewares.

@ant0ine

This comment has been minimized.

Copy link
Owner

ant0ine commented Jan 10, 2015

"as of Go 1.4 the go command introduces a mechanism to define "internal" packages that may not be imported by packages outside the source subtree in which they reside."
https://go.googlecode.com/hg/doc/go1.4.html

I didn't know about this new feature, thanks @mattharden!

@matrixik

This comment has been minimized.

Copy link

matrixik commented Jan 10, 2015

For now it's working only for Go language itself and not for external packages, but they plan to enable it for packages in Go 1.5.

But of course you can use it now to indicate for devs to not depend on packages in internal folder.

@yannk

This comment has been minimized.

Copy link
Contributor

yannk commented Jan 12, 2015

the &rest.Route{} notation used in all examples is handy, but is making golint bark about un-keyed struct fields.

Maybe v3 could be a bit more compliant.

Instead of handler.SetRoutes(...), what about something to that effect:

app := rest.NewApp()
app.AddRoute("GET", "/sample", func) // panic if pathExp or method are invalid
...
app.AddMiddleware(middlewares...)
handler, err := app.Handler() // calls app.router.start(), etc..
http.ListenAndServe(":8080", handler)

Note: the Api{} you had defined has been edited, so I'm not sure what benefit it was providing over App above.

ant0ine added a commit that referenced this issue Jan 20, 2015

Implementation branch for the v3
Spec here: #110

This commit includes the new interfaces and the Api object.
@ant0ine

This comment has been minimized.

Copy link
Owner

ant0ine commented Jan 20, 2015

@yannk I tried to make the code as compliant as possible with golint, but I gave up on the all caps token names, like Url should be URL, Api should be API, and so one. I tried, and really, URLFor, WriteJSON, HTTPMethod feels like the code is yelling at you !
So, it won't be 100% golint compliant.
About the having a method AddRoute, I think that's doable, I'll try it.

@ant0ine

This comment has been minimized.

Copy link
Owner

ant0ine commented Jan 20, 2015

Updated the specs, taking a lot of feedback into account. The new App and Api type definitions feel solid now. Maybe a bit more work left on the Router.
Implementation started here: #123

@ant0ine

This comment has been minimized.

Copy link
Owner

ant0ine commented Jan 20, 2015

And the migration of the examples has started here: ant0ine/go-json-rest-examples#5

@ant0ine

This comment has been minimized.

Copy link
Owner

ant0ine commented Jan 24, 2015

Added the AppSimple and MiddlewareSimple adapters types in the specs. So we can conveniently write:

        api := NewApi(AppSimple(func(w ResponseWriter, r *Request) {
                ...
        }))

        api.Use(MiddlewareSimple(func(handler HandlerFunc) HandlerFunc {
                return func(w ResponseWriter, r *Request) {
                        ...
                }
        }))

and rewrote all the middleware unit tests using the new API, see the v3 Pull Request: #123

@ant0ine

This comment has been minimized.

Copy link
Owner

ant0ine commented Jan 28, 2015

Updated with a slight change on the Api object. The pattern is now:

        api := rest.NewApi()
        api.Use(rest.DefaultDevStack...)
        api.SetApp(rest.AppSimple(func(w rest.ResponseWriter, r *rest.Request) {
                w.WriteJson(map[string]string{"Body": "Hello World!"})
        }))
        log.Fatal(http.ListenAndServe(":8080", api.MakeHandler()))

This enables Apis with no App. It's also easier to read as the things are defined in order. And it also makes the Middlewares easily available to the App, like on the status example.

All examples and tests have been updated with this change.

We are getting closer to the API freeze for v3.

The last remaining open question is: Introducing or not introducing constructor methods for the Route object. Like:

router, err := rest.MakeRouter(
       rest.GET("/stream", MyHandler),
)

instead of:

router, err := rest.MakeRouter(
       &rest.Route{"GET", "/stream", MyHandler},
)

Beside the golint question, what is key is to preserve a Route object. This way we can do stuff with Routes. Like reverse route resolution: route.MakePath(...), ...

My current thinking is that this can be kept for post v3 release.

@ant0ine

This comment has been minimized.

Copy link
Owner

ant0ine commented Feb 3, 2015

This is now merged! Release announcement soon!

@ant0ine ant0ine closed this Feb 3, 2015

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