-
-
Notifications
You must be signed in to change notification settings - Fork 1.5k
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
Documentation for Removal of Routes #1195
Comments
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 ... |
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. |
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 ... |
We should not support remove routes |
@yunstanford thanks, I'll raise this question on the forums then 😉 |
@sjsadowski @yunstanford I just created a more "in depth" analysis of this question in the community forums. |
Just a use case for removing a route: |
Or just have |
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. |
@sjsadowski agreed. |
@vltr could you elaborate? I'm already having the need Thanks |
@Garito on that workflow for |
On how to change apps by visiting a route of one of them, please |
@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 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() |
Did you realize that I can do the same with 4 lines of code with the router removal? |
@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. |
@Garito if you spawn your Sanic application with |
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 |
@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. |
If changing routes by stoping the app first is ok, there is no need to deprecate this method then, isn't it? |
@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. |
Sorry @vltr but seems I'm not explaining the use case correct: 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) |
@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. |
Ok, I'm on my own, roger that, thanks |
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. |
@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). |
Thank you, that's what I'm asking for (ideally with a clear pattern not to crazy complicated if I may) |
Superseded by #1511 |
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
androutes
can add routes with/without trailing slashes,remove_routes
has no behavior to account for that.The text was updated successfully, but these errors were encountered: