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

Namespace catchall #1288

Closed
wants to merge 6 commits into from
Closed

Conversation

mooomooo
Copy link

@mooomooo mooomooo commented Dec 29, 2023

Closes #1285

Note: Existing tests all pass, but no additional tests have been added validate wildcard functionality.
Namespace catchall tests added and also passed

@codecov-commenter
Copy link

codecov-commenter commented Dec 29, 2023

Codecov Report

All modified and coverable lines are covered by tests ✅

Comparison is base (6967eca) 100.00% compared to head (99e40b8) 100.00%.

Additional details and impacted files
@@            Coverage Diff            @@
##              main     #1288   +/-   ##
=========================================
  Coverage   100.00%   100.00%           
=========================================
  Files           31        31           
  Lines         2405      2423   +18     
  Branches       420       421    +1     
=========================================
+ Hits          2405      2423   +18     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

@mooomooo
Copy link
Author

mooomooo commented Dec 29, 2023

Added more tests, which should fix the codecov. I don't know why the lint is failing -- I don't have any experience with tox and no error message is given. @miguelgrinberg can you take a look?

Also, with a catchall namespace handler, you still need to explicitly set namespaces in the server initializer, which makes sense to me (so you can set a list of allowable namespaces to reject others while still taking advantage of the '*' functionality). However, I don't think I had to do that for the event handlers? That is, it didn't reject s.manager.connect('123', '/bar') even though I didn't set any handlers for namespace = '/bar', and I didn't specify namespaces in the server initializer. Is this intentional or a bug that needs to be tracked down somehow else (or am I misunderstanding how the pytest works)?

@miguelgrinberg
Copy link
Owner

miguelgrinberg commented Dec 30, 2023

@mooomooo the flake8 errors are these:

  src/socketio/base_server.py:210:21: E127 continuation line over-indented for visual indent
  src/socketio/base_server.py:213:21: E127 continuation line over-indented for visual indent
  src/socketio/base_server.py:214:21: E127 continuation line over-indented for visual indent
  src/socketio/base_server.py:218:21: E127 continuation line over-indented for visual indent
  src/socketio/base_server.py:222:21: E127 continuation line over-indented for visual indent
  src/socketio/base_server.py:223:21: E127 continuation line over-indented for visual indent

Regarding the connection question, the tests are connecting directly at the client manager level. The logic to reject a connection based on namespace are in the server, before the client manager is invoked. This is why the tests allow you to connect on random namespaces. See these lines.

@mooomooo
Copy link
Author

Got it, makes sense. I think this should be good for review now!

@miguelgrinberg
Copy link
Owner

Looks good, nothing more to comment on. I need to test this out a little bit, write the documentation and then I'll merge it, hopefully within the next week or so. Thanks!

@miguelgrinberg
Copy link
Owner

Merged! I copied your server implementation to the client and also added some documentation. Thanks!

@kdNew23
Copy link

kdNew23 commented Jan 22, 2024

Thank you for working on this enhancement. Has this code been released yet?

async def _trigger_event(self, event, namespace, *args):
doesn't seem to have the latest code changes from Codecov

I'm trying to get catch-all namespaces for event handler to work.

@sio.on('connection_event', namespace='*')
async def connection_event(namespace, sid, data):

Also, for connect and disconnect events, I could get catch-all namespaces to work by looping through list of namespaces and defining decorators.

@miguelgrinberg
Copy link
Owner

@kdNew23 Yes, this is in the latest release.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Wildcard / catch-all namespace for event handlers
4 participants