Skip to content
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

[20.12LTS] Remove prefix from websocket handler name #2021

Merged

Conversation

laggardkernel
Copy link

@laggardkernel laggardkernel commented Feb 5, 2021

Formerly try to open for master branch GH-2020. Cause the Router is overhauled on master, open this pr for 20.12LTS.


Remove the websocket prefix websocket_handler_ introduced in
761eef7. Makes url_for() unable to find the corresponding ws route for non-bp websocket view.

@app.websocket('/ws')
async def websocket(request, ws):
    return ""

The key for the above handler used in Router.route_names is websocket_handler_websocket, not websocket. Removing the websocket_handler_ prefix fixes this bug. Searching in the source code, this very prefix is not used in anywhere.

I guess the contributor of commit 761eef7 tried to mimic the registration behavior of static file handler, which prepend the handler name with prefix _static_, in order to store static file handler separately in Router.routes_static_files, not in Router.routes_names.

@codecov
Copy link

codecov bot commented Feb 5, 2021

Codecov Report

Merging #2021 (a7521f6) into 20.12LTS (9763511) will decrease coverage by 0.017%.
The diff coverage is 85.714%.

Impacted file tree graph

@@              Coverage Diff               @@
##           20.12LTS     #2021       +/-   ##
==============================================
- Coverage    92.796%   92.778%   -0.017%     
==============================================
  Files            29        29               
  Lines          3262      3268        +6     
  Branches        571       573        +2     
==============================================
+ Hits           3027      3032        +5     
  Misses          159       159               
- Partials         76        77        +1     
Impacted Files Coverage Δ
sanic/app.py 95.868% <85.714%> (-0.157%) ⬇️
sanic/server.py 81.614% <0.000%> (ø)

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 9763511...a7521f6. Read the comment docs.

@laggardkernel
Copy link
Author

During reading the source code. I've found inconsistent behaviors among route storing of normal http handler and websocket, static file handlers.

Firstly, the ways used to indicate a websocket handler and static file handler are different.

_static_ is prepend into name param from Sanic.static(name="static"). It's the same name param passed to Router._add(..., name). The _static_ is used as a flag in Router._add() to indicate current handler being registered is a static file handler, not used in anywhere else.

websocket.is_websocket is set in Sanic.websocket(), and the flag is only used in Sanic.handle_request().

Secondly, the ways to pass blueprint names of http handler, websocket handler, static file handler, to be used in Router._add() are different.

Formerly, only handler.__blueprintname__ is used. The bp name is prepended with handler name as the route name (key used in Router.routes_name, Router.routes_static_files) in Router._add().

For blueprint websocket handler (Blueprint.websocket()), not the original handler but a handler wrapper is passed to Router._add(). (Check Sanic._websocket_handler() and static._static_request_handler()) Seems later contributors (938c49b) didn't copy the __blueprintname__ attr to the handler wrapper. They chose to pass bp_name + handler to Router._add() directly. Similar thing happens on static file handler Blueprint.static() registration.

@ahopkins
Copy link
Member

ahopkins commented Feb 8, 2021

The new branch I am working on does not use any special prefixes for static or websockets. Also, there is a change to how routes are named in general. But I'll leave rhat discussion for #2010.

@ahopkins
Copy link
Member

ahopkins commented Feb 12, 2021

Since we are merging this into the LTS, I am hesitant to cause breaking changes. While I agree there is the issue with url_for, potentialling the name with the prefix are in use in some applications. Therefore I am thinking that we probably need to add support for both with and without the handler here.

I think the solution of removing the prefix does not consider a broader set of use cases.

@ahopkins ahopkins added the LTS label Feb 15, 2021
Copy link
Member

@ahopkins ahopkins left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Needs to handle fetching routes with and without so that it is not a breaking change.

@sjsadowski
Copy link
Contributor

@laggardkernel are you able to implement @ahoplins request?

@laggardkernel
Copy link
Author

laggardkernel commented Mar 1, 2021

@sjsadowski Just pushed an update with backward support, in case anyone find the bug and uses url_for("websocket_handler_<view_name>") in his code.

Copy link
Member

@ahopkins ahopkins left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks great @laggardkernel. Can you add a test case to test_url_building.py for both cases?

Something like this. Also, one for Blueprints.

def test_url_for_with_websocket_handlers(app):
    @app.websocket("/ws")
    async def my_handler(...):
        ...
    
    assert app.url_for("my_handler") = "..."
    assert app.url_for("websocket_handler_my_handler") = "..."

ahopkins
ahopkins previously approved these changes Mar 1, 2021
@laggardkernel
Copy link
Author

@ahopkins Thanks for pointing out the correct place to add the test. I've updated and squashed the commits.

@ahopkins
Copy link
Member

ahopkins commented Mar 1, 2021

Approved and will merge when Travis is done 🐢

I'll get this pushed out to the LTS tonight.

@ahopkins
Copy link
Member

ahopkins commented Mar 1, 2021

Thanks again for this.

@ahopkins
Copy link
Member

@laggardkernel Can you address the CI issues? Linting issues can be fixed by running:

make pretty

Remove the websocket prefix "websocket_handler_" introduced in
761eef7. Add a backward support for url_for() calling with this prefix
in param "view_name".
@laggardkernel
Copy link
Author

@ahopkins Sorry, fixed and passed the CI.

@ahopkins ahopkins merged commit 8d86c3c into sanic-org:20.12LTS Mar 14, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants