Deprecate sub-router

[pmlopes]

Currently sub routing is a cause of confusion and introduces many ambiguity on the behaviour of the core Router. On top of that in introduces complex internal code flows when dealing with reroute, error handling, matching, etc...

I propose that we deprecate and later remove the following methods:

 /**
   * Mount a sub router on this router
   *
   * @param mountPoint  the mount point (path prefix) to mount it on
   * @param subRouter  the router to mount as a sub router
   * @return a reference to this, so the API can be used fluently
   */
  @Fluent
Router mountSubRouter(String mountPoint, Router subRouter);

[cescoffier]

👍

[alexanderGalushka]

Subrouters is a great feature, don't deprecate.

[pmlopes]

@alexGalushka could you elaborate why? and why can't you just achieve the same with the base router? We would like to hear everyone before taking any decision

[fclairamb]

We've been using for quite some time and it's very convenient for many things like grouping handler. Here is an example:

Router subRouter = Router.router(vertx);
Server.router.mountSubRouter("/admin/", subRouter);
subRouter.route().handler(new AdminAuthHandler());
subRouter.get("/first").handler(this::askPeopleHowTheyUseIt);
subRouter.post("/second").handler(this::provideAnAlternativeSolution);
subRouter.post("/third").handler(this::deprecateIt);

[pmlopes]

@fclairamb your example is not showing why you can't really live without it, it can be re-written as:

router.route("/admin/").handler(new AdminAuthHandler());
router.get("/admin/first").handler(this::askPeopleHowTheyUseIt);
router.post("/admin/second").handler(this::provideAnAlternativeSolution);
router.post("/admin/third").handler(this::deprecateIt);

I totally agree that we shouldn't deprecate without alternatives but your example does not seem too be a blocker

[alexanderGalushka]

@pmlopes we use subrouters throughout our code base, it helps greatly with seamless extensibility. New components can simply plug their own subrouter to the parent router. It would be a great idea to improve/revise documentation, but please do not deprecate :)

[pmlopes]

@alexGalushka do you perform any reroute in your code? since its behaviour is router bounded it will confuse users expecting it to restart the whole processing. Having everything as a single router would simplify code and fix this.

[fclairamb]

Oh yes that's right, the rewriting effort should be minimal on our side.

[alexanderGalushka]

@pmlopes we don't perform any reroute therefore have not faced with this confusion. Would documentation improvement help?

[dano]

@vietj mentions his take on the best way to modularize routing here: https://github.com/vert-x3/issues/issues/138#issuecomment-233169116

To me, the most natural way to implement what he describes is via sub-routers. If they go away, what's the recommended way to implement a system where many components (or micro-services, or whatever you want to call them) register in their own endpoints in to a centralized router?

I guess you'd need to actually pass the single, top-level Router in to each component, and have them add their endpoints directly to that, instead of components providing their own sub routers, which then get pulled in by a single Verticle and mounted to a top-level router?

[pmlopes]

@dano I don't see where you read that he recommends sub routers. And sub routers have currently no discovery mechanism associated so I don't understand your comment.

[pmlopes]

@dano if you're going to aggregate subrouters in a top level router, then you're not really doing microservices but falling back to the monolith world again. But this is not what we're discussing here.

[dano]

@pmlopes He didn't explicitly mention them, but when I read this:

the user should have a way to discover routers and load them in the same verticle

To me that sounded like creating a way to discover - e.g. via classpath scanning - components that provide Routers, loading them into the same verticle and mounting them all to the same parent router. Maybe I misunderstood?

[kastork]

People seem to want two things:

1. Have a way to compose a full set of endpoints from different modules at compile or deployment time.
2. Have a way for other verticles to add and remove routes from a deployed server verticle's router without redeploying that server.

Case 1 is well supported, and the deprecation of sub-routers would just be an inconvenience for cases where you are making use of the notion of a mount point for a set of endpoints. Say you use them in more than one app and for some reason need the path to be different in each one.

Case 2 isn't really supported at all (correct me if I'm wrong about that), but people have tried to work around that fact using OSGI and other things. Sub-routers seem to be very helpful in those (perhaps ill-advised) workarounds.

Maybe explicit support for case 2 would satisfy those currently opposing this deprecation?

[pmlopes]

@castork doesn't Route remove, disable and enable solve your case nr 2? It can be applied to any handler not just routers.

[kastork]

Maybe, but it isn't clear to me how some verticle being deployed into a Vertx instance would be able to find the existing router in that instance and then add its own routes/handlers into that existing router. It may be possible, I just haven't figured it out, other than the workaround examples that have been mentioned.

Also, if you could do what I just suggested, you have the problem that two separate verticles are poking around in the same Router, which is, as I under stand it, why Case 2 is not supported in the first place.

[aesteve]

The admin example is a pretty common use case for me. The AdminRouter class lies in a jar and every Vert.x user mounts it on the path he chose by himself.

This router comes with a set of utility (supervision) routes. It's just like a framework we define, registering its own routes and and end-users can use without declaring them.

Sure it could be re-written as a static createAdminRoutes(String path, Router existingRouter) or createRouterWithAdminRoutes(String path, Vertx vertx) method, though.

I'm often using subRouter to distinguish between static file routes and api routes, too.

router.mountSubRouter("/static", new StaticRouter());

// where StaticRouter is a wrapper around StaticHandler, defining more cache rules, a proper failureHandler, ETags and stuff.

Here, it could also be replaced by a set of route declarations, it's just a matter of choice, but i'm finding it more readable. So if it has to disappear, I can handle it, especially if this is for a good usability reason.

[pmlopes]

If i can sum up the reasons in a high level they are:

• ambiguity on behaviour
• extra complexity of internal code to handle sub routing
  • reroute
  • error handling
  • success + end handlers
• performance penalty (vertx core vs vertx-web)

While the 1st item probably could be solved by documenting all possible scenarios, the later ones are a bit harder to fix.

[abdlquadri]

Well, before subrouters all our REST endpoints were in one single verticle. This quickly became unmanageble.

When subrouters came any new endpoint groups were placed in their own verticles and mounted on the main verticle router.

If you can propose a better solution to this design problem then, we are good with the removal.

[pmlopes]

@abdlquadri you should be aware that your "solution" is an "anti-pattern" say that you deploy with high availability enabled, if one of your verticles dies it will be respawned anywhere in the cluster making it impossible to load sub routers from across the cluster to the verticle where your http server is listening.

[abdlquadri]

@pmlopes I will be glad to know the correct pattern to solve this challenge.

[Jotschi]

What about sub routers that are shared across multiple endpoints (setup within the same verticle):

Router rootRouter = Router.router(vertx);

Router projectRouter = Router.router(vertx);
subRouter.get("/first").handler(this::dummyHandler);

rootRouter.mountSubRouter("/projectWithNameA/", subRouter);
rootRouter.mountSubRouter("/projectWithNameB/", subRouter);

@pmlopes How could this be rewritten?

I'm not sure whether this is in fact correct but maybe this might be an alternative without the usage of mountSubRouter.

Router rootRouter = Router.router(vertx);

Router projectRouter = Router.router(vertx);
subRouter.get("/first").handler(this::dummyHandler);

rootRouter.route("/projectWithNameA/").handle(projectRouter::handleContext)
rootRouter.route("/projectWithNameB/").handle(projectRouter::handleContext);

[pmlopes]

The example you have is simple to solve, you catch the first argument as a variable and add a prior handler that checks if its value is one of the prefixes.

What can go wrong here is for example if you have a variable in the prefix path that is also declared on the sub router, everything will seem fine and crash at runtime. While if you were mounting a handler you would get the error at start up.

[orbitofdeceit]

I sympathise that this API has proved problematic. Happens to all of us.

We use this feature quite extensively, although only in it simplest form (sharing code for a group of common routes, similar to the AdminRouter mentioned above). We don't use re-route (anywhere), we don't care about independent deployment or (in cases I know about) the performance.

We use it for sharing a set of common handlers like time outs, logging, authentication and body handling. Partly we do this to avoid boilerplate but also to fix the order (avoiding late registration of the body handling and to handle some nasty edge cases like pre-flight CORS requests on authenticated routes). This is code we really don't want all our development teams to repeat or rediscover for every project.

The common Routers we construct are either used as the main router (for simpler services) or mounted as one or more sub-routers for more complex cases.

Given the usage is relatively simple, I'm sure we can find some equivalent but it will be frustrating to replace this API and, to some extent, it will lower confidence in Vert.x 3 as a product given that the feature is being withdrawn so soon after being introduced.

If it's completely unmaintainable, so be it. I understand the need to avoid technical debt. However, the fact that it's can be reimplemented doesn't necessarily translate to this change being low impact to roll out.

[pmlopes]

@orbitofdeceit there is no decision on removing this API, this was just a question I've posted in order to see what the community feels about it. Do not expect it to be gone in the foreseeable future.

[orbitofdeceit]

@pmlopes Sure - I understand and appreciate you're taking the time to seek feedback.

To expand a bit on my previous comment, the part that we find useful is really just the ability to write against an interface that automatically prefixes paths without us having to write extra conditional code.

If there was some alternative API that provided that feature but that was limited to a less problematic subset of what the current Router provides, that would probably be good enough for our purposes.

[elad-yosifon]

@pmlopes we rely on this feature in one of our backend API servers. It gives us the ability to create a tree like routers with authorization hierarchy (recursive ACL of some sort).

(this is just an illustration... not a real life example) Lets say I want to expose a property of a user.. Given this route : /user/{:user_id}/account/{:account_id}/property/{:property_id}

the router is configured this way:

(users router) v
              (accounts sub router) v
                                   (properties sub router)

Each router/sub-router loads ACL permissions and checks them against the requesting user. Instead of generating 3 routers with different URI templates.. and checking all ACL rules per router.. we can cache the "parent router" permissions in the RoutingContext, and just use it in the sub-routers.

[Addvilz]

Yeah, nested routers is super useful, I am using them to compose the app of multiple routers mainly because there are so... many... routes and endpoints. I also use them for modularity, because it allows me to choose which services will be exposed on which deployment.

[vitalif]

I also have another concern about sub-routers: you should make sure that your "normal" router is hierarchical before deprecating sub-routers. I.e. if it just runs a regex for each route that's a super-dump approach - what if you have 2000 routes - do you run 2000 (ok, 1000 in average) regexes for each HTTP request?

[pmlopes]

I will close this issue for now. It seems sub routers are widely used even though there are issues with it. It was a constructive discussion and we can incorporate the discussion in future improvements.

[ORESoftware]

Creating a subrouter seems to be primarily for convenience, but isn't that what most libraries are for? Anyway, right now I have this:

    final Router router = Router.router(vertx);
    final Router sub = router.mountSubRouter("/api",router);

one thing that's weird about this API - why do I need to pass in the router as the second argument? Why not simply like so?

    final Router router = Router.router(vertx);
    final Router sub = router.mountSubRouter("/api");

[vietj]

Sub routers finally will not be deprecated as community use them a lot and we found a way to support them correctly.