Documentation for Removal of Routes

[Vleerian]

Although the functions are fairly self-explanatory, they are completely absent from the documentation.

EDIT: To elaborate, it should be documented that although add_route and routes can add routes with/without trailing slashes, remove_routes has no behavior to account for that.

[vltr]

My personal opinion: why should we even need to remove a route? You just don't need to add it in the first place. The main reason for that is if you remove a route, it will only work if you're using one instance of Sanic, no workers, no k8s, load balancers, nothing. Any other scenario would require a complex system (like a pubsub) to "trigger" the removal of a route so all instances could do the same to reflect the change. Just my two cents here ...

[sjsadowski]

I 100% do not know, but the functionality is there, and so it should be documented. Optionally, toss it to the group and see if it's unneeded? I can't think of a use case where one should be removing routes.

[vltr]

Yeah, well, the functionality is there, I know, the documentation should exist. I think I'll toss it into the group, because this may also affect blueprints, nested blueprints, yadda yadda ...

[yunstanford]

We should not support remove routes

[vltr]

@yunstanford thanks, I'll raise this question on the forums then 😉

[vltr]

@sjsadowski @yunstanford I just created a more "in depth" analysis of this question in the community forums.

[Garito]

Just a use case for removing a route:
"Load a set of routes R2 if condition C else load set of routes R1"
Server starts with R1
Condition C is met -> Remove routes R1 and load R2
C example -> The admin should provide data in order to start the app so R1 is a single route for that mater and R2 are the regular routes
From the developer perspective this seems dumb cause you can create a script to do that and run it before start the server
But if your app needs to be installed and initilized by a non developer you need to provide a UI for this

[vltr]

Or just have app instance or setup instance and know which one to call. You can even stop one and start the other in case setup is done and app needs to take place. Anyway, there are a million ways this can be done without a developer having to put its hands on the instance ...

[sjsadowski]

I'm going to submit a PR to deprecate this feature - provided I don't find it in use elsewhere. There were no real objections in the community discussion. I think we slipped on deprecating in 18.12, which is fine, but we can deprecate in 19.03 and then remove for 19.12.

[vltr]

@sjsadowski agreed.

[Garito]

@vltr could you elaborate?

I'm already having the need

Thanks

[vltr]

@Garito on that workflow for setup and then app ?

[Garito]

On how to change apps by visiting a route of one of them, please
Will be awesome if you show how to destroy the 1st app too
Thanks!

[vltr]

@Garito no problem ... I made a small example here, it's a bit clunky but it works ... Of course, it would be much easier to do using the Sanic.create_server utility and have full control over the loop, but since most people uses Sanic.run for their servers, here's a working example using a simple lock file:

from pathlib import Path

from sanic import Sanic
from sanic.response import text
from sanic.exceptions import abort

LOCK_FILE = Path(__file__).parent / "SETUP"
main_app = Sanic("main_app")
setup_app = Sanic("setup_app")


def check_lock_file():
    return LOCK_FILE.exists()


def run_server(app):
    app.run(host="0.0.0.0", port=8000)


@main_app.get("/")
async def main_app_handler(request):
    return text("Hi, I am the main app after being configured!")


@setup_app.get("/")
async def setup_app_handler(request):
    return text("You need to configure your instance first. "
                "POST {\"setup\": 1} to /setup")


@setup_app.post("/setup")
async def setup_app_do_setup(request):
    if not request.json:
        abort(400)
    if "setup" in request.json and request.json.get("setup") == 1:
        LOCK_FILE.touch()
        return text("Server was configured. Refresh this page in a minute to "
                    "your running application.")
    else:
        return text("You should read the instructions on root")


@setup_app.middleware("response")
async def check_lock_file_response_middleware(request, response):
    if check_lock_file():
        request.app.stop()


@setup_app.listener("after_server_start")
async def check_lock_file_listener(app, loop):
    if check_lock_file():
        app.stop()


def main():
    run_server(setup_app)
    run_server(main_app)


if __name__ == "__main__":
    main()

[Garito]

Did you realize that I can do the same with 4 lines of code with the router removal?
If it depends on me I would not remove this feature
Is there any complain or security reason to remove it?

[sjsadowski]

@Garito the behavior is inconsistent, support in code is inconsistent, and generally it's bad architectural practice to dynamically add and remove routes while the application is running.

The current plan is to deprecate it - which leaves the feature in place if you're using the current LTS version (18.12) but in the future, will remove it.

[vltr]

@Garito if you spawn your Sanic application with multiprocessing (using the workers argument above 1), you'll end up having to make this "routing removal" reflect the changes to all instances - and that's a bit of a problem; just to add how you end up within a situation like @sjsadowski described. This can also happen if you add routes "on-the-fly" ... It's just bad practice. The thing is: add routes is something you really need to do (prior to launch the server), while removing routes is avoidable.

[Garito]

Can I ask then for a better solution than the one above?

From my point of view, this is a pattern that I would use everywhere since I like products that are easy to use and setup them is part of the easy use (webapps)

I will be ok if you tell me that I can add and remove routes if I stop the app first

Do you understand the use case? I think is not a non sense or a stupid think to ask

[sjsadowski]

@Garito if you stop the app, when you start it again you can just add the routes you want.

You could do this by setting an environment file, or updating a config, or anything.

That's the point @vltr and I are trying to make: adding routes happens before the server starts and should be immutable, removing routes is not something that should happen while the server is running - and therefore, when the server starts you only add the routes you want. It doesn't matter if those are different routes from the previous time you ran the server.

[Garito]

If changing routes by stoping the app first is ok, there is no need to deprecate this method then, isn't it?

[vltr]

@Garito well, there is because you can choose just not to add them instead of removing them. Quickly dirty snippet with no test following (inspired by my previous example):

def add_setup_routes(app):
    if not check_lock_file():
      @app.post("/setup")
      async def my_setup_method(request):
        return text("hello, setup")


add_setup_routes(main_app)

And that's it. You don't need to remove anything, you simply don't need is to add them on the first place.

[Garito]

Sorry @vltr but seems I'm not explaining the use case correct:
We need to allow the user to setup the app if there is no previous setup so, now, i'm checking if the setup has been made (by quering the db) and if not I load a unique route (the setup one) and wait
The user runs the setup and I need to remove the setup route (remove is how its made now) and load the final ones
I'm doing this with a before_server_start listener defined in the app factory (the classical create_app function)
Then I return the app and run it in the if __name__ == "__main__": classical section
This allows to me the expected flow and I have no preference for the remove_route or whatever

The point is that the setup route is the trigger for the change of routes

What I would prefer to avoid is a mechanism that has more code than the rest of the factory giving the fact that I'm already doing it easily and no one has complain about remove_route and you noticed (aparently, I would accept that you were thinking about that before but not in public) when someone ask to add documentation

If the mechanism has some issues and has to be done right, even more important than sanic has it well implemented (the raise of webapps will make this pattern a very used one)

[vltr]

@Garito I really do understand your case. What I'm thinking is beyond your use case: imagine some complex deployments, with Sanic instances in different machines behind a load balancer, for example. Now, imagine if Sanic takes any responsibility on making server changes on the fly (either by adding routes or removing ones) and make them reflect to all other instances running (in the same machine or not). It's simply a whole world of mess just to think about with no guarantee it would actually work.

So, having a remove route functionality is already a great deal of responsibility that was added to Sanic that we can't actually provide any guarantee it would work in any production scenario that lays ahead of us.

So, adding routes is something like you really need to do to have a working server, then do it prior to the server have started. Removing routes is like saying "you can remove routes anytime", which brings us to the problem described above. There's too much scenarios to cover that should not be our responsibility. Our responsibility is to provide the proper way for developers to create their services with all "weirdnesses" we know there's out there, which means we'll have to tackle and remove week points and provide enough information for best practices under any scenario. That's what I'm trying to do here.

I'm not being picky or something because for you this works just fine, but we need to consider everyone using Sanic and deliver the smoothest experience possible, even though sometimes they doesn't seem right at the first sight and would just add more "complexity" to the developers code, but that's just the trade-off of this "smooth experience". I'm really sorry if this is not what you wanted and we are here to help, always.

[Garito]

Ok, I'm on my own, roger that, thanks

[Vleerian]

I have no issue with the functionality being deprecated, though I have found that being able to remove routes is actually a very valuable and useful feature.

Though I understand that my use case is relatively niche, being able to add/remove/reload routes has proven incredibly helpful, in that I can modularize the service I am running, and encapsulate individual features, as well as adding/updating functionality without having to restart the application.

Implementing this modularity is how I discovered the behavior in the first place, and I think it'd be a shame to see it go.

[vltr]

@Atagait yeap, we do understand that this functionality is really a niche, because (as I stated), it'll only work properly if you have only one instance running in production with only one worker. Because this condition may be only a fraction of all Sanic instances running, it'll be hard to maintain.

Anyway, if this is something that is also @Garito's case, I think we can elaborate some sort of documentation on how to perform these actions (or a workaround) in the documentation when this functionality becomes extinct from Sanic (having all warnings in the world about how this could go wrong as well).

[Garito]

Thank you, that's what I'm asking for (ideally with a clear pattern not to crazy complicated if I may)

[sjsadowski]

Superseded by #1511