Route Groups

[ScienJus]

Before:

@Override
protected void onInit() {

    GET("/users/{id}", routeContext -> {
        routeContext.send("find user, id: " + routeContext.getParameter("id").toString());
    });

    POST("/users", routeContext -> routeContext.send("create a new user"));

    PUT("/users/{id}", routeContext -> {
        routeContext.send("modify user, id: " + routeContext.getParameter("id").toString());
    });

    DELETE("/users/{id}", routeContext -> {
        routeContext.send("delete user, id: " + routeContext.getParameter("id").toString());
    });
}

After:

@Override
protected void onInit() {

    GROUP("/users", new RouteGroupHandler() {

        @Override
        public void handle() {

            GET("{id}", routeContext -> {
                routeContext.send("find user, id: " + routeContext.getParameter("id").toString());
            });

            POST(routeContext -> routeContext.send("create a new user"));

            PUT("{id}", routeContext -> {
                routeContext.send("modify user, id: " + routeContext.getParameter("id").toString());
            });

            DELETE("{id}", routeContext -> {
                routeContext.send("delete user, id: " + routeContext.getParameter("id").toString());
            });
        }
    });
}

I think it's better

ps: i pulled into a wrong branch?

[gitblit]

We have some utility methods in StringUtils to add and remove prefix or trailing strings. Using those would simplify this method.

[gitblit]

I think Travis fails because there is no import for RouteGroupHandler.

[gitblit]

@ScienJus this looks like a nice improvement to Pippo. I've left some feedback for you on ways I think it could be improved and I've posed a question too. @decebals may have some additional thoughts on this feature; he had planned on implementing this several months ago but then life happened.

[decebals]

@ScienJus thanks for your contribution.
This functionality is on our roadmap (https://groups.google.com/forum/#!topic/pippo-java/ocmVEJ7oLw8) but I don't decide how to show the API from this feature in the final form.
My idea is to have a clean and simple API that does not require too much documentation.

Now in my mind is something similar with:

RouteGroup usersGroup = RouteGroup("/users");
GET("{id}", routeContext -> { ... }).group(usersGroup);
POST("{id}", routeContext -> { ... }).group(usersGroup);
// add more routes in this group

I will reflect a little bit at this feature.
Any advice is welcome.

[ScienJus]

i think we should inject slash on nested groups and remove slash if it's end of pattern. the fault tolerance will be better.

like:
/users + /{id} = /users + {id} = /users/ + {id} = /users/ + /{id}

[ScienJus]

@decebals it's good, you can do that when groups have more options, like:

RouteGroup usersGroup = RouteGroup()
                         .withNamespace("/users")
                         .withController(UserController.class)
                         .withInterceptor(AuthInterceptor.class);
GET("{id}", routeContext -> { ... }).group(usersGroup);
GET("{id}/follow", "follow").group(userGroup);  //UserController.follow

[decebals]

@ScienJus Yes. This is the idea. I want to add more options/attributes to RouteGroup.
The routes from that group inherit the attributes of the group.
For now I see two attributes:

• path (in our example is users)
• filter routes (authentication, inject something in localof Response)

Also. I see two possible API:

• create a RouteGroup and specify this group to each route (our example from previous comments)
• create a RouteGroup and instead of GET("{id}", routeContext -> {}).group(myGroup) we can use this snippet code:

myGroup.addRoute(...); // or a shortcut method like 'myGroup.GET("{id}", routeContext -> { ... })'
addRouteGroup(myGroup); 

[ScienJus]

@decebals @gitblit i think one group have many child groups and many routes, so on current commit, you can use group like that:

// /users/1/like  route in group1, group1 in group2
Route route = new Route("POST", "like", routeContext -> {routeContext.send("i like you");})
    .inGroup(new RouteGroup().withNamespace("{id}")
        .inGroup(new RouteGroup().withNamespace("/users")));
addRoute(route);

or:

RouteGroup group = new RouteGroup()
    .withNamespace("/users")
    .addGroup(new RouteGroup()
        .withNamespace("{id}")
        .addRoute(new Route("POST", "like", routeContext -> {routeContext.send("i like you");})));
addGroup(group);

you also can use onInit method:

@Override
protected void onInit() {

    GROUP(new RouteGroup() {

        @Override
        public void onInit() {

            GROUP(new RouteGroup() {

                @Override
                public void onInit() {
                    POST("like", routeContext -> {
                        routeContext.send("i like you");
                    });
                }
            }.withNamespace("{id}"));
        }
    }.withNamespace("/users"));
}

what do you think?

[decebals]

I think that I see the light and I like what I see. 😄

I prefer to have the same API in Application and RouteGroup related to route adding (addRoute and GET, POST, ... methods). In this variant I can add a route on application or on a group. Clear and simple.

I prefer to add the route group in Application using a simple addRouteGroup method .

So, the code can looks like:

addRouteGroup(new RouteGroup("uriPattern") {    
    // add routes
);

or

// reduce code lines in Application with
addRouteGroup(new UserGroup());
addRouteGroup(new ContactGroup());

I don't like to add GROUP method in Application. I prefer only a simple addRouteGroup method. The uppercase methods are only for HTTP verbs (GET, POST, ... plus ALL as convention for all verbs).

I prefer to preserve uriPattern from Route instead of RouteGroup.namespace (it's much clear what it meaning). Maybe it's util to have a constructor with uriPattern as parameter (personal I will use new RouteGroup("/users") and not new RouteGroup().setUriPattern("/users")).

I don't think that we need RouteGroup.onInit(). We can add routes directly in the constructor.
By the way, in our demo code we add all routes in Application.onInit method BUT it's not mandatory. Application.onInit is called after all Initializers but these initialers are not stoppers for routes adding so I can add routes in the constructor of my application (this is the theory, I will test to see in practice).

That is my correction to the last comment of @ScienJus

[ScienJus]

@decebals .i agree that new RouteGroup(uriPattern) is better than new RouteGroup().withUriPattern("xxx"), because uriPattern is required for the group.

i use withNamespace just for compatible with Application.GROUP, so we can remove them all.

i have a question about Route.getUriPattern, after we create group, route path will be group path concat route path, so i create a method like this:

// path with group
public String getCompletePath() {
    RouteGroup group = this.group;
    String path = this.uriPattern;
    while (group != null) {
        path = StringUtils.addStart(StringUtils.addStart(path, "/"), group.getUriPattern());
        group = group.getParent();
    }
    if (StringUtils.isNullOrEmpty(path)) {
        return path;
    }
    return path.endsWith("/") ? path.substring(0, path.length() - 1) : path;
}

then i modify getUriPattern to call this method, is that ok? or modify code where call getUriPattern change to call getCompletePath?

[decebals]

I see that Route.getUriPattern so I believe that it's safe to return getCompletePath in getUriPattern. I don't know if for the moment we need the initial/relative uriPattern.

I prefer to rename getCompletePath to getFullUriPattern.

I will make some comments on you code

[decebals]

Add a new method addRouteGroup in Router interface, call getRouter().addRouteGroup() here and move the implementation in DefaultRouter. The idea is that I want to add the tests in Router.

[decebals]

That is all.

[ScienJus]

@decebals another question, do we need some methods to remove route from group or remove group from group or remove group from application?

[decebals]

Why this modification?

[decebals]

I understand you. I prefer to put public also for all methods in an interface. For the future it's a good practice to not touch the code if we don't add something new or if not fix a very bad formatted code.

I will change minor things after I merge this PR.

Thanks very much for this important PR!

[ScienJus]

Thanks @decebals and @gitblit , I have learned a lot in this PR, thanks for your comments.

[gitblit]

👍 Cool. Route groups will be pretty handy and give Pippo a little more flexibility in app config. Nice job.